Support "IS TRUE/FALSE" syntax #3189

sarahyurick · 2022-08-17T21:08:02Z

Which issue does this PR close?

What changes are included in this PR?

So far, I've added some basic changes so that IS TRUE works on a scalar (ColumnarValue::Scalar(scalar)), like SELECT TRUE IS TRUE as t, SELECT FALSE IS TRUE as t, SELECT 2 IS TRUE as t, etc.

Mainly, I am looking for feedback on how to handle the ColumnarValue::Array(array) case.

Please also let me know if there are any major issues with this implementation so far. Originally, @alamb suggested using IsNotDistinctFrom, but I was having trouble figuring out how that was supposed to work. Since we can already parse IS TRUE the initial error (without the changes from this PR) is Error: NotImplemented("Unsupported ast node IsTrue(Identifier(Ident { value: \"b\", quote_style: None })) in sqltorel"). Whenever I would try to link IsTrue to IsNotDistinctFrom, I would run into trouble because IsTrue looks like <expr> IS TRUE while IsNotDistinctFrom looks like <expr> IS NOT DISTINCT FROM <expr>. Maybe I'm missing something obvious, but the way I decided to go about this implementation was to mimic the implementations for IsNull and IsNotNull, since they don't have a RHS expression either.

Thanks in advance!

Update: IS TRUE should be fully functional.

datafusion/physical-expr/src/expressions/is_true.rs

datafusion/common/src/scalar.rs

andygrove · 2022-08-18T02:53:30Z

@sarahyurick I tried some expr IS TRUE type queries in Postgres and it only supports the case where expr is a boolean type. Does Dask SQL have different behavior to this?

sarahyurick · 2022-08-18T16:42:19Z

@sarahyurick I tried some expr IS TRUE type queries in Postgres and it only supports the case where expr is a boolean type. Does Dask SQL have different behavior to this?

Good point, thanks! Dask SQL main currently throws a Cannot apply 'IS TRUE' to arguments of type '<BIGINT> IS TRUE'. Supported form(s): '<BOOLEAN> IS TRUE' for all other data types, so it would probably be best to match this.

andygrove · 2022-08-18T17:34:11Z

@sarahyurick Could you target this to the sqlparser-0.21 branch? We'll need to merge PRs there in the order specified in #3192

Also, see discussion on this feature branch approach in #3191

sarahyurick · 2022-08-18T19:38:52Z

IS TRUE and IS FALSE should both be fully functional. I'm not sure what's going on with the proto files, though.

After those are fixed, it should just be formatting checks and tests, if any.

datafusion/proto/proto/datafusion.proto

Co-authored-by: Andy Grove <andygrove73@gmail.com>

datafusion/common/src/scalar.rs

Co-authored-by: Andy Grove <andygrove73@gmail.com>

codecov-commenter · 2022-08-18T21:00:28Z

Codecov Report

Merging #3189 (affa48d) into master (929eb6d) will decrease coverage by 0.12%.
The diff coverage is 42.28%.

@@            Coverage Diff             @@
##           master    #3189      +/-   ##
==========================================
- Coverage   85.87%   85.75%   -0.13%     
==========================================
  Files         291      293       +2     
  Lines       52885    53033     +148     
==========================================
+ Hits        45415    45476      +61     
- Misses       7470     7557      +87

Impacted Files	Coverage Δ
datafusion/core/src/datasource/listing/helpers.rs	`95.01% <ø> (ø)`
datafusion/core/src/physical_plan/planner.rs	`80.55% <0.00%> (-0.49%)`	⬇️
datafusion/expr/src/expr.rs	`81.76% <0.00%> (-3.64%)`	⬇️
datafusion/expr/src/expr_rewriter.rs	`85.34% <0.00%> (-0.75%)`	⬇️
datafusion/expr/src/expr_visitor.rs	`62.19% <0.00%> (-1.56%)`	⬇️
datafusion/expr/src/utils.rs	`90.76% <ø> (ø)`
...tafusion/optimizer/src/common_subexpr_eliminate.rs	`93.81% <0.00%> (-0.49%)`	⬇️
datafusion/optimizer/src/simplify_expressions.rs	`83.76% <0.00%> (-0.18%)`	⬇️
datafusion/physical-expr/src/expressions/mod.rs	`100.00% <ø> (ø)`
datafusion/physical-expr/src/planner.rs	`92.36% <0.00%> (-2.91%)`	⬇️
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

