remove type coercion in the binary physical expr #3396

liukun4515 · 2022-09-08T12:40:05Z

Which issue does this PR close?

Closes #3388

depend on #3421 #3289 #3459 #3472

fixed the row filter predication for the data type of NULL value #3470

Rationale for this change

What changes are included in this PR?

In order to make the pr review more friendly for reviewer, I will explain what I did in this pr:

remove the type coercion in the creation of the binary physical expr and fix the test case
redo the simplify expression again after the type coercion in the optimizer. This is just an minor fix. If the issue move the type coercion out of the optimizer and refactor the optimizer #3582 done, we can remove the duplicated simplify expression in the optimizer.
try do the type coercion before evaluate the logical expr in the file according to the comments in this issue simplify_expressions don't support different data type for binary #3556 from @alamb . This is just an temporary fix, and it will be remove when do move the type coercion out of the optimizer and refactor the optimizer #3582
remove the between optimization in the simplify expression because of issue simplify between expr should consider the data type #3587, and will recovery when do move the type coercion out of the optimizer and refactor the optimizer #3582
refresh the test cases

cc @alamb @andygrove

Are there any user-facing changes?

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

…on_#3387_#3388

liukun4515 · 2022-09-13T22:19:04Z

after rebase #3380, the test case evolved_schema_intersection_filter_with_filter_pushdown and evolved_schema_disjoint_schema_filter_with_pushdown failed

liukun4515 · 2022-09-14T07:15:51Z

this pr depend on the #3470 and #3472

…on_#3387_#3388

liukun4515 · 2022-09-16T06:13:57Z

I meet so many issue when remove the binary type coercion which is caused from these pr #3301 #3246
For example is TRUE, these is a logical expr Expr::IsTrue but convert it to the physical binary op in https://github.com/andygrove/arrow-datafusion/blob/92b1a341aca334c54235ef65737c88037010e30b/datafusion/physical-expr/src/planner.rs#L86

@andygrove why not generate the logical binary op when create the Expr::IsTrue logical expr.

liukun4515 · 2022-09-16T06:18:54Z

I meet so many issue when remove the binary type coercion which is caused from these pr #3301 #3246 For example is TRUE, these is a logical expr Expr::IsTrue but convert it to the physical binary op in https://github.com/andygrove/arrow-datafusion/blob/92b1a341aca334c54235ef65737c88037010e30b/datafusion/physical-expr/src/planner.rs#L86

@andygrove why not generate the logical binary op when create the Expr::IsTrue logical expr.

cc @alamb

I create the issue #3509 to support other logical expr for the typ coercion.
If we can convert such expr like istrue, isfalse,like to binary op in the logical phase, we don't need to do this by adding more case in the TypeCoercion optimizer rule.
I don't know why we don't do this when implement the is true, is false in the https://github.com/apache/arrow-datafusion/pull/3301/files @andygrove , is there any optimization should be done in the logical phase with the Expr:IsTrue?

alamb · 2022-09-17T10:43:20Z

is there any optimization should be done in the logical phase with the Expr:IsTrue?

I think the only coercion it might need is that its argument is DataType::Boolean

…on_#3387_#3388

codecov-commenter · 2022-09-22T06:20:13Z

Codecov Report

Merging #3396 (3062cd5) into master (b54a56f) will increase coverage by 0.05%.
The diff coverage is 98.52%.

@@            Coverage Diff             @@
##           master    #3396      +/-   ##
==========================================
+ Coverage   86.03%   86.08%   +0.05%     
==========================================
  Files         300      300              
  Lines       56253    56313      +60     
==========================================
+ Hits        48395    48477      +82     
+ Misses       7858     7836      -22

Impacted Files	Coverage Δ
datafusion/core/src/physical_plan/planner.rs	`78.03% <ø> (+0.56%)`	⬆️
datafusion/core/tests/sql/aggregates.rs	`99.38% <ø> (+<0.01%)`	⬆️
datafusion/core/tests/sql/decimal.rs	`100.00% <ø> (ø)`
datafusion/optimizer/src/type_coercion.rs	`96.01% <92.30%> (+3.61%)`	⬆️
datafusion/physical-expr/src/expressions/binary.rs	`97.42% <98.33%> (-0.21%)`	⬇️
datafusion/core/src/execution/context.rs	`79.38% <100.00%> (+0.07%)`	⬆️
...ore/src/physical_optimizer/aggregate_statistics.rs	`100.00% <100.00%> (ø)`
...sion/core/src/physical_plan/file_format/parquet.rs	`94.68% <100.00%> (+0.02%)`	⬆️
datafusion/core/tests/sql/predicates.rs	`100.00% <100.00%> (ø)`
datafusion/core/tests/sql/select.rs	`99.78% <100.00%> (ø)`
... and 28 more

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

andygrove · 2022-09-22T21:45:51Z

I've started reviewing this, but there are many changes and referenced issues I need to go read. I will continue with the review tomorrow.

