fix: accept LargeUtf8 and Utf8View patterns in SIMILAR TO planning - #23735
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23735 +/- ##
==========================================
- Coverage 80.69% 80.69% -0.01%
==========================================
Files 1095 1095
Lines 372626 372622 -4
Branches 372626 372622 -4
==========================================
- Hits 300700 300690 -10
- Misses 53978 53979 +1
- Partials 17948 17953 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
f9061a3 to
69f531e
Compare
| SELECT t.s NOT SIMILAR TO arrow_cast(p.pat, 'Utf8View') FROM t CROSS JOIN p; | ||
| ---- | ||
| false | ||
|
|
There was a problem hiding this comment.
Can we add tests for teh null case?
There was a problem hiding this comment.
Done in add3f8d — added a literal NULL pattern in the table context and a non-scalar Null-typed pattern column (CREATE TABLE pn AS SELECT NULL AS pat), both evaluate to NULL.
7738741 to
21f48ec
Compare
| // 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, |
There was a problem hiding this 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:
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.
| ---- | ||
| true | ||
|
|
||
| # non-scalar Utf8View / LargeUtf8 patterns are accepted by the SQL planner and |
There was a problem hiding this 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.
|
Thanks for the review! I've removed the planner-side type gate so the analyzer now owns SIMILAR TO coercion and validation, added dictionary-pattern coverage, and moved the issue reference to the section heading. Fixed in a209834. |
a209834 to
b065774
Compare
Closes apache#23732. Follow-up to apache#23704: now that the TypeCoercion analyzer coerces SIMILAR TO operands to a common string type, the SQL planner no longer needs to reject LargeUtf8/Utf8View patterns (consistent with LIKE, which has no such check).
b065774 to
9e61c37
Compare
|
thanks folks! |
Which issue does this PR close?
Rationale for this change
Follow-up to #23704, which made the
TypeCoercionanalyzer coerceSIMILAR TOoperands to a common string type. The SQL planner still rejectsLargeUtf8andUtf8Viewpatterns before the analyzer ever runs:Now that the analyzer coerces both operands, this planner restriction is unnecessary (and inconsistent with
LIKE, whose planner path has no such check).What changes are included in this PR?
datafusion/sql/src/expr/mod.rs: the pattern-type check insql_similarto_to_exprnow acceptsUtf8/LargeUtf8/Utf8View/Nullinstead of onlyUtf8/Null.datafusion/sqllogictest/test_files/type_coercion.slt: regression tests with non-literalUtf8ViewandLargeUtf8patterns (plus aNOT SIMILAR TOcase), next to the existing SIMILAR TO panics ('failed to downcast array') when operand types differ (e.g. NULL pattern, Utf8View vs Utf8) #22886 regression tests.The planner relaxation and tests were originally part of #22887; this PR lands the remaining piece of it.
Are these changes tested?
Yes — new sqllogictest cases in
type_coercion.slt. The existing planner rejection of non-string patterns (SELECT 'a' SIMILAR TO 1) still errors as before.Are there any user-facing changes?
Yes:
SIMILAR TOqueries with non-literalLargeUtf8/Utf8Viewpatterns that previously failed during SQL planning withInvalid pattern in SIMILAR TO expressionnow plan and execute successfully. No breaking API changes.