datafusion/physical-expr/src/expressions/is_false.rs

sarahyurick · 2022-08-18T23:38:38Z

Should I also add tests in proto/src/lib.rs? I'm not quite sure what those would look like.

datafusion/physical-expr/src/expressions/is_false.rs

datafusion/physical-expr/src/expressions/is_true.rs

datafusion/physical-expr/src/expressions/is_false.rs

andygrove

LGTM! Thanks @sarahyurick

alamb

One thing I would recommend is adding some sort of SQL level test -- like https://github.com/apache/arrow-datafusion/blob/master/datafusion/core/tests/sql/expr.rs#L257 for IS NULL

The rationale for adding this would be to ensure all the plumbing is hooked up through the planner and optimizer

alamb · 2022-08-21T11:09:02Z

datafusion/expr/src/expr_schema.rs

+            | Expr::IsNotNull(_)
+            | Expr::IsTrue(_)
+            | Expr::IsFalse(_)
+            | Expr::Exists { .. } => Ok(false),


alamb · 2022-08-21T11:19:19Z

datafusion/physical-expr/src/expressions/is_false.rs

+                        let array_len = array.len();
+                        let mut result_builder = BooleanBuilder::new(array_len);
+                        for i in 0..array_len {
+                            result_builder.append_value(!bool_array.is_null(i) && !bool_array.value(i));
+                        }
+                        Ok(ColumnarValue::Array(Arc::new(result_builder.finish())))


You can probably do the same thing with less code (and more efficiently) using the FromIter method:

Suggested change

let array_len = array.len();

let mut result_builder = BooleanBuilder::new(array_len);

for i in 0..array_len {

result_builder.append_value(!bool_array.is_null(i) && !bool_array.value(i));

}

Ok(ColumnarValue::Array(Arc::new(result_builder.finish())))

let result: BooleanArray = bool_array.iter()

.map(|v| v.map(|v| !v).or(Some(false)))

.collect();

(same comment applies to is_true.rs)

alamb · 2022-08-21T11:28:44Z

datafusion/sql/src/planner.rs

+            SQLExpr::IsTrue(expr) => Ok(Expr::IsTrue(Box::new(
+                self.sql_expr_to_logical_expr(*expr, schema, ctes)?,
+            ))),


I think you might be able to avoid having to extend Expr and add a PhysicalExpr at all if you rewrote IS TRUE in the sql planner to IS NOT DISTINCT FROM

Something like

Suggested change

SQLExpr::IsTrue(expr) => Ok(Expr::IsTrue(Box::new(

self.sql_expr_to_logical_expr(*expr, schema, ctes)?,

))),

SQLExpr::IsTrue(expr) => Ok(Expr::BinaryExpr {

left: Box::new(self.sql_expr_to_logical_expr(*expr, schema, ctes)?),

op: Operator::IsNotDistinctFrom,

right: Box::new(lit(true)),

}),

That way a query like SELECT x IS TRUE becomes SELECT x IS NOT DISTINCT FROM TRUE

Here is an example from postgres showing they are equivalent:

alamb=# select column1 is true, column1 is not distinct from true from (values (true), (false), (null)) as sq; ?column? | ?column? ----------+---------- t | t f | f f | f (3 rows)

The same type of transformation applies to IS FALSE

Then this PR would likely be 8 lines of code and then the sql level tests

My preference would be to keep the logical plan as close to the original query as possible. Different query engines will have different approaches for how to implement this in the physical plan. I also think that it is better UX for the user to see IsTrue/False in the logical plan if that's what their query contained.

