datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

Remove Sort expression (`Expr::Sort`)

Open findepi opened this issue 1 year ago • 6 comments

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 https://github.com/apache/datafusion/issues/1468#issuecomment-2308275476 Fixes https://github.com/apache/datafusion/issues/12193

findepi avatar Aug 26 '24 15:08 findepi

Not ready for review. Just for FYI.

findepi avatar Aug 26 '24 15:08 findepi

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

findepi avatar Aug 27 '24 09:08 findepi

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

findepi avatar Aug 27 '24 13:08 findepi

there is a new conflict (perhaps due to https://github.com/apache/datafusion/pull/12196), let me rebase.

findepi avatar Aug 27 '24 20:08 findepi

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

findepi avatar Aug 28 '24 07:08 findepi

EPIC!

alamb avatar Aug 28 '24 12:08 alamb

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

findepi avatar Aug 29 '24 10:08 findepi

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

findepi avatar Aug 29 '24 15:08 findepi