cloud-fan commented on code in PR #52185:
URL: https://github.com/apache/spark/pull/52185#discussion_r4360904644
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RewriteMergeIntoTable.scala:
##########
@@ -174,16 +174,29 @@ object RewriteMergeIntoTable extends
RewriteRowLevelCommand with PredicateHelper
readRelation, joinPlan, matchedActions, notMatchedActions,
notMatchedBySourceActions, metadataAttrs, checkCardinality)
- // predicates of the ON condition can be used to filter the target table
(planning & runtime)
+ // predicates that can be used to filter the target table (planning):
+ // - without NOT MATCHED BY SOURCE clauses, target rows that don't satisfy
the ON condition
+ // match no action and are simply carried over, so the ON condition can
filter the target
+ // - if every NOT MATCHED BY SOURCE clause is conditional, a target row
can be skipped only
+ // if it satisfies neither the ON condition nor any of those conditions,
so the filter is
+ // their disjunction; only the target-only part of the ON condition can
take part, as a
+ // disjunction that references the source is not a valid filter for the
target table
+ // - an unconditional NOT MATCHED BY SOURCE clause matches any target row
and disables this
+ val pushableCond = if (notMatchedBySourceActions.isEmpty) {
+ cond
+ } else if (notMatchedBySourceActions.exists(_.condition.isEmpty)) {
+ TrueLiteral
+ } else {
+ val (targetCond, _) = splitTargetPredicates(readRelation, cond)
Review Comment:
**Non-blocking (P2):** When the ON condition has no target-only conjunct,
which is the most common shape (`ON t.pk = s.pk`), `targetCond` here is
`TrueLiteral`. The disjunction then folds to `true` and nothing is pruned. That
fallback is what keeps groups with matched rows in the scan, but no test pins
it:
- All five new tests have a target-only ON conjunct (`t.dep IN (...)`, `1 =
0`, `t.dep = 'hr'`).
- The existing conditional-NMBS cases that use `ON t.pk = s.pk` (`merge
handles metadata columns correctly` and the `MergeIntoTableSuiteBase` /
`MergeIntoDataFrameSuite` cases) use `pk` or `salary` predicates.
`InMemoryBaseTable.canEvaluate` only evaluates `IN` on the partition column, so
those predicates never prune anything.
Suppose a later refactor built the disjunction only from the target
predicates that exist, dropping the `TrueLiteral` fallback. Every current
assertion would still pass. On a connector that prunes on the NMBS predicate,
though, `ON t.pk = s.pk WHEN MATCHED THEN UPDATE ... WHEN NOT MATCHED BY SOURCE
AND t.dep IN ('hr', 'finance') THEN DELETE` would skip the groups of matched
rows in other partitions, and their updates would be silently lost. The code
here is correct today. Could you add one `GroupBasedMergeIntoTableSuite` test
with that shape, including a matched row in a partition outside the NOT MATCHED
BY SOURCE condition?
**Verification:**
- **Regression:** A group-based MERGE with a join-key-only ON condition, a
WHEN MATCHED update of a row whose partition lies outside the NOT MATCHED BY
SOURCE condition, and a conditional NOT MATCHED BY SOURCE clause using IN on
the partition column keeps that row's update. The assertions fail if the pushed
condition drops the always-true target part and pushes only the NOT MATCHED BY
SOURCE condition.
- **Behavior:** For that MERGE, the pushed ReplaceData condition is
TrueLiteral, every target partition is replaced, runtime group filtering stays
disabled, and the final rows reflect the matched update and the NOT MATCHED BY
SOURCE deletes.
--
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]
