-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Push down filter for Unnest plan #10974
Conversation
Signed-off-by: jayzhan211 <[email protected]>
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.
Thank you @jayzhan211 -- very nice. I had a suggestion for improving the docs, and I think we should add a few more test cases, but otherwise I think this is ready to go
statement ok | ||
CREATE TABLE IF NOT EXISTS v AS VALUES(1,[1,2,3]),(2,[3,4,5]); | ||
|
||
# filter plan is pushed down into projection plan |
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.
Can you please also add a negative test with a predicate on uc2
? Something like
explain select uc2 from (select unnest(column2) as uc2, column1 from v) where uc2 > 3;
it might also be good to add tests to actually run those 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.
We hadn't fix the comparison for unnest yet
query error DataFusion error: External error: Arrow error: Invalid argument error: Invalid comparison operation: List\(Field \{ name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\) > Int64
select uc2 from (select unnest(column2) as uc2, column1 from v) where uc2 > 3;
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 will leave the query in slt, and fix it in the follow up PR
@@ -951,6 +939,54 @@ impl OptimizerRule for PushDownFilter { | |||
} | |||
} | |||
|
|||
/// return transformed projection plan and optinal filter's predicate |
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.
This is a nice refactor. While reading it I figured it might also be nice to improve the documenatation
/// return transformed projection plan and optinal filter's predicate | |
/// Attempts to push `predicate` into a `FilterExec` below `projection | |
/// | |
/// # Returns | |
/// (plan, remaining_predicate) | |
/// | |
/// `plan` is a LogicalPlan for `projection` with possibly a new FilterExec below it. | |
/// `remaining_predicate` is any part of the predicate that could not be pushed down | |
/// | |
/// # Example | |
/// | |
/// Pushing a predicate like `foo=5 AND bar=6` with an input plan like this: | |
/// | |
/// ```text | |
/// Projection(foo, c+d as bar) | |
/// ``` | |
/// | |
/// Might result in returning `remaining_predicate` of `bar=6` and a plan like | |
/// | |
/// ```text | |
/// Projection(foo, c+d as bar) | |
/// Filter(foo=5) | |
/// ... | |
/// ``` | |
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
* add unnset Signed-off-by: jayzhan211 <[email protected]> * improve doc and tset Signed-off-by: jayzhan211 <[email protected]> * fix test Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]>
Which issue does this PR close?
Closes #10935.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?