feat: Add plan conversion statistics to extended explain info by andygrove · Pull Request #2412 · apache/datafusion-comet

andygrove · 2025-09-17T15:23:17Z

Which issue does this PR close?

N/A

Rationale for this change

I would like to make it easier to see what % of operators are accelerated by Comet.

What changes are included in this PR?

Add some basic statistics to the "extended explain" output to show % of operators accelerated by Comet.

Example output:

BroadcastHashJoin
:- ColumnarToRow
:  +-  Scan parquet  [COMET: Dynamic Partition Pruning is not supported]
:        +- SubqueryBroadcast
:           +- ReusedExchange
+- BroadcastExchange
   +- CometColumnarToRow
      +- CometFilter
         +- CometScan [native_iceberg_compat] parquet 

Comet accelerated 33% of eligible operators (sparkOperators=4, cometOperators=2, transitions=2, wrappers=1).

How are these changes tested?

Manually

codecov-commenter · 2025-09-17T15:46:33Z

Codecov Report

❌ Patch coverage is 71.79487% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.54%. Comparing base (f09f8af) to head (6436574).
⚠️ Report is 567 commits behind head on main.

Files with missing lines	Patch %	Lines
...n/scala/org/apache/comet/ExtendedExplainInfo.scala	71.05%	6 Missing and 5 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2412      +/-   ##
============================================
+ Coverage     56.12%   58.54%   +2.41%     
- Complexity      976     1445     +469     
============================================
  Files           119      146      +27     
  Lines         11743    13534    +1791     
  Branches       2251     2356     +105     
============================================
+ Hits           6591     7923    +1332     
- Misses         4012     4379     +367     
- Partials       1140     1232      +92

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

andygrove · 2025-09-17T16:58:10Z

Subset of output from benchmarks

Reading query 16 using path /mnt/bigdata/tpch/queries//q16.sql
Comet accelerated 87% of eligible operators (sparkOperators=1, cometOperators=7, transitions=0, wrappers=0).
Comet accelerated 84% of eligible operators (sparkOperators=4, cometOperators=22, transitions=0, wrappers=0).
Comet accelerated 80% of eligible operators (sparkOperators=4, cometOperators=17, transitions=0, wrappers=0).
Comet accelerated 75% of eligible operators (sparkOperators=1, cometOperators=3, transitions=0, wrappers=0).
Comet accelerated 100% of eligible operators (sparkOperators=0, cometOperators=3, transitions=0, wrappers=0).
Comet accelerated 84% of eligible operators (sparkOperators=4, cometOperators=22, transitions=1, wrappers=0).
Comet accelerated 84% of eligible operators (sparkOperators=4, cometOperators=22, transitions=1, wrappers=0).
Comet accelerated 75% of eligible operators (sparkOperators=2, cometOperators=6, transitions=1, wrappers=0).

andygrove · 2025-09-17T17:19:07Z

+    --conf spark.comet.explain.enabled=true \
    --conf spark.memory.offHeap.enabled=true \
-    --conf spark.memory.offHeap.size=16g
+    --conf spark.memory.offHeap.size=2g


Unrelated change, but we don't need 16g for this simple example

comphead · 2025-09-17T23:03:15Z