I think it is a good idea for DataFusion to map the logical IsTrue /False to the physical expression IsDistinctFrom and that removes the need for the new physical expressions here.

Okay, I think that makes sense to me. The only thing I was wondering was how this would handle non-boolean/non-null inputs, since we should error for all other datatypes?

Yes, that is related to the work I am starting in #3222. I think we need to do some type validation here so that if we see <expr> IS TRUE where expr is not boolean, then we either add a cast or throw an error if it cannot be cast to boolean.

alamb · 2022-08-31T11:59:49Z

BTW @sarahyurick -- thank you for sticking with this. It is great to see the functionality coming along so nicely 👌

scalar functionality

79dc1f4

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates sql SQL Planner labels Aug 17, 2022

sarahyurick commented Aug 17, 2022

View reviewed changes

datafusion/physical-expr/src/expressions/is_true.rs Outdated Show resolved Hide resolved

sarahyurick commented Aug 17, 2022

View reviewed changes

datafusion/common/src/scalar.rs Outdated Show resolved Hide resolved

sarahyurick changed the title ~~[DRAFT] Support "IS TRUE/FALSE" syntax~~ Support "IS TRUE/FALSE" syntax Aug 17, 2022

andygrove mentioned this pull request Aug 18, 2022

sqlparser 0.21 upgrade #3192

Closed

5 tasks

sarahyurick changed the base branch from master to sqlparser-0.21 August 18, 2022 17:38

IsTrue full functionality

4ced79f

andygrove changed the title ~~Support "IS TRUE/FALSE" syntax~~ [sqlparser-0.21] Support "IS TRUE/FALSE" syntax Aug 18, 2022

sarahyurick mentioned this pull request Aug 18, 2022

Support "IS [NOT] UNKNOWN" syntax #3195

Closed

IS FALSE functionality

71c0001

andygrove changed the title ~~[sqlparser-0.21] Support "IS TRUE/FALSE" syntax~~ Support "IS TRUE/FALSE" syntax Aug 18, 2022

sarahyurick changed the base branch from sqlparser-0.21 to master August 18, 2022 20:14

andygrove reviewed Aug 18, 2022

View reviewed changes

datafusion/proto/proto/datafusion.proto Outdated Show resolved Hide resolved

Update datafusion/proto/proto/datafusion.proto

20cfe36

Co-authored-by: Andy Grove <andygrove73@gmail.com>

andygrove reviewed Aug 18, 2022

View reviewed changes

datafusion/common/src/scalar.rs Outdated Show resolved Hide resolved

andygrove reviewed Aug 18, 2022

View reviewed changes

datafusion/common/src/scalar.rs Outdated Show resolved Hide resolved

andygrove reviewed Aug 18, 2022

View reviewed changes

datafusion/common/src/scalar.rs Outdated Show resolved Hide resolved

sarahyurick and others added 4 commits August 18, 2022 13:42

formatting

632700d

Merge branch 'master' of https://github.com/sarahyurick/arrow-datafusion

ecc3a7f

Update datafusion/common/src/scalar.rs

15e42be

Co-authored-by: Andy Grove <andygrove73@gmail.com>

Update datafusion/common/src/scalar.rs

450f776

Co-authored-by: Andy Grove <andygrove73@gmail.com>

err instead of panic

11b5559

sarahyurick commented Aug 18, 2022

View reviewed changes

datafusion/physical-expr/src/expressions/is_false.rs Outdated Show resolved Hide resolved

sarahyurick added 2 commits August 18, 2022 16:26

format error

70acf4b

tests

affa48d

andygrove reviewed Aug 19, 2022

View reviewed changes

datafusion/physical-expr/src/expressions/is_false.rs Outdated Show resolved Hide resolved

sarahyurick and others added 2 commits August 19, 2022 09:36

update is_true and is_false

d408b2f

Merge branch 'apache:master' into master

e5c76a7

andygrove reviewed Aug 19, 2022

View reviewed changes

datafusion/physical-expr/src/expressions/is_true.rs Outdated Show resolved Hide resolved

