move the `type coercion` to the beginning of the optimizer rule and support type coercion for subquery by liukun4515 · Pull Request #3636 · apache/datafusion

liukun4515 · 2022-09-28T06:24:29Z

Which issue does this PR close?

move the type coercion to the beginning fo the optimizer
support type coercion for subquery

part of #3582

Closed #3557

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

liukun4515 · 2022-09-28T06:58:50Z

datafusion/core/tests/sql/joins.rs


 #[tokio::test]
+#[ignore]
+// https://github.com/apache/arrow-datafusion/issues/3565


skip the test, #3565
@AssHero want to take it

the issue has been fixed, so I remove the comment and #[ignore]

liukun4515 · 2022-09-28T10:22:24Z

@andygrove @alamb @Dandandan PTAL
I have less knowledge to handle the subquery well

codecov-commenter · 2022-09-28T11:11:46Z

Codecov Report

Merging #3636 (2e9d18e) into master (ea3dbb6) will decrease coverage by 0.09%.
The diff coverage is 91.80%.

@@            Coverage Diff             @@
##           master    #3636      +/-   ##
==========================================
- Coverage   86.07%   85.98%   -0.10%     
==========================================
  Files         300      300              
  Lines       56337    56478     +141     
==========================================
+ Hits        48494    48563      +69     
- Misses       7843     7915      +72

Impacted Files	Coverage Δ
datafusion/core/tests/sql/explain_analyze.rs	`82.97% <0.00%> (-0.90%)`	⬇️
datafusion/core/tests/sql/joins.rs	`99.22% <ø> (-0.12%)`	⬇️
datafusion/core/tests/sql/predicates.rs	`100.00% <ø> (ø)`
datafusion/core/tests/sql/subqueries.rs	`94.95% <ø> (ø)`
datafusion/optimizer/src/type_coercion.rs	`96.07% <98.03%> (+0.06%)`	⬆️
datafusion/core/src/execution/context.rs	`78.69% <100.00%> (-0.03%)`	⬇️
datafusion/expr/src/logical_plan/plan.rs	`77.17% <100.00%> (-0.09%)`	⬇️
datafusion/optimizer/tests/integration-test.rs	`89.04% <100.00%> (-0.15%)`	⬇️
datafusion/physical-expr/src/regex_expressions.rs	`64.38% <0.00%> (-18.66%)`	⬇️
datafusion/core/src/physical_plan/sorts/cursor.rs	`57.14% <0.00%> (-7.15%)`	⬇️
... and 20 more

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

alamb

Thanks @liukun4515 -- I left some comments about the subquery changes. Overall I love where this is heading

alamb · 2022-09-28T19:44:36Z

datafusion/core/src/execution/context.rs

-            // Simplify expressions first to maximize the chance
-            // of applying other optimizations
-            Arc::new(SimplifyExpressions::new()),
            Arc::new(PreCastLitInComparisonExpressions::new()),


Now that we have TypeCoercion I wonder if we still need PreCastLitInComparisonExpressions

https://github.com/apache/arrow-datafusion/blob/d16457a0ba129b077935078e5cf89d028f598e0b/datafusion/optimizer/src/pre_cast_lit_in_comparison.rs#L31-L50

Appears to be a subset of what TypeCoercion is doing

Though perhaps this is something similar to what you have described in #3622

The PreCastLitInComparisonExpressions will be invalid when we move the type coercion to the front of PreCastLitInComparisonExpressions.

In the next pr, I will do #3622 and move the type coercion to the front of the PreCastLitInComparisonExpressions

I think type coercion just do one thing which is make type compatible in all operation, but the PreCastLitInComparisonExpressions or #3622 is to reduce cast for column expr or other expr instead of adding the cast to the literal. This will reduce the cast effort of the runtime.

alamb · 2022-09-28T19:47:43Z

datafusion/core/tests/sql/explain_analyze.rs

        ]];
-    assert_eq!(expected, actual);

    // Also, expect same result with lowercase explain


This comment seems to be out of date -- the different is not just just explain vs EXPLAIN it is also the explicit cast that was added

alamb · 2022-09-28T19:50:09Z

