feat: support inlist in LiteralGurantee for pruning #8654

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

datafusion/physical-expr/src/utils/guarantee.rs

@@ -334,21 +357,33 @@ impl<'a> ColOpLit<'a> {
            binary_expr.op(),
            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

datafusion/physical-expr/src/utils/guarantee.rs

                    let first_term = &terms[0];
                    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!

datafusion/physical-expr/src/utils/guarantee.rs

Co-authored-by: Huaijin <[email protected]>

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

datafusion/core/src/datasource/physical_plan/parquet/row_groups.rs

-        ]);
-        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

datafusion/core/src/datasource/physical_plan/parquet/row_groups.rs

-        ]);
-        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

datafusion/physical-expr/src/utils/guarantee.rs

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


This is really nice 👍

datafusion/physical-expr/src/utils/guarantee.rs

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

datafusion/physical-expr/src/utils/guarantee.rs

                    let first_term = &terms[0];
                    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

datafusion/physical-expr/src/utils/guarantee.rs

@@ -334,21 +357,33 @@ impl<'a> ColOpLit<'a> {
            binary_expr.op(),
            binary_expr.right().as_any(),
        );
-
+        let guarantee = match op {


datafusion/physical-expr/src/utils/guarantee.rs

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

datafusion/physical-expr/src/utils/guarantee.rs

+            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

datafusion/physical-expr/src/utils/guarantee.rs

+    }
+
+    #[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

datafusion/physical-expr/src/utils/guarantee.rs

+            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 invalid if !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

datafusion/physical-expr/src/utils/guarantee.rs

waynexia

This is great! Thanks 👍

datafusion/physical-expr/src/utils/guarantee.rs

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

datafusion/physical-expr/src/utils/guarantee.rs

@@ -120,7 +114,7 @@ impl LiteralGuarantee {
    /// expression is guaranteed to be `null` or `false`.
    ///
    /// # 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 <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

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

Thanks all❤️.

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

datafusion/physical-expr/src/utils/guarantee.rs

@@ -780,6 +779,21 @@ mod test {
                .and(col("b").eq(lit(3)).or(col("b").eq(lit(4)))),
            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-actions bot added the physical-expr Physical Expressions 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

datafusion/physical-expr/src/utils/guarantee.rs Outdated Show resolved Hide resolved

datafusion/physical-expr/src/utils/guarantee.rs Outdated Show resolved Hide resolved

datafusion/physical-expr/src/utils/guarantee.rs Outdated Show resolved Hide resolved

my-vegetable-has-exploded and 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 <[email protected]>

add tests in row_groups

e18ebba

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

github-actions bot 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-exploded and others added 2 commits December 28, 2023 13:12

Apply suggestions from code review

0e14604

Co-authored-by: Ruihang Xia <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

update comment & add more tests

d5a18c2

alamb approved these changes Dec 28, 2023

View reviewed changes

alamb merged commit 1737d49 into apache:main Dec 28, 2023
22 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support inlist in LiteralGurantee for pruning #8654

feat: support inlist in LiteralGurantee for pruning #8654

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

edited

Loading

my-vegetable-has-exploded Dec 26, 2023

alamb Dec 27, 2023

my-vegetable-has-exploded Dec 26, 2023

alamb Dec 27, 2023

alamb commented Dec 26, 2023 •

edited

Loading

haohuaijin left a comment

my-vegetable-has-exploded Dec 27, 2023

alamb Dec 27, 2023

alamb left a comment

alamb Dec 27, 2023

alamb Dec 27, 2023

alamb Dec 27, 2023

alamb Dec 27, 2023

alamb Dec 27, 2023

alamb Dec 27, 2023

alamb Dec 27, 2023

alamb Dec 27, 2023

my-vegetable-has-exploded Dec 28, 2023

alamb Dec 28, 2023 •

edited

Loading

haohuaijin Dec 29, 2023

alamb Dec 30, 2023

waynexia left a comment

waynexia Dec 27, 2023

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

alamb Dec 28, 2023

alamb commented Dec 28, 2023

domyway commented Dec 28, 2023

alamb commented Dec 28, 2023

feat: support inlist in LiteralGurantee for pruning #8654

feat: support inlist in LiteralGurantee for pruning #8654

Conversation

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

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 26, 2023 • edited Loading

haohuaijin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waynexia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

alamb commented Dec 28, 2023

domyway commented Dec 28, 2023

alamb commented Dec 28, 2023

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

edited

Loading

alamb commented Dec 26, 2023 •

edited

Loading

alamb Dec 28, 2023 •

edited

Loading