feat: fall back to Spark for collated predicate operands#4948
Merged
Conversation
Comet's native equality, ordering, and hashing compare raw UTF-8 bytes, so a predicate operand carrying a non-UTF8_BINARY collation (Spark 4+) would produce wrong answers on the native path -- for example, `'a' = 'A'` under UNICODE_CI returns true in Spark but false byte-wise. Add a `ComparisonUtils.collationSupportLevel` shared helper and wire it into `getSupportLevel` for the six binary comparison serdes plus `In`/`InSet`. `CometNot`'s special-case rewrites for `Not(EqualTo|EqualNullSafe|In)` now guard on the same helper so they fall through to the generic path (and thus to Spark) when any operand is collated. The helper walks nested types via `hasNonDefaultStringCollation`, so collated strings inside array/map/struct operands are caught too.
Mix CodegenDispatchFallback into CometStrToMap so that when the input string or a delimiter carries a non-UTF8_BINARY collation the expression is executed by Spark's own doGenCode inside the Comet kernel rather than falling the whole projection back to Spark. Update the str_to_map_collation.sql fixture to drop the expect_fallback(...) assertion accordingly.
Update str_to_map_legacy_truncate.sql to drop the expect_fallback assertions -- with CodegenDispatchFallback on CometStrToMap the expression now runs via Spark's own doGenCode inside the Comet kernel instead of falling the projection back to Spark.
Member
|
Do we need to do the same for Contains, StartsWith, EndsWith, and Like? Maybe they are already handlded correctly? |
andygrove
reviewed
Jul 16, 2026
Comment on lines
+113
to
+114
| override def getSupportLevel(expr: EqualTo): SupportLevel = | ||
| ComparisonUtils.collationSupportLevel(expr.left, expr.right) |
Member
There was a problem hiding this comment.
Could you add/update tests for predicates to demonstrate the fix?
Contributor
Author
There was a problem hiding this comment.
Adding tests including Contains, StartsWith, EndsWith, and Like to increase regression coverage
Extends 903a772b6 to the string-predicate serdes flagged in review. CometLike, CometContains, CometStartsWith, and CometEndsWith now route their `getSupportLevel` through `ComparisonUtils.collationSupportLevel` so any non-UTF8_BINARY operand triggers a clean fallback. Previously Contains / StartsWith / EndsWith reached the native scalar-function bridge via `CometScalarFunction` with no collation guard, and Like's `getSupportLevel` only rejected custom escape characters. Native kernels compare raw UTF-8 bytes, so a case- or accent-insensitive collation would yield wrong answers (e.g. `'Hello' LIKE '%hello%'` returns true in Spark under UNICODE_CI but false byte-wise). `collationSupportLevel` grows an optional `exprName` so each predicate surfaces a fallback reason that names itself in EXPLAIN. Expands `sql-tests/.../collation.sql` with predicate coverage: the six binary comparisons under UTF8_LCASE and UNICODE_CI, `IN`/`NOT IN` (scalar and `InSet`-threshold), CometNot's `NOT (=)` / `NOT (<=>)` rewrites, `LIKE`, `Contains`, `StartsWith`, `EndsWith`, collated strings buried in `array`/`map`/`struct` operands (exercising `hasNonDefaultStringCollation`'s recursion), and NULL propagation through each of the above.
Every predicate query in `collation.sql` now uses `query expect_fallback(non-UTF8_BINARY collated operands)` instead of plain `query`. The predicate serdes correctly return `Unsupported` for collated operands, so the enclosing Project falls back to Spark -- which is the whole point of the fix -- but that violates the default `CheckCoverageAndAnswer` assertion mode's "only Comet native operators" check. `expect_fallback` still verifies the answer against Spark and additionally pins the shared fallback-reason substring, so a regressed guard shows up as either a wrong answer or a missing fallback. Also drops the map test: `CometCreateMap` routes through the JVM codegen dispatcher, so its interaction with the outer EqualTo's collation guard is not the clean path this file is meant to pin down.
Two classes of tests never reached the predicate serdes and so could not exercise the fix: - NULL-literal RHS: Spark's `NullPropagation` rewrites `x <=> NULL` to `IsNull(x)`, folds `x = NULL` and `NULL IN (...)` to `Literal(null)`, etc., so the predicate serde is never asked about a collated NULL operand. The row `(7, NULL)` in `test_collated_predicates` still exercises NULL propagation through every remaining predicate query. - UNICODE_CI + LIKE / Contains / StartsWith / EndsWith: Spark 4.0 restricts these to `StringTypeNonCSAICollation`, which rejects UNICODE_CI at analysis time with a DATATYPE_MISMATCH. UTF8_LCASE is accepted and already covers the same fallback path.
Drop the "what" narration and shrink each remaining note to a single sentence about *why* the case exists (NullPropagation folding, NonCSAICollation restriction, InSet threshold, etc.). No SQL changes.
Member
|
The collation guard looks right. One question on routing. Since #4728, an Could |
andygrove
reviewed
Jul 17, 2026
| * from Spark's collation-aware evaluation. | ||
| */ | ||
| trait CollationAwareBinaryPredicate[T <: BinaryExpression] extends CometExpressionSerde[T] { | ||
| override def getSupportLevel(expr: T): SupportLevel = |
Member
There was a problem hiding this comment.
Would it also make sense to implement getUnsupportedReasons so that these fallback reasons get documented?
Addresses two review comments on apache#4948. CollationAwareBinaryPredicate, CometIn, CometInSet, and CometLike now mix in CodegenDispatchFallback, so an `Unsupported` result from a collated operand stays in the Comet pipeline via Spark's own `doGenCode` rather than falling the whole projection or filter back to Spark. CometContains / CometStartsWith / CometEndsWith inherit the mixin through the trait. Collation semantics still come from Spark's codegen path, so answers stay Spark-correct. Each of these serdes now overrides `getUnsupportedReasons` so the collation case (and, for Like, the custom-escape case) is picked up by GenerateDocs and surfaces in the compatibility guide instead of being undocumented. The SQL tests in `collation.sql` switch from `expect_fallback(non-UTF8_BINARY collated operands)` back to plain `query`: with the dispatcher taking over, the operator stays native and the fallback-coverage assertion catches any regression that would force it back to Spark.
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
2 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.
Splitting #4799
Comet's native equality, ordering, and hashing compare raw UTF-8 bytes, so a predicate operand carrying a non-UTF8_BINARY collation (Spark 4+) would produce wrong answers on the native path -- for example,
'a' = 'A'under UNICODE_CI returns true in Spark but false byte-wise.Add a
ComparisonUtils.collationSupportLevelshared helper and wire it intogetSupportLevelfor the six binary comparison serdes plusIn/InSet.CometNot's special-case rewrites forNot(EqualTo|EqualNullSafe|In)now guard on the same helper so they fall through to the generic path (and thus to Spark) when any operand is collated. The helper walks nested types viahasNonDefaultStringCollation, so collated strings inside array/map/struct operands are caught too.Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
How are these changes tested?