chore: Auto scan mode no longer falls back to native_comet#3236
Merged
andygrove merged 14 commits intoapache:mainfrom Jan 23, 2026
Merged
chore: Auto scan mode no longer falls back to native_comet#3236andygrove merged 14 commits intoapache:mainfrom
native_comet#3236andygrove merged 14 commits intoapache:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3236 +/- ##
============================================
+ Coverage 56.12% 60.19% +4.07%
- Complexity 976 1437 +461
============================================
Files 119 172 +53
Lines 11743 15919 +4176
Branches 2251 2626 +375
============================================
+ Hits 6591 9583 +2992
- Misses 4012 5006 +994
- Partials 1140 1330 +190 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Use local `root_op` variable instead of unwrapping `exec_context.root_op` - Replace `is_some()` + `unwrap()` pattern with `if let Some(...)` Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
andygrove
commented
Jan 22, 2026
andygrove
commented
Jan 22, 2026
andygrove
commented
Jan 22, 2026
| val path = new Path(dir.toURI.toString, "test.parquet") | ||
| makeParquetFileAllPrimitiveTypes(path, dictionaryEnabled = dictionaryEnabled, 10000) | ||
| withSQLConf(CometConf.COMET_SCAN_ALLOW_INCOMPATIBLE.key -> "false") { | ||
| // this test requires native_comet scan due to unsigned u8/u16 issue |
Contributor
There was a problem hiding this comment.
Should we divert the scan type to native_comet only for u8/u16? So that the rest of the types can be tested for iceberg_compat?
Member
Author
There was a problem hiding this comment.
Sure. I will make that change. Thanks for the review!
The csv_scan.rs file uses types from datafusion_datasource but the dependency was not declared in native/core/Cargo.toml. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add duplicate tests for "basic data type support" and "uint data type support" that skip reading _9 (UINT_8) and _10 (UINT_16) columns. This restores test coverage for the default scan implementation which can be overridden in CI. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
kazuyukitanimura
approved these changes
Jan 23, 2026
Contributor
kazuyukitanimura
left a comment
kazuyukitanimura
left a comment
There was a problem hiding this comment.
Thanks @andygrove
pending ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #2186
This is a step towards #2177
Rationale for this change
We need to start deprecating usage of
native_comet. This seems like a good place to start.What changes are included in this PR?
How are these changes tested?