feat(frontend): order by arbitrary expr #2329
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2329 +/- ##
==========================================
- Coverage 71.07% 71.07% -0.01%
==========================================
Files 675 675
Lines 84502 84612 +110
==========================================
+ Hits 60061 60137 +76
- Misses 24441 24475 +34
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
xiangjinwu
changed the title
src/frontend/src/binder/query.rs
Outdated
| @@ -30,6 +31,7 @@ pub struct BoundQuery { | |||
| pub order: Vec<FieldOrder>, | |||
| pub limit: Option<usize>, | |||
| pub offset: Option<usize>, | |||
| pub order_exprs: Vec<ExprImpl>, | |||
There was a problem hiding this comment.
extra_order_exprs- Maybe we can put this on
BoundSelectrather thanBoundQuery, because this feature only works whenbodymatchesBoundSetExpr::Select. They cannot be used withVALUES,UNION, or evenSELECT DISTINCT.
Side note: as of now we push and pop context in bind_query. But in select a1 from a union select b1 from b, the context should actually be different for each select. We do not need to fix in this PR, but this is why we cannot bind order by a1 here - it is only available in the context of the select.
There was a problem hiding this comment.
I orginally wanted to put it in BoundSelect, but ORDER BY clause is bound after BoundSelect was generated and because BoundSetExpr is an enum, it is difficult to put extra_order_exprs into BoundSelect.
There was a problem hiding this comment.
It is not difficult as matching enum also helps reject union here.
However, after rethinking, I agree that putting extra_order_exprs at the same level of order and out_fields is better. It can be confusing that each BoundSelect has extra_order_exprs, which is only meaningful when it has no siblings.
As for rejecting union as well as using the correct context when binding extra_order_exprs, the same idea can be extended from values: (1) Without union, the query and its body select share the same context (current behavior); (2) With union, each descendant select will use their own context, leaving the context of query empty, and extra_order_exprs would return bind error as expected.
src/frontend/src/binder/query.rs
Outdated
| @@ -30,6 +31,7 @@ pub struct BoundQuery { | |||
| pub order: Vec<FieldOrder>, | |||
| pub limit: Option<usize>, | |||
| pub offset: Option<usize>, | |||
| pub order_exprs: Vec<ExprImpl>, | |||
There was a problem hiding this comment.
It is not difficult as matching enum also helps reject union here.
However, after rethinking, I agree that putting extra_order_exprs at the same level of order and out_fields is better. It can be confusing that each BoundSelect has extra_order_exprs, which is only meaningful when it has no siblings.
As for rejecting union as well as using the correct context when binding extra_order_exprs, the same idea can be extended from values: (1) Without union, the query and its body select share the same context (current behavior); (2) With union, each descendant select will use their own context, leaving the context of query empty, and extra_order_exprs would return bind error as expected.
What's changed and what's your intention?
PLEASE DO NOT LEAVE THIS EMPTY !!!
Please explain IN DETAIL what the changes are in this PR and why they are needed:
This pr solve frontend: Support
ORDER BYarbitrary column #2124, so we can order by number of output column, column label and any expression that is valid in select list now. It uses BatchProject for Batch and out_fields for stream to do the projection. Some part of the pr may not be good enough, and I'm open for any advice. Thx!Checklist
Refer to a related PR or issue link (optional)
close #2124 #1635