bugfix: fix eval nullalbe() in simplify_exprs
#5208
Conversation
jackwener
changed the title
nullalbe() in simplify_exprs`nullalbe() in simplify_exprs
jackwener
force-pushed
the
fix_simplify
branch
from
8178c24
to
f76e667
Compare
There was a problem hiding this comment.
Thank you @jackwener
Is there some test we could write that would show the effect of this change?
| // Pass down the `children merge schema` and `plan schema` to evaluate expression types. | ||
| // pass all `child schema` and `plan schema` isn't enough, because like `t1 semi join t2 on | ||
| // on t1.id = t2.id`, each individual schema can't contain all the columns in it. | ||
| let children_merge_schema = DFSchemaRef::new(merge_schema(plan.inputs())); |
There was a problem hiding this comment.
In some ways, it seems to me that we should only be using the children's schemas as the expressions within he LogicalPlan should be in terms of the plan's inputs (the children's schemas) not the plan's output (plan.schema())
There was a problem hiding this comment.
it seems to me that we should only be using the children's schemas.
I can't agree with it more.
But current code get attribution of some Expression by get it from plan.schema() instead of inferring/computing them from children ouput.
For example, some test will fail like csv_query_group_by_and_having_and_where.
There was a problem hiding this comment.
Something else to work on over time, perhaps
current tests can cover this problem, but it was hidden by set Before this PR, I'm working on #4685 and try to fix all bug in it. |
There was a problem hiding this comment.
current tests can cover this problem, but it was hidden by skip_failed_rules.
👍
Sounds good. Thank you @jackwener
|
Benchmark runs are scheduled for baseline = e222bd6 and contender = f0c6719. f0c6719 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #5191.
Rationale for this change
What changes are included in this PR?
Remove use
all_schemas()but use the plan schema + children_merge_schema().set
pub skip_failed_rules: bool, default = falseBefore this PR,
right_anti_filter_push_downandright_semi_with_alias_filterwill fail.After this PR, they will success.
Are these changes tested?
Current test can cover it (we just need to disable skip_fail)
Are there any user-facing changes?