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

alamb · 2024-07-31T19:43:22Z

Which issue does this PR close?

Rationale for this change

I am trying to write up the remaining work for StringView, and one remaining item is add StringView support for StringFunctions. I want to know what functions already support StringView

It turns out we have a lot of string functions that probably need work to convert to natively support string view

What changes are included in this PR?

Add some tests to string_view.slt showing if functions support StringView or not
Fix regexp_like and regexp_match to work with StringView

Are these changes tested?

All tests

Are there any user-facing changes?

alamb · 2024-08-03T10:34:52Z

datafusion/functions/src/regex/regexplike.rs

-                    "The regexp_like function can only accept strings. Got {other}"
-                );
-            }
+            _ => Boolean,


Without this change, regexp_like fails with an error about unsupported types if passed a StringView

With this change it succeeds (though the StringView is cast to a String)

@tshauck and I are going to organize a bunch of changes to get StringView working across functions as part of #11790

Shouldn't we return error for non-string type?

LargeUtf8 | Utf8 | Utf8View => Boolean
Null => Null
other => not supported

I was thinking the type coercion already handles, in theory, ensuring the correct types are passed in, so the extra check was not necessary. I can put back the explicit check if you prefer

Seems #11618 is striking again. I think @alamb is right, but it is very counter intuitive to return a Boolean because that case shouldn't exists.

Should we use 'unreachable!' ?

I was thinking the type coercion already handles, in theory, ensuring the correct types are passed in, so the extra check was not necessary. I can put back the explicit check if you prefer

I think you are right, we don't need another type check in return_type.

it is very counter intuitive to return a Boolean because that case shouldn't exists.

The type check should be handled in signature before return_type. We could improve docs about how the types and length check are handled in UDF/UDAF

How about I'll at least add some comments to make it clear why this isn't doing the typechecks here

Should we use 'unreachable!' ?

I prefer using internal error when possible as unreachable! will panic. But now that you mention this, the function is going to panic at runtime anyways 🤔

My bad I mixed up two things. In the thread about better error handling I think we agreed to return a planning error in this scenario, and some form of unreachable within the invoke function to signal that branch should never be executed

I added comments in ead4a7c

alamb · 2024-08-03T10:36:16Z

datafusion/sqllogictest/test_files/string_view.slt

+
+
+## Ensure no casts for ASCII
+## TODO file ticket


We plan to file tickets for these functions as follow ons -- see #11787 (comment) for details

Here's the one for ASCII: #11834

I don't have the ability to update the label, so maybe it could be changed to a good first issue? Might help it get picked up sooner, and with the examples, it's not doing it from scratch.

tshauck · 2024-08-05T23:08:46Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  BTRIM(column1_utf8view, 'foo') AS l


BTRIM: #11835

tshauck · 2024-08-05T23:11:54Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  CHARACTER_LENGTH(column1_utf8view) AS l


Done in #11677

🤔 but for some reason the argument is still cast to Utf8

01)Projection: character_length(CAST(test.column1_utf8view AS Utf8)) AS l

I debugged and fixed it here: f42aa84

Ah, thanks!

tshauck · 2024-08-05T23:16:09Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  concat(column1_utf8view, column2_utf8view) as c


CONCAT: #11836

tshauck · 2024-08-05T23:18:09Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  concat_ws(', ', column1_utf8view, column2_utf8view) as c


tshauck · 2024-08-05T23:20:14Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  CONTAINS(column1_utf8view, 'foo') as c1,


tshauck

Next batch

tshauck · 2024-08-06T20:30:16Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  ENDS_WITH(column1_utf8view, 'foo') as c1,


tshauck · 2024-08-06T20:31:53Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  INITCAP(column1_utf8view) as c


tshauck · 2024-08-06T20:33:27Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  levenshtein(column1_utf8view, 'foo') as c1,


tshauck · 2024-08-06T20:35:00Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  LOWER(column1_utf8view) as c1


tshauck · 2024-08-06T20:36:31Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  LTRIM(column1_utf8view) as c1


tshauck · 2024-08-06T20:39:00Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  LPAD(column1_utf8view, 12, ' ') as c1


tshauck · 2024-08-06T20:40:15Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  OCTET_LENGTH(column1_utf8view) as c1


tshauck · 2024-08-06T20:40:32Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  STARTS_WITH(column1_utf8view, 'foo') as c,


Done in #11787

jayzhan211

👍

alamb

Thank you @tshauck @jayzhan211 and others who helped with this PR

I updated the tests / added the ticket links, and I now plan to merge this PR once the CI is green

alamb · 2024-08-08T16:45:42Z

datafusion/sqllogictest/test_files/string_view.slt

@@ -425,102 +425,26 @@ logical_plan
 01)Projection: starts_with(test.column1_utf8view, Utf8View("äöüß")) AS c1, starts_with(test.column1_utf8view, Utf8View("")) AS c2, starts_with(test.column1_utf8view, Utf8View(NULL)) AS c3, starts_with(Utf8View(NULL), test.column1_utf8view) AS c4
 02)--TableScan: test projection=[column1_utf8view]

-statement ok


I just moved these tests down to the other test for ASCII

alamb · 2024-08-08T20:39:09Z

Thanks again@

tshauck · 2024-08-09T16:36:29Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  OVERLAY(column1_utf8view PLACING 'foo' FROM 2 ) as c1