andygrove reviewed Aug 19, 2022

View reviewed changes

datafusion/physical-expr/src/expressions/is_true.rs Outdated Show resolved Hide resolved

andygrove reviewed Aug 19, 2022

View reviewed changes

datafusion/physical-expr/src/expressions/is_false.rs Show resolved Hide resolved

sarahyurick added 2 commits August 19, 2022 13:24

update with andy's suggestions

6b3ba49

Merge branch 'master' of https://github.com/sarahyurick/arrow-datafusion

9b6d92b

andygrove mentioned this pull request Aug 19, 2022

[DF] Remaining DataFusion / sqlparser work for the GA release dask-contrib/dask-sql#706

Closed

24 tasks

andygrove approved these changes Aug 19, 2022

View reviewed changes

rust format fix

46bf619

alamb reviewed Aug 21, 2022

View reviewed changes

This was referenced Aug 23, 2022

Support "IS TRUE/FALSE" syntax (redo) #3235

Merged

Support "IS NOT TRUE/FALSE" syntax #2265

Closed

isidentical mentioned this pull request Aug 24, 2022

Implement IS UNKNOWN/IS NOT UNKNOWN operators #3246

Merged

sarahyurick closed this Aug 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support "IS TRUE/FALSE" syntax #3189

Support "IS TRUE/FALSE" syntax #3189

sarahyurick commented Aug 17, 2022 •

edited

Loading

andygrove commented Aug 18, 2022

sarahyurick commented Aug 18, 2022 •

edited

Loading

andygrove commented Aug 18, 2022

sarahyurick commented Aug 18, 2022

codecov-commenter commented Aug 18, 2022 •

edited

Loading

sarahyurick commented Aug 18, 2022

andygrove left a comment

alamb left a comment •

edited

Loading

alamb Aug 21, 2022

alamb Aug 21, 2022

alamb Aug 21, 2022

alamb Aug 21, 2022

andygrove Aug 22, 2022

sarahyurick Aug 22, 2022

andygrove Aug 22, 2022

alamb commented Aug 31, 2022

-            SQLExpr::IsTrue(expr) => Ok(Expr::IsTrue(Box::new(
-                self.sql_expr_to_logical_expr(*expr, schema, ctes)?,
-            ))),
+            SQLExpr::IsTrue(expr) => Ok(Expr::BinaryExpr {
+                left: Box::new(self.sql_expr_to_logical_expr(*expr, schema, ctes)?),
+                op: Operator::IsNotDistinctFrom,
+                right: Box::new(lit(true)),
+            }),

Support "IS TRUE/FALSE" syntax #3189

Support "IS TRUE/FALSE" syntax #3189

Conversation

sarahyurick commented Aug 17, 2022 • edited Loading

Which issue does this PR close?

What changes are included in this PR?

andygrove commented Aug 18, 2022

sarahyurick commented Aug 18, 2022 • edited Loading

andygrove commented Aug 18, 2022

sarahyurick commented Aug 18, 2022

codecov-commenter commented Aug 18, 2022 • edited Loading

Codecov Report

sarahyurick commented Aug 18, 2022

andygrove left a comment

Choose a reason for hiding this comment

alamb left a comment • edited Loading

Choose a reason for hiding this comment

alamb Aug 21, 2022

Choose a reason for hiding this comment

alamb Aug 21, 2022

Choose a reason for hiding this comment

alamb Aug 21, 2022

Choose a reason for hiding this comment

alamb Aug 21, 2022

Choose a reason for hiding this comment

andygrove Aug 22, 2022

Choose a reason for hiding this comment

sarahyurick Aug 22, 2022

Choose a reason for hiding this comment

andygrove Aug 22, 2022

Choose a reason for hiding this comment

alamb commented Aug 31, 2022

sarahyurick commented Aug 17, 2022 •

edited

Loading

sarahyurick commented Aug 18, 2022 •

edited

Loading

codecov-commenter commented Aug 18, 2022 •

edited

Loading

alamb left a comment •

edited

Loading