Add `LiteralGuarantee` on columns to extract conditions required for `PhysicalExpr` expressions to evaluate to true #8437

alamb · 2023-12-06T12:44:40Z

Which issue does this PR close?

Part of #8376 (see the POC PR #8397 for how this all fits together)

Broken out for easier PR review. I will update the bloom filter code to actually use this as a follow on PR

Rationale for this change

I am generalizing pruning a set of files/row_groups/containers based on information known before looking at the data (aka "pruning predicates") to support bloom filters and other similar structures which can tell if a given value is present/present in a given container (see #8376).

Part of this analysis requires identifying if a given predicate ensures that a given column must take a a literal value (aka a constant) or set of values. Given this is an important building block and non trivial analysis, I wanted to make it a first class concept in DataFusion.

Example of what the code in this PR can do

This PR adds the generic analysis of what, if any, single column equality guarantees of a boolean Expr must hold true for the Expr itself to hold true.

For example, for the filter expression (host = 'host1' OR host = 'host2') AND region != 'us-east-1' AND ... it can programmatically extract the intuition that the predicate can not be true if

host is not one of ('host1', 'host2')
region is 'us-east-1'

What changes are included in this PR?

refactored out the code in the BloomFilterPruningPredicate originally written by @haohuaijin into its own struct
Made it more general
Added many tests

Are these changes tested?

Yes, this code is mostly new tests

Are there any user-facing changes?

Not yet

alamb · 2023-12-06T12:47:41Z

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

+/// AND a != 2)` or `a NOT IN (1, 2, 3)`
+///
+#[derive(Debug, Clone, PartialEq)]
+pub struct LiteralGuarantee {


This is a generalization of the code in the BloomFilterPruningPredicate

waynexia

The implementation and tests are very concrete 👍

waynexia · 2023-12-12T08:07:21Z

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

+            left.downcast_ref::<crate::expressions::Column>(),
+            right.downcast_ref::<crate::expressions::Literal>(),
+        ) {
+            Some(Self { col, op, lit })


Should we call Operator::swap() as the operator is reversed? Although only Eq and NotEq will occur here.

Yes, good call -- done in af13798

waynexia · 2023-12-12T09:02:19Z

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

+                    // if not all terms are of the form (col <op> literal),
+                    // can't infer anything
+                    if terms.len() != disjunctions.len() {
+                        return builder;
+                    }


Why is this check necessary? We can infer the guarantee for expr like below, as dropping a part of OR-chain the condition still holds

... AND ( a != 1 OR b > 1 OR a != 2 )

That is an excellent point @waynexia. In fact I think this logic could potentially be improved even more

For example a = 1 OR a != 2 could actually provide two guarantees that could prove the predicate was false

a != 1

a = 2

I thought a lot about this last night and this morning -- at the core what this code is trying to do is help prove an expression can not be true (so rows can be filtered)

To do so, it is trying to provide one of the following guarantees:

For the expression to be true, a column can ONLY be one of a set of constants

For the expression to be true a column can NOT BE ANY of a set of constants

For the expression

a != 1 OR b > 1 OR a != 2

We can't prove that the expression is true ONLY if a is not in (1, 2) -- it could be true if b was greater than 1 as well.

This subtelty was not clear from the comments so I have:

Updated the comments

Added more tests that illustrate this type of situation

I am pretty sure the new tests have uncovered a bug which I am working on fixing .

Thanks again for raising this point

I plan to look at this tomorrow closely

Thank k you -- I plan to have updated the code again by that time

alamb · 2023-12-12T20:47:59Z

Thank you @waynexia -- your comments are very helpful. I am going to incorporate your thoughts and suggestions and will reopen this PR for review when ready

alamb · 2023-12-14T20:04:04Z

BTW this PR is not ready for re-review yet. I will try and get it ready as soon as possible but I fear I will run out of time today

alamb · 2023-12-15T14:08:59Z

Ok, I have updated the documentation and tests more and I think this is ready for another review

NGA-TRAN

I have to admit that this is a very tricky PR because of the guarantee reasoning. Thanks @alamb for all the detailed comments and tests. I have gone over all the tests carefully. Very nice and great cover. I just have a question for one test and suggest to add one more test

NGA-TRAN · 2023-12-15T15:36:43Z

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

+/// 2. The column must NOT be one of a particular set of values for the predicate to
+/// be `true`. If the column takes on any of the values, the predicate can not
+/// evaluate to `true`. For example,
+/// `(a != 1)`, `(a != 1 AND a != 2)` or `a NOT IN (1, 2, 3)`


The detailed comments help to review the code 👍

This helps a lot 👍 The guarantee is similar to bloom filter: it might be false-positive, but negative result is reliable.

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

NGA-TRAN · 2023-12-15T16:20:36Z

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

+        test_analyze(
+            col("b")
+                .not_eq(lit(1))
+                .and(col("b").gt(lit(2)).or(col("b").eq(lit(3)))),


Do you want to test one in the above comment // b != 1 AND (b > 2)

Suggested change

.and(col("b").gt(lit(2)).or(col("b").eq(lit(3)))),

.and(col("b").gt(lit(2))),

Or // b != 1 AND (b > 2 OR b = 3)?

Yes, good catch -- I have updated it

NGA-TRAN · 2023-12-15T16:34:05Z

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

+                .and(col("b").gt(lit(2)).or(col("b").eq(lit(3)))),
+            vec![
+                // for the expression to be true, b can not be one
+                not_in_guarantee("b", [1]),


I have to admit that I need to think hard how Bloom filter works to review these. I stared at this for a while and this is what I think. I may misunderstand it though

If the test is b !=1 and b > 2, what is the right result here? I think it should be an empty vector, right?
My reasoning is if b = 0, then the not_in_guarantee("b", [1]) is true but the expression b !=1 and b > 2 is false.

I think it is the same for b != 1 AND (b > 2 OR b = 3)

The idea for using this for bloom filters is to try and prove that this expression is not true:

b != 1 AND (b > 2)

In order for the expression to be true, both of these have to hold

`b != 1

b > 2

Thus, if we can prove either does not hold, we can prove the expression is not true

So therefore, to prove the expression can't possibly be true, we need to prove that b ONLY takes the value of 1.

But as you point out this is not what the documentation says at the moment

/// 2. The column must NOT be one of a particular set of values for the predicate to /// be `true`. If the column takes on any of the values, the predicate can not /// evaluate to `true`. For example, /// `(a != 1)`, `(a != 1 AND a != 2)` or `a NOT IN (1, 2, 3)`

It should probably say

/// 2. The column must NOT be one of a particular set of values for the predicate to /// be `true`. If the column can ONLY take one of these values, the predicate can not /// evaluate to `true`.

🤔 I need to think about this more deeply

Here is my understanding: as guarantee is allowed to be false positive, it's fine to drop arbitrary parts of AND in a expr -- we only need to make sure the false condition is reliable.

I tried to clarify more in f65a1dd

For the not_in_garantee, to prove the expression is false, we have to show that the column can only be one of the set of values.

NGA-TRAN · 2023-12-15T16:47:38Z

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

+                .and(col("a").eq(lit("baz"))),
+            // this could potentially be represented as 2 constraints with a more
+            // sophisticated analysis
+            vec![],


NGA-TRAN · 2023-12-15T16:53:13Z

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

+            match existing.guarantee {
+                // knew that the column could not be a set of values
+                // For example, if we previously had `a != 5` and now we see another `a != 6` we know
+                // if


Unfinished sentence?

Good call -- fixed.

NGA-TRAN · 2023-12-15T16:54:20Z

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

+                    } else {
+                        // this is like (a != foo AND (a != bar OR a != baz)).
+                        // We can't combine the (a!=bar OR a!=baz) part, but it
+                        // also doesn't invalidate a != foo guarantee.


Co-authored-by: Nga Tran <[email protected]>

waynexia

Thanks @alamb and @NGA-TRAN, I think this PR is excellent 💯. The guarantee logic makes fully sense to me

waynexia · 2023-12-16T07:59:58Z

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

+/// 2. The column must NOT be one of a particular set of values for the predicate to
+/// be `true`. If the column takes on any of the values, the predicate can not
+/// evaluate to `true`. For example,
+/// `(a != 1)`, `(a != 1 AND a != 2)` or `a NOT IN (1, 2, 3)`


This helps a lot 👍 The guarantee is similar to bloom filter: it might be false-positive, but negative result is reliable.

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

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

…fusion into alamb/literal_guarantee

alamb

Thank you @NGA-TRAN and @waynexia for the careful reviews. It is much appreciated.

cc @wjones127 who might be interested as this is similar to the guarantees used to simplify expressions, added in #7467

alamb · 2023-12-16T13:18:54Z

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

+                .and(col("b").gt(lit(2)).or(col("b").eq(lit(3)))),
+            vec![
+                // for the expression to be true, b can not be one
+                not_in_guarantee("b", [1]),


I tried to clarify more in f65a1dd

For the not_in_garantee, to prove the expression is false, we have to show that the column can only be one of the set of values.

alamb · 2023-12-18T14:14:13Z

I plan to merge this in later today unless I hear any more comments

Introduce LiteralGurantee to find col=const

b8fb3cf

alamb marked this pull request as ready for review December 6, 2023 12:44

github-actions bot added the physical-expr Physical Expressions label Dec 6, 2023

alamb mentioned this pull request Dec 6, 2023

Support IN lists with more than three constants in predicates for bloom filters #8436

Closed

Improve comments

e1df3fe

alamb changed the title ~~Add LiteralGurantee to extract if any PhysicalExpr is know to be a literal (or a set of literals)~~ Add LiteralGuarantee to extract if any PhysicalExpr is know to be a literal (or a set of literals) Dec 6, 2023

alamb commented Dec 6, 2023

View reviewed changes

This was referenced Dec 6, 2023

Implement contained API in PruningPredicate #8440

Merged

Support general pruning based on <col> = 'const' in PruningPredicate #8376

Closed

waynexia self-requested a review December 12, 2023 03:48

waynexia reviewed Dec 12, 2023

View reviewed changes

alamb added 2 commits December 12, 2023 15:12

Merge remote-tracking branch 'apache/main' into alamb/literal_guarantee

c5b98b3

Improve documentation

7c8c8f8

alamb marked this pull request as draft December 12, 2023 20:48

Add more documentation and tests

bca390e

alamb added 2 commits December 15, 2023 09:06

refine documentation and tests

d73136b

Merge remote-tracking branch 'apache/main' into alamb/literal_guarantee

116cebe

alamb marked this pull request as ready for review December 15, 2023 14:08

NGA-TRAN approved these changes Dec 15, 2023

View reviewed changes

alamb and others added 5 commits December 15, 2023 13:24

Apply suggestions from code review

941b0f3

Co-authored-by: Nga Tran <[email protected]>

Fix half comment

8da221f

swap operators before analysis

af13798

More tests

4ce193c

cmt

27f5c36

waynexia approved these changes Dec 16, 2023

View reviewed changes

Merge remote-tracking branch 'apache/main' into alamb/literal_guarantee

94ffb2f

alamb changed the title ~~Add LiteralGuarantee to extract if any PhysicalExpr is know to be a literal (or a set of literals)~~ Add LiteralGuarantee on columns to extract conditions required for PhysicalExpr expressions to evaluate to true Dec 16, 2023

alamb and others added 3 commits December 16, 2023 08:07

Apply suggestions from code review

cfb08cc

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

refine comments more

f65a1dd

Merge branch 'alamb/literal_guarantee' of github.com:alamb/arrow-data…

8ea637b

…fusion into alamb/literal_guarantee

alamb commented Dec 16, 2023

View reviewed changes

alamb merged commit b5e94a6 into apache:main Dec 18, 2023
22 checks passed

alamb deleted the alamb/literal_guarantee branch December 18, 2023 19:15

alamb mentioned this pull request Dec 23, 2023

Rewrite bloom filters to use contains API #8442

Merged

my-vegetable-has-exploded mentioned this pull request Dec 28, 2023

feat: support inlist in LiteralGurantee for pruning #8654

Merged

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

Add `LiteralGuarantee` on columns to extract conditions required for `PhysicalExpr` expressions to evaluate to true #8437

Add `LiteralGuarantee` on columns to extract conditions required for `PhysicalExpr` expressions to evaluate to true #8437

alamb commented Dec 6, 2023 •

edited

Loading

alamb Dec 6, 2023

waynexia left a comment

waynexia Dec 12, 2023

alamb Dec 15, 2023

waynexia Dec 12, 2023

alamb Dec 12, 2023

alamb Dec 13, 2023

waynexia Dec 14, 2023

alamb Dec 14, 2023

alamb commented Dec 12, 2023

alamb commented Dec 14, 2023

alamb commented Dec 15, 2023

NGA-TRAN left a comment

NGA-TRAN Dec 15, 2023

waynexia Dec 16, 2023

NGA-TRAN Dec 15, 2023

alamb Dec 15, 2023

NGA-TRAN Dec 15, 2023

alamb Dec 15, 2023

waynexia Dec 16, 2023

alamb Dec 16, 2023

NGA-TRAN Dec 15, 2023

NGA-TRAN Dec 15, 2023

alamb Dec 15, 2023

NGA-TRAN Dec 15, 2023

waynexia left a comment

waynexia Dec 16, 2023

alamb left a comment

alamb Dec 16, 2023

alamb commented Dec 18, 2023

	.and(col("b").gt(lit(2)).or(col("b").eq(lit(3)))),
	.and(col("b").gt(lit(2))),

Add LiteralGuarantee on columns to extract conditions required for PhysicalExpr expressions to evaluate to true #8437

Add LiteralGuarantee on columns to extract conditions required for PhysicalExpr expressions to evaluate to true #8437

Conversation

alamb commented Dec 6, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

Example of what the code in this PR can do

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

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

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 commented Dec 12, 2023

alamb commented Dec 14, 2023

alamb commented Dec 15, 2023

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

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

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 18, 2023

Add `LiteralGuarantee` on columns to extract conditions required for `PhysicalExpr` expressions to evaluate to true #8437

Add `LiteralGuarantee` on columns to extract conditions required for `PhysicalExpr` expressions to evaluate to true #8437

alamb commented Dec 6, 2023 •

edited

Loading