Fix ORDER BY on aggregate #1506
Conversation
datafusion/src/logical_plan/expr.rs
Outdated
| input, | ||
| group_expr: _, | ||
| aggr_expr, | ||
| schema: _, |
There was a problem hiding this comment.
i guess you can use .. to ignore these fields?
datafusion/src/logical_plan/expr.rs
Outdated
| } | ||
|
|
||
| /// Rewrite sort on aggregate expressions to sort on the column of aggregate output | ||
| #[inline] |
There was a problem hiding this comment.
why would this need to be inlined? or can't the compiler infer that?
There was a problem hiding this comment.
Hmm, I saw there is #[inline] for normalize_cols above. rewrite_sort_cols_by_aggs's looks similar (i.e., into_iter().map().collect()), so I added it too here. Should I remove it?
There was a problem hiding this comment.
In Rust, a unit of (separate) compilation is a crate. If a function f is defined in a crate A, then all calls to f from within A can be inlined, as the compiler has full access to f. If, however, f is called from some downstream crate B, such calls can’t be inlined. B has access only to the signature of f, not its body.
So I think both of them don't need #[inline], let the compiler do it.
| ]; | ||
| assert_batches_eq!(expected, &actual); | ||
|
|
||
| let sql = "SELECT MIN(c12) FROM aggregate_test_100 ORDER BY MIN(c12) + 0.1"; |
Co-authored-by: Andrew Lamb <[email protected]>
Co-authored-by: Andrew Lamb <[email protected]>
Co-authored-by: Andrew Lamb <[email protected]>
|
Thank you @alamb . Committed the suggestions. |
|
Thanks again @viirya ! |
|
Thank you @alamb @jimexist @xudong963 ! |
4 participants
Which issue does this PR close?
Closes #1479.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?