Support `Binary`/`LargeBinary` --> `Utf8`/`LargeUtf8` in ilike and string functions #7840

alamb · 2023-10-16T21:26:57Z

Which issue does this PR close?

Closes #7342
Closes #7345
Closes #7344

It also closes several draft / PRs:
Closes #7349
Closes #7350
Closes #7365

Rationale for this change

Some of the clickbench queries have this pattern

What changes are included in this PR?

Add a bunch of comments
Add automatic coercion rules from Binary and LargeBinary to the appropriate Utf8 (string) variants for LIKE and ILIKE
Add appropriate binary --> string logic in return type calculation for functions

Are these changes tested?

Yes, new tests

Are there any user-facing changes?

Yes now some queries will not error

…ring functions fixup

alamb · 2023-10-17T11:30:45Z

datafusion/expr/src/built_in_function.rs

+                DataType::LargeUtf8  => $largeUtf8Type,
+                // LargeBinary inputs are automatically coerced to Utf8
+                DataType::LargeBinary => $largeUtf8Type,
+                DataType::Utf8  => $utf8Type,


Here is the fix for string functions taking binary arguments

The new cases for Binary/LargeBinary are all that is new here -- the rest is just comments

alamb · 2023-10-17T11:31:13Z

datafusion/expr/src/type_coercion/binary.rs