datafusion/core/tests/sql/subqueries.rs

                Filter: #part.p_name LIKE Utf8("forest%")
                  TableScan: part projection=[p_partkey, p_name], partial_filters=[#part.p_name LIKE Utf8("forest%")]
-            Projection: #lineitem.l_partkey, #lineitem.l_suppkey, Decimal128(Some(50000000000000000),38,17) * CAST(#SUM(lineitem.l_quantity) AS Decimal128(38, 17)) AS __value, alias=__sq_3
+            Projection: #lineitem.l_partkey, #lineitem.l_suppkey, CAST(Float64(0.5) AS Decimal128(38, 17)) * CAST(#SUM(lineitem.l_quantity) AS Decimal128(38, 17)) AS __value, alias=__sq_3


This seems like a regression somehow -- the CAST hasn't been evaluated into a constant

It's the desired result.
I think the evaluating of cast is done in the SimplifyExpressions rule, but the rule don't support the subquery now.
In this pr, we move the SimplifyExpressions to the front of the some rule about subquery, so the optimization of SimplifyExpressions can't apply some expr which is in the subquery.

If we want to do the evaluation cast for the expr in the subquery, there are two way:

add additional SimplifyExpressions in the tail of the optimizer rules

support subquery case for SimplifyExpressions rule.

If we don't want the regression in this pr, I can use the method of 1 to fix it temporarily.
@alamb

I think we should avoid regressions -- so perhaps we can do 1 as a temporary workaround and then support subquery in SimplifyExpressons as a follow on PR (I am trying to get some time next week to help out in some of these issues)

Ok, I will recovery them in the follow up pr

alamb · 2022-09-28T19:50:31Z

datafusion/core/tests/sql/subqueries.rs

              Aggregate: groupBy=[[#lineitem.l_partkey, #lineitem.l_suppkey]], aggr=[[SUM(#lineitem.l_quantity)]]
-                Filter: #lineitem.l_shipdate >= Date32("8766")
-                  TableScan: lineitem projection=[l_partkey, l_suppkey, l_quantity, l_shipdate], partial_filters=[#lineitem.l_shipdate >= Date32("8766")]"#
+                Filter: #lineitem.l_shipdate >= CAST(Utf8("1994-01-01") AS Date32)


likewise, this isn't right because it should have been evaluated to a constant

alamb · 2022-09-28T19:51:49Z

datafusion/optimizer/src/type_coercion.rs

-        };
+fn optimize_internal(
+    // use the external schema to handle the correlated subqueries case
+    externel_schema: &DFSchema,


Suggested change

externel_schema: &DFSchema,

external_schema: &DFSchema,

alamb · 2022-09-28T19:53:44Z

datafusion/optimizer/src/type_coercion.rs

    fn mutate(&mut self, expr: Expr) -> Result<Expr> {
        match expr {
+            Expr::ScalarSubquery(Subquery { subquery }) => {
+                let mut optimizer_config = OptimizerConfig::new();


I think we should pass through the same OptimizerConfig (so it has the same context needed to evaluate now() rather than creating new ones

After we move the type coercion out of the optimizer framework, we don't need the parameter of OptimizerConfig to iterate the plan when doing the type coercion.

I think the new of OptimizerConfig will not affect the rule of type coercion, because the rule of type coercion don't use the OptimizerConfig to generate some values.

I think when we complete this issue #3582, this parameter will be dropped.
We don't need to concern this.

alamb · 2022-09-28T19:55:54Z

datafusion/optimizer/src/type_coercion.rs

+            }
+            Expr::Exists { subquery, negated } => {
+                let mut optimizer_config = OptimizerConfig::new();
+                let new_plan = optimize_internal(


I don't understand why the outer query's schema needs to be passed while optimizing the subquery. I would expect that we would recursively optimize the subquery. The subquery doesn't have the same schema as its containing query 🤔

As in I think this should be more like:

let optimizer = TypeCoercion::new(); let new_plan = optimizer.optimize(&subquery.subquery, &mut optimizer_config)

I also want this, but there are some correlated subquery, like the comments.
I have added the comments about the usage of external_schema schema
// merge the outer schema for correlated subqueries
// like case:
// select t2.c2 from t1 where t1.c1 in (select t2.c1 from t2 where t2.c2=t1.c3)

For the subquery select t2.c1 from t2 where t2.c2= t1.c3, if we don't know the schema about the t1, we can't get the data type for t1.c3 and can't do the type coercion for t2.c2=t1.c3.

This is way that I find, do you @andygrove @alamb have any thoughts about this?

I think I would need to try it myself

alamb · 2022-09-28T19:57:50Z

datafusion/optimizer/src/type_coercion.rs

+        let result = expr.rewrite(&mut rewriter)?;
+        assert_eq!(expected, result);
+        Ok(())
+        // TODO add more test for this


I agree some more tests would be good

alamb · 2022-09-28T19:58:33Z

datafusion/optimizer/src/type_coercion.rs

+        let expr = is_true(
+            lit(ScalarValue::Int32(Some(12))).eq(lit(ScalarValue::Int64(Some(13)))),
+        );


I wonder if you can write these tests like this to make them easier to read?

Suggested change

let expr = is_true(

lit(ScalarValue::Int32(Some(12))).eq(lit(ScalarValue::Int64(Some(13)))),

);

let expr = is_true(

lit(12i32).eq(lit(12i64),

);

lit(12i32).eq(lit(13i64)),

alamb · 2022-09-28T19:58:41Z

datafusion/optimizer/tests/integration-test.rs

-        // of applying other optimizations
-        Arc::new(SimplifyExpressions::new()),
        Arc::new(PreCastLitInComparisonExpressions::new()),
+        Arc::new(TypeCoercion::new()),


…n_to_begin#3557

alamb

I think this PR is good enough to merge and we should keep hacking on it in a follow on PR

ursabot · 2022-09-29T15:03:01Z

Benchmark runs are scheduled for baseline = 9af2337 and contender = 29b8bbd. 29b8bbd 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

liukun4515 added 4 commits September 28, 2022 08:08

support subquery for type coercion

79dad7b

support subquery

fc95f62

move the type coercion to the begine of the rules

c041b90

fix all test case

9a86e7a

liukun4515 requested review from alamb and andygrove September 28, 2022 06:24

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules labels Sep 28, 2022

fix test

972449e

liukun4515 commented Sep 28, 2022

View reviewed changes

remove useless code

29449b3

github-actions bot added logical-expr Logical plan and expressions and removed logical-expr Logical plan and expressions labels Sep 28, 2022

add subquery in type coercion

7bd00d7

liukun4515 changed the title ~~move the type coercion to the beginning of the optimizer rule~~ move the type coercion to the beginning of the optimizer rule and support type coercion for subquery Sep 28, 2022

liukun4515 force-pushed the move_type_coercion_to_begin#3557 branch from be39550 to 7bd00d7 Compare September 28, 2022 10:22

alamb reviewed Sep 28, 2022

View reviewed changes

liukun4515 mentioned this pull request Sep 29, 2022

Add more test case for type coercion rule #3650

Open

liukun4515 added 4 commits September 29, 2022 08:50

address comments

904949d

fix test

2e9d18e

support case apache#3565

824ad99

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

e28cf75

…n_to_begin#3557

alamb approved these changes Sep 29, 2022

View reviewed changes

liukun4515 merged commit 29b8bbd into apache:master Sep 29, 2022

liukun4515 mentioned this pull request Sep 29, 2022

remove the type coercion in the simplify_expressions rule #3657

Merged

andygrove mentioned this pull request Oct 7, 2022

Fix optimizer regression with simplifying expressions in subquery filters #3764

Merged

alex-spies mentioned this pull request Oct 12, 2022

after type coercion CommonSubexprEliminate will produce invalid projection #3635

Closed

Comments

Conversation

liukun4515 commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liukun4515 commented Sep 28, 2022

Uh oh!

codecov-commenter commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liukun4515 Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liukun4515 Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liukun4515 Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

ursabot commented Sep 29, 2022

liukun4515 commented Sep 28, 2022 •

edited

Loading

codecov-commenter commented Sep 28, 2022 •

edited

Loading

liukun4515 Sep 29, 2022 •

edited

Loading

liukun4515 Sep 29, 2022 •

edited

Loading

liukun4515 Sep 29, 2022 •

edited

Loading