diff --git a/datafusion/common/src/config.rs b/datafusion/common/src/config.rs index f5742f09f9b08..45173764d7b3a 100644 --- a/datafusion/common/src/config.rs +++ b/datafusion/common/src/config.rs @@ -311,7 +311,7 @@ config_namespace! { /// /// By default, `nulls_max` is used to follow Postgres's behavior. /// postgres rule: - pub default_null_ordering: String, default = "nulls_max".to_string() + pub default_null_ordering: NullOrdering, default = NullOrdering::NullsMax /// When set to true, DataFusion may remove `ORDER BY` clauses from /// subqueries or CTEs during SQL planning when their ordering cannot @@ -795,6 +795,80 @@ impl Display for MapKeyDedupPolicy { } } +/// Default null ordering for query results when `ORDER BY` does not specify +/// `NULLS FIRST` or `NULLS LAST`. +/// +/// Valid values are validated at configuration time so invalid +/// `datafusion.sql_parser.default_null_ordering` settings are rejected +/// immediately instead of being stored and later treated as `nulls_max`. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Hash)] +pub enum NullOrdering { + /// Nulls appear last in ascending order and first in descending order. + #[default] + NullsMax, + /// Nulls appear first in ascending order and last in descending order. + NullsMin, + /// Nulls always appear first. + NullsFirst, + /// Nulls always appear last. + NullsLast, +} + +impl NullOrdering { + /// Evaluates the null ordering based on the given ascending flag. + /// + /// # Returns + /// * `true` if nulls should appear first. + /// * `false` if nulls should appear last. + pub fn nulls_first(&self, asc: bool) -> bool { + match self { + Self::NullsMax => !asc, + Self::NullsMin => asc, + Self::NullsFirst => true, + Self::NullsLast => false, + } + } +} + +impl FromStr for NullOrdering { + type Err = DataFusionError; + + fn from_str(s: &str) -> Result { + match s.to_ascii_lowercase().as_str() { + "nulls_max" => Ok(Self::NullsMax), + "nulls_min" => Ok(Self::NullsMin), + "nulls_first" => Ok(Self::NullsFirst), + "nulls_last" => Ok(Self::NullsLast), + other => Err(DataFusionError::Configuration(format!( + "Invalid default null ordering: {other}. Expected one of: nulls_max, nulls_min, nulls_first, nulls_last" + ))), + } + } +} + +impl Display for NullOrdering { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let s = match self { + Self::NullsMax => "nulls_max", + Self::NullsMin => "nulls_min", + Self::NullsFirst => "nulls_first", + Self::NullsLast => "nulls_last", + }; + write!(f, "{s}") + } +} + +impl ConfigField for NullOrdering { + fn visit(&self, v: &mut V, key: &str, description: &'static str) { + v.some(key, self, description) + } + + fn set(&mut self, _: &str, value: &str) -> Result<()> { + *self = Self::from_str(value)?; + Ok(()) + } +} + impl From for Option { fn from(c: SpillCompression) -> Self { match c { @@ -4443,6 +4517,53 @@ mod tests { ); } + #[test] + fn test_default_null_ordering_validation() { + use crate::assert_contains; + use crate::config::{ConfigOptions, NullOrdering}; + + let mut config = ConfigOptions::default(); + assert_eq!( + config.sql_parser.default_null_ordering, + NullOrdering::NullsMax + ); + + for (value, expected) in [ + ("nulls_max", NullOrdering::NullsMax), + ("nulls_min", NullOrdering::NullsMin), + ("nulls_first", NullOrdering::NullsFirst), + ("nulls_last", NullOrdering::NullsLast), + // Values are case-insensitive, matching other enum config options. + ("NULLS_FIRST", NullOrdering::NullsFirst), + ] { + config + .set("datafusion.sql_parser.default_null_ordering", value) + .unwrap(); + assert_eq!(config.sql_parser.default_null_ordering, expected); + } + + // Invalid values, including the previous silent-fallback empty string, + // should error immediately at SET time. + for value in ["nuls_max", "", "nulls first"] { + let err = config + .set("datafusion.sql_parser.default_null_ordering", value) + .unwrap_err(); + assert_contains!( + err.to_string(), + "Invalid or Unsupported Configuration: Invalid default null ordering:" + ); + assert_contains!( + err.to_string(), + "Expected one of: nulls_max, nulls_min, nulls_first, nulls_last" + ); + // Previous valid value remains active on error. + assert_eq!( + config.sql_parser.default_null_ordering, + NullOrdering::NullsFirst + ); + } + } + #[cfg(feature = "parquet")] #[test] fn set_cdc_enabled_flag() { diff --git a/datafusion/core/src/execution/session_state.rs b/datafusion/core/src/execution/session_state.rs index a9d6198f953ed..ff8ebb67e594a 100644 --- a/datafusion/core/src/execution/session_state.rs +++ b/datafusion/core/src/execution/session_state.rs @@ -609,10 +609,7 @@ impl SessionState { support_varchar_with_length: sql_parser_options.support_varchar_with_length, map_string_types_to_utf8view: sql_parser_options.map_string_types_to_utf8view, collect_spans: sql_parser_options.collect_spans, - default_null_ordering: sql_parser_options - .default_null_ordering - .as_str() - .into(), + default_null_ordering: sql_parser_options.default_null_ordering, } } diff --git a/datafusion/sql/src/planner.rs b/datafusion/sql/src/planner.rs index 3a696811be499..cd17e762e8f95 100644 --- a/datafusion/sql/src/planner.rs +++ b/datafusion/sql/src/planner.rs @@ -17,13 +17,14 @@ //! [`SqlToRel`]: SQL Query Planner (produces [`LogicalPlan`] from SQL AST) use std::collections::HashMap; -use std::str::FromStr; use std::sync::{Arc, Mutex}; use std::vec; use crate::utils::make_decimal_type; use arrow::datatypes::*; use datafusion_common::TableReference; +/// Default null ordering for sorting expressions. +pub use datafusion_common::config::NullOrdering; use datafusion_common::config::SqlParserOptions; use datafusion_common::datatype::{DataTypeExt, FieldExt}; use datafusion_common::error::add_possible_columns_to_diag; @@ -159,62 +160,11 @@ impl From<&SqlParserOptions> for ParserOptions { enable_options_value_normalization: options .enable_options_value_normalization, collect_spans: options.collect_spans, - default_null_ordering: options.default_null_ordering.as_str().into(), + default_null_ordering: options.default_null_ordering, } } } -/// Represents the null ordering for sorting expressions. -#[derive(Debug, Clone, Copy)] -pub enum NullOrdering { - /// Nulls appear last in ascending order. - NullsMax, - /// Nulls appear first in descending order. - NullsMin, - /// Nulls appear first. - NullsFirst, - /// Nulls appear last. - NullsLast, -} - -impl NullOrdering { - /// Evaluates the null ordering based on the given ascending flag. - /// - /// # Returns - /// * `true` if nulls should appear first. - /// * `false` if nulls should appear last. - pub fn nulls_first(&self, asc: bool) -> bool { - match self { - Self::NullsMax => !asc, - Self::NullsMin => asc, - Self::NullsFirst => true, - Self::NullsLast => false, - } - } -} - -impl FromStr for NullOrdering { - type Err = DataFusionError; - - fn from_str(s: &str) -> Result { - match s { - "nulls_max" => Ok(Self::NullsMax), - "nulls_min" => Ok(Self::NullsMin), - "nulls_first" => Ok(Self::NullsFirst), - "nulls_last" => Ok(Self::NullsLast), - _ => plan_err!( - "Unknown null ordering: Expected one of 'nulls_first', 'nulls_last', 'nulls_min' or 'nulls_max'. Got {s}" - ), - } - } -} - -impl From<&str> for NullOrdering { - fn from(s: &str) -> Self { - Self::from_str(s).unwrap_or(Self::NullsMax) - } -} - /// Ident Normalizer #[derive(Debug)] pub struct IdentNormalizer { diff --git a/datafusion/sqllogictest/test_files/order.slt b/datafusion/sqllogictest/test_files/order.slt index 4b136d24b0751..c735a1bcb46bc 100644 --- a/datafusion/sqllogictest/test_files/order.slt +++ b/datafusion/sqllogictest/test_files/order.slt @@ -158,26 +158,11 @@ SELECT * FROM (VALUES (1, 'one'), (2, 'two'), (null, 'three')) AS t (num,letter) 1 one NULL three -statement ok +statement error Invalid default null ordering set datafusion.sql_parser.default_null_ordering = ''; -# test asc with an empty `default_null_ordering`. Expected to use the default null ordering which is `nulls_max` - -query IT -SELECT * FROM (VALUES (1, 'one'), (2, 'two'), (null, 'three')) AS t (num,letter) ORDER BY num ----- -1 one -2 two -NULL three - -# test desc with an empty `default_null_ordering`. Expected to use the default null ordering which is `nulls_max` - -query IT -SELECT * FROM (VALUES (1, 'one'), (2, 'two'), (null, 'three')) AS t (num,letter) ORDER BY num DESC ----- -NULL three -2 two -1 one +statement error Invalid default null ordering: nuls_max +set datafusion.sql_parser.default_null_ordering = 'nuls_max'; statement error DataFusion error: Error during planning: Unsupported value Null set datafusion.sql_parser.default_null_ordering = null; diff --git a/datafusion/sqllogictest/test_files/set_variable.slt b/datafusion/sqllogictest/test_files/set_variable.slt index b8db761e796fe..a506b8099c272 100644 --- a/datafusion/sqllogictest/test_files/set_variable.slt +++ b/datafusion/sqllogictest/test_files/set_variable.slt @@ -759,6 +759,14 @@ caused by Invalid or Unsupported Configuration: value must be greater than 0 +statement error +SET datafusion.sql_parser.default_null_ordering = nuls_max +---- +DataFusion error: Error setting config datafusion.sql_parser.default_null_ordering +caused by +Invalid or Unsupported Configuration: Invalid default null ordering: nuls_max. Expected one of: nulls_max, nulls_min, nulls_first, nulls_last + + # max_buffered_batches_per_output_file is halved to size an internal channel # capacity, so 0 and 1 both round down to a zero-capacity channel and must be # rejected, not just 0.