feat: support `Utf8View` type in `starts_with` function #11787

tshauck · 2024-08-02T17:49:13Z

Which issue does this PR close?

Closes #11786

Rationale for this change

Utf8Views don't appear to be supported starts_with yet.

What changes are included in this PR?

Update the type signature of the UDF and percolate the new data type support in the arrow interactions.

Are these changes tested?

Yes, added some unittests and did manual testing via the CLI.

Are there any user-facing changes?

No

tshauck · 2024-08-02T18:00:21Z

datafusion/functions/src/string/starts_with.rs

+        let string_types = vec![Utf8, LargeUtf8, Utf8View];
+        let mut type_signatures = vec![];
+
+        for left in &string_types {


Not sure if there's a better way to do this? Maybe I should just list them out?

This is a interesting idea, i liked it compared to earlier implementation.

I don't think the underlying kernel supports different string types (you can't actually call it with (Utf8, LargeUtf8) so the existing function actually has a bug

In fact here is reproducer showing the probelm on main

DataFusion CLI v40.0.0 > create table foo as values (arrow_cast('foo', 'Utf8'), arrow_cast('bar', 'LargeUtf8')); 0 row(s) fetched. Elapsed 0.046 seconds. > select * from foo; +---------+---------+ | column1 | column2 | +---------+---------+ | foo | bar | +---------+---------+ 1 row(s) fetched. Elapsed 0.010 seconds. > select starts_with(column1, column2) from foo; Internal error: could not cast value to arrow_array::array::byte_array::GenericByteArray<arrow_array::types::GenericStringType<ii32>>. This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

So in other words, I think the correct signature is

signature: Signature::one_of( vec![ Exact(vec![Utf8, Utf8]), Exact(vec![LargeUtf8, LargeUtf8]), Exact(vec![Utf8View, Utf8View]), ], Volatility::Immutable, ), }

Thanks for this and link to the tests -- very helpful!

I made this update along with simplifying the arrow starts_with call (#11787 (comment)), and I'm noticing that STARTS_WITH(column1_utf8view, 'foo') results in a cast of the first column to utf8 while keeping the second one utf8, but I think we'd want to keep the first argument as a utf8view while coercing the second argument to utf8view? It feels like calling starts_with with a second argument as a scalar is common.

For example, with the changes, this test passes:

query TT EXPLAIN SELECT STARTS_WITH(column1_utf8view, 'foo') as c, STARTS_WITH(column1_utf8view, column2_utf8view) as c2 FROM test; ---- logical_plan 01)Projection: starts_with(CAST(test.column1_utf8view AS Utf8), Utf8("foo")) AS c, starts_with(test.column1_utf8view, test.column2_utf8view) AS c2 02)--TableScan: test projection=[column1_utf8view, column2_utf8view]

Yes I agree figuring out how to coerce the constant to Utf8view is important

I think the coercion logic just goes linearly through the signature and stops at the first type that works.

Thus I wonder if you can make Utf8View preferred by listing the types in a different order and put the Utf8View signature at the first option? Like

