chore(query): null first behavior

datafuselabs · Sep 19, 2024 · 1adb900 · 1adb900
1 parent 047f081
commit 1adb900
Show file tree

Hide file tree

Showing 8 changed files with 61 additions and 38 deletions.
diff --git a/src/common/storage/src/stage.rs b/src/common/storage/src/stage.rs
@@ -266,7 +266,7 @@ impl StageFilesInfo {
         let file_exact: Option<Result<StageFileInfo>> = match prefix_meta {
             Ok(meta) if meta.is_file() => {
                 let f = StageFileInfo::new(path.to_string(), &meta);
-                if max_files == Some(1) {
+                if max_files == Some(1) || pattern.is_none() {
                     return Ok(Box::pin(stream::once(async { Ok(f) })));
                 }
                 Some(Ok(f))

diff --git a/src/query/ast/src/parser/input.rs b/src/query/ast/src/parser/input.rs
@@ -129,11 +129,11 @@ impl Dialect {
         }
     }
 
-    pub fn is_null_biggest(&self) -> bool {
+    pub fn is_null_biggest(&self, asc: bool) -> bool {
         match self {
             Dialect::MySQL => false,
             Dialect::Hive => false,
-            Dialect::Experimental | Dialect::PostgreSQL | Dialect::PRQL => true,
+            Dialect::Experimental | Dialect::PostgreSQL | Dialect::PRQL => asc,
         }
     }
 

diff --git a/src/query/sql/src/executor/physical_plans/physical_window.rs b/src/query/sql/src/executor/physical_plans/physical_window.rs
@@ -241,14 +241,20 @@ impl PhysicalPlanBuilder {
             }
         }
 
+        let dialect = self.ctx.get_settings().get_sql_dialect().unwrap();
+        let default_nulls_first = |asc: bool| !dialect.is_null_biggest(asc);
+
         let order_by_items = w
             .order_by
             .iter()
-            .map(|v| SortDesc {
-                asc: v.asc.unwrap_or(true),
-                nulls_first: v.nulls_first.unwrap_or(false),
-                order_by: v.order_by_item.index,
-                display_name: self.metadata.read().column(v.order_by_item.index).name(),
+            .map(|v| {
+                let asc = v.asc.unwrap_or(true);
+                SortDesc {
+                    asc,
+                    nulls_first: v.nulls_first.unwrap_or_else(|| default_nulls_first(asc)),
+                    order_by: v.order_by_item.index,
+                    display_name: self.metadata.read().column(v.order_by_item.index).name(),
+                }
             })
             .collect::<Vec<_>>();
         let partition_items = w.partition_by.iter().map(|v| v.index).collect::<Vec<_>>();

diff --git a/src/query/sql/src/planner/binder/bind_query/bind.rs b/src/query/sql/src/planner/binder/bind_query/bind.rs
@@ -121,16 +121,23 @@ impl Binder {
             self.ctes_map.clone(),
         );
         let mut order_by_items = Vec::with_capacity(query.order_by.len());
+
+        let dialect = self.ctx.get_settings().get_sql_dialect().unwrap();
+        let default_nulls_first = |asc: bool| !dialect.is_null_biggest(asc);
+
         for order in query.order_by.iter() {
             match order.expr {
                 Expr::ColumnRef { .. } => {
                     let scalar = scalar_binder.bind(&order.expr)?.0;
                     match scalar {
                         ScalarExpr::BoundColumnRef(BoundColumnRef { column, .. }) => {
+                            let asc = order.asc.unwrap_or(true);
                             let order_by_item = SortItem {
                                 index: column.index,
-                                asc: order.asc.unwrap_or(true),
-                                nulls_first: order.nulls_first.unwrap_or(false),
+                                asc,
+                                nulls_first: order
+                                    .nulls_first
+                                    .unwrap_or_else(|| default_nulls_first(asc)),
                             };
                             order_by_items.push(order_by_item);
                         }

diff --git a/src/query/sql/src/planner/binder/sort.rs b/src/query/sql/src/planner/binder/sort.rs
@@ -67,14 +67,8 @@ impl Binder {
         distinct: bool,
     ) -> Result<OrderItems> {
         bind_context.set_expr_context(ExprContext::OrderByClause);
-        // null is the largest value in databend, smallest in hive
-        // TODO: rewrite after https://github.com/jorgecarleitao/arrow2/pull/1286 is merged
-        let default_nulls_first = !self
-            .ctx
-            .get_settings()
-            .get_sql_dialect()
-            .unwrap()
-            .is_null_biggest();
+        let dialect = self.ctx.get_settings().get_sql_dialect().unwrap();
+        let default_nulls_first = |asc: bool| !dialect.is_null_biggest(asc);
 
         let mut order_items = Vec::with_capacity(order_by.len());
         for order in order_by {
@@ -94,11 +88,15 @@ impl Binder {
 
                     let index = index - 1;
                     let projection = &projections[index];
+
+                    let asc = order.asc.unwrap_or(true);
                     order_items.push(OrderItem {
                         index: projection.index,
                         name: projection.column_name.clone(),
-                        asc: order.asc.unwrap_or(true),
-                        nulls_first: order.nulls_first.unwrap_or(default_nulls_first),
+                        asc,
+                        nulls_first: order
+                            .nulls_first
+                            .unwrap_or_else(|| default_nulls_first(asc)),
                     });
                 }
                 _ => {
@@ -119,11 +117,14 @@ impl Binder {
                         .find(|(_, (_, scalar))| bound_expr.eq(scalar))
                     {
                         // The order by expression is in the select list.
+                        let asc = order.asc.unwrap_or(true);
                         order_items.push(OrderItem {
                             index: projections[idx].index,
                             name: alias.clone(),
-                            asc: order.asc.unwrap_or(true),
-                            nulls_first: order.nulls_first.unwrap_or(default_nulls_first),
+                            asc,
+                            nulls_first: order
+                                .nulls_first
+                                .unwrap_or_else(|| default_nulls_first(asc)),
                         });
                     } else if distinct {
                         return Err(ErrorCode::SemanticError(
@@ -165,11 +166,14 @@ impl Binder {
                             index: column_binding.index,
                         };
                         scalar_items.insert(column_binding.index, item);
+                        let asc = order.asc.unwrap_or(true);
                         order_items.push(OrderItem {
                             index: column_binding.index,
                             name: column_binding.column_name,
-                            asc: order.asc.unwrap_or(true),
-                            nulls_first: order.nulls_first.unwrap_or(default_nulls_first),
+                            asc,
+                            nulls_first: order
+                                .nulls_first
+                                .unwrap_or_else(|| default_nulls_first(asc)),
                         });
                     }
                 }

diff --git a/src/query/sql/src/planner/binder/window.rs b/src/query/sql/src/planner/binder/window.rs
@@ -89,29 +89,28 @@ impl Binder {
             child
         };
 
-        let default_nulls_first = !self
-            .ctx
-            .get_settings()
-            .get_sql_dialect()
-            .unwrap()
-            .is_null_biggest();
+        let dialect = self.ctx.get_settings().get_sql_dialect().unwrap();
+        let default_nulls_first = |asc: bool| !dialect.is_null_biggest(asc);
 
         let mut sort_items: Vec<SortItem> = vec![];
         if !window_plan.partition_by.is_empty() {
             for part in window_plan.partition_by.iter() {
                 sort_items.push(SortItem {
                     index: part.index,
                     asc: true,
-                    nulls_first: default_nulls_first,
+                    nulls_first: default_nulls_first(true),
                 });
             }
         }
 
         for order in window_plan.order_by.iter() {
+            let asc = order.asc.unwrap_or(true);
             sort_items.push(SortItem {
                 index: order.order_by_item.index,
-                asc: order.asc.unwrap_or(true),
-                nulls_first: order.nulls_first.unwrap_or(default_nulls_first),
+                asc,
+                nulls_first: order
+                    .nulls_first
+                    .unwrap_or_else(|| default_nulls_first(asc)),
             });
         }
 

diff --git a/src/query/storages/parquet/src/parquet_rs/statistics/column.rs b/src/query/storages/parquet/src/parquet_rs/statistics/column.rs
@@ -61,7 +61,7 @@ pub fn convert_column_statistics(s: &Statistics, typ: &TableDataType) -> Option<
                         Scalar::Decimal(DecimalScalar::Decimal256(I256::from_i128(max), *size)),
                         Scalar::Decimal(DecimalScalar::Decimal256(I256::from_i128(min), *size)),
                     ),
-                    _ => (Scalar::Null, Scalar::Null),
+                    _ => return None,
                 }
             }
             Statistics::Int64(s) => {
@@ -92,7 +92,7 @@ pub fn convert_column_statistics(s: &Statistics, typ: &TableDataType) -> Option<
                         Scalar::Decimal(DecimalScalar::Decimal256(I256::from_i128(max), *size)),
                         Scalar::Decimal(DecimalScalar::Decimal256(I256::from_i128(min), *size)),
                     ),
-                    _ => (Scalar::Null, Scalar::Null),
+                    _ => return None,
                 }
             }
             Statistics::Int96(s) => {
@@ -124,12 +124,12 @@ pub fn convert_column_statistics(s: &Statistics, typ: &TableDataType) -> Option<
                         decode_decimal256_from_bytes(max, *size),
                         decode_decimal256_from_bytes(min, *size),
                     ),
-                    _ => (Scalar::Null, Scalar::Null),
+                    _ => return None,
                 }
             }
         }
     } else {
-        (Scalar::Null, Scalar::Null)
+        return None;
     };
     Some(ColumnStatistics::new(
         min,

diff --git a/tests/sqllogictests/suites/query/order.test b/tests/sqllogictests/suites/query/order.test
@@ -17,9 +17,9 @@ NULL
 query I
 select * from order_test order by a desc
 ----
+NULL
 2
 1
-NULL
 
 query I
 select * from order_test order by a nulls first
@@ -28,6 +28,13 @@ NULL
 1
 2
 
+query I
+select * from order_test order by a nulls last
+----
+1
+2
+NULL
+
 query II
 select number  d , max(1-number) c from numbers(4) group by 1  order by 2;
 ----