alamb · 2022-09-23T12:27:42Z

I also plan to review this PR, but I may not get to it today

liukun4515 · 2022-09-23T12:54:11Z

I try to explain what I do and why i do in the description What changes are included in this PR?

I hope this works for your review.

cc @andygrove @alamb

alamb

This was a great PR to read @liukun4515 -- really nice.

Thank you for sticking with it.

I think we should merge it asap to minimize conflicts.

alamb · 2022-09-24T10:42:30Z

datafusion/core/src/execution/context.rs

@@ -1452,7 +1452,11 @@ impl SessionState {
            rules.push(Arc::new(FilterNullJoinKeys::default()));
        }
        rules.push(Arc::new(ReduceOuterJoin::new()));
+        // TODO: https://github.com/apache/arrow-datafusion/issues/3557


it makes sense to me that we need to simplify expressons after coercion

alamb · 2022-09-24T10:43:54Z

datafusion/core/tests/sql/aggregates.rs

-        "+--------------+---------------------------+---------------------------+---------------------------+",
-        "| 1.5          | 2.5                       | 3.5                       | 2.5                       |",
-        "+--------------+---------------------------+---------------------------+---------------------------+",
+        "+--------------+-------------------------+-------------------------+-------------------------+",


alamb · 2022-09-24T10:45:23Z

datafusion/expr/src/binary_rule.rs

+            Operator::Modulo,
+            DataType::Decimal128(10, 2)
+        );
+        // TODO add other data type


I wonder if we should file another ticket to track this gap? Specifically, a "help wanted" ticket that explained what was needed might encourage some additional contributions

alamb · 2022-09-24T10:47:55Z

datafusion/expr/src/binary_rule.rs

+            Operator::GtEq,
+            DataType::Decimal128(15, 3)
+        );
+


I reviewed the tests cases in this file carefully. Very nice 👌

alamb · 2022-09-24T10:48:52Z

datafusion/expr/src/binary_rule.rs

+            DataType::Boolean,
+            DataType::Boolean,
+            Operator::Or,
+            DataType::Boolean


Is there any other types that are coerced to boolean for logical operations? Or are the tests for boolean just showing that boolean types are not changed when coerced?

datafusion/optimizer/src/simplify_expressions.rs

alamb · 2022-09-24T10:53:03Z

datafusion/optimizer/src/type_coercion.rs

@@ -69,14 +67,8 @@ impl OptimizerRule for TypeCoercion {
            },
        );

-        let mut execution_props = ExecutionProps::new();


datafusion/optimizer/tests/integration-test.rs

alamb · 2022-09-24T10:54:07Z

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

@@ -892,25 +891,6 @@ impl BinaryExpr {
    }
 }

