fix(binder): ExprVisitor and ExprMutator should visit agg_call order_by and filter #5664
Conversation
| if select_exprs | ||
| .iter() | ||
| .chain(group_exprs.iter()) | ||
| .any(|e| e.has_table_function()) | ||
| { | ||
| return Err(ErrorCode::NotImplemented( | ||
| "Table functions in agg call or group by is not supported yet".to_string(), | ||
| 3814.into(), | ||
| ) | ||
| .into()); | ||
| } |
There was a problem hiding this comment.
This check was redundant and too restrictive:
- When table function is an input argument of agg call, or part of group by, it is already rejected by the
Projectnode builder. - When table function is a sibling of agg call, we are able to handle it.
Refer to planner tests in agg.yaml for examples of both cases above.
| @@ -72,13 +72,14 @@ | |||
| └─BatchValues { rows: [[]] } | |||
| - sql: | | |||
| select array_cat(array[233], array[array[array[66]]]); | |||
| binder_error: "Bind error: unable to find least restrictive type between integer[] and integer[][][]" | |||
| binder_error: 'Bind error: unable to find least restrictive type between integer[] | |||
There was a problem hiding this comment.
Styling differences generated by ./risedev do-apply-planner-test. Not sure why they were introduced by #5345
There was a problem hiding this comment.
I guess it's because styling difference won't make run-planner-test fail 😇
And the tests are hand-written
Codecov Report
@@ Coverage Diff @@
## main #5664 +/- ##
==========================================
- Coverage 74.32% 74.31% -0.02%
==========================================
Files 919 921 +2
Lines 143957 143990 +33
==========================================
+ Hits 106999 107000 +1
- Misses 36958 36990 +32
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -72,13 +72,14 @@ | |||
| └─BatchValues { rows: [[]] } | |||
| - sql: | | |||
| select array_cat(array[233], array[array[array[66]]]); | |||
| binder_error: "Bind error: unable to find least restrictive type between integer[] and integer[][][]" | |||
| binder_error: 'Bind error: unable to find least restrictive type between integer[] | |||
There was a problem hiding this comment.
I guess it's because styling difference won't make run-planner-test fail 😇
And the tests are hand-written
|
|
||
| pub fn order_by(&self) -> &OrderBy { | ||
| &self.order_by | ||
| } | ||
|
|
||
| pub fn order_by_mut(&mut self) -> &mut OrderBy { | ||
| &mut self.order_by | ||
| } | ||
|
|
||
| pub fn filter(&self) -> &Condition { | ||
| &self.filter | ||
| } | ||
|
|
||
| pub fn filter_mut(&mut self) -> &mut Condition { | ||
| &mut self.filter | ||
| } |
There was a problem hiding this comment.
I'm ponding whether it would be better to simply publicize all fields 🤪
There was a problem hiding this comment.
Was thinking the same, and an alternative to simply expose children_iter/children_mut to return a chained iterator over inputs + order_by + filter. Not sure which is better as of now.
Is there a test case/issue for that? |
Yes. It is the same issue but in comments #4762 (comment) . For this reason I am not marking it closed by this PR. |
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
As title. By missing these fields during iteration, correlated input refs were not handled and caused panic as mentioned in #4762.
Note that correlated input refs in the
filterclause are now correctly detected, but the optimizer rule is not handling it yet. Unlikeinputororder_by, subexpr infilterare not extracted to a childProjectnode, thus need special treatment in decorrelation rules. This shall be fixed separately.Checklist
./risedev check(or alias,./risedev c)Refer to a related PR or issue link (optional)