8000 Use correct expression function after filter pushdown by Tmonster · Pull Request #17860 · duckdb/duckdb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use correct expression function after filter pushdown #17860

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 10 commits into
base: v1.3-ossivalis
Choose a base branch
from
Open

Use correct expression function after filter pushdown #17860

wants to merge 10 commits into from

Conversation

Tmonster
Copy link
Contributor

Fixes https://github.com/duckdblabs/duckdb-internal/issues/5064

We would only perform statistics propagation on a copy of the filter. Statistics propagation sets the expression function, so we need to make sure we set the expression function back on the original expression function as well.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 10, 2025 09:23
@Tmonster Tmonster marked this pull request as ready for review June 10, 2025 09:23
@Mytherin
Copy link
Collaborator

Thanks for the PR!

The change location makes sense - but I wonder if we can't move the result expression back instead of copying the function? It seems that that is quite limited in the scenarios it would correctly support (e.g. what if the LIKE expression is inside another function).

@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 11, 2025 07:39
@Tmonster
Copy link
Contributor Author
Tmonster commented Jun 11, 2025 •
edited
Loading

Hmmm, I started working on it and am a bit stuck.
All expressions in the filter expression are translated from BoundReferenceExpression to BoundColumnReference Expressions when expr_filter.ToExpression(*column_ref); is called. This is so we can then use the stats that we have in the stats_binding. If we want to move the result expression back, we have to recreate the BoundReferenceExpressions, but that also means we will need to store the storage_t value for each bound reference expression.

Maybe we can transform the expression again via the column binding resolver?

@Tmonster
Copy link
Contributor Author

nvm, figured it out. The physical column index is the same for all bound refs, so we can just iterate through the table filter once to find it, do the transformation & statistics propagation, then create the BoundRefExpressions again with the same physical column index

@Tmonster Tmonster marked this pull request as ready for review June 11, 2025 09:15
@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 13, 2025 10:14
@Tmonster Tmonster marked this pull request as ready for review June 13, 2025 10:18
@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 13, 2025 11:04
@Tmonster Tmonster marked this pull request as ready for review June 13, 2025 11:04
Mytherin
Mytherin reviewed Jun 20, 2025
View reviewed changes
Copy link
Collaborator
@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good - one minor comment

benchmark/tpcds_plan_cost/queries/04.sql Outdated
@@ -0,0 +1,120 @@
WITH year_total AS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these intended to be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, must added it accidentally. will revert it

@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 20, 2025 08:17
@Mytherin Mytherin marked this pull request as ready for review June 20, 2025 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mytherin Mytherin Mytherin left review comments

Assignees
No one assigned
Labels
Ready To Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Tmonster @Mytherin
0