Cast `Utf8View` to `Utf8` to support `||` from `StringViewArray` by dharanad · Pull Request #11796 · apache/datafusion

dharanad · 2024-08-03T20:25:20Z

Which issue does this PR close?

Partially make #11766 work.

Rationale for this change

What changes are included in this PR?

coerced Utf8View to Utf8 make concat work
Added slt tests cases

Are these changes tested?

Existing test cases

carog test

Are there any user-facing changes?

alamb

Thanks @dharanad -- this is ceratinly better than what happens on main (which is the query doesn't run)

I do think the plans could be improved if they cast to Utf8Vew instead, but we can do that as a follow on PR as well

alamb · 2024-08-05T20:41:56Z

datafusion/expr/src/type_coercion/binary.rs

-        (LargeUtf8, from_type) | (from_type, LargeUtf8) => {
-            string_concat_internal_coercion(from_type, &LargeUtf8)
+    match (lhs_type, rhs_type) {
+        // If Utf8View is in any side, we coerce to Utf8.


I think it would be better to coerce to Utf8View as that coercsion will often be faster (it is faster to cast Utf8 -> Utf8View than the other way around)

Is that possible?

Agree, similar to this policy:

datafusion/datafusion/expr/src/type_coercion/binary.rs

Lines 935 to 947 in b50887f

fn string_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {

use arrow::datatypes::DataType::*;

match (lhs_type, rhs_type) {

// If Utf8View is in any side, we coerce to Utf8View.

(Utf8View, Utf8View | Utf8 | LargeUtf8) | (Utf8 | LargeUtf8, Utf8View) => {

Some(Utf8View)

}

// Then, if LargeUtf8 is in any side, we coerce to LargeUtf8.

(LargeUtf8, Utf8 | LargeUtf8) | (Utf8, LargeUtf8) => Some(LargeUtf8),

(Utf8, Utf8) => Some(Utf8),

_ => None,

}

}

I think it would be better to coerce to Utf8View as that coercsion will often be faster (it is faster to cast Utf8 -> Utf8View than the other way around)

Is that possible?

How about we do in a seperate PR. Previously, we were coerced to Utf8View, so concat was failing. As a temporary workaround to resolve the issue, I've coerce Utf8 instead.

Make sense!

filed #11881 to track

alamb · 2024-08-05T20:42:35Z

datafusion/sqllogictest/test_files/string_view.slt

+select column2||' is fast' from temp;
+----
+rust is fast
+datafusion is fast


alamb · 2024-08-05T20:43:21Z

datafusion/sqllogictest/test_files/string_view.slt

+explain select column2 || 'is' || column3 from temp;
+----
+logical_plan
+01)Projection: CAST(temp.column2 AS Utf8) || Utf8("is") || CAST(temp.column3 AS Utf8)


This would be a better plan (likely faster) if the casting was to Utf8View rather than Utf8

alamb · 2024-08-05T20:43:47Z

datafusion/sqllogictest/test_files/string_view.slt

+explain select column2||' is fast' from temp;
+----
+logical_plan
+01)Projection: CAST(temp.column2 AS Utf8) || Utf8(" is fast")


likewise here it would be better to use Utf8View

dharanad · 2024-08-07T19:44:16Z

@alamb Are we good here ? Or do you have more suggestions for this PR
I'm actively investigating alternative solutions for this problem here #11836

alamb · 2024-08-07T22:02:48Z

@alamb Are we good here ? Or do you have more suggestions for this PR I'm actively investigating alternative solutions for this problem here #11836

Nope, sorry @dharanad -- this is good. I am behind on merging / upstream reviews.

alamb · 2024-08-07T22:06:11Z

Thanks again @dharanad and @XiangpengHao -- sorry for the delay

make query work

a3c239b

github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Aug 3, 2024

dharanad added 2 commits August 5, 2024 12:56

hack string_concat_coercion

610fdec

more tests

b50887f

dharanad changed the title ~~[WIP] Cast Utf8View to Utf8 to support || from StringViewArray~~ Cast Utf8View to Utf8 to support || from StringViewArray Aug 5, 2024

dharanad marked this pull request as ready for review August 5, 2024 10:21

alamb approved these changes Aug 5, 2024

View reviewed changes

alamb mentioned this pull request Aug 7, 2024

Cast Utf8 -> Utf8View (not the other way around) for binary operators #11881

Closed

alamb merged commit 60d1d3a into apache:main Aug 7, 2024

dharanad deleted the fix/11766 branch August 13, 2024 14:11

	fn string_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {
	use arrow::datatypes::DataType::*;
	match (lhs_type, rhs_type) {
	// If Utf8View is in any side, we coerce to Utf8View.
	(Utf8View, Utf8View \| Utf8 \| LargeUtf8) \| (Utf8 \| LargeUtf8, Utf8View) => {
	Some(Utf8View)
	}
	// Then, if LargeUtf8 is in any side, we coerce to LargeUtf8.
	(LargeUtf8, Utf8 \| LargeUtf8) \| (Utf8, LargeUtf8) => Some(LargeUtf8),
	(Utf8, Utf8) => Some(Utf8),
	_ => None,
	}
	}

Comments

Conversation

dharanad commented Aug 3, 2024 • 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 these changes tested?

Are there any user-facing changes?

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!

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!

dharanad commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Aug 7, 2024

Uh oh!

alamb commented Aug 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dharanad commented Aug 3, 2024 •

edited

Loading

dharanad commented Aug 7, 2024 •

edited

Loading