Skip to content

Conversation

@andygrove
Copy link
Member

@andygrove andygrove commented Jan 6, 2026

Which issue does this PR close?

Closes #3019

Rationale for this change

If benchmark queries are not running fully natively then it could be due to a bug or incorrect config and we want to know this.

Example:

================================================================================
WARNING: Benchmark plan is NOT fully Comet native!
First non-Comet operator: Project
================================================================================
Query plan:
*(1) Project [cast(c1#11 as decimal(10,2)) AS c1#3226]
+- *(1) CometColumnarToRow
   +- CometScan [native_iceberg_compat] parquet [c1#11] Batched: true, DataFilters: [], Format: CometParquet, Location: InMemoryFileIndex(1 paths)[file:/tmp/spark-72dac88c-8ae1-4c75-8648-53306da6c526/parquetV1], PartitionFilters: [], PushedFilters: [], ReadSchema: struct<c1:string>

================================================================================

What changes are included in this PR?

Show a warning if a query does not run fully natively when running Comet (Scan + Exec).

How are these changes tested?

@andygrove andygrove marked this pull request as ready for review January 6, 2026 16:27
}

/** Runs function `f` with Comet on and off. */
final def runWithComet(name: String, cardinality: Long)(f: => Unit): Unit = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is no longer used

*
* Based on CometTestBase.findFirstNonCometOperator.
*/
protected def findFirstNonCometOperator(plan: SparkPlan): Option[SparkPlan] = {
Copy link
Contributor

@comphead comphead Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we keep one version for this methods to keep in sync the test behavior?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this to a new CometPlanChecker trait to avoid duplication.

We may want to rename this trait in the future if we move more code there.

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm thanks @andygrove

Copy link
Contributor

@mbutrovich mbutrovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix, @andygrove!

Copy link
Contributor

@parthchandra parthchandra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.55%. Comparing base (f09f8af) to head (bc1dfa1).
⚠️ Report is 826 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3045      +/-   ##
============================================
+ Coverage     56.12%   59.55%   +3.43%     
- Complexity      976     1379     +403     
============================================
  Files           119      167      +48     
  Lines         11743    15496    +3753     
  Branches       2251     2569     +318     
============================================
+ Hits           6591     9229    +2638     
- Misses         4012     4970     +958     
- Partials       1140     1297     +157     

☔ 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 andygrove merged commit 77899ee into apache:main Jan 7, 2026
214 of 217 checks passed
@andygrove andygrove deleted the benchmark-check-native branch January 7, 2026 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ability for microbenchmarks to verify that queries actually ran in Comet

5 participants