fix: coerce SIMILAR TO operands to a common string type to avoid 'failed to downcast array' panic#22887
Open
adriangb wants to merge 2 commits into
Open
fix: coerce SIMILAR TO operands to a common string type to avoid 'failed to downcast array' panic#22887adriangb wants to merge 2 commits into
adriangb wants to merge 2 commits into
Conversation
…led to downcast array' panic SIMILAR TO operands were never type-coerced: `Expr::SimilarTo` was listed in the no-op arm of the `TypeCoercion` analyzer, so a NULL pattern or mixed string types (e.g. Utf8View vs Utf8) reached `regex_match_dyn`, which blind-downcasts the right array to the left array's type and panicked with 'failed to downcast array'. - Coerce `Expr::SimilarTo` operands with `regex_coercion`, mirroring the existing `Expr::Like` handling (including the Dictionary(_, Utf8) left-hand-side exception that preserves the dictionary fast path). - Return an internal error instead of panicking on downcast failure in the `regexp_is_match_flag!` / `regexp_is_match_flag_scalar!` kernels. - Allow LargeUtf8 / Utf8View patterns in the SQL planner's SIMILAR TO pattern type check (previously only Utf8 and NULL were accepted). Closes apache#22886 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-actions
Bot
added
sql
Contributor
Author
|
@kumarUjjawal wonder if you could review this? |
kumarUjjawal
left a comment
kumarUjjawal
left a comment
Contributor
There was a problem hiding this comment.
Thank you @adriangb
Just had one comment please let me know what you think.
Type coercion deliberately preserves Dictionary(_, Utf8) inputs to SIMILAR TO so that literal patterns can use the dictionary fast path in regex_match_dyn_scalar, but regex_match_dyn (array-array) only handled flat string types, so a dictionary input matched against a NULL or non-literal pattern returned an internal error. Flatten dictionary inputs in regex_match_dyn and cast to the pattern type when the dictionary's value type differs from it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| let left = flatten_dictionary(left)?; | ||
| let mut right = flatten_dictionary(right)?; | ||
| if right.data_type() != left.data_type() { | ||
| right = cast(&right, left.data_type())?; |
Contributor
There was a problem hiding this comment.
if the left side is Dictionary(Int32, Utf8) and the pattern is LargeUtf8, type coercion should keep the common type as LargeUtf8. But we are flatenning the dictionary to Utf8, then casts the LargeUtf8 pattern back to Utf8. That can fail for valid large strings.
We can cast the dictionary side to the already-coerced pattern type, or compute the common string type again.
Jefffrey
reviewed
Jun 17, 2026
| 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 { | ||
| if !matches!( |
Contributor
There was a problem hiding this comment.
can use DataType::is_string here, though I wonder if we should be allowing dictionaries here too?
| ) | ||
| })?; | ||
| let expr = match left_type { | ||
| DataType::Dictionary(_, inner) if *inner == DataType::Utf8 => expr, |
Contributor
There was a problem hiding this comment.
it seems a bit odd here to limit it only to Utf8 and not other string types
Contributor
Author
|
Closing in favor of #23704 and followups. Thanks for reviewing @Jefffrey and @kumarUjjawal |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
SIMILAR TOpanics withfailed to downcast arraywhenever its operands end up as arrays of different physical types — e.g. aNULLpattern (which even panics at plan time via constant folding) or aUtf8Viewinput matched against a non-literalUtf8pattern. The root cause is twofold:Expr::SimilarTowas listed in the "nothing to coerce" arm of theTypeCoercionanalyzer, so its operands were never coerced to a common string type (unlikeLIKEand the regex binary operators).regex_match_dynpicks the downcast type from the left array and blind-.expect()downcasts the right array to the same type, panicking on any mismatch.What changes are included in this PR?
TypeCoercionRewriternow coercesExpr::SimilarTooperands to a common string type usingregex_coercion, mirroring the existingExpr::Likehandling (including theDictionary(_, Utf8)left-hand-side exception that preserves the dictionary fast path).regexp_is_match_flag!/regexp_is_match_flag_scalar!kernels inphysical-exprnow return an internal error instead of panicking if a downcast fails (defense in depth).SIMILAR TOpattern type check now also acceptsLargeUtf8andUtf8Viewpatterns (previously onlyUtf8andNULL); the coercion added above makes these work.Are these changes tested?
Yes:
test_files/strings.sltcoveringNULLpatterns / inputs (now returnNULLinstead of panicking), andUtf8View/LargeUtf8/Utf8inputs matched against non-literal patterns of a different string type (previously panicked).similar_to_for_type_coercionunit test intype_coercion.rsfollowing the existinglike_for_type_coercionpattern, including the error case for non-string operands.Are there any user-facing changes?
'a' SIMILAR TO NULL, mixed string types with non-literal patterns) now returnNULL/ correct results.SIMILAR TOoperands now produce a planning error (There isn't a common type to coerce ... in SIMILAR TO expression) instead of a runtime panic.LargeUtf8andUtf8Viewpatterns are now accepted by the SQL planner.