fix: make simplify_expressions use a single schema for resolution #6077
Conversation
github-actions
bot
added
logical-expr
|
cc @alamb (for when you get back from vacation) @jackwener I am also interested in your thoughts on this PR since you worked on #5208. |
| context.with_schema(schema.clone()) | ||
| }); | ||
| let schema = if plan.inputs().is_empty() { | ||
| // When predicates are pushed into a table scan, there needs to be |
There was a problem hiding this comment.
I think this schema should be empty (aka have no columns) rather than use the same plan schema. Logically the schema should be the columns that are available to evaluate expressions within the plan node
There was a problem hiding this comment.
If I made this so it used the empty schema whenever the plan node had no inputs, the test csv_query_group_by_and_having_and_where would fail, since it attempts to simplify predicates that are inlined into a scan:
https://github.com/apache/arrow-datafusion/blob/caa60337c7a57572d93d8bd3cbc18006aabe55e6/datafusion/expr/src/logical_plan/plan.rs#L1428-L1429
It seems to be that inlined scan filters are a bit of an exception in that they are evaluated on top of the scan itself.
For other kinds of node (e.g., Values) I think you're right though, so I pushed a commit that refines the logic a bit more to use the plan's schema only for table scans.
| context.with_schema(schema.clone()) | ||
| }); | ||
| let schema = if plan.inputs().is_empty() { | ||
| // When predicates are pushed into a table scan, there needs to be |
2 participants
Which issue does this PR close?
Closes #5996.
Rationale for this change
Fixing this will allow lots of expression simplification to succeed where it failed before.
In IOx we would like to be able to run the logical optimizer with
skip_failed_rulesset tofalse, since some of our custom rules are required to run and produce user-facing errors.When I ran IOx tests with
skip_failed_rulesset tofalse, I found issue #5996.What changes are included in this PR?
The issue is with the
SimplifyExpressionslogical rule. As it traverses the logical plan, it includes both the output schema for the current node and the merged input schema from the node's inputs.The inclusion of two schemas causes an issue when various expression simplifiers inquire about attributes of expressions, e.g., here, which was the case for #5996:
https://github.com/apache/arrow-datafusion/blob/011adc203e4a02a822a8f0008852b90fb7441264/datafusion/optimizer/src/simplify_expressions/context.rs#L130-L139
This PR refines the schema used when simplifying to just the node's merged input schema. There is just one exception to using the child schema: for table scans with inlined predicates, those expressions need to be evaluated against the scan's output schema.
Are these changes tested?
I added unit tests for both the issue with
power(f, 1)from the issue, and also for the case of an inlined table scan.As a baseline I ran all the unit tests on
mainwithskip_failed_rulesset to false. I found just two failures:sql::select::use_between_expression_in_select_querysql::subqueries::support_limit_subquery(Seems like there should be more, based on #4685? Maybe I am not running all of the tests)
When I run my branch wtih
skip_failed_rulesset tofalse. There was just one failure, which was already onmain:sql::subqueries::support_limit_subquerySo this PR also allows
use_between_expression_in_select_queryto pass when skipping failed rules. It was failing in a similar way as #5996.I also ensured that the tests mentioned in #5208 (a recent bug fix that also touches on what schemas to use) all pass on my branch with
skip_failed_rulesset tofalse:right_anti_filter_push_downright_semi_with_alias_filtercsv_query_group_by_and_having_and_where(has inlined table predicate)Are there any user-facing changes?
No.