Add type coercion for UDFs in logical plan #3254

andygrove · 2022-08-24T18:19:37Z

Which issue does this PR close?

Related to #2355
Follows on from #3222 and #3250

Rationale for this change

We currently perform type coercion for UDFs in the physical planner/optimizer. I would like to have this logic in the logical plan so that other projects (such as Dask SQL) can benefit from this.

What changes are included in this PR?

Update the TypeCoercion rule to support Scalar UDFs

Are there any user-facing changes?

Yes, logical plans will be different in some cases.

codecov-commenter · 2022-08-24T19:19:52Z

Codecov Report

Merging #3254 (55c8d13) into master (c89b10f) will increase coverage by 0.01%.
The diff coverage is 97.72%.

@@            Coverage Diff             @@
##           master    #3254      +/-   ##
==========================================
+ Coverage   85.58%   85.59%   +0.01%     
==========================================
  Files         296      296              
  Lines       54179    54231      +52     
==========================================
+ Hits        46367    46418      +51     
- Misses       7812     7813       +1

Impacted Files	Coverage Δ
datafusion/optimizer/src/type_coercion.rs	`98.96% <97.72%> (-1.04%)`	⬇️
datafusion/common/src/scalar.rs	`85.06% <0.00%> (-0.10%)`	⬇️
datafusion/core/src/catalog/information_schema.rs	`94.37% <0.00%> (-0.05%)`	⬇️
datafusion/core/tests/sql/mod.rs	`97.77% <0.00%> (ø)`
datafusion/expr/src/logical_plan/plan.rs	`77.28% <0.00%> (ø)`
datafusion/core/src/physical_plan/explain.rs	`66.66% <0.00%> (ø)`
datafusion/physical-expr/src/aggregate/sum.rs	`69.79% <0.00%> (ø)`
datafusion/physical-expr/src/array_expressions.rs	`50.00% <0.00%> (ø)`
datafusion/physical-expr/src/expressions/binary.rs	`97.71% <0.00%> (ø)`
...tafusion/physical-expr/src/expressions/try_cast.rs	`98.79% <0.00%> (ø)`
... and 4 more

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

andygrove · 2022-09-06T14:09:47Z

@alamb Here is another type coercion rule that we can now add

alamb

👍 looks great @andygrove

This will likely conflict with #3379

For what it is worth, I would love to move all type coercion rules out of physical planning and into this phase (aka consolidate all coercion)

alamb · 2022-09-06T14:53:17Z

datafusion/optimizer/src/type_coercion.rs

