Convert Option<Vec<sort expression>> to Vec<sort expression>#16615
Convert Option<Vec<sort expression>> to Vec<sort expression>#16615findepi merged 7 commits intoapache:mainfrom
Conversation
github-actions
bot
added
sql
|
@findepi Please take a look. |
| from_substrait_sorts(consumer, &f.sorts, input.schema()) | ||
| .await?, | ||
| ) | ||
| from_substrait_sorts(consumer, &f.sorts, input.schema()).await? |
There was a problem hiding this comment.
can we make from_substrait_sorts work with an empty slice?
| Some(exprs) => create_physical_sort_exprs( | ||
| exprs, | ||
| let order_bys = if !order_by.is_empty() { | ||
| create_physical_sort_exprs( |
There was a problem hiding this comment.
can we make create_physical_sort_exprs work with empty slices?
datafusion/expr/src/expr.rs
Outdated
| pub filter: Option<Box<Expr>>, | ||
| /// Optional ORDER BY applied prior to aggregating | ||
| pub order_by: Option<Vec<Expr>>, | ||
| pub order_by: Vec<Sort>, |
There was a problem hiding this comment.
I understand why the Optional was removed (that's the purpose of this PR). I don't yet understand why Expr was replaced with Sort thought.
There was a problem hiding this comment.
This was a mistake during the batch replacement process, but since this struct has never been used in the project, it did not cause any exceptions. I'll change it back.
There was a problem hiding this comment.
And we can open another issue to discuss whether it is more reasonable to change it to the Sort type, because other order_by fields are defined as the Sort type. Or we can directly delete this unused struct.
There was a problem hiding this comment.
if it's unused, let's delete it, prefferrably in a separate PR
There was a problem hiding this comment.
OK. Should it be before or after this PR?
| pub fn first_value(expression: Expr, order_by: Option<Vec<SortExpr>>) -> Expr { | ||
| if let Some(order_by) = order_by { | ||
| pub fn first_value(expression: Expr, order_by: Vec<SortExpr>) -> Expr { | ||
| if !order_by.is_empty() { |
There was a problem hiding this comment.
This condition seems redundant now.
| pub fn last_value(expression: Expr, order_by: Option<Vec<SortExpr>>) -> Expr { | ||
| if let Some(order_by) = order_by { | ||
| pub fn last_value(expression: Expr, order_by: Vec<SortExpr>) -> Expr { | ||
| if !order_by.is_empty() { |
There was a problem hiding this comment.
This condition seems redundant now.
| && a.nulls_first == b.nulls_first | ||
| && a.expr.normalize_eq(&b.expr) | ||
| }) | ||
| && self_order_by.len() == other_order_by.len() |
There was a problem hiding this comment.
The length check was missing, thanks for adding it.
Coincidentally, recently i was reflecting about precisely this -- how Iter::zip is error-prone to use.
| let fun_expr = match fun { | ||
| ExprFuncKind::Aggregate(mut udaf) => { | ||
| udaf.params.order_by = order_by; | ||
| udaf.params.order_by = order_by.unwrap_or_default(); |
There was a problem hiding this comment.
as a follow-up, the builder order_by field can be changed to non-optional as well
There was a problem hiding this comment.
since this type is private and the builder fn order_by already takes a Vec w/o an Option, could we just do this in this PR as well? It's like a 2-line change.
|
@alamb this is a breaking change, right? |
Probably -- we normally add the api change label, which I just did This is the stated policy: https://datafusion.apache.org/contributor-guide/api-health.html |
|
We have a long history of releasing breaking API changes so i think it is best just to use your judgement here |
While it is a breaking change, from my PoV it's a rather minor one (you really just remove the |
Now there isn't and indeed that was the motivation for the change.
Do i understand correctly the next major is the next release going off |
yes
Let's do it! |
4 participants
Which issue does this PR close?
Option<Vec<sort expression>>toVec<sort expression>#12195.Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?