andygrove commented on code in PR #6696:
URL: https://github.com/apache/datafusion-comet/pull/6696#discussion_r4196812030
##########
spark/src/main/spark-4.1+/org/apache/comet/serde/operator/CometMergeRows.scala:
##########
@@ -134,7 +131,7 @@ object CometMergeRows extends
CometOperatorSerde[MergeRowsExec] {
.addAllNotMatchedInstructions(notMatched.map(_.get).asJava)
.addAllNotMatchedBySourceInstructions(notMatchedBySource.map(_.get).asJava)
.addAllOutputTypes(outputTypes.map(_.get).asJava)
- .setSemanticMetricsRequired(true)
+
.setSemanticMetricsRequired(ShimCometMergeRows.hasNativeMergeSummary(op))
Review Comment:
On the insert-only path this turns off the native semantic counters, but
`CometMergeRowsExec.metrics` still declares all eight of them through
`MergeRowsMetricsShim`. So the node shows `number of target rows inserted: 0`
in the SQL UI, while Spark's `MergeRowsExec` counts the inserts on the same
plan. I ran the two-clause MERGE from `CometInsertOnlyMergeSuite` on Spark 4.2.
Spark's node reported 3 and `CometMergeRows` reported 0, with AQE on and off.
The commit itself is fine because `InsertOnlyMergeExec` builds its summary from
its own `numOutputRows`. Every `Keep(Insert)` here carries
`MERGE_ACTION_INSERT`, so could we keep `setSemanticMetricsRequired(true)`
unconditionally? I tried that locally. The node reported 3 and all nine
`CometInsertOnlyMergeSuite` tests still passed. The assertion at
`CometMergeRowsSuite.scala:82` would need to flip.
--
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]
