You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/09/28 15:26:36 UTC
[PR] fix: publish metrics on the update interval for native blocks without a JVM input [datafusion-comet]
andygrove opened a new pull request, #6314:
URL: https://github.com/apache/datafusion-comet/pull/6314
## Which issue does this PR close?
Closes #6313.
## Rationale for this change
A native block with no JVM input publishes its whole metric tree to the JVM after every output batch, ignoring `spark.comet.metrics.updateInterval`. Each publish walks the plan, aggregates every metric, encodes a protobuf and calls into the JVM. That cost 19–38 µs per batch in the measurements on the issue, or 40–78 ms for a task that returns 2,048 batches.
## What changes are included in this PR?
The channel branch of `executePlan`, which serves native blocks with no JVM input, now calls `update_metrics_on_interval` instead of `update_metrics`. That's the check the `ScanExec` path already uses. `releasePlan` still publishes the final values. The task-completion listeners that read them (`reportScanInputMetrics`, `reportSpillMetrics`) are already registered to run after the iterator closes, since blocks with a JVM input rely on the same ordering.
## How are these changes tested?
A new test in `CometTaskMetricsSuite` reads a 10,000-row Parquet file as ten batches with the interval disabled. The scan's `output_rows` must still be 0 after the first batch and reach 10,000 once the iterator closes. Without the fix the test fails with `output_rows` at 4,000 after one batch, because the producer runs ahead.
I also ran `CometTaskMetricsSuite`, `CometExecIteratorLifecycleSuite`, the "Comet native metrics" tests in `CometExecSuite` and the metrics tests in `CometIcebergNativeSuite` locally against Spark 4.1. All 49 passed.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: publish metrics on the update interval for native blocks without a JVM input [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6314:
URL: https://github.com/apache/datafusion-comet/pull/6314#issuecomment-5920111752
The merge of main in 00a55ed4e7 already picked up #6362, so the change is in `BatchProducer::next_batch` now.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: publish metrics on the update interval for native blocks without a JVM input [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #6314:
URL: https://github.com/apache/datafusion-comet/pull/6314#discussion_r4146849837
##########
spark/src/test/scala/org/apache/spark/sql/comet/CometTaskMetricsSuite.scala:
##########
@@ -1097,6 +1097,39 @@ class CometTaskMetricsSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
+ test("native block without a JVM input publishes SQL metrics on the update interval") {
Review Comment:
Nit: the name says this publishes on the update interval, but the body only covers the disabled interval. It would also pass if this branch never published mid-stream. Would it make sense to add a case with `"1"` and a short sleep before the first `batches.next()`, asserting `outputRows.value > 0` afterwards? I haven't run this, so please skip it if it turns out flaky.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: publish metrics on the update interval for native blocks without a JVM input [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #6314:
URL: https://github.com/apache/datafusion-comet/pull/6314#discussion_r4149555143
##########
spark/src/test/scala/org/apache/spark/sql/comet/CometTaskMetricsSuite.scala:
##########
@@ -1097,6 +1097,39 @@ class CometTaskMetricsSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
+ test("native block without a JVM input publishes SQL metrics on the update interval") {
Review Comment:
Added the `1` ms case in 299144ee16, plus 275327a6a6 to keep it compiling on Spark 3.x, where `withSQLConf` returns `Unit`. The task creates the native plan, which starts the interval clock, when it builds the iterator, so sleeping before the first `next()` makes the interval due deterministically rather than by timing. I checked that it catches the case you described: with the publish removed from this branch, the new case fails with `output_rows` still at 0 after the first batch. It passed three runs in a row with the fix.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: publish metrics on the update interval for native blocks without a JVM input [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #6314:
URL: https://github.com/apache/datafusion-comet/pull/6314
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org