Support `=`, `<`, `<=`, `>`, `>=`, `!=`, `is distinct from`, `is not distinct from` for `BooleanArray` #1163

alamb · 2021-10-21T18:17:39Z

Which issue does this PR close?

Resolves #1159

PR is mostly tests

Sorry for what looks like a large PR :( I blame the test

Rationale for this change

This is mostly interesting so that during constant folding / simplification we can simplify down de-generate expressions like true = false

Now that @jimexist added apache/arrow-rs#860 and @Dandandan added apache/arrow-rs#844 in arrow, this PR hooks that up

Also, it has the nice side effect benefit parquet row group pruning is now supported for boolean columns as well 🎉

What changes are included in this PR?

Update to arrow 6.2.0
Support =, <, <=, >, >=, !=,is distinct from, is not distinct from for BooleanArray(aka for boolean columns)
Simple implementations of *_scalar_bool
Many tests
Update the pruning tests to reflect the fact that boolean pruning now happens

Are there any user-facing changes?

Less errors

alamb · 2021-10-21T19:27:42Z

datafusion/src/physical_plan/expressions/binary.rs

@@ -814,14 +870,68 @@ pub fn binary(
    Ok(Arc::new(BinaryExpr::new(l, op, r)))
 }

+// TODO file a ticket with arrow-rs to include these kernels


Suggested change

// TODO file a ticket with arrow-rs to include these kernels

// When arrow-rs has these kernels, can remove this implementation

// see https://github.com/apache/arrow-rs/issues/842

Filed ticket in arrow apache/arrow-rs#842

FWIW @Dandandan has added these kernels upstream to Arrow so we can use 6.1.0 when that comes out (in a week or so): apache/arrow-rs#844

@jimexist has actually implemented operations like bool_lt etc in apache/arrow-rs#860 so when that is available in datafusion (next week) I will update this PR to include those operations as well

alamb · 2021-11-02T20:03:59Z

This one is waiting on arrow-rs 6.1 to be released, and then I should be able to clean it up and get it ready for a proper review

alamb · 2021-11-05T16:56:14Z

Turns out that we forgot to make the required functions public 🤦 . Will wait for arrow 6.2 to include apache/arrow-rs#913

…distinct from` for `BooleanArray`

alamb

FYI @Dandandan @jimexist

alamb · 2021-11-15T21:51:01Z

datafusion/src/physical_optimizer/pruning.rs

-            result
-        )
+        let result = p.prune(&statistics).unwrap();
+        assert_eq!(result, expected_true);


pruning works for boolean columns now

datafusion/src/physical_plan/expressions/binary.rs

alamb · 2021-11-15T21:53:27Z

datafusion/src/physical_plan/expressions/binary.rs

@@ -276,6 +377,7 @@ macro_rules! binary_array_op_scalar {
            DataType::Date64 => {
                compute_op_scalar!($LEFT, $RIGHT, $OP, Date64Array)
            }
+            DataType::Boolean => compute_bool_op_scalar!($LEFT, $RIGHT, $OP, BooleanArray),


adding this line and the one below it adds all the new support, which is kind of cool! It is terrifying how many functions end up being called :)

alamb · 2021-11-15T21:53:56Z

datafusion/src/physical_plan/file_format/parquet.rs

-        // where a null array is generated for some statistics columns
-        // int > 1 and bool = true => c1_max > 1 and null
-        let expr = col("c1").gt(lit(15)).and(col("c2").eq(lit(true)));
+        // test row group predicate with an unknown (Null) expr


now bool stats don't result in null columns, so I needed to use a constant to get the same effect

alamb · 2021-11-18T22:15:06Z

This PR is ready for review / analysis if/when you get a chance @jimexist / @Dandandan / @houqp / @rdettai. It looks much bigger than it is because of the tests. It is mostly about hooking up some more arrow compute kernels

There are many PRs flying in DataFusion recently 😅 fun times

rdettai

Great addition @alamb ! thanks !

datafusion/src/physical_plan/expressions/binary.rs

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

alamb

Thanks for the review @rdettai -- I'll plan to merge this one in tomorrow and file arrow-rs tickets if there are no other comments.

jimexist · 2021-11-20T10:26:13Z

datafusion/src/physical_plan/expressions/binary.rs

+            .expect("compute_op failed to downcast array");
+        // generate the scalar function name, such as lt_scalar, from the $OP parameter
+        // (which could have a value of lt) and the suffix _scalar
+        Ok(Arc::new(paste::expr! {[<$OP _bool_scalar>]}(


For the record this pattern is used elsewhere in this file, I was just following it :)

datafusion/src/physical_plan/expressions/binary.rs

Co-authored-by: Jiayu Liu <[email protected]>

github-actions bot added the datafusion Changes in the datafusion crate label Oct 21, 2021

This was referenced Oct 21, 2021

(WIP) Cleanup: remove redundant constant evaluation in Simplifier #1164

Closed

Add boolean equality and inequality kernels apache/arrow-rs#842

Closed

add compute kernels that operate on ArrayRef -- e.g eq_dyn apache/arrow-rs#843

Closed

alamb commented Oct 21, 2021

View reviewed changes

alamb marked this pull request as draft October 26, 2021 10:51

alamb changed the title ~~Support <bool col> = <bool col> and <bool col> != <bool col>~~ (WIP) Support <bool col> = <bool col> and <bool col> != <bool col> Oct 26, 2021

alamb force-pushed the alamb/bool_expr branch from fda35ac to 3cb11a4 Compare November 4, 2021 20:13

alamb mentioned this pull request Nov 5, 2021

fix: not do boolean folding on NULL and/or expr #1245

Merged

alamb force-pushed the alamb/bool_expr branch from 3cb11a4 to 5d09920 Compare November 15, 2021 19:18

alamb changed the title ~~(WIP) Support <bool col> = <bool col> and <bool col> != <bool col>~~ Support <bool col> = <bool col> and <bool col> != <bool col> Nov 15, 2021

alamb marked this pull request as ready for review November 15, 2021 19:19

alamb changed the title ~~Support <bool col> = <bool col> and <bool col> != <bool col>~~ Support =, <, <=, >, >=, != for BooleanArray Nov 15, 2021

alamb changed the title ~~Support =, <, <=, >, >=, != for BooleanArray~~ Support =, <, <=, >, >=, !=, is distinct from, is not distinct from for BooleanArray Nov 15, 2021

Support =, <, <=, >, >=, !=, is distinct from, `is not …

c990851

…distinct from` for `BooleanArray`

alamb force-pushed the alamb/bool_expr branch from 5d09920 to c990851 Compare November 15, 2021 21:50

alamb commented Nov 15, 2021

View reviewed changes

alamb requested review from Dandandan and jimexist November 15, 2021 22:06

rdettai reviewed Nov 19, 2021

View reviewed changes

datafusion/src/physical_plan/expressions/binary.rs Outdated Show resolved Hide resolved

datafusion/src/physical_plan/expressions/binary.rs Outdated Show resolved Hide resolved

Update datafusion/src/physical_plan/expressions/binary.rs

c847623

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

alamb commented Nov 19, 2021

View reviewed changes

Merge remote-tracking branch 'apache/master' into alamb/bool_expr

d2aafc7

jimexist reviewed Nov 20, 2021

View reviewed changes

datafusion/src/physical_plan/expressions/binary.rs Outdated Show resolved Hide resolved

jimexist reviewed Nov 20, 2021

View reviewed changes

datafusion/src/physical_plan/expressions/binary.rs Outdated Show resolved Hide resolved

jimexist reviewed Nov 20, 2021

View reviewed changes

datafusion/src/physical_plan/expressions/binary.rs Outdated Show resolved Hide resolved

jimexist approved these changes Nov 20, 2021

View reviewed changes

This was referenced Nov 20, 2021

Add boolean comparison to scalar kernels for less then, greater than apache/arrow-rs#959

Closed

Consider adding is_distinct_from kernels apache/arrow-rs#960

Closed

alamb and others added 2 commits November 20, 2021 06:59

Apply suggestions from code review

2740e7b

Co-authored-by: Jiayu Liu <[email protected]>

Merge remote-tracking branch 'apache/master' into alamb/bool_expr

aa8b39c

alamb merged commit 00850a4 into apache:master Nov 20, 2021

alamb deleted the alamb/bool_expr branch November 20, 2021 12:59

liukun4515 mentioned this pull request Dec 25, 2021

Implement DECIMAL type #122

Open

alamb added the enhancement New feature or request label Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `=`, `<`, `<=`, `>`, `>=`, `!=`, `is distinct from`, `is not distinct from` for `BooleanArray` #1163

Support `=`, `<`, `<=`, `>`, `>=`, `!=`, `is distinct from`, `is not distinct from` for `BooleanArray` #1163

alamb commented Oct 21, 2021 •

edited

Loading

alamb Oct 21, 2021

alamb Oct 23, 2021

alamb Oct 26, 2021

alamb commented Nov 2, 2021

alamb commented Nov 5, 2021

alamb left a comment

alamb Nov 15, 2021

alamb Nov 15, 2021

alamb Nov 15, 2021

alamb commented Nov 18, 2021

rdettai left a comment

alamb left a comment

jimexist Nov 20, 2021

alamb Nov 20, 2021

	// TODO file a ticket with arrow-rs to include these kernels
	// When arrow-rs has these kernels, can remove this implementation
	// see https://github.com/apache/arrow-rs/issues/842

Support =, <, <=, >, >=, !=, is distinct from, is not distinct from for BooleanArray #1163

Support =, <, <=, >, >=, !=, is distinct from, is not distinct from for BooleanArray #1163

Conversation

alamb commented Oct 21, 2021 • edited Loading

Which issue does this PR close?

PR is mostly tests

Rationale for this change

What changes are included in this PR?

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

alamb commented Nov 2, 2021

alamb commented Nov 5, 2021

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

alamb commented Nov 18, 2021

rdettai left a 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

Support `=`, `<`, `<=`, `>`, `>=`, `!=`, `is distinct from`, `is not distinct from` for `BooleanArray` #1163

Support `=`, `<`, `<=`, `>`, `>=`, `!=`, `is distinct from`, `is not distinct from` for `BooleanArray` #1163

alamb commented Oct 21, 2021 •

edited

Loading