cloud-fan commented on code in PR #58867:
URL: https://github.com/apache/spark/pull/58867#discussion_r4052346771
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpressionSuite.scala:
##########
@@ -127,20 +127,6 @@ class RewriteWithExpressionSuite extends PlanTest {
assert(rewritten == (Literal(1) + Literal(1)) * (Literal(1) + Literal(1)))
}
- test("applyForExpression rejects an impure foldable definition referenced
more than once") {
Review Comment:
**Non-blocking (P2):** Removing the obsolete AES fixture also removes the
only direct check that applyForExpression rejects a multiply referenced,
foldable NonSQLExpression. The remaining attribute, current-like, cast,
literal-tree, and canonical-ID cases exercise other guards, so the blanket
impurity rejection could regress unnoticed. Please replace this with a
purpose-built impure foldable fixture and update the stale AES examples in
RewriteWithExpression.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/NondeterministicExpressionCollection.scala:
##########
@@ -33,8 +33,18 @@ object NondeterministicExpressionCollection {
case udf: UserDefinedExpression if !udf.deterministic => udf
case udf: ExternalUserDefinedFunction if !udf.deterministic => udf
}
+ val expressionsToCollect = if (leafNondeterministic.nonEmpty) {
Review Comment:
**Non-blocking (P2):** The legacy-leaf and generic-wrapper searches are
mutually exclusive. For a valid expression such as GROUP BY aes_encrypt(uuid(),
'0000111122223333'), collecting Uuid suppresses the generic branch; after Uuid
is replaced, omitted-IV AesEncrypt is still nondeterministic, so Aggregate
throws the internal 'should not appear in grouping expression' error. The new
standalone AES cases leave the legacy set empty and cannot detect this mixed
frontier.
**Recommended change:** Compute a complete nondeterministic evaluation
frontier even when legacy leaves and generic intrinsically nondeterministic
wrappers coexist, then add mixed Aggregate and Sort regression coverage.
**Why this works:** Discover legacy leaves and generic boundaries as one
bottom-up decision rather than an if/else. Account for wrappers that remain
nondeterministic after selected descendants are treated as deterministic, and
materialize the appropriate enclosing evaluation unit once without separately
duplicating its nested nondeterministic child.
**Scope:** Correct shared nondeterministic frontier discovery and validate
mixed legacy-plus-wrapper plan shapes.
**Compatibility:** PrintToStderr and DynamicPruningExpression property
fixes, fixed-IV classification, and execution-time random IV generation remain
intact.
**Risks:** Collecting both an enclosing expression and its nested
nondeterministic child can evaluate the child twice or create unused aliases.
Changing the frontier can affect existing Rand and nondeterministic UDF
extraction order.
**Constraints:** Each selected nondeterministic evaluation unit must execute
once per input row. Standalone Rand, Uuid, and nondeterministic UDF handling
must retain existing behavior. Standalone omitted-IV AES and deterministic
fixed-IV AES properties must remain unchanged.
**Success:** A resolved Aggregate or Sort expression that combines a legacy
nondeterministic leaf with random-IV AesEncrypt contains no unextracted
nondeterministic operator expression after analysis. The enclosing AES
computation and any nested nondeterministic input are not duplicated. Existing
standalone nondeterministic expression cases retain their evaluation order and
multiplicity.
--
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]
