andygrove commented on code in PR #5143:
URL: https://github.com/apache/datafusion-comet/pull/5143#discussion_r4219832363
##########
spark/src/main/scala/org/apache/spark/sql/comet/CometNativeWriteExec.scala:
##########
@@ -101,7 +101,10 @@ case class CometNativeWriteExec(
override lazy val metrics: Map[String, SQLMetric] = Map(
"files_written" -> SQLMetrics.createMetric(sparkContext, "number of
written data files"),
"bytes_written" -> SQLMetrics.createSizeMetric(sparkContext, "written
data"),
- "rows_written" -> SQLMetrics.createMetric(sparkContext, "number of written
rows"))
+ "rows_written" -> SQLMetrics.createMetric(sparkContext, "number of written
rows"),
+ "elapsed_compute" -> SQLMetrics.createNanoTimingMetric(
Review Comment:
Since #5763, Spark 4.0+ writes go through `CometWriteFilesExec` instead, and
this node only runs on 3.4 and 3.5. `CometWriteFilesExec.metrics` is
`Map.empty`, and `CometMetricNode.set` drops any native metric that has no JVM
entry, so on the 4.x builds the new timer is computed and thrown away. On Spark
4.1 with main merged in, the `CometWriteFiles` node reports no metrics at all.
Adding the same `elapsed_compute` entry to `CometWriteFilesExec.metrics` makes
it report about 27 ms for a 1,000-row write, in line with what this node
reports on 3.5. Could we add it there too? The scaladoc on that `metrics` val
would need an update as well. It says the node has no metrics of its own, and
that `bytes_written` comes from `std::fs::metadata`, which this PR changes.
--
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]
