kosiew commented on code in PR #23735:
URL: https://github.com/apache/datafusion/pull/23735#discussion_r3664046911
##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -1009,7 +1009,12 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
) -> Result<Expr> {
let pattern = self.sql_expr_to_logical_expr(pattern, schema,
planner_context)?;
let pattern_type = pattern.get_type(schema)?;
- if pattern_type != DataType::Utf8 && pattern_type != DataType::Null {
+ // The analyzer coerces both operands to a common string type, so any
+ // string-typed (or untyped NULL) pattern is accepted here.
+ if !matches!(
+ pattern_type,
Review Comment:
Thanks for expanding this to `LargeUtf8` and `Utf8View`. I think we can go a
bit further here though.
The planner still rejects dictionary encoded string patterns, so we still
have the same duplicated planner and analyzer type gate that this PR is trying
to remove. `regex_coercion` already handles dictionary string operands via
`dictionary_coercion`, and it also owns the broader string-like coercion
behaviour, such as REE when constructible.
Unlike `SIMILAR TO`, `LIKE` does not have an equivalent planner side check.
As a result, this still fails during planning:
```sql
SELECT t.s SIMILAR TO arrow_cast(p.pat, 'Dictionary(Int32, Utf8)')
FROM t CROSS JOIN p;
```
with `Invalid pattern in SIMILAR TO expression`.
Could we let the analyzer own `SIMILAR TO` operand coercion and validation
entirely, or otherwise accept all of the string-like types that it already
supports? It would also be great to add a regression test for a dictionary
encoded pattern.
##########
datafusion/sqllogictest/test_files/type_coercion.slt:
##########
@@ -349,6 +349,41 @@ SELECT arrow_cast(t.s, 'Dictionary(Int32, Utf8)') SIMILAR
TO p.pat FROM t CROSS
----
true
+# non-scalar Utf8View / LargeUtf8 patterns are accepted by the SQL planner and
Review Comment:
Small suggestion. Could we move the issue URL into the section heading
instead of only this subsection comment? The existing `Utf8View` and
`LargeUtf8` value cases and these new pattern cases are all exercising the same
`SIMILAR TO` coercion invariant, so it seems nice to have the issue reference
apply to the whole section.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
