feat: support inlist in LiteralGurantee for pruning by my-vegetable-has-exploded · Pull Request #8654 · apache/datafusion

my-vegetable-has-exploded · 2023-12-26T10:36:52Z

Which issue does this PR close?

Closes#8436

Rationale for this change

What changes are included in this PR?

refactor some codes
Adding InList support in LiteralGurantee code
Add tests in LiteralGurantee
Add a test for Bloom filters in row_groups.rs

Are these changes tested?

Are there any user-facing changes?

my-vegetable-has-exploded · 2023-12-26T10:54:42Z

 binary_expr.right().as_any(),
 );
-
+ let guarantee = match op {


Since operator except = and != would be evaluate in predicate_expr, I put the conversion process here.

my-vegetable-has-exploded · 2023-12-26T11:03:46Z

 if terms.iter().all(|term| {
 term.col.name() == first_term.col.name()
- && term.op == first_term.op
+ && term.guarantee == Guarantee::In


a != foo OR a != bar would be filtered out here, So we don't need to check new_values.len() == 1 in aggregate_multi_conjunct. If not, new_values.len() == 1 will confilct with the LiteralGuarantee result from InList who may have multi literals. And I think refactored code is easier to follow for me.

I also double checked that this will verify that first_term.guarantee needs to be In as well ✅

alamb · 2023-12-26T13:42:23Z

Thank you @my-vegetable-has-exploded -- I plan to review this tomorrow.

cc @waynexia and @haohuaijin

haohuaijin

Thank you @my-vegetable-has-exploded , LGTM!

Co-authored-by: Huaijin <haohuaijin@gmail.com>

my-vegetable-has-exploded · 2023-12-27T09:05:20Z

- ]);
- let sql =
- "SELECT * FROM tbl WHERE \"String\" IN ('Hello_Not_Exists', 'Hello_Not_Exists2')";
- let expr = sql_to_physical_plan(sql).unwrap();


This previous test relied on the optimizer who will convert in_list expr with no more 3 elements to and expr. Now we don't need this. logical2physical would convert logical_expr to physical_expr without optimizer.

I agree -- this change makes sense
BTW I tried to improve these tests to avoid duplication in #8435 (not yet merged). I'll try and consolidate this test too when I get a chance

alamb

I reviewed this PR carefully -- thank you @my-vegetable-has-exploded (and @haohuaijin for the review). This PR looks very nice 👌

I left some small suggestions but nothing I think is required -- we could make them as a follow on PR as well

alamb · 2023-12-27T12:09:29Z

- ]);
- let sql =
- "SELECT * FROM tbl WHERE \"String\" IN ('Hello_Not_Exists', 'Hello_Not_Exists2')";
- let expr = sql_to_physical_plan(sql).unwrap();


I agree -- this change makes sense
BTW I tried to improve these tests to avoid duplication in #8435 (not yet merged). I'll try and consolidate this test too when I get a chance

alamb · 2023-12-27T12:11:03Z

