Add type coercion rule for `concat` and `concat_ws` #3721

HaoYang670 · 2022-10-05T10:49:55Z

Which issue does this PR close?

Closes #3720.

Rationale for this change

Before

❯ create table t as select 1 as a;

❯ explain verbose select concat(a, 'utf8', true, false, null, 1, 0.2) from t;
+------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type                                                  | plan                                                                                                                                                                                                                                        |
+------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| initial_logical_plan                                       | Projection: concat(t.a, Utf8("utf8"), Boolean(true), Boolean(false), NULL, Int64(1), Float64(0.2))                                                                                                                                          |
|                                                            |   TableScan: t                                                                                                                                                                                                                              |
| logical_plan after type_coercion                           | SAME TEXT AS ABOVE                                                                                                                                                                                                                          |
| logical_plan after simplify_expressions                    | SAME TEXT AS ABOVE                                                                                                                                                                                                                          |
| logical_plan after unwrap_cast_in_comparison               | SAME TEXT AS ABOVE 
...

After

❯ create table t as select 1 as a;

❯ explain verbose select concat(a, 'utf8', true, false, null, 1, 0.2) from t;
+------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type                                                  | plan                                                                                                                                                                                              |
+------------------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| initial_logical_plan                                       | Projection: concat(t.a, Utf8("utf8"), Boolean(true), Boolean(false), NULL, Int64(1), Float64(0.2))                                                                                                |
|                                                            |   TableScan: t                                                                                                                                                                                    |
| logical_plan after type_coercion                           | Projection: concat(CAST(t.a AS Utf8), Utf8("utf8"), CAST(Boolean(true) AS Utf8), CAST(Boolean(false) AS Utf8), CAST(NULL AS Utf8), CAST(Int64(1) AS Utf8), CAST(Float64(0.2) AS Utf8))            |
|                                                            |   TableScan: t                                                                                                                                                                                    |
| logical_plan after simplify_expressions                    | Projection: concat(CAST(t.a AS Utf8), Utf8("utf8"), Utf8("1"), Utf8("0"), Utf8(NULL), Utf8("1"), Utf8("0.2")) AS concat(t.a,Utf8("utf8"),Boolean(true),Boolean(false),NULL,Int64(1),Float64(0.2)) |
|                                                            |   TableScan: t                                                                                                                                                                                    |
| logical_plan after unwrap_cast_in_comparison               | SAME TEXT AS ABOVE
...

What changes are included in this PR?

Are there any user-facing changes?

Signed-off-by: remzi <[email protected]>

…n_concat

Signed-off-by: remzi <[email protected]>

andygrove · 2022-10-05T13:14:40Z

Thanks @HaoYang670. This is looking good.

I ran the example query in Postgres and compared it with DataFusion:

Postgres

postgres=# select concat(a, 'utf8', true, false, null, 1, 0.2) from t;select concat(a, 'utf8', true, false, null, 1, 0.2) from t;
   concat    
-------------
 1utf8tf10.2

DataFusion

❯ select concat(a, 'utf8', true, false, null, 1, 0.2) from t;
+----------------------------------------------------------------------------------+
| concat(t.a,Utf8("utf8"),Boolean(true),Boolean(false),NULL,Int64(1),Float64(0.2)) |
+----------------------------------------------------------------------------------+
| 1utf81010.2                                                                      |
+----------------------------------------------------------------------------------+

I have two observations:

Postgres produces t and f when casting bool to string and we are producing 1 and 0
The schema name for the expression looks much nicer in Postgres. Perhaps we can change the Expr::name implementation for concat or scalar functions in general - that is unrelated to this issue though, so I filed Use simpler schema names for expressions #3722 for this

HaoYang670 · 2022-10-05T13:38:52Z

Postgres produces t and f when casting bool to string and we are producing 1 and 0

Hmm, this is also the behavior of the cast kernel in arrow-rs: https://github.com/apache/arrow-rs/blob/master/arrow/src/compute/kernels/cast.rs#L704

andygrove · 2022-10-05T13:49:49Z

Postgres produces t and f when casting bool to string and we are producing 1 and 0

Hmm, this is also the behavior of the cast kernel in arrow-rs: https://github.com/apache/arrow-rs/blob/master/arrow/src/compute/kernels/cast.rs#L704

I think 1 and 0 makes sense for Arrow. In DataFusion we'll need to add additional logic to match Postgres.

alamb

Thank you @HaoYang670

Postgres produces t and f when casting bool to string and we are producing 1 and 0

Hmm, this is also the behavior of the cast kernel in arrow-rs:

I think 1 and 0 makes sense for Arrow. In DataFusion we'll need to add additional logic to match Postgres.

I don't think this PR changes (or should change) the behavior of how booleans are cast to strings. I recommend we file a follow on issue / PR to sort that out.

datafusion/optimizer/src/type_coercion.rs

alamb · 2022-10-05T21:34:10Z

datafusion/optimizer/src/type_coercion.rs

