HyukjinKwon commented on code in PR #59227:
URL: https://github.com/apache/spark/pull/59227#discussion_r4231419416
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala:
##########
@@ -194,22 +205,18 @@ class V2ExpressionBuilder(e: Expression, isPredicate:
Boolean = false) extends L
case _: Signum => generateExpressionWithName("SIGN", expr, isPredicate)
case _: WidthBucket => generateExpressionWithName("WIDTH_BUCKET", expr,
isPredicate)
case and: And =>
- // AND expects predicate
- val l = generateExpression(and.left, true)
- val r = generateExpression(and.right, true)
+ val l = generatePredicate(and.left)
+ val r = generatePredicate(and.right)
if (l.isDefined && r.isDefined) {
- assert(l.get.isInstanceOf[V2Predicate] &&
r.get.isInstanceOf[V2Predicate])
Review Comment:
**Nit (P3):** Removing this assert (and the matching ones in the `Or` and
`Not` arms) leaves the existing test `VariantGet boolean targetType does not
crash under Or (isPredicate path)` in `DataSourceV2StrategySuite` explaining
itself by it. Its comment (lines 1018-1020) says a boolean `VariantGet` must
translate to a `V2Predicate` "or the enclosing And/Or's
`isInstanceOf[V2Predicate]` assert crashes planning", and its assertion message
(line 1022) says the `Or` "should translate without AssertionError".
After this PR, `generatePredicate` wraps a scalar Boolean operand as
`BOOLEAN_EXPRESSION`, or returns `None` with wrapping disabled, so that crash
can no longer happen. The test still passes and still checks something useful
(`VariantGet`'s own `BOOLEAN_EXPRESSION` wrapping under `Or`), but its
explanation now points at code that is gone. Could you reword the comment and
message, and optionally the test name, to describe that predicate translation
instead?
**Verification:**
- **Inspection:** The test's comment and assertion message no longer
describe the removed And/Or assert or an AssertionError crash, and they
describe the predicate translation the test asserts.
- **Behavior:** The VariantGet-under-Or translation test still passes with
unchanged assertions, so a boolean VariantGet under Or still translates to a V2
predicate.
--
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]