+/// `signature`, if possible.
+///
+/// See the module level documentation for more detail on coercion.
+pub fn coerce_arguments_for_signature(


I feel like this code should already exist somewhere and it would be great to consolidate into a single implementation rather than have multiple implementations around

Yes, this is adapted from the coerce method in the physical-expr crate that operates on physical expressions rather than logical expressions.

Do we need to public this method?
I think it's better to make it private.

alamb · 2022-09-06T14:53:39Z

datafusion/optimizer/src/type_coercion.rs

+        let mut config = OptimizerConfig::default();
+        let plan = rule.optimize(&plan, &mut config)?;
+        assert_eq!(
+            "Projection: TestScalarUDF(CAST(Int32(123) AS Float32))\n  EmptyRelation",


liukun4515 · 2022-09-06T14:58:35Z

@andygrove @alamb Can we give a plan for the migration of type coercion?

liukun4515 · 2022-09-06T14:59:50Z

👍 looks great @andygrove

This will likely conflict with #3379

For what it is worth, I would love to move all type coercion rules out of physical planning and into this phase (aka consolidate all coercion)

we can do this after migrating the type coercion from the physical phase to logical phase.

liukun4515 · 2022-09-06T15:00:23Z

I will review it tomorrow, it's too later for me today

alamb · 2022-09-07T10:27:25Z

@andygrove @alamb Can we give a plan for the migration of type coercion?

@liukun4515 there is #2355 (which I will also add to the description of this PR). Is that what you had in mind?

alamb

I think this looks good -- let's wait for @liukun4515 to review and then merge it in

liukun4515

LGTM
only comments about public/private api

liukun4515 · 2022-09-07T10:31:19Z

datafusion/optimizer/src/type_coercion.rs

+/// `signature`, if possible.
+///
+/// See the module level documentation for more detail on coercion.
+pub fn coerce_arguments_for_signature(


Do we need to public this method?
I think it's better to make it private.

liukun4515 · 2022-09-07T10:37:17Z

After the refactor of the type coercion, do we need to forbid the creation of physical expr directly？

alamb · 2022-09-07T10:40:37Z

After the refactor of the type coercion, do we need to forbid to creation of physical expr directly？

I would personally prefer allow creating PhysicalExprs but not providing automatic coercion (leaving it up to the user to use the proper types)

liukun4515 · 2022-09-07T11:02:35Z

After the refactor of the type coercion, do we need to forbid to creation of physical expr directly？

I would personally prefer allow creating PhysicalExprs but not providing automatic coercion (leaving it up to the user to use the proper types)

A+B will get the common data type C in the logical phase, but it will also affected in the physical phase.
C+C will convert the D data type, but I can't find this changes after this #3222 merged.
I want to find out why

liukun4515 · 2022-09-07T12:48:13Z

After the refactor of the type coercion, do we need to forbid to creation of physical expr directly？

I would personally prefer allow creating PhysicalExprs but not providing automatic coercion (leaving it up to the user to use the proper types)

A+B will get the common data type C in the logical phase, but it will also affected in the physical phase. C+C will convert the D data type, but I can't find this changes after this #3222 merged. I want to find out why

I think I find a bug https://github.com/apache/arrow-datafusion/blob/c359018baa8bbb0a227e83df948c903cde4d701f/datafusion/expr/src/binary_rule.rs#L293 for the type coercion in arithmetic op.

@andygrove @alamb

If we move the type coercion to the logical phase, the type coercion will apply the binary op twice.
For the arithmetic operation, the decimal128(10,0) + decimal128(10,0) will get decimal128(11,0).
If the rule was applied to the op twice, the result type should be decimal(12,0);

andygrove · 2022-09-07T14:04:35Z

After the refactor of the type coercion, do we need to forbid to creation of physical expr directly？

I would personally prefer allow creating PhysicalExprs but not providing automatic coercion (leaving it up to the user to use the proper types)

A+B will get the common data type C in the logical phase, but it will also affected in the physical phase. C+C will convert the D data type, but I can't find this changes after this #3222 merged. I want to find out why

I think I find a bug

https://github.com/apache/arrow-datafusion/blob/c359018baa8bbb0a227e83df948c903cde4d701f/datafusion/expr/src/binary_rule.rs#L293
for the type coercion in arithmetic op.

@andygrove @alamb

If we move the type coercion to the logical phase, the type coercion will apply the binary op twice. For the arithmetic operation, the decimal128(10,0) + decimal128(10,0) will get decimal128(11,0). If the rule was applied to the op twice, the result type should be decimal(12,0);

Thanks @liukun4515. I have filed #3388 to track this. I will look at this next.

ursabot · 2022-09-07T16:04:52Z

Benchmark runs are scheduled for baseline = e6d1364 and contender = 7c04964. 7c04964 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 core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions labels Aug 24, 2022

andygrove mentioned this pull request Aug 24, 2022

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

Closed

24 tasks

andygrove mentioned this pull request Aug 25, 2022

[DF] Add TypeCoercion optimizer rule dask-contrib/dask-sql#723

Merged

Add type coercion for scalar UDFs

141d2b0

andygrove force-pushed the type-coercion-udf branch from e6b4df3 to 141d2b0 Compare September 6, 2022 14:08

andygrove changed the title ~~Early draft: Add type coercion for UDFs in logical plan~~ Add type coercion for UDFs in logical plan Sep 6, 2022

github-actions bot removed physical-expr Physical Expressions logical-expr Logical plan and expressions core Core DataFusion crate labels Sep 6, 2022

andygrove marked this pull request as ready for review September 6, 2022 14:09

andygrove requested a review from liukun4515 September 6, 2022 14:10

alamb approved these changes Sep 6, 2022

View reviewed changes

andygrove added 3 commits September 6, 2022 09:01

clippy

e9bc400

upmerge

f24acdc

clippy

6b37258

alamb mentioned this pull request Sep 7, 2022

[Epic] Type-coercion improvements #2355

Closed

16 tasks

alamb approved these changes Sep 7, 2022

View reviewed changes

liukun4515 approved these changes Sep 7, 2022

View reviewed changes

liukun4515 mentioned this pull request Sep 7, 2022

Need to type coercion for arithmetic op when the left and right type are same #3387

Closed

make coerce_arguments_for_signature private

55c8d13

andygrove mentioned this pull request Sep 7, 2022

Remove type-coercion from physical planner #3388

Closed

andygrove merged commit 7c04964 into apache:master Sep 7, 2022

andygrove deleted the type-coercion-udf branch September 7, 2022 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type coercion for UDFs in logical plan #3254

Add type coercion for UDFs in logical plan #3254

andygrove commented Aug 24, 2022 •

edited by alamb

Loading

codecov-commenter commented Aug 24, 2022 •

edited

Loading

andygrove commented Sep 6, 2022

alamb left a comment

alamb Sep 6, 2022

andygrove Sep 6, 2022

liukun4515 Sep 7, 2022

alamb Sep 6, 2022

liukun4515 commented Sep 6, 2022

liukun4515 commented Sep 6, 2022

liukun4515 commented Sep 6, 2022

alamb commented Sep 7, 2022

alamb left a comment

liukun4515 left a comment

liukun4515 Sep 7, 2022

liukun4515 commented Sep 7, 2022 •

edited

Loading

alamb commented Sep 7, 2022

liukun4515 commented Sep 7, 2022

liukun4515 commented Sep 7, 2022

andygrove commented Sep 7, 2022

ursabot commented Sep 7, 2022

Add type coercion for UDFs in logical plan #3254

Add type coercion for UDFs in logical plan #3254

Conversation

andygrove commented Aug 24, 2022 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Aug 24, 2022 • edited Loading

Codecov Report

andygrove commented Sep 6, 2022

alamb left a comment

Choose a reason for hiding this comment

alamb Sep 6, 2022

Choose a reason for hiding this comment

andygrove Sep 6, 2022

Choose a reason for hiding this comment

liukun4515 Sep 7, 2022

Choose a reason for hiding this comment

alamb Sep 6, 2022

Choose a reason for hiding this comment

liukun4515 commented Sep 6, 2022

liukun4515 commented Sep 6, 2022

liukun4515 commented Sep 6, 2022

alamb commented Sep 7, 2022

alamb left a comment

Choose a reason for hiding this comment

liukun4515 left a comment

Choose a reason for hiding this comment

liukun4515 Sep 7, 2022

Choose a reason for hiding this comment

liukun4515 commented Sep 7, 2022 • edited Loading

alamb commented Sep 7, 2022

liukun4515 commented Sep 7, 2022

liukun4515 commented Sep 7, 2022

andygrove commented Sep 7, 2022

ursabot commented Sep 7, 2022

andygrove commented Aug 24, 2022 •

edited by alamb

Loading

codecov-commenter commented Aug 24, 2022 •

edited

Loading

liukun4515 commented Sep 7, 2022 •

edited

Loading