diff --git a/datafusion/common/src/config.rs b/datafusion/common/src/config.rs index f5742f09f9b08..e7c6566ad27b3 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,106 @@ impl Display for MapKeyDedupPolicy { } } +/// Default null placement used when an `ORDER BY` clause does not specify +/// `NULLS FIRST` / `NULLS LAST`. +/// +/// This is the typed value of [`SqlParserOptions::default_null_ordering`]. +/// Invalid strings are rejected when the option is set rather than silently +/// falling back to [`NullOrdering::NullsMax`]. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub enum NullOrdering { + /// Nulls appear last in ascending order (Postgres default). + #[default] + NullsMax, + /// Nulls appear first in ascending order. + NullsMin, + /// Nulls always appear first, regardless of sort direction. + NullsFirst, + /// Nulls always appear last, regardless of sort direction. + NullsLast, +} + +impl NullOrdering { + /// All accepted config values, for error messages and docs. + pub const fn available() -> &'static str { + "nulls_max, nulls_min, nulls_first, nulls_last" + } + + /// Canonical config string for this variant. + pub const fn as_str(&self) -> &'static str { + match self { + Self::NullsMax => "nulls_max", + Self::NullsMin => "nulls_min", + Self::NullsFirst => "nulls_first", + Self::NullsLast => "nulls_last", + } + } + + /// 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 AsRef for NullOrdering { + fn as_ref(&self) -> &str { + self.as_str() + } +} + +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: {}", + Self::available() + ))), + } + } +} + +impl From<&str> for NullOrdering { + /// Parses a null-ordering name, defaulting to [`NullOrdering::NullsMax`] + /// when the string is not one of the supported values. + /// + /// Prefer [`FromStr`] when an invalid value should be reported as an error. + fn from(s: &str) -> Self { + Self::from_str(s).unwrap_or_default() + } +} + +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 Display for NullOrdering { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.as_str()) + } +} + impl From for Option { fn from(c: SpillCompression) -> Self { match c { @@ -4576,6 +4676,79 @@ mod tests { assert!(error.contains(Dialect::available())); } + #[test] + fn test_default_null_ordering_roundtrip() { + use crate::config::NullOrdering; + use std::str::FromStr; + + assert_eq!(NullOrdering::default(), NullOrdering::NullsMax); + + for (value, expected) in [ + ("nulls_max", NullOrdering::NullsMax), + ("nulls_min", NullOrdering::NullsMin), + ("nulls_first", NullOrdering::NullsFirst), + ("nulls_last", NullOrdering::NullsLast), + ] { + assert_eq!(NullOrdering::from_str(value).unwrap(), expected); + assert_eq!( + NullOrdering::from_str(&value.to_ascii_uppercase()).unwrap(), + expected + ); + assert_eq!(expected.as_str(), value); + assert_eq!(expected.to_string(), value); + } + } + + #[test] + fn test_default_null_ordering_nulls_first() { + use crate::config::NullOrdering; + + assert!(!NullOrdering::NullsMax.nulls_first(true)); + assert!(NullOrdering::NullsMax.nulls_first(false)); + assert!(NullOrdering::NullsMin.nulls_first(true)); + assert!(!NullOrdering::NullsMin.nulls_first(false)); + assert!(NullOrdering::NullsFirst.nulls_first(true)); + assert!(NullOrdering::NullsFirst.nulls_first(false)); + assert!(!NullOrdering::NullsLast.nulls_first(true)); + assert!(!NullOrdering::NullsLast.nulls_first(false)); + } + + #[test] + fn test_invalid_default_null_ordering_error_lists_available_values() { + use crate::config::NullOrdering; + use std::str::FromStr; + + let error = NullOrdering::from_str("nulls_mx").unwrap_err().to_string(); + + assert!(error.contains("Invalid default_null_ordering: 'nulls_mx'")); + assert!(error.contains(NullOrdering::available())); + } + + #[test] + fn test_default_null_ordering_config_set_rejects_invalid_values() { + use crate::config::ConfigOptions; + + let mut options = ConfigOptions::new(); + let err = options + .set("datafusion.sql_parser.default_null_ordering", "nulls_mx") + .unwrap_err() + .to_string(); + + assert!(err.contains("Invalid default_null_ordering: 'nulls_mx'")); + assert_eq!( + options.sql_parser.default_null_ordering, + crate::config::NullOrdering::NullsMax + ); + + options + .set("datafusion.sql_parser.default_null_ordering", "NULLS_FIRST") + .unwrap(); + assert_eq!( + options.sql_parser.default_null_ordering, + crate::config::NullOrdering::NullsFirst + ); + } + #[test] fn max_row_group_bytes_rejects_zero() { use crate::config::MaxRowGroupBytes; 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..9fc8f04bf9715 100644 --- a/datafusion/sql/src/planner.rs +++ b/datafusion/sql/src/planner.rs @@ -17,7 +17,6 @@ //! [`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; @@ -40,6 +39,8 @@ use sqlparser::ast::{ArrayElemTypeDef, ExactNumberInfo, TimezoneInfo}; use sqlparser::ast::{ColumnDef as SQLColumnDef, ColumnOption}; use sqlparser::ast::{DataType as SQLDataType, Ident, ObjectName, TableAlias}; +pub use datafusion_common::config::NullOrdering; + /// SQL parser options #[derive(Debug, Clone, Copy)] pub struct ParserOptions { @@ -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(), - } - } -} - -/// 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}" - ), + default_null_ordering: options.default_null_ordering, } } } -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..cdcbec7df4b90 100644 --- a/datafusion/sqllogictest/test_files/order.slt +++ b/datafusion/sqllogictest/test_files/order.slt @@ -158,26 +158,13 @@ SELECT * FROM (VALUES (1, 'one'), (2, 'two'), (null, 'three')) AS t (num,letter) 1 one NULL three -statement ok +statement error 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 +DataFusion error: Error setting config datafusion.sql_parser.default_null_ordering +caused by +Invalid or Unsupported Configuration: Invalid default_null_ordering: ''. Expected one of: nulls_max, nulls_min, nulls_first, nulls_last -# 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 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..2a8581e4abf91 100644 --- a/datafusion/sqllogictest/test_files/set_variable.slt +++ b/datafusion/sqllogictest/test_files/set_variable.slt @@ -759,6 +759,55 @@ caused by Invalid or Unsupported Configuration: value must be greater than 0 +# default_null_ordering is an enum; reject typos instead of silently +# falling back to the default (nulls_max). +statement ok +SET datafusion.sql_parser.default_null_ordering = 'nulls_first' + +query TT +SHOW datafusion.sql_parser.default_null_ordering +---- +datafusion.sql_parser.default_null_ordering nulls_first + +statement ok +SET datafusion.sql_parser.default_null_ordering = 'NULLS_LAST' + +query TT +SHOW datafusion.sql_parser.default_null_ordering +---- +datafusion.sql_parser.default_null_ordering nulls_last + +statement error +SET datafusion.sql_parser.default_null_ordering = 'nulls_mx' +---- +DataFusion error: Error setting config datafusion.sql_parser.default_null_ordering +caused by +Invalid or Unsupported Configuration: Invalid default_null_ordering: 'nulls_mx'. Expected one of: nulls_max, nulls_min, nulls_first, nulls_last + + +statement error +SET datafusion.sql_parser.default_null_ordering = '' +---- +DataFusion error: Error setting config datafusion.sql_parser.default_null_ordering +caused by +Invalid or Unsupported Configuration: Invalid default_null_ordering: ''. Expected one of: nulls_max, nulls_min, nulls_first, nulls_last + + +# The previous valid value remains active after a rejected SET. +query TT +SHOW datafusion.sql_parser.default_null_ordering +---- +datafusion.sql_parser.default_null_ordering nulls_last + +statement ok +RESET datafusion.sql_parser.default_null_ordering + +query TT +SHOW datafusion.sql_parser.default_null_ordering +---- +datafusion.sql_parser.default_null_ordering nulls_max + + # 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.