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

equivalence classes: use normalized mapping for projection #14327

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

askalt
Copy link
Contributor

@askalt askalt commented Jan 27, 2025

Which issue does this PR close?

Closes #14326.

Rationale for this change

See the issue above.

What changes are included in this PR?

Now projection mapping source expression is normalized before we lookup in the map.

Are these changes tested?

Yes, there is a unit test.

Are there any user-facing changes?

No

@github-actions github-actions bot added the physical-expr Physical Expressions label Jan 27, 2025
@askalt askalt force-pushed the askalt/fix-project-equivalence branch 2 times, most recently from 2f76a6a to 453fb30 Compare January 27, 2025 18:53
@berkaysynnada
Copy link
Contributor

Thank you @askalt. I'll review this in detail.

One initial suggestion: instead of adding a unit test and introducing additional testing components, I believe the same logic can be reproduced in an .slt test. You can assert the intended plan more effectively.

@askalt askalt force-pushed the askalt/fix-project-equivalence branch from 453fb30 to a777890 Compare January 28, 2025 17:56
@askalt
Copy link
Contributor Author

askalt commented Jan 28, 2025

Thank you @askalt. I'll review this in detail.

One initial suggestion: instead of adding a unit test and introducing additional testing components, I believe the same logic can be reproduced in an .slt test. You can assert the intended plan more effectively.

Thank you!
I checked .slt tests, but I don't sure that for now we can assert plan properties like equivalence classes. If I overlooked it, please correct me.

@ozankabak
Copy link
Contributor

I am guessing what is meant was writing an SLT test whose output plan would change if this logic/fix weren't there. Maybe an optimization that wouldn't take place because of the normalization is not done properly.

@askalt askalt force-pushed the askalt/fix-project-equivalence branch from a777890 to 13265a1 Compare January 29, 2025 19:04
@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 29, 2025
This patch fixes the logic that projects equivalence classes:
when run over the projection mapping to find new equivalent expressions,
we need to normalize a source expression.
@askalt askalt force-pushed the askalt/fix-project-equivalence branch from 13265a1 to ff1c139 Compare January 29, 2025 19:06
@askalt
Copy link
Contributor Author

askalt commented Jan 29, 2025

I added a test to .slt. It was slightly tricky because setting up a table with existing equivalence classes (e.g., a being an alias for b) is not very straightforward. I took advantage of the fact that the projection [x, y] with the filter x == y generates a new equivalence class.

Additionally, I left a small unit test in class.rs. It doesn’t introduce new logic with some Execs, but it might be useful for understanding what's happening during debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Equivalence class projection does not find new equivalent classes correctly
3 participants