Skip to content

Conversation

@andygrove
Copy link
Member

@andygrove andygrove commented Feb 1, 2026

Which issue does this PR close?

Part of #2177

Rationale for this change

This is the next logical step towards removing support for the legacy native_comet Parquet scan implementation.

Note that this PR does not remove the native_comet implementation. It just stops Comet from using it as a scan type.

What changes are included in this PR?

  • Drop native_comet as a valid option for COMET_NATIVE_SCAN_IMPL config
  • Update CI workflows to stop specifying native_comet
  • Ignore tests that only test native_comet. I did not remove them because some of them are tests for v2 scans and we may want to support v2 scans in the future with native_datafusion and native_iceberg_compat scans.

How are these changes tested?

@andygrove andygrove changed the title feat: Drop native_comet add a valid option for COMET_NATIVE_SCAN_IMPL config [PROPOSAL] feat: Drop native_comet as a valid option for COMET_NATIVE_SCAN_IMPL config [PROPOSAL] Feb 1, 2026
andygrove and others added 2 commits February 1, 2026 12:41
…action

The macOS CI jobs were failing because the java-test action defaults
scan_impl to native_comet, which is no longer a valid option.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
# Conflicts:
#	spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala
#	spark/src/test/scala/org/apache/comet/rules/CometScanRuleSuite.scala
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2026

Codecov Report

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

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3358      +/-   ##
============================================
+ Coverage     56.12%   58.60%   +2.47%     
- Complexity      976     1404     +428     
============================================
  Files           119      175      +56     
  Lines         11743    16162    +4419     
  Branches       2251     2681     +430     
============================================
+ Hits           6591     9471    +2880     
- Misses         4012     5350    +1338     
- Partials       1140     1341     +201     

☔ 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 changed the title feat: Drop native_comet as a valid option for COMET_NATIVE_SCAN_IMPL config [PROPOSAL] feat: Drop native_comet as a valid option for COMET_NATIVE_SCAN_IMPL config Feb 1, 2026
@andygrove andygrove marked this pull request as ready for review February 1, 2026 21:24
@parthchandra
Copy link
Contributor

But DSV2 isn't supported by native_iceberg_compat so what will happen there (what if its a Parquet file with complex types)?

@andygrove
Copy link
Member Author

Comet will fall back to Spark for v2 scans until we add the support for v2 scans using native iceberg compat.

@andygrove
Copy link
Member Author

But DSV2 isn't supported by native_iceberg_compat so what will happen there (what if its a Parquet file with complex types)?

The current v2 support using native_comet wouldn't support the complex type case would it?

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.

This makes sense to me. I think a V2 compatible native_datafusion-backed Parquet scan will be pretty straightforward to implement after #3349, so we could close this gap quickly if we needed to. That said, V2 Parquet scans are still opt in so I'd be surprised if too many people noticed the feature gap at the moment.

@andygrove
Copy link
Member Author

This makes sense to me. I think a V2 compatible native_datafusion-backed Parquet scan will be pretty straightforward to implement after #3349, so we could close this gap quickly if we needed to. That said, V2 Parquet scans are still opt in so I'd be surprised if too many people noticed the feature gap at the moment.

Thanks @mbutrovich. @parthchandra does this make sense? Any objection to moving forward with this change?

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.

4 participants