+ .as_any()
+ .downcast_ref::<crate::expressions::InListExpr>()
+ {
+ //Only support single-column inlist currently, multi-column inlist is not supported


This is really nice 👍

alamb · 2023-12-27T12:15:32Z

 if terms.iter().all(|term| {
 term.col.name() == first_term.col.name()
- && term.op == first_term.op
+ && term.guarantee == Guarantee::In


I also double checked that this will verify that first_term.guarantee needs to be In as well ✅

alamb · 2023-12-27T12:16:31Z

 binary_expr.right().as_any(),
 );
-
+ let guarantee = match op {


alamb · 2023-12-27T12:18:00Z

+ col("b")
+ .in_list(vec![lit(1), lit(2), lit(3)], false)
+ .and(col("b").in_list(vec![lit(2), lit(3), lit(4)], false)),
+ vec![in_guarantee("b", [2, 3])],


this is very cool

This type of simplification might also be valuable to do in the Expr simplifier code too (so that other optimizer passes could see simpler predicates).
However, I am not sure how common such predicates are 🤔

alamb · 2023-12-27T12:23:17Z

+ }
+
+ #[test]
+ fn test_inlist_with_disjunction() {


Could you also please add (negative) tests for the inlist directly used with OR too ?
For example:
b IN (1, 2, 3) OR b = 2 b IN (1, 2, 3) OR b != 3
I think the code above will work correctly, but it would be nice to verify

alamb · 2023-12-27T12:25:22Z

+ col("b")
+ .in_list(vec![lit(1), lit(2), lit(3)], false)
+ .and(col("b").eq(lit(4)).or(col("b").eq(lit(5)))),
+ vec![],


I theory this could be in_guarantee(1,2,3) right? As it can only be true when b is 1,2,3.
However in this case this expression can never be true so maybe it doesn't really matter 🤔

This would be invalid since intersection between [1,2,3] and [4,5] is empty.
let intersection = new_values .into_iter().filter(|new_value| existing.literals.contains(*new_value)).collect::<Vec<_>>();// for an In guarantee, if the intersection is not empty, we can extend the guarantee// e.g. `a IN (1,2,3) AND a IN (2,3,4)` is `a IN (2,3)`// otherwise, we invalidate the guarantee// e.g. `a IN (1,2,3) AND a IN (4,5,6)` is `a IN ()`, which is invalidif !intersection.is_empty(){ existing.literals = intersection.into_iter().cloned().collect();}else{// at least one was not, so invalidate the guarantee*entry = None;}
BTW, I left a comment in #8437. Please cc when you are free.

This would be invalid since intersection between [1,2,3] and [4,5] is empty.
Right -- I wonder if in general for cases where the intersection is empty, can we infer that the expression can not be true ever 🤔 (there is no way to represent this today in the guarantee framework, maybe something like
enum Guarantee { /// This predicate can not be true CanNotBeTrue, /// This predicate can only be true if all the `LiteralGurantee`s are valie List(Vec<LiteralGurantee>) }
And then return Gurantee::CanNotBeTrue if we discover that the expression can not be true 🤔
BTW, I left a comment in #8437. Please cc when you are free.
I am not quite sure what you are referring to. #8437 doesn't seem to have had any recent activity that I can see.

Maybe we can enhance eliminate_filter optimizer rule to eliminate the filters(in where clause) that always not be true and replace by a EmptyRelation.
like we do for where false in
https://github.com/apache/arrow-datafusion/blob/8284371cb5dbeb5d0b1d50c420affb9be86b1599/datafusion/optimizer/src/eliminate_filter.rs#L55-L62

I filed this idea in #8687

waynexia

This is great! Thanks 👍

waynexia · 2023-12-27T15:21:29Z

 ///
 /// # Notes:
- /// 1. `expr` must be a boolean expression.
+ /// 1. `expr` must be a boolean expression or inlist expression.


More specifically, the boolean expr can only be in a form that can be transformed into a ColOpLit

Co-authored-by: Ruihang Xia <waynestxia@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

my-vegetable-has-exploded · 2023-12-28T05:43:17Z

Thanks all❤️.

alamb · 2023-12-28T12:12:35Z

 vec![not_in_guarantee("b", [1, 2, 3]), in_guarantee("b", [3, 4])],
 );
+ // b IN (1, 2, 3) OR b = 2
+ // TODO this should be in_guarantee("b", [1, 2, 3]) but currently we don't support to anylize this kind of disjunction. Only `ColOpLit OR ColOpLit` is supported.


alamb · 2023-12-28T12:12:53Z

Thanks again @my-vegetable-has-exploded

domyway · 2023-12-28T13:46:02Z

if items more than 10, it still cant use bloom filter

alamb · 2023-12-28T19:38:40Z

if items more than 10, it still cant use bloom filter

In case anyone is following along, the discussion about this is at #8436 (comment)

github-actionsBot added the physical-expr Changes to the physical-expr crates label Dec 26, 2023

my-vegetable-has-exploded commented Dec 26, 2023
View reviewed changes

alamb mentioned this pull request Dec 26, 2023
DataFusion weekly project plan (Andrew Lamb) - Dec 25, 2023 #8655
Closed
7 tasks

haohuaijin approved these changes Dec 27, 2023
View reviewed changes

Comment threaddatafusion/physical-expr/src/utils/guarantee.rs Outdated
Comment threaddatafusion/physical-expr/src/utils/guarantee.rs Outdated
Comment threaddatafusion/physical-expr/src/utils/guarantee.rs Outdated

my-vegetable-has-explodedand others added 5 commits December 27, 2023 16:03

support inlist in LiteralGuarantee for pruning.
3e8c22b

add more tests
0c403d9

rm useless notes
d37574e

Apply suggestions from code review
c0b839c
Co-authored-by: Huaijin <haohuaijin@gmail.com>

add tests in row_groups
e18ebba

my-vegetable-has-exploded force-pushed the in-guarantee branch from 9baab8d to e18ebbaCompare December 27, 2023 08:55

github-actionsBot added the core Core DataFusion crate label Dec 27, 2023

my-vegetable-has-exploded commented Dec 27, 2023
View reviewed changes

alamb approved these changes Dec 27, 2023
View reviewed changes

waynexia approved these changes Dec 27, 2023
View reviewed changes

my-vegetable-has-explodedand others added 2 commits December 28, 2023 13:12

Apply suggestions from code review
0e14604
Co-authored-by: Ruihang Xia <waynestxia@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

update comment & add more tests
d5a18c2

alamb approved these changes Dec 28, 2023
View reviewed changes

alamb merged commit 1737d49 into apache:mainDec 28, 2023

my-vegetable-has-exploded deleted the in-guarantee branch December 28, 2023 12:31

alamb mentioned this pull request Dec 30, 2023
Remove / simplify "impossible" predicates #8687
Closed

matthewgapp mentioned this pull request Jan 11, 2024
matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3
Closed

Uh oh!

Conversation

my-vegetable-has-exploded commented Dec 26, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Dec 26, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haohuaijin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alambDec 28, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

waynexia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

my-vegetable-has-exploded commented Dec 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Dec 28, 2023

Uh oh!

domyway commented Dec 28, 2023

Uh oh!

alamb commented Dec 28, 2023

Uh oh!

Reviewers

my-vegetable-has-exploded commented Dec 26, 2023•
edited
Loading

alamb commented Dec 26, 2023•
edited
Loading

alambDec 28, 2023•
edited
Loading