-
Notifications
You must be signed in to change notification settings - Fork 281
feat: Drop native_comet as a valid option for COMET_NATIVE_SCAN_IMPL config
#3358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
native_comet add a valid option for COMET_NATIVE_SCAN_IMPL config [PROPOSAL]native_comet as a valid option for COMET_NATIVE_SCAN_IMPL config [PROPOSAL]
…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 Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
native_comet as a valid option for COMET_NATIVE_SCAN_IMPL config [PROPOSAL]native_comet as a valid option for COMET_NATIVE_SCAN_IMPL config
|
But DSV2 isn't supported by |
|
Comet will fall back to Spark for v2 scans until we add the support for v2 scans using native iceberg compat. |
The current v2 support using native_comet wouldn't support the complex type case would it? |
mbutrovich
left a comment
There was a problem hiding this 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.
Thanks @mbutrovich. @parthchandra does this make sense? Any objection to moving forward with this change? |
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_cometParquet scan implementation.Note that this PR does not remove the
native_cometimplementation. It just stops Comet from using it as a scan type.What changes are included in this PR?
native_cometas a valid option forCOMET_NATIVE_SCAN_IMPLconfignative_cometnative_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 withnative_datafusionandnative_iceberg_compatscans.How are these changes tested?