Remove Sort expression (`Expr::Sort`) by findepi · Pull Request #12177 · apache/datafusion

findepi · 2024-08-26T15:27:14Z

Remove sort as an expression, i.e. remove Expr::Sort from Expr enum.
Use expr::Sort directly when sorting.

The sort expression was used in context of ordering (sort, topk, create
table, file sorting). Those places require their sort expression to be
of type Sort anyway and no other expression was allowed, so this change
improves static typing. Sort as an expression was illegal in other
contexts.

For #1468 (comment)
Fixes#12193

findepi · 2024-08-26T15:28:06Z

Not ready for review. Just for FYI.

findepi · 2024-08-27T09:42:00Z

The build is all green (https://github.com/findepi/datafusion/actions/runs/10574858019)!

findepi · 2024-08-27T13:31:52Z

Rebased, should be ready for initial review pass.

I split the change into couple commits, but bulk of it is still one big commit.
Should i continue along this pattern?
Should i extract some commits into own PRs
let me know what's best from review perspective.

cc @alamb @comphead @jonahgao @jayzhan211 @andygrove @crepererum

crepererum

Looking at all the invalid states (panics and "unimplemented" errors) that are removed in this PR, this is a clear improvement. I also agree with the overall premise that "sorts" are not really a subtype/variant of Expr.

This should probably get a 2nd ACK by another maintainer.

findepi · 2024-08-27T20:03:18Z

there is a new conflict (perhaps due to #12196), let me rebase.

Part of effort to remove `Expr::Sort`.

Take `expr::Sort` in `LogicalPlanBuilder.sort`. Accept any `Expr` in new function, `LogicalPlanBuilder.sort_by` which apply default sort ordering. Part of effort to remove `Expr::Sort`.

Part of effort to remove `Expr::Sort`.

Remove sort as an expression, i.e. remove `Expr::Sort` from `Expr` enum. Use `expr::Sort` directly when sorting. The sort expression was used in context of ordering (sort, topk, create table, file sorting). Those places require their sort expression to be of type Sort anyway and no other expression was allowed, so this change improves static typing. Sort as an expression was illegal in other contexts.

jayzhan211 · 2024-08-28T02:26:44Z

+ }))
+}
+
+pub fn replace_sort_expressions(sorts: Vec<Sort>, new_expr: Vec<Expr>) -> Vec<Sort> {


Maybe returns with Result<Vec<Sort>>?

Good question. This is used in two places. In one we know the list of expressions is exact size.
the other is LogicalPlan.with_new_exprs which does assert! on provided new expr length. so it looks we don't need to be permissive here?

jayzhan211 · 2024-08-28T02:33:27Z

 .build()?;
 // Do nothing
- let expected = "Aggregate: groupBy=[[test.c]], aggr=[[sum(test.a) ORDER BY [test.a], count(DISTINCT test.b)]] [c:UInt32, sum(test.a) ORDER BY [test.a]:UInt64;N, count(DISTINCT test.b):Int64]\
+ let expected = "Aggregate: groupBy=[[test.c]], aggr=[[sum(test.a) ORDER BY [test.a ASC NULLS LAST], count(DISTINCT test.b)]] [c:UInt32, sum(test.a) ORDER BY [test.a ASC NULLS LAST]:UInt64;N, count(DISTINCT test.b):Int64]\


Is this the same as previous?

the default ASC/DESC NULLS FIRST/LAST behavior seems to come from
datafusion/datafusion/sql/src/expr/order_by.rs
Lines 101 to 107 in c22e13f
let asc = asc.unwrap_or(true);
expr_vec.push(Expr::Sort(Sort::new(
Box::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),

if i read this correctly, ASC NULLS LAST is the default

findepi · 2024-08-28T07:42:44Z

thank you @jayzhan211 for your review! would you mind taking another look?

this commit is longer than advised in the review comment, but after squashing the diff will be smaller

jayzhan211

👍

alamb · 2024-08-28T12:24:39Z

EPIC!

findepi · 2024-08-29T10:36:47Z

thank you @jayzhan211 @crepererum @alamb for all your time spent reviewing this!

findepi · 2024-08-29T15:47:38Z

❤️ thank you for the (brave) merge, @crepererum !

findepi marked this pull request as draft August 26, 2024 15:27

github-actionsBot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate proto Related to proto crate functions Changes to functions implementation labels Aug 26, 2024

findepi force-pushed the findepi/sort-is-not-expression branch from eaeb216 to f2a254fCompare August 26, 2024 17:03

github-actionsBot added the substrait Changes to the substrait crate label Aug 26, 2024

This was referenced Aug 26, 2024
Require sort expressions to be of type Sort #12171
Merged
Deduplicate sort unparsing logic #12175
Merged
Remove parse_vec_expr helper #12178
Merged
Reuse bulk serialization helpers for protobuf #12179
Merged

findepi commented Aug 26, 2024
View reviewed changes

Comment threaddatafusion/sql/src/unparser/rewrite.rs Outdated

findepi commented Aug 26, 2024
View reviewed changes

Comment threaddatafusion/core/src/datasource/listing/table.rs
Comment threaddatafusion/core/tests/expr_api/mod.rs
Comment threaddatafusion/optimizer/src/eliminate_limit.rs

findepi force-pushed the findepi/sort-is-not-expression branch from c29d147 to cdea34eCompare August 27, 2024 08:50

github-actionsBot added the documentation Improvements or additions to documentation label Aug 27, 2024

findepi changed the title ~~Remove Expr::Sort~~Remove Sort expression (Expr::Sort)Aug 27, 2024

findepi mentioned this pull request Aug 27, 2024
Remove Alias from Expr#1468
Open

findepi force-pushed the findepi/sort-is-not-expression branch from cdea34e to 1c3f194Compare August 27, 2024 13:28

findepi marked this pull request as ready for review August 27, 2024 13:31

crepererum approved these changes Aug 27, 2024
View reviewed changes

findepi added 4 commits August 27, 2024 22:07

Take Sort (SortExpr) in file options
fa3604e
Part of effort to remove `Expr::Sort`.

Return Sort from Expr.Sort
48578d3
Part of effort to remove `Expr::Sort`.

Accept Sort (SortExpr) in LogicalPlanBuilder.sort
e1e7814
Take `expr::Sort` in `LogicalPlanBuilder.sort`. Accept any `Expr` in new function, `LogicalPlanBuilder.sort_by` which apply default sort ordering. Part of effort to remove `Expr::Sort`.

Operate on Sort in to_substrait_sort_field / from_substrait_sorts
75d3db5
Part of effort to remove `Expr::Sort`.

findepi added 2 commits August 27, 2024 22:10

Take Sort (SortExpr) in tests' TopKPlanNode
57573ca
Part of effort to remove `Expr::Sort`.

findepi force-pushed the findepi/sort-is-not-expression branch from 1c3f194 to 9840b8fCompare August 27, 2024 20:11

jayzhan211 reviewed Aug 28, 2024
View reviewed changes

findepi added 2 commits August 28, 2024 09:24

use assert_eq just like in LogicalPlan.with_new_exprs
cc6e991

avoid clone in replace_sort_expressions
39877e4

findepi mentioned this pull request Aug 28, 2024
Remove Box from Sort #12207
Merged

findepi force-pushed the findepi/sort-is-not-expression branch from 2f0ea05 to 39877e4Compare August 28, 2024 07:26

reduce cloning in EliminateDuplicatedExpr
09b20d5

findepi added 2 commits August 28, 2024 11:48

restore SortExprWrapper
9931e1c
this commit is longer than advised in the review comment, but after squashing the diff will be smaller

shorthand SortExprWrapper struct definition
0ab9348

jayzhan211 approved these changes Aug 28, 2024
View reviewed changes

alamb added the api change Changes the API exposed to users of the crate label Aug 28, 2024

crepererum merged commit 85adb6c into apache:mainAug 29, 2024

findepi deleted the findepi/sort-is-not-expression branch August 29, 2024 15:46

alamb mentioned this pull request Sep 11, 2024
DataFusion weekly project plan (Andrew Lamb) - Sep 9, 2024 #12391
Closed
5 tasks

nathanielc mentioned this pull request Sep 18, 2024
feat: upgrade to datafusion 42 and arrow 53 datafusion-contrib/datafusion-federation#59
Merged

peter-toth mentioned this pull request Nov 5, 2024
Remove Expr clones from SortExprs #13258
Merged

evenyag mentioned this pull request Apr 2, 2025
Crash: with_new_exprs for Distinct does not support sort expressions GreptimeTeam/greptimedb#5799
Closed

	let asc = asc.unwrap_or(true);
	expr_vec.push(Expr::Sort(Sort::new(
	Box::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),

Uh oh!

Conversation

findepi commented Aug 26, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented Aug 26, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi commented Aug 27, 2024

Uh oh!

findepi commented Aug 27, 2024

Uh oh!

crepererum left a comment

Choose a reason for hiding this comment

Uh oh!

findepi commented Aug 27, 2024

Uh oh!

jayzhan211Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

findepiAug 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jayzhan211Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

findepiAug 28, 2024

Choose a reason for hiding this comment

Uh oh!

findepi commented Aug 28, 2024

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 28, 2024

Uh oh!

findepi commented Aug 29, 2024

Uh oh!

findepi commented Aug 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

findepi commented Aug 26, 2024•
edited
Loading