implement utf8_view for replace by thinh2 · Pull Request #12004 · apache/datafusion

thinh2 · 2024-08-15T07:41:11Z

Which issue does this PR close?

Closes #11913 .

Rationale for this change

Before this change, the replace function only support utf8 and largeUtf8. We used to cast utf8View to utf8, which reduces the performance of the function. This PR is to add support for utf8View inside replace function.

What changes are included in this PR?

Create a new function to handle replace for utf8View
Implement unit test for the replace function
Update sqllogictest to match our utf8View expectation (no CAST in the logical plans)
Update the signature of replace to handle utf8View. Without this change, the sql query will still cast utf8View to utf8

Are these changes tested?

Implement unit test for the replace function
Update sqllogictest to match our utf8View expectation (no CAST in the logical plans)

Are there any user-facing changes?

No

alamb

Looking nice @thinh2 🙏 -- I'll start CI for this PR

datafusion/functions/src/string/replace.rs

alamb · 2024-08-15T13:53:16Z

datafusion/functions/src/string/replace.rs

    }
 }

+fn replace_view(args: &[ArrayRef]) -> Result<ArrayRef> {


alamb · 2024-08-15T13:54:03Z

datafusion/sqllogictest/test_files/string_view.slt

-01)Projection: replace(__common_expr_1, Utf8("foo"), Utf8("bar")) AS c1, replace(__common_expr_1, CAST(test.column2_utf8view AS Utf8), Utf8("bar")) AS c2
-02)--Projection: CAST(test.column1_utf8view AS Utf8) AS __common_expr_1, test.column2_utf8view
-03)----TableScan: test projection=[column1_utf8view, column2_utf8view]
+01)Projection: replace(test.column1_utf8view, Utf8("foo"), Utf8("bar")) AS c1, replace(test.column1_utf8view, test.column2_utf8view, Utf8("bar")) AS c2


I think adding a test that shows this query running (aka run

SELECT REPLACE(column1_utf8view, 'foo', 'bar') as c1, REPLACE(column1_utf8view, column2_utf8view, 'bar') as c2 FROM test;

in addition to EXPLAIN
)

Hi @alamb, after updating the replace.rs to process Utf8View, my sql logic test still uses the CAST in its logical plan and this test is failing. Do you have any idea to debug this issue?

Hi @alamb, I already found the issue. The reason is that I didn't update the function's signature to Utf8View.

alamb

Looks good to me @thinh2 -- thank you 🙏

I pushed another commit to your branch that adds a slt test to very execution, and merged up from main

Thank you very much for your contribution

alamb

Thanks again @thinh2

alamb · 2024-08-17T11:02:51Z

datafusion/functions/src/string/replace.rs

+                vec![
+                    Exact(vec![Utf8View, Utf8View, Utf8View]),
+                    Exact(vec![Utf8, Utf8, Utf8]),
+                    Exact(vec![LargeUtf8, LargeUtf8, LargeUtf8]),


Thank you for also implementing support for LargeUTF8

alamb · 2024-08-17T11:04:07Z

Looks like there are some clippy errors and test updates needed to get CI clean on this PR

thinh2 · 2024-08-17T15:54:04Z

Hi @alamb, currently my code fails at this sql logic test https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/functions.slt#L825

query T
SELECT replace(arrow_cast('foobar', 'Dictionary(Int32, Utf8)'), 'bar', 'hello')
----
foohello

The reason is because of replace's signature order. If I push Utf8 before Utf8View, it will pass. However, test in string_view.slt will fail. I am trying to understand the functions test above to fix it.

thinh2 · 2024-08-19T05:29:15Z

Hi @alamb, I already fixed all the test errors. Could you please check and run the CI pipeline again?

alamb · 2024-08-19T15:51:27Z

Thank you @thinh2

alamb · 2024-08-19T17:26:39Z

🚀

draft implement utf8_view for replace

60ff4f6

github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Aug 15, 2024

alamb reviewed Aug 15, 2024

View reviewed changes

thinh2 and others added 3 commits August 16, 2024 00:17

add function signature

7a3569b

Add sql test

b08f96e

Merge remote-tracking branch 'apache/main' into utf8view_for_replace

14a124b

alamb approved these changes Aug 16, 2024

View reviewed changes

thinh2 added 2 commits August 16, 2024 12:15

move macro util to replace function

ab85d92

remove unused import

8ce4028

thinh2 changed the title ~~[draft] implement utf8_view for replace~~ implement utf8_view for replace Aug 16, 2024

rust format

b948b0f

alamb approved these changes Aug 17, 2024

View reviewed changes

change return type from utf8view to utf8

d26f21a

alamb merged commit bd48262 into apache:main Aug 19, 2024

Comments

Conversation

thinh2 commented Aug 15, 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!

Uh oh!

alamb Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

alamb Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

thinh2 Aug 15, 2024

Choose a reason for hiding this comment

Uh oh!

thinh2 Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Aug 17, 2024

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 17, 2024

Uh oh!

thinh2 commented Aug 17, 2024

Uh oh!

thinh2 commented Aug 19, 2024

Uh oh!

alamb commented Aug 19, 2024

Uh oh!

alamb commented Aug 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thinh2 commented Aug 15, 2024 •

edited

Loading