uros-b commented on code in PR #59227:
URL: https://github.com/apache/spark/pull/59227#discussion_r4217465552
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -238,15 +245,11 @@ class V2ExpressionBuilder(e: Expression, isPredicate:
Boolean = false) extends L
} else {
None
}
- case Not(child) => generateExpression(child, true) // NOT expects predicate
- .map { v =>
- assert(v.isInstanceOf[V2Predicate])
- new V2Not(v.asInstanceOf[V2Predicate])
- }
+ case Not(child) => generatePredicate(child).map(new V2Not(_))
case UnaryMinus(_, true) => generateExpressionWithName("-", expr,
isPredicate)
case _: BitwiseNot => generateExpressionWithName("~", expr, isPredicate)
case caseWhen @ CaseWhen(branches, elseValue) =>
- val conditions = branches.map(_._1).flatMap(generateExpression(_, true))
+ val conditions = branches.map(_._1).flatMap(generatePredicate)
Review Comment:
The CASE WHEN change goes beyond the fix and is untested.
- **Why it is outside the fix:** CASE WHEN conditions never asserted on
master (`flatMap(generateExpression(_, true))`). This line changed only in
response to a review nit, and it changes two behaviors.
- **Default conf:** a scalar Boolean condition is now
`BOOLEAN_EXPRESSION(...)` instead of the bare expression. Examples are `CASE
WHEN CAST(c AS BOOLEAN) THEN ...`, a Boolean UDF, or `arr[0]` on an
`array<boolean>`.
- The generated JDBC SQL and `V2ExpressionUtils.toCatalyst` output are
identical, because the wrapper renders and converts as its child.
- However, third-party V2 connectors that do not know `BOOLEAN_EXPRESSION`
may now decline expressions they accepted before.
- **`alwaysCreateV2Predicate=false`:** such a CASE WHEN used to translate
and now returns None, so both filter and aggregate pushdown are lost for it.
- **The consistency is partial:** `If` (line 270) also lowers to
`CASE_WHEN`, but it still translates its condition with `generateExpression`.
- **Ask:** pick one.
- Keep the change, add a `DataSourceV2StrategySuite` case that pins the CASE
WHEN tree under both conf values (the suite's `testTranslateFilter` helper
takes an exact expected `Predicate`), and mention the change in the PR
description.
- Or revert line 252 to keep the PR scoped to the assertion fix.
--
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]