tshauck · 2024-08-09T16:38:33Z

datafusion/sqllogictest/test_files/string_view.slt

+## Ensure no casts for REGEXP_LIKE
+query TT
+EXPLAIN SELECT
+  REGEXP_LIKE(column1_utf8view, '^https?://(?:www\.)?([^/]+)/.*$') AS k


tshauck

another batch

tshauck · 2024-08-09T16:40:01Z

datafusion/sqllogictest/test_files/string_view.slt

+## Ensure no casts for REGEXP_MATCH
+query TT
+EXPLAIN SELECT
+  REGEXP_MATCH(column1_utf8view, '^https?://(?:www\.)?([^/]+)/.*$') AS k


tshauck · 2024-08-09T16:41:43Z

datafusion/sqllogictest/test_files/string_view.slt

+## Ensure no casts for REGEXP_REPLACE
+query TT
+EXPLAIN SELECT
+  REGEXP_REPLACE(column1_utf8view, '^https?://(?:www\.)?([^/]+)/.*$', '\1') AS k


tshauck · 2024-08-09T16:43:36Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  REPLACE(column1_utf8view, 'foo', 'bar') as c1,


tshauck · 2024-08-09T16:44:43Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  REPEAT(column1_utf8view, 2) as c1


tshauck · 2024-08-09T16:45:58Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  REVERSE(column1_utf8view) as c1


tshauck · 2024-08-09T16:47:16Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  RTRIM(column1_utf8view) as c1,


tshauck · 2024-08-09T16:47:46Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  RTRIM(column1_utf8view) as c,


RTRIM is here twice, ticket: #11916

tshauck · 2024-08-09T16:48:38Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  RIGHT(column1_utf8view, 3) as c2


tshauck · 2024-08-09T16:49:33Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  RPAD(column1_utf8view, 1) as c1,


tshauck

final batch

tshauck · 2024-08-12T15:45:59Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  SPLIT_PART(column1_utf8view, 'f', 1) as c


tshauck · 2024-08-12T15:46:53Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  STRPOS(column1_utf8view, 'f') as c,


tshauck · 2024-08-12T15:48:06Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  SUBSTR(column1_utf8view, 1) as c,


tshauck · 2024-08-12T15:49:15Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  TRANSLATE(column1_utf8view, 'foo', 'bar') as c


tshauck · 2024-08-12T15:50:27Z

datafusion/sqllogictest/test_files/string_view.slt

+## TODO file ticket
+query TT
+EXPLAIN SELECT
+  FIND_IN_SET(column1_utf8view, 'a,b,c,d') as c


tshauck · 2024-08-12T15:52:32Z

datafusion/sqllogictest/test_files/string_view.slt

+('value2', arrow_cast('datafusion', 'Utf8View'), arrow_cast('cool', 'Utf8View'));
+
+query T
+select column2||' is fast' from temp;


Ticket for concat operator is at: #11766

alamb · 2024-08-14T19:53:25Z

You are the best @tshauck -- given how fast these implementations are pouring in I am not even going to try and update the URLs in the text :)

alamb marked this pull request as draft July 31, 2024 19:43

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

This was referenced Jul 31, 2024

Implement LIKE for StringView arrays #11024

Closed

Implement REGEXP_REPLACE for StringView #11025

Closed

Minor: Add tests for StringView / character functions

45117f6

alamb force-pushed the alamb/test_string_view branch from 264523b to 45117f6 Compare July 31, 2024 20:13

Fix regexp_like and regexp_match to work with StringVeiw

9f3c651

alamb changed the title ~~Minor: Add tests for StringView / character functions~~ Add tests for StringView / character functions, fix regexp_like and regexp_match to work with StringView Jul 31, 2024

This was referenced Jul 31, 2024

[Epic] Complete Initial StringView in DataFusion #11752

Closed

Add support for StringView and BinaryView statistics in StatisticsConverter apache/arrow-rs#6164

Closed

feat: support Utf8View type in starts_with function #11787

Merged

alamb marked this pull request as ready for review August 2, 2024 21:13

alamb commented Aug 3, 2024

View reviewed changes

edmondop approved these changes Aug 3, 2024

View reviewed changes

tshauck reviewed Aug 5, 2024

View reviewed changes

tshauck reviewed Aug 6, 2024

View reviewed changes

jayzhan211 approved these changes Aug 8, 2024

View reviewed changes

alamb added 5 commits August 8, 2024 12:14

Merge remote-tracking branch 'apache/main' into alamb/test_string_view

066facb

Update for ASCII and BTRIM

268efcd

Add comment about why it is ok to return boolean with catchall match

ead4a7c

Fix character_length

f42aa84

Add ticket references

4cb25d0

alamb commented Aug 8, 2024

View reviewed changes

alamb merged commit 368df80 into apache:main Aug 8, 2024
24 checks passed

alamb deleted the alamb/test_string_view branch August 8, 2024 20:39

tshauck reviewed Aug 9, 2024

View reviewed changes

tshauck reviewed Aug 12, 2024

View reviewed changes

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

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

Conversation

alamb commented Jul 31, 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?

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

jayzhan211 Aug 8, 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

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

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

jayzhan211 left a 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

alamb commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Aug 14, 2024

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

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

alamb commented Jul 31, 2024 •

edited

Loading

jayzhan211 Aug 8, 2024 •

edited

Loading