Skip to content

Conversation

wForget
Copy link
Member

@wForget wForget commented Sep 4, 2025

Which issue does this PR close?

Closes #2300.

Rationale for this change

Comet cannot accelerate ProjectExec because: Comet is not enabled

We do not need to add fallback info for spark plan node when Comet is not enabled

Comet cannot accelerate ProjectExec because: Comet Scan is not enabled

Comet Scan is not enabled fallback reason should be placed in the scan node

Comet cannot accelerate CometProjectExec because: Metadata column is not supported

ditto

Comet cannot accelerate ShuffleExchangeExec because: spark.shuffle.manager is not set to org.apache.spark.sql.comet.execution.shuffle.CometShuffleManager$

CometShuffleManager class name should be org.apache.spark.sql.comet.execution.shuffle.CometShuffleManager

What changes are included in this PR?

Improve some confusing fallback reasons

How are these changes tested?

disable comet shuffle manager conf in CometTestBase and run the following test case:

  test("test fallback reasons") {
    withSQLConf(CometConf.COMET_LOG_FALLBACK_REASONS.key -> "true") {
      withTable("t1") {
        sql("create table t1 using parquet as select id as c1, id + 1 as c2 from range(10)")

        // Comet cannot accelerate ProjectExec because: Comet is not enabled
        withSQLConf(CometConf.COMET_ENABLED.key -> "false") {
          sql("select c1 + c2 from t1").collect()
        }
        // Comet cannot accelerate ProjectExec because: Comet Scan is not enabled
        withSQLConf(CometConf.COMET_NATIVE_SCAN_ENABLED.key -> "false") {
          sql("select c1 + c2 from t1").collect()
        }
        // Comet cannot accelerate CometProjectExec because: Metadata column is not supported
        sql("select _metadata from t1").collect()
        // Comet cannot accelerate ShuffleExchangeExec because: spark.shuffle.manager is not set to org.apache.spark.sql.comet.execution.shuffle.CometShuffleManager$
        sql("select /*+ repartition */ * from t1").collect()
      }
    }
  }

before this:

Comet cannot accelerate ProjectExec because: Comet is not enabled

Comet cannot accelerate ProjectExec because: Comet Scan is not enabled
Comet cannot accelerate FileSourceScanExec because: Scan parquet spark_catalog.default.t1 is not supported

Comet cannot accelerate ProjectExec because: Metadata column is not supported
Comet cannot accelerate FileSourceScanExec because: Scan parquet spark_catalog.default.t1 is not supported

Comet cannot accelerate ShuffleExchangeExec because: spark.shuffle.manager is not set to org.apache.spark.sql.comet.execution.shuffle.CometShuffleManager$

after this:


Comet cannot accelerate FileSourceScanExec because: Comet Scan is not enabled

Comet cannot accelerate FileSourceScanExec because: Metadata column is not supported

CometSparkSessionExtensions: Comet cannot accelerate ShuffleExchangeExec because: spark.shuffle.manager is not set to org.apache.spark.sql.comet.execution.shuffle.CometShuffleManager

@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2025

Codecov Report

❌ Patch coverage is 83.33333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.00%. Comparing base (f09f8af) to head (7e5c900).
⚠️ Report is 459 commits behind head on main.

Files with missing lines Patch % Lines
...n/scala/org/apache/comet/rules/CometScanRule.scala 80.55% 2 Missing and 5 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2301      +/-   ##
============================================
+ Coverage     56.12%   58.00%   +1.87%     
- Complexity      976     1294     +318     
============================================
  Files           119      146      +27     
  Lines         11743    13378    +1635     
  Branches       2251     2378     +127     
============================================
+ Hits           6591     7760    +1169     
- Misses         4012     4356     +344     
- Partials       1140     1262     +122     

☔ 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.

@comphead
Copy link
Contributor

comphead commented Sep 4, 2025

Thanks @wForget would you mind attach how reasons look before and after

@wForget
Copy link
Member Author

wForget commented Sep 5, 2025

Thanks @wForget would you mind attach how reasons look before and after

Thanks, I have edited description to add more test information.

@@ -540,8 +540,13 @@ case class CometExecRule(session: SparkSession) extends Rule[SparkPlan] {
// these cases specially here so we do not add a misleading 'info' message
op
case _ =>
// An operator that is not supported by Comet
withInfo(op, s"${op.nodeName} is not supported")
if (!hasExplainInfo(op)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Already has fallback reason, we don't need to overwrite it

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.

Thanks @wForget this is LGTM

@mbutrovich mbutrovich merged commit ab8a7b2 into apache:main Sep 5, 2025
93 checks passed
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.

Improve some confusing fallback reasons
4 participants