signature: Signature::one_of( vec![ // make `Utf8View` first Exact(vec![Utf8View, Utf8View]), Exact(vec![Utf8, Utf8]), Exact(vec![LargeUtf8, LargeUtf8]), ], Volatility::Immutable, ), }

Still seems to be happening with this latest push 🤔 (222263c).

I'll try to poke around and see if I can understand what's happening.

I think it needed this update in the argument coercion logic... before this even with Utf8View being preferred in the signature it wasn't recognized in the type to/from statement: https://github.com/apache/datafusion/pull/11787/files#diff-6e1fb265597317a8256c60670ff3ea7be6896b2df1199a40ca79419ce29b4ce3

tshauck · 2024-08-02T18:01:16Z

datafusion/functions/src/string/starts_with.rs

+    #[test]
+    fn test_functions() -> Result<()> {
+        // Generate test cases for starts_with
+        let test_cases = vec![


Not sure if this is the best way to test this change. It tests the function but doesn't a view wouldn't be coerced.

I have tests for the coercion in #11753 -- you beat me to filing the ticket ❤️ -- I plan to make an epic / start filing items for the various other functions

I recommend using a slt test (example of sql test above)

Just to throw it out there, if you make the top level epic, I'd be happy to work though that PR and make individual issues for the problematic functions.

Deal! I filed #11790

What I recommend doing is starting with a single ticket and making it a complete template / description with the idea being that that someone that is not familar DataFusion or Utf8View can work on it

Then when we file tickets for the rest of the functions we can reuse the same description over again. This

Reduces the barrier to contribution

I have found people love working on the code when there is a clear description

I took a shot at updating #11786 with background for use a template. Let me know what you think

Sounds good to me. I'll have a go at replicating one of the other functions following #11786, and then if we're happy with it, I can grind through the others.

Sounds good -- thank you. What I have found works well in the past is to do like 10 at a time (if you make more than that the number of parallel threads / work I found overwhelmed our ability to review them)

dharanad · 2024-08-02T18:15:46Z

datafusion/functions/src/string/starts_with.rs

-            }
-            _ => internal_err!("Unsupported data type"),
+            DataType::LargeUtf8 => make_scalar_function(starts_with::<i64>, vec![])(args),
+            DataType::Utf8View => make_scalar_function(starts_with::<i32>, vec![])(args),


I think we can do | operator to handle Utf8 and Utf8View together.

( DataType::Utf8View | DataType::Utf8 ) => make_scalar_function(starts_with::<i32>, vec![])(args)

alamb

Thanks @tshauck and @dharanad -- this is quite cool. Thank you 🙏

Once we get this one sorted out, I think it will be a good example of how to add native StringViewArray support

alamb · 2024-08-02T18:18:15Z

datafusion/functions/src/string/starts_with.rs

-    let left = as_generic_string_array::<T>(&args[0])?;
-    let right = as_generic_string_array::<T>(&args[1])?;
+    let bool_result = match (args[0].data_type(), args[1].data_type()) {
+        (DataType::Utf8View, DataType::Utf8View) => {


Since https://docs.rs/arrow/latest/arrow/compute/kernels/comparison/fn.starts_with.html already does this downcasting, the I think you could simply call this with two &dyn Array rather than having to check the types

In particular I think you could simply call

arrow::compute::kernels::comparison::starts_with(left, right)?

and remove the <T: OffsetSizeTrait> from this call

BTW I double checked that arrow has native StringView support (thanks to @XiangpengHao ):
https://docs.rs/arrow-string/52.2.0/src/arrow_string/like.rs.html#129-148 ✅

This is super cool.

alamb · 2024-08-02T18:21:57Z

datafusion/functions/src/string/starts_with.rs

+        let string_types = vec![Utf8, LargeUtf8, Utf8View];
+        let mut type_signatures = vec![];
+
+        for left in &string_types {


I don't think the underlying kernel supports different string types (you can't actually call it with (Utf8, LargeUtf8) so the existing function actually has a bug

In fact here is reproducer showing the probelm on main

DataFusion CLI v40.0.0 > create table foo as values (arrow_cast('foo', 'Utf8'), arrow_cast('bar', 'LargeUtf8')); 0 row(s) fetched. Elapsed 0.046 seconds. > select * from foo; +---------+---------+ | column1 | column2 | +---------+---------+ | foo | bar | +---------+---------+ 1 row(s) fetched. Elapsed 0.010 seconds. > select starts_with(column1, column2) from foo; Internal error: could not cast value to arrow_array::array::byte_array::GenericByteArray<arrow_array::types::GenericStringType<ii32>>. This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

So in other words, I think the correct signature is

signature: Signature::one_of( vec![ Exact(vec![Utf8, Utf8]), Exact(vec![LargeUtf8, LargeUtf8]), Exact(vec![Utf8View, Utf8View]), ], Volatility::Immutable, ), }

alamb · 2024-08-02T18:22:22Z

datafusion/functions/src/string/starts_with.rs

+    #[test]
+    fn test_functions() -> Result<()> {
+        // Generate test cases for starts_with
+        let test_cases = vec![


I recommend using a slt test (example of sql test above)

tshauck · 2024-08-02T23:21:00Z

Gonna mark this ready for review, though happy to make any additional updates.

XiangpengHao · 2024-08-03T00:17:52Z

This pr is really nicely done! Looks good to me!

datafusion/functions/src/string/starts_with.rs

2010YOUY01

Thank you for this nice work, it looks good to me.

Given this PR can become an example for supporting StringView for remaining string functions, I suggest also:

Add more sqllogictests, since they're easier to understand and follow.
In sqllogictest end-to-end tests, include more edge cases like empty strings, mismatched argument types like starts_with(utf8_col, utf8view_col), this way we can encourage other contibutors to follow the pattern and write better tests for the remaining functions.

2010YOUY01 · 2024-08-03T04:50:10Z

datafusion/expr/src/type_coercion/functions.rs

@@ -573,6 +573,8 @@ fn coerced_from<'a>(
        (Interval(_), _) if matches!(type_from, Utf8 | LargeUtf8) => {
            Some(type_into.clone())
        }
+        // We can go into a Utf8View from a Utf8 or LargeUtf8
+        (Utf8View, _) if matches!(type_from, Utf8 | LargeUtf8) => Some(type_into.clone()),


LargeUtf8 has 64 bit length, Utf8View only got 32 bit length.
Loos like LargeUtf8 -> Utf8View is not possible?

The arrow cast kernel supports this conversion

Though I am not sure if we should allow the automatic coercion as it could potentially fail if the strings were over 2GB 🤔

I see, the semantics is < 32 bit length LargeUtf8 conversion is supported, otherwise, some runtime error will occur. This makes sense.

datafusion/functions/src/string/starts_with.rs

alamb · 2024-08-03T10:18:15Z

datafusion/functions/src/string/starts_with.rs

-    let result = arrow::compute::kernels::comparison::starts_with(left, right)?;
-
+pub fn starts_with(args: &[ArrayRef]) -> Result<ArrayRef> {
+    let result = arrow::compute::kernels::comparison::starts_with(&args[0], &args[1])?;


datafusion/functions/src/string/starts_with.rs

alamb · 2024-08-03T10:39:12Z

Thank you for this nice work, it looks good to me.

Given this PR can become an example for supporting StringView for remaining string functions, I suggest also:

Add more sqllogictests, since they're easier to understand and follow.

In sqllogictest end-to-end tests, include more edge cases like empty strings, mismatched argument types like starts_with(utf8_col, utf8view_col), this way we can encourage other contibutors to follow the pattern and write better tests for the remaining functions.

I think this is a great suggestion @2010YOUY01 -- I can help with this too

Here are the instructions: https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest

Ideally you should be able to extend string_view.slt perhaps in https://github.com/apache/datafusion/tree/main/datafusion/sqllogictest/test_files

alamb

From my perspective this PR could be merged as is (thank you @tshauck @dharanad @2010YOUY01 for the work)

I plan to leave it open until @tshauck has a chance to respond to the outstanding comments. While I think they could be done as follow on work, @2010YOUY01 has a good point that if this PR will be used as an example / template for how to add native StringView support getting all the changes (esp the testing) in a single PR has a lot of value

Co-authored-by: Yongting You <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

tshauck · 2024-08-03T17:17:47Z

Thanks @alamb @2010YOUY01 @dharanad for the feedback. Please give me a bit to add additional tests in this PR, then I'll re-request review.

alamb · 2024-08-06T10:24:03Z

🚀

tshauck commented Aug 2, 2024

View reviewed changes

tshauck changed the title ~~feat: support Utf8View for starts_with~~ feat: support Utf8View type in starts_with function Aug 2, 2024

dharanad reviewed Aug 2, 2024

View reviewed changes

alamb reviewed Aug 2, 2024

View reviewed changes

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Aug 2, 2024

alamb mentioned this pull request Aug 2, 2024

Support starts_with for Utf8View #11786

Closed

github-actions bot added the logical-expr Logical plan and expressions label Aug 2, 2024

tshauck added 5 commits August 2, 2024 15:58

feat: support Utf8View for starts_with

1bf07f1

style: clippy

2c1dda9

simplify string view handling

65ba700

fix: allow utf8 and largeutf8 to be cast into utf8view

f38b363

fix: fix test

96c0552

tshauck force-pushed the starts_with branch from a0006f5 to 96c0552 Compare August 2, 2024 23:00

tshauck marked this pull request as ready for review August 2, 2024 23:21

dharanad suggested changes Aug 3, 2024

View reviewed changes

datafusion/functions/src/string/starts_with.rs Outdated Show resolved Hide resolved

2010YOUY01 reviewed Aug 3, 2024

View reviewed changes

datafusion/functions/src/string/starts_with.rs Show resolved Hide resolved

alamb reviewed Aug 3, 2024

View reviewed changes

This was referenced Aug 3, 2024

[Epic] Native StringView support for string functions #11790

Open

Add tests for StringView / character functions, fix regexp_like and regexp_match to work with StringView #11753

Merged

alamb approved these changes Aug 3, 2024

View reviewed changes

tshauck and others added 2 commits August 3, 2024 10:13

Apply suggestions from code review

f203553

Co-authored-by: Yongting You <[email protected]> Co-authored-by: Andrew Lamb <[email protected]>

style: fix format

2a15df3

tshauck added 3 commits August 3, 2024 11:56

feat: add addiontal tests

221dc76

tests: improve tests

a80e1bc

fix: fix null case

f06f342

alamb merged commit 3d76aa2 into apache:main Aug 6, 2024
24 checks passed

tshauck deleted the starts_with branch August 6, 2024 17:04

PsiACE mentioned this pull request Aug 10, 2024

Implement native support StringView for Ends With #11924

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support `Utf8View` type in `starts_with` function #11787

feat: support `Utf8View` type in `starts_with` function #11787

tshauck commented Aug 2, 2024

tshauck Aug 2, 2024

dharanad Aug 2, 2024

alamb Aug 2, 2024

tshauck Aug 2, 2024

alamb Aug 2, 2024

tshauck Aug 2, 2024

tshauck Aug 2, 2024

tshauck Aug 2, 2024

alamb Aug 2, 2024

alamb Aug 2, 2024

tshauck Aug 2, 2024

alamb Aug 2, 2024

tshauck Aug 2, 2024

alamb Aug 3, 2024

dharanad Aug 2, 2024

alamb left a comment

alamb Aug 2, 2024

dharanad Aug 2, 2024

alamb Aug 2, 2024

alamb Aug 2, 2024

tshauck commented Aug 2, 2024

XiangpengHao commented Aug 3, 2024

2010YOUY01 left a comment

2010YOUY01 Aug 3, 2024

alamb Aug 3, 2024 •

edited

Loading

2010YOUY01 Aug 3, 2024

alamb Aug 3, 2024

alamb commented Aug 3, 2024

alamb left a comment

tshauck commented Aug 3, 2024

alamb commented Aug 6, 2024

feat: support Utf8View type in starts_with function #11787

feat: support Utf8View type in starts_with function #11787

Conversation

tshauck commented Aug 2, 2024

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?

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

Choose a reason for hiding this comment

tshauck commented Aug 2, 2024

XiangpengHao commented Aug 3, 2024

2010YOUY01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Aug 3, 2024

alamb left a comment

Choose a reason for hiding this comment

tshauck commented Aug 3, 2024

alamb commented Aug 6, 2024

feat: support `Utf8View` type in `starts_with` function #11787

feat: support `Utf8View` type in `starts_with` function #11787

alamb Aug 3, 2024 •

edited

Loading