Fix: common_sub_expression_eliminate optimizer rule failed #16066
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Common_sub_expression_eliminate rule failed with error:
`SchemaError(FieldNotFound {field: <name>}, valid_fields: []})`
due to the schema being changed by the second application of
`find_common_exprs`
As I understood the source of the problem was in sequential call of
`find_common_exprs`. First call returned original names as `aggr_expr`
and changed names as `new_aggr_expr`. Second call takes into account
only `new_aggr_expr` and if names was already changed by first call
will return changed names as `aggr_expr`(original ones)
and put them into Projection logic.
I used NamePreserver mechanism to restore original schema names and
generate Projection with original name at the end of aggregate
optimization.
github-actions
bot
added
optimizer
alamb
reviewed
May 16, 2025
| .map(|expr| Some(name_preserver.save(expr))) | ||
| .collect::<Vec<_>>() | ||
| } else { | ||
| new_aggr_expr |
There was a problem hiding this comment.
Is this the same as a vec of all None?
vec![None; new_agg_expr.len()]`There was a problem hiding this comment.
Yes it is, thanks for the comment. But this will not work, because vec! requires value to implement Clone trait but the Option<SavedName> doesn't. It seems to me easier to do this by map instead of adding trait to SavedName.
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
|
Hi @alamb, thanks again for your comment, please have a look on this. Please let me know if this problem was already solved or move my commit out of stale label. |
alamb
reviewed
Jul 28, 2025
| (2.0, 20, -5), | ||
| (3.0, 20, 4); | ||
|
|
||
| # https://github.com/apache/datafusion/issues/15291 |
There was a problem hiding this comment.
for some reason this query is failing for me
> WITH s AS (
SELECT
COUNT(a) FILTER (WHERE (b * b) - 3600 <= b),
COUNT(a) FILTER (WHERE (b * b) - 3000 <= b AND (c >= 0)),
COUNT(a) FILTER (WHERE (b * b) - 3000 <= b AND (c >= 0) AND (c >= 0))
FROM t
) SELECT * FROM s; 🤔 Invalid statement: SQL error: ParserError("Expected: ), found: ( at Line: 3, Column: 25")
There was a problem hiding this comment.
Nm, I can get it to reproduce like this
> set datafusion.sql_parser.dialect = 'postgres';
0 row(s) fetched.
Elapsed 0.000 seconds.
> CREATE TABLE t (
a DOUBLE,
b BIGINT,
c INT
) AS VALUES
(1.0, 10, -5),
(2.0, 20, -5),
(3.0, 20, 4);
0 row(s) fetched.
Elapsed 0.002 seconds.
> WITH s AS (
SELECT
COUNT(a) FILTER (WHERE (b * b) - 3600 <= b),
COUNT(a) FILTER (WHERE (b * b) - 3000 <= b AND (c >= 0)),
COUNT(a) FILTER (WHERE (b * b) - 3000 <= b AND (c >= 0) AND (c >= 0))
FROM t
) SELECT * FROM s
;
Optimizer rule 'common_sub_expression_eliminate' failed
caused by
Schema error: No field named "count(t.a) FILTER (WHERE t.b * t.b - Int64(3600) <= t.b)". Did you mean 'count(t.a) FILTER (WHERE __common_expr_1 AS t.b * t.b - Int64(3600) <= t.b)'?.
|
I am not sure, I merged up to main and started the CI again. |
|
Hi, @alamb, just a kind reminder, if all checks have passed and my test reproduces the issue maybe we can merge the changes or there are still any problems with my fix? |
|
Sorry for the delay -- I lost track of this PR. I merged up again from main and I will give it a more thorough review tomorrow |
|
🤖 |
|
🤖: Benchmark completed Details
|
Jefffrey
approved these changes
Sep 21, 2025
There was a problem hiding this comment.
Looks in line with other parts of CSE:
datafusion/datafusion/optimizer/src/common_subexpr_eliminate.rs
Lines 374 to 396 in 602475f
datafusion/datafusion/optimizer/src/common_subexpr_eliminate.rs
Lines 185 to 203 in 602475f
So I think this is good to merge; perhaps copy those comments above in with this fix for visibility.
(btw what does find_common_exprs() refer to now; I can't grep that function 🤔 )
|
I merged up from main and plan to merge this PR once Ci passes. Thank you for your patience @Col-Waltz and for the revie @Jefffrey |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Common_sub_expression_eliminate rule failed with error:
SchemaError(FieldNotFound {field: <name>}, valid_fields: []})due to the schema being changed by the second application of
find_common_exprsAs I understood the source of the problem was in sequential call of
find_common_exprs. First call returned original names asaggr_exprand changed names as
new_aggr_expr. Second call takes into accountonly
new_aggr_exprand if names was already changed by first callwill return changed names as
aggr_expr(original ones)and put them into Projection logic.
What changes are included in this PR?
I used existing NamePreserver mechanism to restore original schema names and
generate Projection with original name at the end of aggregate optimization.
Are these changes tested?
Yes this changes are tested and I added test to this commit.
Error emerges not only in PREPARE requests, I created SELECT request which
causes the same error.
Are there any user-facing changes?
No the change only fixes optimization rule.