-/// return two physical expressions that are optionally coerced to a
-/// common type that the binary operator supports.
-fn binary_cast(


alamb · 2022-09-24T10:55:15Z

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

@@ -1080,7 +1071,7 @@ mod tests {
            Operator::Plus,
            Int32Array,
            DataType::Int32,
-            vec![2i32, 4i32]
+            vec![2i32, 4i32],
        );
        test_coercion!(


I wonder if these tests are doing anything useful anymore now that we are coercing in the logical layer? 🤔 It seems like they are now testing the test code 😆

alamb · 2022-09-24T10:57:37Z

I plan to get this PR ready to merge by: add my suggested comments, fix clippy, and merge to master

alamb · 2022-09-24T10:57:55Z

(since I already have it checked out this will be a simple thing for me)

…_#3387_#3388

ursabot · 2022-09-24T12:01:42Z

Benchmark runs are scheduled for baseline = b625277 and contender = d7c0e42. d7c0e42 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions labels Sep 8, 2022

liukun4515 force-pushed the remove_phy_coercion_#3387_#3388 branch from c150de1 to c2884f7 Compare September 8, 2022 13:39

andygrove reviewed Sep 8, 2022

View reviewed changes

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

liukun4515 force-pushed the remove_phy_coercion_#3387_#3388 branch from c2884f7 to 1d7c2a0 Compare September 9, 2022 08:01

github-actions bot added the core Core datafusion crate label Sep 9, 2022

This was referenced Sep 9, 2022

pruning support cast/try_cast expr #3414

Closed

support cast/try_cast in prune with signed integer and decimal #3422

Merged

DataFusion 12.0.0 Release #3097

Closed

liukun4515 added 3 commits September 13, 2022 17:29

remove type coercion binary from phy

818f642

Merge remote-tracking branch 'upstream/master' into remove_phy_coerci…

c71db45

…on_#3387_#3388

fix test case

8c38433

liukun4515 force-pushed the remove_phy_coercion_#3387_#3388 branch from 1d7c2a0 to 8c38433 Compare September 13, 2022 10:54

github-actions bot added the optimizer Optimizer rules label Sep 13, 2022

Merge remote-tracking branch 'upstream/master' into remove_phy_coerci…

3013e04

…on_#3387_#3388

liukun4515 mentioned this pull request Sep 13, 2022

move type coercion for inlist from physical phase to logical phase #3468

Closed

This was referenced Sep 14, 2022

Pushdown RowFilter in ParquetExec #3380

Merged

The data type of predicate in the row filter should be same in the binary expr #3469

Closed

liukun4515 added 2 commits September 16, 2022 11:57

revert the fix for apache#3387

cbe943b

Merge remote-tracking branch 'upstream/master' into remove_phy_coerci…

863ac3f

…on_#3387_#3388

github-actions bot removed the optimizer Optimizer rules label Sep 16, 2022

liukun4515 mentioned this pull request Sep 16, 2022

type coercion: support is/is_not_bool/like/unknown expr #3510

Merged

alamb mentioned this pull request Sep 19, 2022

DataFusion serialization doesn't handle ScalarValue::Dictionary, Binary, LargeBinary, Time64, IntervalMonthDayNano, Struct #3531

Closed

6 tasks

Merge remote-tracking branch 'upstream/master' into remove_phy_coerci…

a6c6724

…on_#3387_#3388

liukun4515 mentioned this pull request Sep 21, 2022

simplify_expressions don't support different data type for binary #3556

Closed

complete remove the type coercion in the physical plan

0f7891d

liukun4515 marked this pull request as ready for review September 21, 2022 15:00

github-actions bot added the optimizer Optimizer rules label Sep 21, 2022

liukun4515 mentioned this pull request Sep 22, 2022

Can't push down projection after do type coercion #3583

Closed

liukun4515 added 4 commits September 22, 2022 10:59

refactor

7b0d87a

Merge remote-tracking branch 'upstream/master' into remove_phy_coerci…

c409326

…on_#3387_#3388

merge master

4b1fa72

refactor

bb7f0ac

liukun4515 marked this pull request as draft September 22, 2022 10:12

liukun4515 mentioned this pull request Sep 22, 2022

make type coercion simple and remove the evaluate logic #3585

Closed

do type coercion in the simplify expression

3062cd5

liukun4515 marked this pull request as ready for review September 22, 2022 14:27

liukun4515 requested review from andygrove and alamb September 22, 2022 14:27

alamb mentioned this pull request Sep 23, 2022

Custom window frame logic (support ROWS, RANGE, PRECEDING and FOLLOWING for window functions) #3570

Merged

alamb approved these changes Sep 24, 2022

View reviewed changes

alamb added 3 commits September 24, 2022 06:59

Add comments

01d1745

Merge remote-tracking branch 'apache/master' into remove_phy_coercion…

6c2b91f

…_#3387_#3388

fix: fmt

1a125ed

alamb merged commit d7c0e42 into apache:master Sep 24, 2022

liukun4515 mentioned this pull request Sep 30, 2022

restore optimization for between in simplify expression rule #3661

Merged

mildbyte mentioned this pull request Oct 27, 2022

Upgrade to DataFusion 13 (784f10bb) / Arrow 25.0.0 splitgraph/seafowl#176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove type coercion in the binary physical expr #3396

remove type coercion in the binary physical expr #3396

liukun4515 commented Sep 8, 2022 •

edited

liukun4515 commented Sep 13, 2022

liukun4515 commented Sep 14, 2022

liukun4515 commented Sep 16, 2022

liukun4515 commented Sep 16, 2022 •

edited

alamb commented Sep 17, 2022

codecov-commenter commented Sep 22, 2022 •

edited

andygrove commented Sep 22, 2022

alamb commented Sep 23, 2022

liukun4515 commented Sep 23, 2022

alamb left a comment

alamb Sep 24, 2022

alamb Sep 24, 2022

alamb Sep 24, 2022

alamb Sep 24, 2022

alamb Sep 24, 2022

alamb Sep 24, 2022

alamb Sep 24, 2022

alamb Sep 24, 2022

alamb Sep 24, 2022

alamb commented Sep 24, 2022

alamb commented Sep 24, 2022

ursabot commented Sep 24, 2022

remove type coercion in the binary physical expr #3396

remove type coercion in the binary physical expr #3396

Conversation

liukun4515 commented Sep 8, 2022 • edited

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

liukun4515 commented Sep 13, 2022

liukun4515 commented Sep 14, 2022

liukun4515 commented Sep 16, 2022

liukun4515 commented Sep 16, 2022 • edited

alamb commented Sep 17, 2022

codecov-commenter commented Sep 22, 2022 • edited

Codecov Report

andygrove commented Sep 22, 2022

alamb commented Sep 23, 2022

liukun4515 commented Sep 23, 2022

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 commented Sep 24, 2022

alamb commented Sep 24, 2022

ursabot commented Sep 24, 2022

liukun4515 commented Sep 8, 2022 •

edited

liukun4515 commented Sep 16, 2022 •

edited

codecov-commenter commented Sep 22, 2022 •

edited