Skip to content

Use correct expression function after filter pushdown#17860

Merged
Mytherin merged 10 commits intoduckdb:v1.3-ossivalisfrom
Tmonster:use_correct_expression_function_after_filter_pushdown
Jun 24, 2025
Merged

Use correct expression function after filter pushdown#17860
Mytherin merged 10 commits intoduckdb:v1.3-ossivalisfrom
Tmonster:use_correct_expression_function_after_filter_pushdown

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

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
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

@@ -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
@Mytherin Mytherin merged commit 3cb90b8 into duckdb:v1.3-ossivalis Jun 24, 2025
52 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Jun 27, 2025
Use correct expression function after filter pushdown (duckdb/duckdb#17860)
Disable constexpr std::mutex on Windows (duckdb/duckdb#17991)
github-actions bot added a commit to duckdb/duckdb-r that referenced this pull request Jun 27, 2025
Use correct expression function after filter pushdown (duckdb/duckdb#17860)
Disable constexpr std::mutex on Windows (duckdb/duckdb#17991)

Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants