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

Handle nested projection with derived column optimization #8951

Closed
wants to merge 1 commit into from

Conversation

gruuya
Copy link
Contributor

@gruuya gruuya commented Jan 22, 2024

Which issue does this PR close?

Closes #8942.

Rationale for this change

Before discarding an outer projection that has the same output schema as the optimized inner projection, check for the equality of their expressions as well, and if these are different (e.g. having a non-trivial but aliased expression) keep the outer node.

What changes are included in this PR?

Changes to the OptimizeProjections logical optimizer.

Are these changes tested?

Yes, a test is added.

Are there any user-facing changes?

Fixes a regression from #8942.

mustafasrepo
mustafasrepo previously approved these changes Jan 23, 2024
Copy link
Contributor

@mustafasrepo mustafasrepo left a comment

Choose a reason for hiding this comment

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

LGTM!. Thanks for this fix

@mustafasrepo
Copy link
Contributor

@gruuya after some thinking through, I think best approach is your initial suggestion in the issue. I think, the only safe condition we can remove top projection is schema having same, and all expressions are trivial. And window test changes are another issue, that this fix reveals. And we should fix them separately. I filed the PR8960 with is_expr_trivial check per your original suggestion and with test changes.

I think safest way to proceed is that PR. What do you think?

@mustafasrepo mustafasrepo dismissed their stale review January 23, 2024 08:29

after some thinking through, we can proceed with a safer approach

@gruuya
Copy link
Contributor Author

gruuya commented Jan 23, 2024

I think safest way to proceed is that PR. What do you think?

Agreed, that makes sense; closing this one.

@gruuya gruuya closed this Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimizer Optimizer rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Logical optimizer causes invalid query result with case expression
2 participants