feat: ORDER BY ALL #15772
feat: ORDER BY ALL #15772
Conversation
github-actions
bot
added
sql
datafusion/expr/src/expr.rs
Outdated
| /// OrderBy Expressions | ||
| pub enum OrderByExprs { | ||
| OrderByExprVec(Vec<OrderByExpr>), | ||
| All { | ||
| exprs: Vec<Expr>, | ||
| options: OrderByOptions, | ||
| }, | ||
| } |
There was a problem hiding this comment.
I'm not sure if we need the Enum.
Maybe it's enough to wrap Vec<OrderByExpr> as a struct and add an extra flag to indicate the ALL
pub struct OrderByExprs {
exprs: Vec<OrderByExpr>,
all: bool
}There was a problem hiding this comment.
This requires converting a datafusion_expr::Expr to a SQLExpr, but I'm not sure if there's a good way to do that.
There was a problem hiding this comment.
Instead of this new enum, can we utilize https://github.com/apache/datafusion-sqlparser-rs/blob/3ec80e187d163c4f90c5bfc7c04ef71a2705a631/src/ast/query.rs#L2222 ?
There was a problem hiding this comment.
this enum seems to only be used in the sql planner, so I don't think it is needed in datafusion-expr -- maybe we can just move this into the datafusion-sql crate and make it pub(crate)
There was a problem hiding this comment.
Okay, I’ll try to move it this weekend
datafusion/sql/src/expr/order_by.rs
Outdated
| if order_by_exprs.is_empty() { | ||
| return Ok(vec![]); | ||
| } |
There was a problem hiding this comment.
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.
You are right. I moved it by accident.
datafusion/sql/src/expr/order_by.rs
Outdated
| return Ok(vec![]); | ||
| } | ||
|
|
||
| let mut sort_expr_vec = vec![]; |
There was a problem hiding this comment.
I usually incline to give it a pre-allocation
datafusion/sql/src/query.rs
Outdated
| options: order_by_options.clone(), | ||
| with_fill: None, | ||
| }), | ||
| _ => None, |
There was a problem hiding this comment.
Thank you @PokIsemaine, the PR seems good, but I have one question: Are we going to exclude select expressions which are not column? Is this the same behavior of duckDB?
There was a problem hiding this comment.
Currently, it is directly filtered out. DuckDB is capable of handling other types of expressions. Perhaps I can add a TODO comment and use not_impl_err! to notify the user, then gradually add support for other types of SELECT expressions in future updates.
There was a problem hiding this comment.
if the user gives something like select foo as bar from ... order by all, it doesn't order anything, am I wrong? if so, we should return a not_impl_err!
…ta columns (apache#15935) * fix query results for predicates referencing partition columns and data columns * fmt * add e2e test * newline
Bumps [substrait](https://github.com/substrait-io/substrait-rs) from 0.55.0 to 0.55.1. - [Release notes](https://github.com/substrait-io/substrait-rs/releases) - [Changelog](https://github.com/substrait-io/substrait-rs/blob/main/CHANGELOG.md) - [Commits](substrait-io/substrait-rs@v0.55.0...v0.55.1) --- updated-dependencies: - dependency-name: substrait dependency-version: 0.55.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* feat: create helpers to set the max_temp_directory_size Signed-off-by: Jérémie Drouet <[email protected]> * refactor: use helper in cli Signed-off-by: Jérémie Drouet <[email protected]> * refactor: update error message Signed-off-by: Jérémie Drouet <[email protected]> * refactor: use setter in tests Signed-off-by: Jérémie Drouet <[email protected]> --------- Signed-off-by: Jérémie Drouet <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>
* refactor filter pushdown apis * remove commented out code * fix tests * fail to fix bug * fix * add/fix docs * lint * add some docstrings, some minimal cleaup * review suggestions * add more comments * fix doc links * fmt * add comments * make test deterministic * add bench * fix bench * register bench * fix bench * cargo fmt --------- Co-authored-by: berkaysynnada <[email protected]> Co-authored-by: Berkay Şahin <[email protected]>
github-actions
bot
added
optimizer
github-actions
bot
removed
optimizer
There was a problem hiding this comment.
This looks ready now, thanks @PokIsemaine. I'll merge it after a few hours unless someone else brings any suggestion here
Thanks @PokIsemaine and @berkaysynnada - I also added this to the list of things we could add to a blog about DataFusion 48 in #15771 |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Added support for
ORDER BY ALLsyntaxAre these changes tested?
order.slttest case from https://duckdb.org/docs/stable/sql/query_syntax/orderby.html#order-by-all-examples
Are there any user-facing changes?