+            Expr::ScalarFunction { fun, args } => match fun {
+                BuiltinScalarFunction::Concat
+                | BuiltinScalarFunction::ConcatWithSeparator => {
+                    let new_args = args


I wonder if we should do something with LargeUtf8?

Also, would it make sense to check the types before clone()ing them to do a cast that might not be needed?

would it make sense to check the types before clone()ing them to do a cast that might not be needed?

I think this has been done in the cast_to function:

fn cast_to<S: ExprSchema>(self, cast_to_type: &DataType, schema: &S) -> Result<Expr> { // TODO(kszucs): most of the operations do not validate the type correctness // like all of the binary expressions below. Perhaps Expr should track the // type of the expression? let this_type = self.get_type(schema)?; if this_type == *cast_to_type { Ok(self) ... }

I wonder if we should do something with LargeUtf8?

This is a good suggestion. My opinion is that we could use LargeUtf8 if one of the arguments has this type.

Is there type coercion rule for the function Concat or ConcatWithSeparator?

Now the type coercion are not supported in the logical phase for some expr which is Expr::ScalarFunction, Expr::AggregateFunction,Expr::WindowFunction and Expr::AggregateUDF in the follow-up pr for this #3582 (comment)

I think after moving the type coercion rule to the logical phase, this issue can be resolved

I am not sure what you mean about "type coercion rule for the function Concat or ConcatWithSeparator"

Since it is a Expr::ScalarFunction { fun, args } it currently gets coerced using data_types https://github.com/apache/arrow-datafusion/blob/3eb55e9a0510d872f6f7765b1a5f17db46486e45/datafusion/expr/src/type_coercion.rs#L44-L47

Are you suggesting we move the logic that picks what argument types (in this case string) for concat into data_types? (I think this is a good idea, for the record)

Yes, we can do it in the #3582 (comment)

And this pr can be merged first.

…n_concat

Signed-off-by: remzi <[email protected]>

liukun4515

LGTM
But I will move the type coercion for Expr:: ScalarFunction to logical phase, and remove the type coercion in the physical phase.

liukun4515 · 2022-10-06T11:42:12Z

@HaoYang670 Sorry for that commit aa0d14c
Can you help to fix this?

alamb · 2022-10-06T12:51:46Z

I pushed c6e1208 both to fix this PR as well as test to see if github actions are fixed yet.

Seems they are not :(

…n_concat

alamb · 2022-10-07T14:55:16Z

Thanks again @HaoYang670

ursabot · 2022-10-07T15:02:17Z

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

concat type coercion

14e145a

Signed-off-by: remzi <[email protected]>

HaoYang670 marked this pull request as draft October 5, 2022 10:50

github-actions bot added the optimizer Optimizer rules label Oct 5, 2022

HaoYang670 added 2 commits October 5, 2022 19:56

Merge remote-tracking branch 'upstream/master' into 3720_type_coercio…

586329a

…n_concat

fix test

a459c58

Signed-off-by: remzi <[email protected]>

HaoYang670 marked this pull request as ready for review October 5, 2022 12:06

alamb approved these changes Oct 5, 2022

View reviewed changes

HaoYang670 added 2 commits October 6, 2022 09:28

Merge remote-tracking branch 'upstream/master' into 3720_type_coercio…

1adfe58

…n_concat

use builder function

354be11

Signed-off-by: remzi <[email protected]>

liukun4515 approved these changes Oct 6, 2022

View reviewed changes

liukun4515 mentioned this pull request Oct 6, 2022

move the type coercion out of the optimizer and refactor the optimizer #3582

Closed

liukun4515 approved these changes Oct 6, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into 3720_type_coercio…

63c2587

…n_concat

HaoYang670 force-pushed the 3720_type_coercion_concat branch from c6e1208 to 63c2587 Compare October 6, 2022 23:53

alamb merged commit d863853 into apache:master Oct 7, 2022

HaoYang670 deleted the 3720_type_coercion_concat branch October 7, 2022 23:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type coercion rule for `concat` and `concat_ws` #3721

Add type coercion rule for `concat` and `concat_ws` #3721

HaoYang670 commented Oct 5, 2022 •

edited

Loading

andygrove commented Oct 5, 2022

HaoYang670 commented Oct 5, 2022

andygrove commented Oct 5, 2022

alamb left a comment

alamb Oct 5, 2022

HaoYang670 Oct 6, 2022

HaoYang670 Oct 6, 2022

liukun4515 Oct 6, 2022

liukun4515 Oct 6, 2022

alamb Oct 6, 2022

liukun4515 Oct 6, 2022

liukun4515 left a comment

liukun4515 commented Oct 6, 2022

alamb commented Oct 6, 2022

alamb commented Oct 7, 2022

ursabot commented Oct 7, 2022

Add type coercion rule for concat and concat_ws #3721

Add type coercion rule for concat and concat_ws #3721

Conversation

HaoYang670 commented Oct 5, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

Before

After

What changes are included in this PR?

Are there any user-facing changes?

andygrove commented Oct 5, 2022

Postgres

DataFusion

HaoYang670 commented Oct 5, 2022

andygrove commented Oct 5, 2022

alamb left a comment

Choose a reason for hiding this comment

alamb Oct 5, 2022

Choose a reason for hiding this comment

HaoYang670 Oct 6, 2022

Choose a reason for hiding this comment

HaoYang670 Oct 6, 2022

Choose a reason for hiding this comment

liukun4515 Oct 6, 2022

Choose a reason for hiding this comment

liukun4515 Oct 6, 2022

Choose a reason for hiding this comment

alamb Oct 6, 2022

Choose a reason for hiding this comment

liukun4515 Oct 6, 2022

Choose a reason for hiding this comment

liukun4515 left a comment

Choose a reason for hiding this comment

liukun4515 commented Oct 6, 2022

alamb commented Oct 6, 2022

alamb commented Oct 7, 2022

ursabot commented Oct 7, 2022

Add type coercion rule for `concat` and `concat_ws` #3721

Add type coercion rule for `concat` and `concat_ws` #3721

HaoYang670 commented Oct 5, 2022 •

edited

Loading