cloud-fan commented on code in PR #55711:
URL: https://github.com/apache/spark/pull/55711#discussion_r3273236673
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DeleteFromTableSuiteBase.scala:
##########
@@ -425,6 +427,46 @@ abstract class DeleteFromTableSuiteBase extends
RowLevelOperationSuiteBase {
}
}
+ test("metric values are stable across stage retries") {
+ // Force a shuffle in the DELETE plan via an IN-subquery (with broadcast
disabled), then
+ // have the DAGScheduler corrupt the first attempt of every upstream
shuffle map stage so
+ // the writer stage has to retry. With plain SQLMetrics the writer-side
numCopiedRows /
+ // numDeletedRows and the scan-side numOutputRows would all double up
across attempts;
+ // SQLLastAttemptMetric reports only the last attempt, so the values
surfaced via
+ // `DeleteSummary` (including the group-based driver derivation
+ // numDeletedRows = numScannedRows - numCopiedRows in
`ReplaceDataExec.getWriteSummary`)
+ // remain correct.
Review Comment:
The DELETE test *does* exercise the fix, but via the scan-side path only —
per the TODO in the conversation thread, the writer stage doesn't retry under
the current injection, so the writer-side `numCopiedRows` doesn't actually
double up; only the scan-side `numOutputRows` does, and the driver-side
derivation propagates the doubling. Suggested rewording:
```suggestion
// Force a shuffle in the DELETE plan via an IN-subquery (with broadcast
disabled), then
// have the DAGScheduler corrupt the first attempt of every upstream
shuffle map stage.
// The scan-side numOutputRows doubles up across attempts, and the
driver-side derivation
// numDeletedRows = numScannedRows - numCopiedRows in
`ReplaceDataExec.getWriteSummary`
// propagates that doubling into `DeleteSummary`. With
SQLLastAttemptMetric on the scan,
// the surfaced numDeletedRows stays correct. (The current fetch-failure
injection does
// not retry the writer stage, so writer-side numCopiedRows isn't
actually exercised
// here; follow-up #55738 will fill that gap.)
```
--
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]
