chore: Update CometTestBase to stop setting the scan implementation to native_comet#2176
Conversation
andygrove
changed the title
CometTestBase to stop setting the scan implementation to native_comet
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2176 +/- ##
============================================
+ Coverage 56.12% 58.52% +2.40%
- Complexity 976 1279 +303
============================================
Files 119 143 +24
Lines 11743 13187 +1444
Branches 2251 2358 +107
============================================
+ Hits 6591 7718 +1127
- Misses 4012 4245 +233
- Partials 1140 1224 +84 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
lgtm thanks @andygrove
Thanks for commenting test failures
parthchandra
left a comment
parthchandra
left a comment
There was a problem hiding this comment.
For the tests that are failing with native_iceberg_compat, are these regressions? We have the Spark SQL Tests (native_iceberg_compat) pipeline that is required for CI.
| // usingDataFusionParquetExec does not support CometBatchScanExec yet | ||
| if (!isDataFusionScan) { | ||
| assert(CometBatchScanExec.isTypeSupported(dt, "", fallbackReasons) == expected) | ||
| withSQLConf(CometConf.COMET_NATIVE_SCAN_IMPL.key -> CometConf.SCAN_NATIVE_COMET) { |
There was a problem hiding this comment.
This test should pass for all scan impls. (the isDataFusionScan check modifies the test appropriately)
There was a problem hiding this comment.
Alas, it fails with:
true did not equal false
ScalaTestFailureLocation: org.apache.comet.parquet.ParquetReadSuite at (ParquetReadSuite.scala:118)
Expected :false
Actual :true
The error wasn't super helpful.
There was a problem hiding this comment.
After improving the error message:
Failed on isTypeSupported check for ArrayType(TimestampType,true); expected=false, actual=true
If I set the scan impl to native_iceberg_compat, native_datafusion, or native_comet, then it passes. It just does not work with auto. I will file a separate issue.
There was a problem hiding this comment.
I see the issue ... I will push a fix
There was a problem hiding this comment.
I spent some time trying to get these two tests to pass but the implementations are not correct. I updated them to explicitly use native_comet for now (so no change in behavior) and filed a follow-on issue to reimplement these two tests.
We would also need to update or remove these tests as part of #2177
There was a problem hiding this comment.
This looks okay. I also filed #2195 to follow up on the signed/unsigned incompatibility which was, iirc, one of the reasons for conditional code in many tests.
I checked, and that workflow does not run on PRs. It has to be run manually, and we haven't done that in a while. |
Actually, we update the default Spark SQL tests to run with |
Ah, thanks for checking and clarifying. |
|
Thanks for the reviews @comphead and @parthchandra |
… to `native_comet` (apache#2176)
4 participants
Which issue does this PR close?
Closes #2172
Rationale for this change
Comet's default scan implementation is
auto, so we should use that same default in the test suite.What changes are included in this PR?
How are these changes tested?