yadavay-amzn commented on code in PR #55858:
URL: https://github.com/apache/spark/pull/55858#discussion_r3469634240
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -367,7 +367,10 @@ case class ReplaceData(
originalTable: NamedRelation,
projections: ReplaceDataProjections,
groupFilterCondition: Option[Expression] = None,
- write: Option[Write] = None) extends RowLevelWrite {
+ write: Option[Write] = None)
+ extends RowLevelWrite with SupportsNonDeterministicExpression {
+
+ override def allowNonDeterministicExpression: Boolean = operation.command()
== MERGE
Review Comment:
Thanks for catching this; you were right that it was unsafe. I reproduced
the silent lost update (a non-deterministic source whose key set differs
between the two scans makes the pruning subquery drop a target group the main
merge matches, losing the update) and pushed a fix.
Rather than materialize/checkpoint the source, I went at the root of the
hazard: skip runtime group filtering when its condition is non-deterministic -
a `cond.deterministic` gate in `canInjectGroupFilters`, since the
`groupFilterCondition` is an `Exists` over the source and is non-deterministic
exactly when the source is. That removes the double-scan entirely, and being a
single centralized gate it covers both the ReplaceData and WriteDelta paths.
Deterministic-source MERGE still gets the group filter, so the optimization is
unaffected for the common case.
I did evaluate materialization first, but it turned out to depend on
exchange/subquery reuse, which deliberately doesn't dedup non-deterministic
plans - so correctness would hinge on an optimization that can be disabled; the
guard instead follows Spark's existing stance of not optimizing
non-deterministic plans (e.g. InlineCTE).
Kept the `SupportsNonDeterministicExpression` relaxation so the analysis
false-positive is fixed.
Tests: an end-to-end suite over `InMemoryRowLevelOperationTable` that (a)
reproduces the original `INVALID_NON_DETERMINISTIC_EXPRESSIONS` failure now
passing and (b) runs with the runtime group filter enabled and asserts correct
rows (fails without the guard), plus a deterministic-source test confirming the
filter is still injected, and UPDATE/DELETE negative cases. Also folded the
parens nit and updated the PR description.
Does this approach look okay to you?
--
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]
