Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add alias check to optimize projections merge #8438

Merged
merged 3 commits into from
Dec 7, 2023
Merged

Add alias check to optimize projections merge #8438

merged 3 commits into from
Dec 7, 2023

Conversation

mustafasrepo
Copy link
Contributor

@mustafasrepo mustafasrepo commented Dec 6, 2023

Which issue does this PR close?

Closes #8432.

Rationale for this change

Currently, optimize_projection rule fails for the query in the issue. Because of schema difference before and after the rule.
This PR fixes this bug. To see more rationale see [#8432]

What changes are included in this PR?

To not change schema, we add alias when name changes during merging

Are these changes tested?

Yes

Are there any user-facing changes?

@github-actions github-actions bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Dec 6, 2023
@mustafasrepo mustafasrepo changed the title Relax schema check for optimize projections. Add alias check to optimize projections merge Dec 6, 2023
Copy link
Contributor

@haohuaijin haohuaijin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mustafasrepo , LGTM!
I also check the output column name, it's correct

❯ create table t(x bigint, y bigint) as values (1,2), (1,3);
0 rows in set. Query took 0.016 seconds.

❯ select z+1, y from (select x+1 as z, y from t) where y > 1;
+--------------+---+
| z + Int64(1) | y |
+--------------+---+
| 3            | 2 |
| 3            | 3 |
+--------------+---+
2 rows in set. Query took 0.009 seconds.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @mustafasrepo and @haohuaijin

This looks good to me -- @jackwener do you have a moment to double check this as well?

@mustafasrepo mustafasrepo merged commit 33fc110 into apache:main Dec 7, 2023
22 checks passed
appletreeisyellow pushed a commit to appletreeisyellow/datafusion that referenced this pull request Dec 15, 2023
* Relax schema check for optimize projections.

* Minor changes

* Update datafusion/optimizer/src/optimize_projections.rs

Co-authored-by: jakevin <[email protected]>

---------

Co-authored-by: jakevin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optimize_projections fail due to difference schemas
4 participants