cloud-fan commented on code in PR #58413:
URL: https://github.com/apache/spark/pull/58413#discussion_r3913812755
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/ProtobufSerializer.scala:
##########
@@ -112,7 +112,7 @@ private[sql] class ProtobufSerializer(
(getter, ordinal) => getter.getFloat(ordinal)
Review Comment:
**Non-blocking (P2):** This widening also changes the ENUM branch here and
the `StringValue` branch below, but the SPARK-59112 test serializes only an
ordinary protobuf STRING field. A regression in either separately dispatched
branch would still pass. Please add `CharType` and `VarcharType` cases for
valid and invalid enum symbols and for `StringValue` payloads.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3848,7 +3848,10 @@ class AstBuilder extends DataTypeAstBuilder
throw QueryParsingErrors.emptyQuantifiedPatternError(ctx)
}
val expressions = expressionList(ctx.expression)
- if (expressions.forall(_.foldable) &&
expressions.forall(_.dataType == StringType)) {
+ if (expressions.forall(_.foldable) &&
+ expressions.forall(
+ expression => expression.resolved &&
+
DataTypeUtils.isDefaultStringCharOrVarcharType(expression.dataType))) {
Review Comment:
**Non-blocking (P2):** Explicit `UTF8_BINARY` patterns matched the old
`dataType == StringType` gate, but this predicate now sends them through the
documented `reduceLeft(Or/And)` stack-overflow path. Please preserve their
explicit collation while building the fallback as a balanced tree, and add a
large-list regression for ANY/SOME/ALL.
**Recommended change:** Build a balanced Or/And fallback for explicitly
collated quantified patterns instead of re-admitting them to the
collation-dropping LikeAny/LikeAll representation.
**Why this works:** Combine the collation-preserving Like/ILike predicates
pairwise for ANY/SOME and ALL so expression depth grows logarithmically while
each original pattern expression retains its collation.
**Scope:** AstBuilder quantified LIKE/ILIKE fallback construction plus
focused large-list coverage in CollationSQLRegexpSuite.
**Compatibility:** This preserves explicit-collation semantics while
restoring the bounded-depth behavior that explicit UTF8_BINARY patterns had
before this patch.
**Risks:** The balancing helper must preserve quantifier, negation, and
left-to-right predicate semantics. The fallback plan shape changes for
explicitly collated lists and needs coverage across LIKE and ILIKE.
**Constraints:** Do not route explicitly non-default collations through
LikeAny/LikeAll unless semantic equivalence is proven. Retain current
empty-list validation and ANY/SOME/ALL behavior.
**Success:** Large explicit UTF8_BINARY pattern lists retain their
collation, have bounded expression depth, and evaluate without
StackOverflowError for ANY/SOME/ALL and negated forms.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3848,7 +3848,10 @@ class AstBuilder extends DataTypeAstBuilder
throw QueryParsingErrors.emptyQuantifiedPatternError(ctx)
}
val expressions = expressionList(ctx.expression)
- if (expressions.forall(_.foldable) &&
expressions.forall(_.dataType == StringType)) {
+ if (expressions.forall(_.foldable) &&
+ expressions.forall(
+ expression => expression.resolved &&
+
DataTypeUtils.isDefaultStringCharOrVarcharType(expression.dataType))) {
Review Comment:
**Non-blocking (P2):** The widened gate admits foldable CHAR/VARCHAR NULLs,
but `patterns.map(_.toLowerCase)` then dereferences the null `UTF8String`.
`ILIKE ANY`/`SOME`/`ALL` with `CAST(NULL AS CHAR/VARCHAR)` should follow SQL
null semantics rather than throw. Please preserve null entries while
lowercasing non-null patterns and cover both ANY and ALL.
--
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]