+
+          withSQLConf(CometConf.COMET_EXPLAIN_VERBOSE_ENABLED.key -> "true") {
+            val extendedExplain = new ExtendedExplainInfo().generateExtendedInfo(cometPlan)
+            assert(extendedExplain.contains("Comet accelerated 33% of eligible operators"))


would be this number fluctuating?

It is currently stable across all Spark versions that we test with.

I was wrong. There is a failure due to a different percentage. I will make the test less specific.

- DPP fallback *** FAILED *** (1 second, 553 milliseconds) "BroadcastHashJoin :- ColumnarToRow : +- Scan parquet [COMET: Dynamic Partition Pruning is not supported] : +- SubqueryBroadcast : +- BroadcastExchange : +- CometColumnarToRow : +- CometFilter : +- CometScan [native_iceberg_compat] parquet +- BroadcastExchange +- CometColumnarToRow +- CometFilter +- CometScan [native_iceberg_compat] parquet Comet accelerated 4 out of 9 eligible operators (44%). Final plan contains 3 transitions between Spark and Comet." did not contain "Comet accelerated 33% of eligible operators" (CometExecSuite.scala:124)

comphead · 2025-09-17T23:46:58Z

Comet accelerated 33% of eligible operators (sparkOperators=4, cometOperators=2, transitions=2, wrappers=1).

From the user perspective it might be not very clear what is accelerated?
When seeing message first time, I thought all of them are accelerated?

 (sparkOperators=4, cometOperators=2, transitions=2, wrappers=1).

which is not the case?

andygrove · 2025-09-18T20:03:26Z

Comet accelerated 33% of eligible operators (sparkOperators=4, cometOperators=2, transitions=2, wrappers=1).
From the user perspective it might be not very clear what is accelerated? When seeing message first time, I thought all of them are accelerated?
 (sparkOperators=4, cometOperators=2, transitions=2, wrappers=1).
which is not the case?

Thanks, I will add documentation as part of this PR

parthchandra · 2025-09-18T22:14:54Z

+  var transitions: Int = 0
+
+  override def toString: String = {
+    s"sparkOperators=$sparkOperators, cometOperators=$cometOperators, " +


Perhaps we could use a more verbose string here so that the meaning of these stats is a little more obvious

parthchandra · 2025-09-18T22:20:24Z

We could potentially add a method to get the plan stats independent of the explain string too. I can see this being useful in writing some tool to analyse queries/plans without executing them.

andygrove · 2025-09-29T19:56:47Z

Thanks for the reviews @comphead and @parthchandra. Could you take another look?

parthchandra

lgtm. Minor improvement suggested. Also, is the comment re the unit test addressed?

Co-authored-by: Parth Chandra <parthc@apache.org>

andygrove · 2025-10-01T15:48:37Z

Thanks for the reviews @parthchandra @comphead. I have addressed feedback and plan on merging this later today, unless you have any other feedback

…#2412)

andygrove added 5 commits September 17, 2025 08:25

Add plan stats to extended explain

79310d0

save

5157f83

save

4ef5293

prep for review

8b6dcfc

prep for review

8dca97d

save progress

45aa334

andygrove commented Sep 17, 2025

View reviewed changes

andygrove added 3 commits September 17, 2025 11:23

docs

2dc8f70

update docs

22407fc

Revert

f85ffd5

andygrove marked this pull request as ready for review September 17, 2025 18:12

andygrove requested review from kazuyukitanimura and parthchandra September 17, 2025 18:12

fix

3552888

andygrove marked this pull request as draft September 17, 2025 18:29

andygrove added 2 commits September 17, 2025 12:33

revert some changes

8fc2108

revert

6f99dd9

andygrove marked this pull request as ready for review September 17, 2025 18:36

revert

9cd439f

comphead reviewed Sep 17, 2025

View reviewed changes

Comment thread spark/src/main/scala/org/apache/comet/ExtendedExplainInfo.scala Outdated

andygrove marked this pull request as draft September 18, 2025 01:12

parthchandra reviewed Sep 18, 2025

View reviewed changes

andygrove added 2 commits September 29, 2025 13:45

upmerge

ad7ff51

rename

ac57ee9

andygrove added 2 commits September 29, 2025 13:51

address feedback

36c0439

address feedback

bdc8300

andygrove marked this pull request as ready for review September 29, 2025 19:55

parthchandra reviewed Sep 30, 2025

View reviewed changes

Comment thread spark/src/main/scala/org/apache/comet/ExtendedExplainInfo.scala Outdated

parthchandra approved these changes Sep 30, 2025

View reviewed changes

andygrove and others added 3 commits September 30, 2025 08:23

Update spark/src/main/scala/org/apache/comet/ExtendedExplainInfo.scala

62473ad

Co-authored-by: Parth Chandra <parthc@apache.org>

Refactor

e483ffa

make check more generic

6a0500d

fix

6436574

andygrove merged commit ca2e611 into apache:main Oct 1, 2025
102 checks passed

andygrove deleted the extended-explain-stats branch October 1, 2025 20:10

coderfender pushed a commit to coderfender/datafusion-comet that referenced this pull request Dec 13, 2025

feat: Add plan conversion statistics to extended explain info (apache…

f06845d

…#2412)

Conversation

andygrove commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

codecov-commenter commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andygrove commented Sep 17, 2025

Uh oh!

andygrove Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

comphead Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

andygrove Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

andygrove Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

comphead commented Sep 17, 2025

Uh oh!

andygrove commented Sep 18, 2025

Uh oh!

parthchandra Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

parthchandra commented Sep 18, 2025

Uh oh!

andygrove commented Sep 29, 2025

Uh oh!

Uh oh!

parthchandra left a comment

Choose a reason for hiding this comment

Uh oh!

andygrove commented Oct 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andygrove commented Sep 17, 2025 •

edited

Loading

codecov-commenter commented Sep 17, 2025 •

edited

Loading