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

@@ -681,6 +708,7 @@ fn binary_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType>
 /// 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

 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

@@ -648,8 +652,9 @@ fn string_concat_internal_coercion(
    }
 }

-/// 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
23 of 24 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

alamb Oct 17, 2023

alamb Oct 17, 2023

alamb Oct 17, 2023

alamb Oct 17, 2023

alamb Oct 17, 2023

alamb commented Oct 17, 2023

jackwener left a comment

jonahgao commented Oct 18, 2023

Weijun-H left a comment

alamb commented Oct 18, 2023

viirya Oct 18, 2023

alamb Oct 18, 2023

viirya Oct 18, 2023

viirya Oct 18, 2023

viirya Oct 18, 2023

viirya left a comment

alamb commented Oct 19, 2023

alamb commented Oct 19, 2023

		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.

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

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

Conversation

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

Choose a reason for hiding this comment

alamb commented Oct 17, 2023

jackwener left a comment

Choose a reason for hiding this comment

jonahgao commented Oct 18, 2023

Weijun-H left a comment

Choose a reason for hiding this comment

alamb commented Oct 18, 2023

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

viirya left a comment

Choose a reason for hiding this comment

alamb commented Oct 19, 2023

alamb commented Oct 19, 2023

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