cloud-fan commented on code in PR #58867:
URL: https://github.com/apache/spark/pull/58867#discussion_r4047296328
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -412,6 +416,26 @@ case class CurrentUser()
final override val nodePatterns: Seq[TreePattern] = Seq(CURRENT_LIKE)
}
+private object AesEncryptDeterminism {
+ def apply(arguments: Seq[Expression]): Boolean = {
+ arguments.forall(_.deterministic) &&
+ (hasNonEmptyLiteral(arguments(4)) || isEcbLiteral(arguments(2)))
Review Comment:
**Blocking (P1):** Spark documents fixed IVs in forms such as `unhex(...)`,
but this helper recognizes only `Literal` or `Cast(Literal)`. Analyzer checks
read `deterministic` before constant folding reduces `Unhex`, so an otherwise
fixed AES call can be rejected as nondeterministic in a join condition and
loses valid folding in projections.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -471,12 +495,15 @@ case class AesEncrypt(
aad: Expression)
extends RuntimeReplaceable with ImplicitCastInputTypes {
+ override lazy val deterministic: Boolean = AesEncryptDeterminism(children)
Review Comment:
**Blocking (P1):** The wrapper now reports nondeterministic for an omitted
or empty IV, but `NondeterministicExpressionCollection` cannot find anything to
extract: `AesEncrypt` is neither `Nondeterministic` nor a UDF, and its
nondeterministic replacement is not a child. `GROUP BY aes_encrypt(...)` can
therefore hit the rule's internal error, while `Sort` leaves the call in place
for `CheckAnalysis` to reject.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MiscExpressionsSuite.scala:
##########
@@ -117,6 +120,36 @@ class MiscExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
assert(outputEval.contains(s"Result of $inputExpr is 1"))
}
+ test("DynamicPruningExpression is not foldable") {
+ assert(!DynamicPruningExpression(Literal.TrueLiteral).foldable)
+ }
+
+ test("AesEncrypt determinism reflects whether it generates an IV") {
+ val randomIvExpression = new AesEncrypt(Literal("Spark"),
Literal("0000111122223333"))
+ assert(!randomIvExpression.deterministic)
+ assert(!randomIvExpression.replacement.deterministic)
+ assert(!randomIvExpression.replacement.foldable)
+
+ val explicitIvExpression = new AesEncrypt(
+ Literal("Spark"),
+ Literal("0000111122223333"),
+ Literal("GCM"),
+ Literal("DEFAULT"),
+ Literal(Array.fill[Byte](12)(0)))
Review Comment:
**Non-blocking (P2):** These direct `Literal` constructions bypass
analyzer-resolved forms and plan consumers. They remain green for the
documented `unhex` fixed-IV misclassification, do not test a nondeterministic
child, and never exercise `PullOutNondeterministic` through Aggregate or Sort,
so the material parts of the new contract are not regression-covered.
See **Shared repair plan 1** in the review body.
--
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]