+/// Coercion rules for binary (Binary/LargeBinary) to string (Utf8/LargeUtf8):
+/// If one argument is binary and the other is a string then coerce to string
+/// (e.g. for `like`)
+fn binary_to_string_coercion(


this is the new rule, only used for ilke

I originally tried modifying string_coercion to also include Binary --> String coercion, however, that has the (bad) side effect of making a binary = string comparison into cast(binary)= string which is bad because:

The comparison is valid even if the binary column contains non UTF8 data (but the cast will fail in this case)

The cast from binary --> utf8 is not free (it has to check each value for utf8 correctness)

alamb · 2023-10-17T11:33:49Z

datafusion/expr/src/type_coercion/binary.rs

 /// This is a union of string coercion rules and dictionary coercion rules
 pub fn like_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {
    string_coercion(lhs_type, rhs_type)
+        .or_else(|| binary_to_string_coercion(lhs_type, rhs_type))


this is the fix for like

alamb · 2023-10-17T11:34:08Z

datafusion/expr/src/type_coercion/binary.rs

-            let (lhs, rhs) = get_input_types(&$A_TYPE, &$OP, &$B_TYPE)?;
-            assert_eq!(lhs, $C_TYPE);
-            assert_eq!(rhs, $C_TYPE);
+        ($LHS_TYPE:expr, $RHS_TYPE:expr, $OP:expr, $RESULT_TYPE:expr) => {{


This is the same logic, but I renamed the arguments for clarity

alamb · 2023-10-17T11:34:25Z

datafusion/sqllogictest/test_files/binary.slt

-
-query error DataFusion error: type_coercion
-SELECT largebinary FROM t where largebinary LIKE '%F';
+query ?


They work 🎉

alamb · 2023-10-17T15:54:39Z

@jonahgao I wonder if you have some time to review this PR

cc @Weijun-H and @JayjeetAtGithub and @parkma99

jackwener

I review this PR carefully, a great job!
Thanks @alamb .

jonahgao · 2023-10-18T05:55:02Z

@alamb LGTM 👍.

I also tested some other string functions on this branch, and they worked as expected.
Although some outputs may look strange, it’s just a display issue.

DataFusion CLI v32.0.0
❯ select lower(X'0000102023');
+-------------------------------+
| lower(Binary("0,0,16,32,35")) |
+-------------------------------+
|  #                            |
+-------------------------------+
1 row in set. Query took 0.004 seconds.

Weijun-H

LGTM! Thanks @alamb 👍

alamb · 2023-10-18T21:23:30Z

Thanks everyone for the reviews!

viirya · 2023-10-18T21:38:31Z

datafusion/expr/src/built_in_function.rs

+// `utf8_to_str_type`: returns either a Utf8 or LargeUtf8 based on the input type size.
 make_utf8_to_return_type!(utf8_to_str_type, DataType::LargeUtf8, DataType::Utf8);
+
+// `utf8_to_str_type`: returns either a Int32 or Int64 based on the input type size.


utf8_to_str_type -> utf8_to_int_type

Nice find -- filed #7861

viirya · 2023-10-18T21:47:27Z

datafusion/expr/src/type_coercion/binary.rs


-/// Coercion rules for Strings: the type that both lhs and rhs can be
-/// casted to for the purpose of a string computation
+/// Coercion rules for string types (Utf8/LargeUtf8): If at least on argument is


at least on argument -> at least one argument

viirya · 2023-10-18T21:47:36Z

datafusion/expr/src/type_coercion/binary.rs

+    }
+}
+
+/// Coercion rules for binary types (Binary/LargeBinary): If at least on argument is


at least on argument -> at least one argument

viirya · 2023-10-18T21:47:59Z

datafusion/expr/src/type_coercion/functions.rs

+///
+/// For example, if a function `func` accepts arguments of  `(int64, int64)`,
+/// but was called with `(int32, int64)`, this function could match the
+/// valid_types by by coercing the first argument to `int64`, and would return


valid_types by by coercing -> valid_types by coercing

viirya

Found a few typos.

alamb · 2023-10-19T10:02:56Z

Thank you @viirya -- I will fix the typos

alamb · 2023-10-19T15:55:00Z

Found a few typos.

PR to fix: #7873

github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Oct 16, 2023

alamb mentioned this pull request Oct 16, 2023

Minor: Add tests for binary / utf8 coercion #7839

Merged

alamb marked this pull request as draft October 16, 2023 21:28

alamb changed the title ~~Support automatic Binary/LargeBinary --> Utf8/LargeUtf8 coercion~~ Support Binary/LargeBinary --> Utf8/LargeUtf8 in ilike Oct 17, 2023

alamb force-pushed the alamb/ilike_coercion branch from dd8f41e to b932e1e Compare October 17, 2023 10:44

alamb changed the title ~~Support Binary/LargeBinary --> Utf8/LargeUtf8 in ilike~~ Support Binary/LargeBinary --> Utf8/LargeUtf8 in ilike and string functions Oct 17, 2023

alamb mentioned this pull request Oct 17, 2023

feat: support Binary for regexp_replace #7365

Closed

Support binary argumnets (via coercion) for like and ilike and st…

56514be

…ring functions fixup

alamb force-pushed the alamb/ilike_coercion branch from b932e1e to 56514be Compare October 17, 2023 11:29

alamb marked this pull request as ready for review October 17, 2023 11:29

alamb commented Oct 17, 2023

View reviewed changes

jackwener approved these changes Oct 18, 2023

View reviewed changes

jonahgao approved these changes Oct 18, 2023

View reviewed changes

Weijun-H approved these changes Oct 18, 2023

View reviewed changes

alamb merged commit 7772453 into apache:main Oct 18, 2023

viirya reviewed Oct 18, 2023

View reviewed changes

alamb mentioned this pull request Oct 18, 2023

Minor: fix typo in comments #7861

Merged

viirya reviewed Oct 18, 2023

View reviewed changes

alamb deleted the alamb/ilike_coercion branch October 19, 2023 15:52

alamb mentioned this pull request Oct 19, 2023

Minor: Fix docstring typos #7873

Merged

andygrove added the enhancement New feature or request label Nov 5, 2023

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Support Binary/LargeBinary --> Utf8/LargeUtf8 in ilike and string functions #7840

Support Binary/LargeBinary --> Utf8/LargeUtf8 in ilike and string functions #7840

Uh oh!

Conversation

alamb commented Oct 16, 2023 • 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!

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!

alamb commented Oct 17, 2023

Uh oh!

jackwener left a comment

Choose a reason for hiding this comment

Uh oh!

jonahgao commented Oct 18, 2023

Uh oh!

Weijun-H left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 18, 2023

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!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 19, 2023

Uh oh!

alamb commented Oct 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Support `Binary`/`LargeBinary` --> `Utf8`/`LargeUtf8` in ilike and string functions #7840

Support `Binary`/`LargeBinary` --> `Utf8`/`LargeUtf8` in ilike and string functions #7840

alamb commented Oct 16, 2023 •

edited

Loading