inlist: move type coercion to logical phase by liukun4515 · Pull Request #3472 · apache/datafusion

liukun4515 · 2022-09-14T05:59:53Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

liukun4515 · 2022-09-14T06:00:43Z

-                let (cast_expr, cast_list_exprs) =
-                    in_list_cast(value_expr, list_exprs, input_schema)?;
-                expressions::in_list(cast_expr, cast_list_exprs, negated, input_schema)
+                expressions::in_list(value_expr, list_exprs, negated, input_schema)


type coercion done in the logical phase

liukun4515 · 2022-09-14T06:01:09Z

    negated: &bool,
    schema: &Schema,
 ) -> Result<Arc<dyn PhysicalExpr>> {
+    // check the data type


check the data type and make sure the same type for value expr and list exprs.

liukun4515 · 2022-09-14T06:03:57Z

+                    .iter()
+                    .map(|list_expr| list_expr.get_type(&self.schema))
+                    .collect::<Result<Vec<_>>>()?;
+                let result_type =


Do we need a common function in a new file like in_list_rule.rs to do the coerce_types?
cc @alamb @andygrove

it is also can be used in the in_list.rs for physical inlist test cases.

I am not sure what you are asking @liukun4515

In general I think trying to centralize the coercion rules into the same place will make the code easier to reason about and make the coercion logic easier to find

I will a issue to track this and centralize the code together for the inlist

liukun4515 · 2022-09-14T06:05:23Z

        let mut session_state = make_session_state();
        session_state.config.target_partitions = 4;
+        // optimize the logical plan
+        let logical_plan = session_state.optimize(logical_plan)?;


the type coercion is done in the optimization

codecov-commenter · 2022-09-14T06:50:07Z

Codecov Report

Merging #3472 (22da6f0) into master (9fbee1a) will increase coverage by 0.00%.
The diff coverage is 97.56%.

@@           Coverage Diff           @@
##           master    #3472   +/-   ##
=======================================
  Coverage   85.73%   85.73%           
=======================================
  Files         299      299           
  Lines       55170    55219   +49     
=======================================
+ Hits        47298    47344   +46     
- Misses       7872     7875    +3

Impacted Files	Coverage Δ
...atafusion/physical-expr/src/expressions/in_list.rs	`85.19% <91.30%> (+0.19%)`	⬆️
datafusion/core/src/physical_plan/planner.rs	`77.31% <100.00%> (+0.06%)`	⬆️
datafusion/optimizer/src/type_coercion.rs	`93.56% <100.00%> (+2.16%)`	⬆️
datafusion/physical-expr/src/planner.rs	`92.36% <100.00%> (-0.33%)`	⬇️
...tafusion/core/src/physical_plan/file_format/csv.rs	`93.54% <0.00%> (-0.93%)`	⬇️
datafusion/common/src/scalar.rs	`85.11% <0.00%> (-0.07%)`	⬇️
datafusion/expr/src/logical_plan/plan.rs	`77.68% <0.00%> (ø)`
datafusion/expr/src/window_frame.rs	`93.27% <0.00%> (+0.84%)`	⬆️

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

andygrove · 2022-09-14T14:40:34Z

            .build()?;
        let execution_plan = plan(&logical_plan).await?;
-        let expected = "expr: [(InListExpr { expr: Column { name: \"c1\", index: 0 }, list: [Literal { value: Utf8(\"a\") }, TryCastExpr { expr: Literal { value: Int64(1) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(2) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(3) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(4) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(5) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(6) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(7) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(8) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(9) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(10) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(11) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(12) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(13) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(14) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(15) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(16) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(17) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(18) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(19) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(20) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(21) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(22) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(23) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(24) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(25) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(26) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(27) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(28) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(29) }, cast_type: Utf8 }, TryCastExpr { expr: Literal { value: Int64(30) }, cast_type: Utf8 }], negated: false, set: Some(InSet { set: ";
+        let expected = "expr: [(InListExpr { expr: Column { name: \"c1\", index: 0 }, list: [Literal { value: Utf8(\"a\") }, Literal { value: Utf8(\"1\") }, Literal { value: Utf8(\"2\") },";


Well that is much easier to read ❤️

yes, i only show the prefix and suffix of the output

andygrove

LGTM. Thanks @liukun4515

ursabot · 2022-09-14T15:12:24Z

Benchmark runs are scheduled for baseline = 0388682 and contender = f3bb84f. f3bb84f 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

alamb

This looks great @liukun4515

alamb · 2022-09-14T18:58:35Z

        // the cast here is implicit so has CastOptions with safe=true
-        let expected = "BinaryExpr { left: Column { name: \"c7\", index: 6 }, op: Lt, right: TryCastExpr { expr: Literal { value: UInt8(5) }, cast_type: Int64 } }";
-        assert!(format!("{:?}", plan).contains(expected));
+        let expected = "BinaryExpr { left: Column { name: \"c7\", index: 2 }, op: Lt, right: Literal { value: Int64(5) } }";


these plans are looking very nice now 👌 Nice work @liukun4515

that optimization was done by @Dandandan in #3444

alamb · 2022-09-14T18:59:15Z

+                    .iter()
+                    .map(|list_expr| list_expr.get_type(&self.schema))
+                    .collect::<Result<Vec<_>>>()?;
+                let result_type =


I am not sure what you are asking @liukun4515

In general I think trying to centralize the coercion rules into the same place will make the code easier to reason about and make the coercion logic easier to find

@overload

liukun4515 requested review from alamb and andygrove and removed request for alamb September 14, 2022 05:59

github-actions Bot added core Core DataFusion crate optimizer Optimizer rules physical-expr Changes to the physical-expr crates labels Sep 14, 2022

liukun4515 commented Sep 14, 2022

View reviewed changes

liukun4515 mentioned this pull request Sep 14, 2022

remove type coercion in the binary physical expr #3396

Merged

inlist: move type coercion to logical phase

22da6f0

liukun4515 force-pushed the inlist_type_coercion_#3468 branch from b320f9a to 22da6f0 Compare September 14, 2022 06:15

liukun4515 mentioned this pull request Sep 14, 2022

Union columns with different types #3467

Closed

andygrove reviewed Sep 14, 2022

View reviewed changes

andygrove approved these changes Sep 14, 2022

View reviewed changes

Dandandan approved these changes Sep 14, 2022

View reviewed changes

Dandandan merged commit f3bb84f into apache:master Sep 14, 2022

alamb reviewed Sep 14, 2022

View reviewed changes

liukun4515 mentioned this pull request Sep 15, 2022

centralize the type coercion code for InList rule #3491

Open

Navigation Menu

Uh oh!

inlist: move type coercion to logical phase - #3472

inlist: move type coercion to logical phase#3472
Dandandan merged 1 commit into
apache:masterfrom
liukun4515:inlist_type_coercion_#3468

liukun4515 commented Sep 14, 2022

liukun4515 Sep 14, 2022

liukun4515 Sep 14, 2022

liukun4515 Sep 14, 2022

liukun4515 Sep 14, 2022

alamb Sep 14, 2022

liukun4515 Sep 15, 2022

liukun4515 Sep 14, 2022 •

edited

Loading

codecov-commenter commented Sep 14, 2022

andygrove Sep 14, 2022

liukun4515 Sep 15, 2022

andygrove left a comment

ursabot commented Sep 14, 2022

alamb left a comment

alamb Sep 14, 2022

liukun4515 Sep 15, 2022

alamb Sep 14, 2022

Labels

6 participants

Uh oh!

Conversation

liukun4515 commented Sep 14, 2022

Which issue does this PR close?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

codecov-commenter commented Sep 14, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove left a comment

Choose a reason for hiding this comment

ursabot commented Sep 14, 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

Labels

6 participants

liukun4515 Sep 14, 2022 •

edited

Loading