Update the CONCAT scalar function to support Utf8View #12224

devanbenz · 2024-08-29T02:01:54Z

Which issue does this PR close?

Closes #11836

Rationale for this change

Explained within ticket #11836

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: Devan <devandbenz@gmail.com>

tshauck

one minor comment regarding LargeUtf8, but overall LGTM

tshauck · 2024-08-29T20:12:31Z

datafusion/functions/src/string/concat.rs

@@ -46,7 +45,7 @@ impl ConcatFunc {
    pub fn new() -> Self {
        use DataType::*;
        Self {
-            signature: Signature::variadic(vec![Utf8], Volatility::Immutable),
+            signature: Signature::variadic(vec![Utf8, Utf8View], Volatility::Immutable),


Perhaps as a follow on PR we can expand this to also support LargeUtf8s... though it looks like maybe they are from the tests? Does this just need to be updated and it'll work?

I think it's casting to Utf8 from LargeUtf8? Not sure. I can follow up and modify this code to include that though it should be relatively quick.

https://github.com/apache/datafusion/pull/12224/files#diff-71970189679c6dd5b3b677bb21603234b488e68d1601be9c4d400d40e430a909R70 pretty sure this line captures the LargeUtf8 usage at the moment.

Sorry, should've been clearer, to your point, I think it works but causes a cast to utf8.

> CREATE TABLE ttt AS SELECT arrow_cast('T', 'LargeUtf8') AS a; 0 row(s) fetched. Elapsed 0.012 seconds. > SELECt ttt.a FROM ttt; +---+ | a | +---+ | T | +---+ 1 row(s) fetched. Elapsed 0.007 seconds. > EXPLAIN SELECt ttt.a FROM ttt; +---------------+-----------------------------------------------+ | plan_type | plan | +---------------+-----------------------------------------------+ | logical_plan | TableScan: ttt projection=[a] | | physical_plan | MemoryExec: partitions=1, partition_sizes=[1] | | | | +---------------+-----------------------------------------------+ 2 row(s) fetched. Elapsed 0.007 seconds. > EXPLAIN SELECt concat(ttt.a, ttt.a) FROM ttt; +---------------+--------------------------------------------------------------------------------------------+ | plan_type | plan | +---------------+--------------------------------------------------------------------------------------------+ | logical_plan | Projection: concat(__common_expr_1 AS ttt.a, __common_expr_1 AS ttt.a) | | | Projection: CAST(ttt.a AS Utf8) AS __common_expr_1 | | | TableScan: ttt projection=[a] | | physical_plan | ProjectionExec: expr=[concat(__common_expr_1@0, __common_expr_1@0) as concat(ttt.a,ttt.a)] | | | ProjectionExec: expr=[CAST(a@0 AS Utf8) as __common_expr_1] | | | MemoryExec: partitions=1, partition_sizes=[1] | | | | +---------------+--------------------------------------------------------------------------------------------+

I think if you were to concat two largeutf8s together, you should get that same type as the return?

Correct - I will need to modify the input/return values for this. I'll add an additional test for LargeUtf8 and fix in this PR tonight. It shouldn't take long, I think I have a path forward to change that and it should be relatively painless.

Correction** I fixed it now it was really quick resolution lol

> SELECT ttt.a from ttt; +---+ | a | +---+ | T | +---+ 1 row(s) fetched. Elapsed 0.009 seconds. > EXPLAIN SELECT ttt.a from ttt; +---------------+-----------------------------------------------+ | plan_type | plan | +---------------+-----------------------------------------------+ | logical_plan | TableScan: ttt projection=[a] | | physical_plan | MemoryExec: partitions=1, partition_sizes=[1] | | | | +---------------+-----------------------------------------------+ 2 row(s) fetched. Elapsed 0.007 seconds. > EXPLAIN SELECT concat(ttt.a, ttt.a) from ttt; +---------------+----------------------------------------------------------------+ | plan_type | plan | +---------------+----------------------------------------------------------------+ | logical_plan | Projection: concat(ttt.a, ttt.a) | | | TableScan: ttt projection=[a] | | physical_plan | ProjectionExec: expr=[concat(a@0, a@0) as concat(ttt.a,ttt.a)] | | | MemoryExec: partitions=1, partition_sizes=[1] | | | | +---------------+----------------------------------------------------------------+ 2 row(s) fetched. Elapsed 0.009 seconds.

Actually it's erroring on cast to generic string array. I need to modify 😆 whoops still not fixed

It's resolved 👍

tshauck · 2024-08-29T20:15:10Z

datafusion/sqllogictest/test_files/string_view.slt

@@ -776,7 +776,7 @@ EXPLAIN SELECT
 FROM test;
 ----
 logical_plan
-01)Projection: concat(CAST(test.column1_utf8view AS Utf8), CAST(test.column2_utf8view AS Utf8)) AS c
+01)Projection: concat(test.column1_utf8view, test.column2_utf8view) AS c


datafusion/functions/src/string/concat.rs

findepi · 2024-08-29T21:27:22Z

datafusion/functions/src/string/concat.rs

+        Ok(match &arg_types[0] {
+            Utf8View => Utf8View,
+            LargeUtf8 => LargeUtf8,
+            _ => Utf8,
+        })


the logic seems to assume all arguments are of the same type?

also, why not always return Utf8?
the code performing actual concatenation seems to be always the same.

Ah yeah @findepi I think the logic is "Whatever the first argument type is the output should be of that type" so if the received values were: Utf8, Utf8View the output would be Utf8. I'm taking the logic from other UDFs and applying it here. It may not be the best way of doing this though.

I think the logic is "Whatever the first argument type is the output should be of that type

i understand this is what's implemented. but not sure why it is so.
what's the exact benefit of presenting the data as string view, if we computed the exact string anyway, and we technically don't need to represent it as a string view?

Ah okay. So what you're saying is that here: https://github.com/apache/datafusion/pull/12224/files#diff-71970189679c6dd5b3b677bb21603234b488e68d1601be9c4d400d40e430a909R204 I'm building a Utf8 string anyways?

So I suspect I should change that bit of code to use a StringViewArrayBuilder?

yes, that's my intuition
except that i would keep building StringArray and just declare return type as Utf8 always

from the issue #11836

Currently, a call to CONCAT with a Utf8View datatypes induces a cast. After the change that fixes this issue, it should not.

this is about inputs to the function, not the return type

Side note:
String view could be an interesting return type if we wanted to optimize for single non-null string view input and let it pass-through; but the code doesn't do this today, not sure it's worth implementing for this edge case and it should be independent of arguments order, ie not tied to the first input's type.
end of side note.

@findepi what about for LargeUtf8? I suspect that if a LargeUtf8 is the input then the output should also be that since its an i64 datatype vs the i32 datatype for Utf8?

i don't know the exact rules for how we handle LargeUtf8.
i focused on the string view portion of the PR. from "string views perspective", LargeUtf8 is non-issue, so IMO it's fine not to change the return type with respect to LargeUtf8 in this PR. but i agree that we probably should return LargeUtf8 when any input is LargeUtf8 (pr what exactly the logic should be).

in fact, what does the binary concat operator do?

@findepi when you say binary concat operator are you talking about || as an operator?

when you say binary concat operator are you talking about || as an operator?

yes

the logic seems to assume all arguments are of the same type?

also, why not always return Utf8? the code performing actual concatenation seems to be always the same.

the question still holds (why exactly we bias towards the first param type), but i am no longer convinced about my suggestion to use Utf8 always.

i think we should "just" make sure concat(a, b, c) is type-equivalent to a || b || c.
the ||'s logic apparently is

if any of the operands are Utf8View, the result is Utf8View

else, if any of the operands are LargeUtf8, the result is LargeUtf8

cc @alamb

Sounds good @findepi i like that logic. I can adjust to make it so.

Signed-off-by: Devan <devandbenz@gmail.com>

devanbenz · 2024-08-30T01:12:49Z

datafusion/functions/src/string/common.rs

@@ -418,6 +451,88 @@ impl StringArrayBuilder {
    }
 }

