-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: skip predicates on struct unnest in PushDownFilter #16790
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
base: main
Are you sure you want to change the base?
fix: skip predicates on struct unnest in PushDownFilter #16790
Conversation
@adriangb is there any chance you have time to review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that we should not be pushing down filters that touch unnested columns. This seems to achieve that goal.
CREATE TABLE d AS VALUES (named_struct('a', 1, 'b', 2)), (named_struct('a', 3, 'b', 4)), (named_struct('a', 5, 'b', 6)); | ||
|
||
query II | ||
select * from (select unnest(column1) from d) where "__unnest_placeholder(d.column1).b" > 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akoshchiy is there documentation anywhere on what "__unnest_placeholder" is? Is it a private implementation detail we are using to test, or something that users could / should use in their queries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I can see, it's a result of struct unnest preprocessing in SqlToRel::try_process_unnest
. unnest(struct_col)
will be transformed into __unnest_placeholder(struct_col).field1, __unnest_placeholder(struct_col).field2
etc.
To be honest, I'm not sure, that it was supposed to be like that or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a little more docs / context / comments are needed otherwise this is good to merge.
@akoshchiy could you add some comments explaining what's going on for future reference, and documenting what the |
I've added some comments to the docs. btw, I've checked behaviour on duckdb, and it looks more clearly - there is no prefixes at all. Maybe we can do the same?
|
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Added new test case in
push_down_filter.slt
Are there any user-facing changes?
No.