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 non-column expression equality tracking to filter exec #9819

Merged

Conversation

mustafasrepo
Copy link
Contributor

Which issue does this PR close?

Related to #9812.

Rationale for this change

In the issue #9812. @suremarc recognized that some of the expression in the equality predicate of the FilterExec is not considered by subsequent stages. since they are lost during analysis. This PR modifies FilterExec to add support for equality tracking between Physical Exprs (e.g not just between Columns).

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Mar 27, 2024
Copy link
Contributor

@suremarc suremarc left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. I thought about adding this in my PR but, not knowing this part of the codebase well, I wasn't sure if there would be any knock-on effects or bugs. I like the sqllogictest demonstration as well

Copy link
Contributor

@comphead comphead 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 @mustafasrepo
I was thinking to wrap

(&Arc<dyn PhysicalExpr>, &Arc<dyn PhysicalExpr>)
``` into separate type for readability, but I see it is used with and without lifetimes, not sure if its easily doable then

@mustafasrepo
Copy link
Contributor Author

lgtm thanks @mustafasrepo I was thinking to wrap

(&Arc<dyn PhysicalExpr>, &Arc<dyn PhysicalExpr>)
``` into separate type for readability, but I see it is used with and without lifetimes, not sure if its easily doable then

I sent a commit for this solution. I think it is better. Thanks for the suggestion.

@mustafasrepo mustafasrepo merged commit 7f497b3 into apache:main Mar 29, 2024
23 checks passed
Lordworms pushed a commit to Lordworms/arrow-datafusion that referenced this pull request Apr 1, 2024
* Add non-column expression equality tracking to filter exec

* Minor changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants