Support `Binary`/`LargeBinary` --> `Utf8`/`LargeUtf8` in ilike and string functions by alamb · Pull Request #7840 · apache/datafusion

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

+ 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

+/// 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

 /// 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

- 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

-
-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

+// `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


-/// 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

+ }
+}
+
+/// 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

+///
+/// 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-actionsBot 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 b932e1eCompare 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 functionsOct 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 56514beCompare 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:mainOct 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

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

alamb commented Oct 16, 2023•
edited
Loading