+pub(crate) struct LargeStringArrayBuilder {


I wanted to make the already existing StringArrayBuilder generic but was having issues 😢

Signed-off-by: Devan <devandbenz@gmail.com>

findepi · 2024-08-31T09:56:52Z

datafusion/functions/src/string/concat.rs

+        Ok(match &arg_types[0] {
+            LargeUtf8 => LargeUtf8,
+            _ => Utf8,
+        })


currently Utf8View isn't declared as an expected return type,
but the code does return Utf8View values.
should anything be failing when the runtime value type doesn't match the declared return type?
(if so, what would be a test case exercising such case?)

cc @comphead

I've added test cases for Utf8View and LargeUtf8 return types. Have gone ahead and adjusted this.

Signed-off-by: Devan <devandbenz@gmail.com>

devanbenz · 2024-09-01T16:20:29Z

datafusion/functions/src/string/common.rs

+    block: String
+}
+
+impl StringViewArrayBuilder {


StringViewArrayBuilder works but it almost feels "hacky"

I think the original intent of StringViewBuilder when @JasonLi-cn added it was to have a way to append (but not complete) strings to avoid a copy and not incrementally update the null mask.

Specifically, StringBuilder in arrow https://docs.rs/arrow/latest/arrow/array/type.StringBuilder.html#method.append_value lets you append an entire string but it requires the input be contiguous bytes

StringArrayBuilder is that you can push bytes (via write) wthout copying the data.

The reason I think it feels hacky is that the API for building up single values in a StringArray or StringViewArray without a copy is not present

As we see we want to do the same thing with Utf8View I wonder if we should spend some time making the API eaiser to use 🤔

I filed apache/arrow-rs#6347 to discuss adding an API like this in arrow as well

Signed-off-by: Devan <devandbenz@gmail.com>

alamb

Thank you @devanbenz @findepi and @tshauck

I think this PR is good enough to merge, but I agree with @devanbenz 's observation https://github.com/apache/datafusion/pull/12224/files#r1740156822 that StringViewArrayBuilder feels hacky.

Specifically I think what is happening is that StringArrayBuilder is for a pretty specialized usecase, and thus so is StringViewArrayBuilder

I filed apache/arrow-rs#6347 upstream with a description of the use case to see if we can come up with something more general

alamb · 2024-09-02T11:42:40Z

datafusion/functions/src/string/concat.rs

+            if data_type == &Utf8View {
+                dt = data_type;
+            }
+            if data_type == &LargeUtf8 && dt != &Utf8View {


alamb · 2024-09-02T11:43:35Z

datafusion/functions/src/string/common.rs

+    block: String
+}
+
+impl StringViewArrayBuilder {


I think the original intent of StringViewBuilder when @JasonLi-cn added it was to have a way to append (but not complete) strings to avoid a copy and not incrementally update the null mask.

Specifically, StringBuilder in arrow https://docs.rs/arrow/latest/arrow/array/type.StringBuilder.html#method.append_value lets you append an entire string but it requires the input be contiguous bytes

StringArrayBuilder is that you can push bytes (via write) wthout copying the data.

The reason I think it feels hacky is that the API for building up single values in a StringArray or StringViewArray without a copy is not present

As we see we want to do the same thing with Utf8View I wonder if we should spend some time making the API eaiser to use 🤔

alamb · 2024-09-02T12:02:25Z

datafusion/functions/src/string/common.rs

+    block: String
+}
+
+impl StringViewArrayBuilder {


I filed apache/arrow-rs#6347 to discuss adding an API like this in arrow as well

devanbenz · 2024-09-02T13:56:28Z

Thank you @devanbenz @findepi and @tshauck

I think this PR is good enough to merge, but I agree with @devanbenz 's observation #12224 (files) that StringViewArrayBuilder feels hacky.

Specifically I think what is happening is that StringArrayBuilder is for a pretty specialized usecase, and thus so is StringViewArrayBuilder

I filed apache/arrow-rs#6347 upstream with a description of the use case to see if we can come up with something more general

Great, sounds good @alamb 🙏

alamb · 2024-09-03T20:12:53Z

Thanks again @devanbenz

devanbenz added 3 commits August 28, 2024 12:31

wip

723ceb7

feat: Update the CONCAT scalar function to support Utf8View

f7abdd5

fmt

503d5b9

github-actions bot added sqllogictest SQL Logic Tests (.slt) functions labels Aug 29, 2024

devanbenz added 3 commits August 28, 2024 21:14

fmt and add default return type for concat

b30330d

fix clippy lint

76d6b5f

Signed-off-by: Devan <devandbenz@gmail.com>

fmt

9798ed3

Signed-off-by: Devan <devandbenz@gmail.com>

devanbenz marked this pull request as ready for review August 29, 2024 03:01

devanbenz mentioned this pull request Aug 29, 2024

Update concat_ws scalar function to support Utf8View #11837

Closed

add more tests for sqllogic

91d04ff

Signed-off-by: Devan <devandbenz@gmail.com>

tshauck approved these changes Aug 29, 2024

View reviewed changes

make sure no casting with LargeUtf8

6d28927

findepi reviewed Aug 29, 2024

View reviewed changes

devanbenz added 3 commits August 29, 2024 18:04

fixing utf8large

769d99d

fix large utf8

ac30a83

Signed-off-by: Devan <devandbenz@gmail.com>

fix large utf8

7ea6e0a

Signed-off-by: Devan <devandbenz@gmail.com>

devanbenz commented Aug 30, 2024

View reviewed changes

devanbenz added 3 commits August 29, 2024 20:16

add test

dd3ad39

Signed-off-by: Devan <devandbenz@gmail.com>

fmt

504459c

Signed-off-by: Devan <devandbenz@gmail.com>

make it so Utf8View just returns Utf8

e081934

Signed-off-by: Devan <devandbenz@gmail.com>

findepi reviewed Aug 31, 2024

View reviewed changes

devanbenz added 2 commits September 1, 2024 09:51

wip -- trying to build a stringview with columnar refs

0069c1a

Signed-off-by: Devan <devandbenz@gmail.com>

built stringview builder but it does allocate a new String each iter :(

0929a4b

Signed-off-by: Devan <devandbenz@gmail.com>

devanbenz commented Sep 1, 2024

View reviewed changes

devanbenz added 2 commits September 1, 2024 11:29

add some testing

f16de44

Signed-off-by: Devan <devandbenz@gmail.com>

clippy

d0bf3ba

Signed-off-by: Devan <devandbenz@gmail.com>

alamb approved these changes Sep 2, 2024

View reviewed changes

alamb merged commit e4a9424 into apache:main Sep 3, 2024
24 checks passed

This was referenced Sep 9, 2024

Implement std::fmt::Write for StringViewBuilder to permit non contiguous data to be written apache/arrow-rs#6373

Open

DataFusion weekly project plan (Andrew Lamb) - Sep 9, 2024 #12391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the CONCAT scalar function to support Utf8View #12224

Update the CONCAT scalar function to support Utf8View #12224

devanbenz commented Aug 29, 2024 •

edited

Loading

tshauck left a comment

tshauck Aug 29, 2024

devanbenz Aug 29, 2024

devanbenz Aug 29, 2024

tshauck Aug 29, 2024 •

edited

Loading

devanbenz Aug 29, 2024

devanbenz Aug 29, 2024 •

edited

Loading

devanbenz Aug 29, 2024

devanbenz Aug 29, 2024

devanbenz Aug 30, 2024

tshauck Aug 29, 2024

findepi Aug 29, 2024

devanbenz Aug 30, 2024

findepi Aug 30, 2024

devanbenz Aug 30, 2024

findepi Aug 30, 2024

devanbenz Aug 30, 2024

findepi Aug 30, 2024

devanbenz Aug 30, 2024

findepi Aug 31, 2024

devanbenz Aug 31, 2024

devanbenz Aug 30, 2024

findepi Aug 31, 2024

devanbenz Sep 1, 2024

devanbenz Sep 1, 2024

alamb Sep 2, 2024

alamb Sep 2, 2024

alamb left a comment

alamb Sep 2, 2024

alamb Sep 2, 2024

alamb Sep 2, 2024

devanbenz commented Sep 2, 2024 •

edited

Loading

alamb commented Sep 3, 2024

Update the CONCAT scalar function to support Utf8View #12224

Update the CONCAT scalar function to support Utf8View #12224

Conversation

devanbenz commented Aug 29, 2024 • edited Loading

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?

tshauck 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

tshauck Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devanbenz Aug 29, 2024 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

devanbenz commented Sep 2, 2024 • edited Loading

alamb commented Sep 3, 2024

devanbenz commented Aug 29, 2024 •

edited

Loading

tshauck Aug 29, 2024 •

edited

Loading

devanbenz Aug 29, 2024 •

edited

Loading

devanbenz commented Sep 2, 2024 •

edited

Loading