-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: ORDER BY ALL #15772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ORDER BY ALL #15772
Changes from 5 commits
96638f8
91fc9f6
a7262a3
b2d48a2
e0f3189
86526fd
a65a9a7
e9a5038
6fc1122
a9f758a
02cd396
3f9839d
cd1bcaf
a328983
590be4d
646dc8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,16 +41,12 @@ impl<S: ContextProvider> SqlToRel<'_, S> { | |
| /// If false, interpret numeric literals as constant values. | ||
| pub(crate) fn order_by_to_sort_expr( | ||
| &self, | ||
| exprs: Vec<OrderByExpr>, | ||
| order_by_exprs: Vec<OrderByExpr>, | ||
| input_schema: &DFSchema, | ||
| planner_context: &mut PlannerContext, | ||
| literal_to_column: bool, | ||
| additional_schema: Option<&DFSchema>, | ||
| ) -> Result<Vec<SortExpr>> { | ||
| if exprs.is_empty() { | ||
| return Ok(vec![]); | ||
| } | ||
|
|
||
| let mut combined_schema; | ||
| let order_by_schema = match additional_schema { | ||
| Some(schema) => { | ||
|
|
@@ -61,13 +57,27 @@ impl<S: ContextProvider> SqlToRel<'_, S> { | |
| None => input_schema, | ||
| }; | ||
|
|
||
| let mut expr_vec = vec![]; | ||
| for e in exprs { | ||
| if order_by_exprs.is_empty() { | ||
| return Ok(vec![]); | ||
| } | ||
|
|
||
| let mut sort_expr_vec = vec![]; | ||
|
||
|
|
||
| let make_sort_expr = | ||
| |expr: Expr, asc: Option<bool>, nulls_first: Option<bool>| { | ||
| let asc = asc.unwrap_or(true); | ||
| // When asc is true, by default nulls last to be consistent with postgres | ||
| // postgres rule: https://www.postgresql.org/docs/current/queries-order.html | ||
| let nulls_first = nulls_first.unwrap_or(!asc); | ||
| Sort::new(expr, asc, nulls_first) | ||
| }; | ||
|
|
||
| for order_by_expr in order_by_exprs { | ||
| let OrderByExpr { | ||
| expr, | ||
| options: OrderByOptions { asc, nulls_first }, | ||
| with_fill, | ||
| } = e; | ||
| } = order_by_expr; | ||
|
|
||
| if let Some(with_fill) = with_fill { | ||
| return not_impl_err!("ORDER BY WITH FILL is not supported: {with_fill}"); | ||
|
|
@@ -102,15 +112,9 @@ impl<S: ContextProvider> SqlToRel<'_, S> { | |
| self.sql_expr_to_logical_expr(e, order_by_schema, planner_context)? | ||
| } | ||
| }; | ||
| let asc = asc.unwrap_or(true); | ||
| expr_vec.push(Sort::new( | ||
| expr, | ||
| asc, | ||
| // When asc is true, by default nulls last to be consistent with postgres | ||
| // postgres rule: https://www.postgresql.org/docs/current/queries-order.html | ||
| nulls_first.unwrap_or(!asc), | ||
| )) | ||
| sort_expr_vec.push(make_sort_expr(expr, asc, nulls_first)); | ||
| } | ||
| Ok(expr_vec) | ||
|
|
||
| Ok(sort_expr_vec) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,14 +22,15 @@ use crate::planner::{ContextProvider, PlannerContext, SqlToRel}; | |
| use crate::stack::StackGuard; | ||
| use datafusion_common::{not_impl_err, Constraints, DFSchema, Result}; | ||
| use datafusion_expr::expr::Sort; | ||
| use datafusion_expr::select_expr::SelectExpr; | ||
|
|
||
| use datafusion_expr::{ | ||
| CreateMemoryTable, DdlStatement, Distinct, LogicalPlan, LogicalPlanBuilder, | ||
| CreateMemoryTable, DdlStatement, Distinct, Expr, LogicalPlan, LogicalPlanBuilder, | ||
| }; | ||
| use sqlparser::ast::{ | ||
| Expr as SQLExpr, Offset as SQLOffset, OrderBy, OrderByExpr, OrderByKind, Query, | ||
| SelectInto, SetExpr, | ||
| Expr as SQLExpr, Ident, Offset as SQLOffset, OrderBy, OrderByExpr, OrderByKind, | ||
| Query, SelectInto, SetExpr, | ||
| }; | ||
| use sqlparser::tokenizer::Span; | ||
|
|
||
| impl<S: ContextProvider> SqlToRel<'_, S> { | ||
| /// Generate a logical plan from an SQL query/subquery | ||
|
|
@@ -158,7 +159,7 @@ fn to_order_by_exprs(order_by: Option<OrderBy>) -> Result<Vec<OrderByExpr>> { | |
| /// Returns the order by expressions from the query with the select expressions. | ||
| pub(crate) fn to_order_by_exprs_with_select( | ||
| order_by: Option<OrderBy>, | ||
| _select_exprs: Option<&Vec<SelectExpr>>, // TODO: ORDER BY ALL | ||
| select_exprs: Option<&Vec<Expr>>, | ||
| ) -> Result<Vec<OrderByExpr>> { | ||
| let Some(OrderBy { kind, interpolate }) = order_by else { | ||
| // If no order by, return an empty array. | ||
|
|
@@ -168,7 +169,27 @@ pub(crate) fn to_order_by_exprs_with_select( | |
| return not_impl_err!("ORDER BY INTERPOLATE is not supported"); | ||
| } | ||
| match kind { | ||
| OrderByKind::All(_) => not_impl_err!("ORDER BY ALL is not supported"), | ||
| OrderByKind::All(order_by_options) => { | ||
| let Some(exprs) = select_exprs else { | ||
| return Ok(vec![]); | ||
| }; | ||
| let order_by_exprs = exprs | ||
| .iter() | ||
| .filter_map(|select_expr| match select_expr { | ||
| Expr::Column(column) => Some(OrderByExpr { | ||
| expr: SQLExpr::Identifier(Ident { | ||
| value: column.name.clone(), | ||
| quote_style: None, | ||
| span: Span::empty(), | ||
| }), | ||
| options: order_by_options.clone(), | ||
| with_fill: None, | ||
| }), | ||
| _ => None, | ||
|
||
| }) | ||
| .collect::<Vec<_>>(); | ||
| Ok(order_by_exprs) | ||
| } | ||
| OrderByKind::Expressions(order_by_exprs) => Ok(order_by_exprs), | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why put the early return here? IMO, the earlier we do the check, the more useless computation we can avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I moved it by accident.