alamb commented on code in PR #16963:
URL: https://github.com/apache/datafusion/pull/16963#discussion_r2240591332
##########
datafusion/sql/src/planner.rs:
##########
@@ -147,10 +159,60 @@ 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 eval(&self, asc: bool) -> bool {
Review Comment:
Maybe we could call this function something like `nulls_first` as that is
what it appears to be calculating (if nulls should be first given the setting
and the `ASC` flag)
##########
datafusion/sqllogictest/test_files/order.slt:
##########
@@ -94,6 +94,74 @@ NULL three
1 one
2 two
+statement ok
+set datafusion.sql_parser.default_null_ordering = 'nulls_min';
+
+# test asc with `nulls_min` null ordering
+
+query IT
+SELECT * FROM (VALUES (1, 'one'), (2, 'two'), (null, 'three')) AS t
(num,letter) ORDER BY num
+----
+NULL three
+1 one
+2 two
+
+# test desc with `nulls_min` null ordering
+
+query IT
+SELECT * FROM (VALUES (1, 'one'), (2, 'two'), (null, 'three')) AS t
(num,letter) ORDER BY num DESC
+----
+2 two
+1 one
+NULL three
+
+statement ok
+set datafusion.sql_parser.default_null_ordering = 'nulls_first';
+
+# test asc with `nulls_first` null ordering
+
+query IT
+SELECT * FROM (VALUES (1, 'one'), (2, 'two'), (null, 'three')) AS t
(num,letter) ORDER BY num
+----
+NULL three
+1 one
+2 two
+
+# test desc with `nulls_first` null ordering
+
+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 ok
+set datafusion.sql_parser.default_null_ordering = 'nulls_last';
+
+# test asc with `nulls_last` null ordering
+
+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 `nulls_last` null ordering
+
+query IT
+SELECT * FROM (VALUES (1, 'one'), (2, 'two'), (null, 'three')) AS t
(num,letter) ORDER BY num DESC
+----
+2 two
+1 one
+NULL three
+
+# reset to default null ordering
Review Comment:
Can you please also add a test for an invalid value like `NULLS` or ``?
```
set datafusion.sql_parser.default_null_ordering = 'NULLS_LAST';
```
##########
datafusion/sql/src/planner.rs:
##########
@@ -147,10 +159,60 @@ 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 eval(&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<Self> {
+ 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: {s}"),
Review Comment:
I think the error message would be be better if it had the expected values,
like
```suggestion
_ => plan_err!("Unknown null ordering: Expected one of
'nulls_first', 'nulls_last', 'nulls_min' or 'nulls_max'. Got {s}"),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
