feat: Add experimental auto mode for COMET_PARQUET_SCAN_IMPL
#1747
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1747 +/- ##
============================================
+ Coverage 56.12% 59.33% +3.21%
- Complexity 976 1151 +175
============================================
Files 119 130 +11
Lines 11743 12680 +937
Branches 2251 2380 +129
============================================
+ Hits 6591 7524 +933
+ Misses 4012 3945 -67
- Partials 1140 1211 +71 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Looking good so far. Will do a final review once it is ready. |
| # Compatibility Guide | ||
|
|
||
| Comet aims to provide consistent results with the version of Apache Spark that is being used. | ||
|
|
||
| This guide offers information about areas of functionality where there are known differences. | ||
|
|
There was a problem hiding this comment.
This section appeared twice
|
@parthchandra @mbutrovich This is ready for review now. I don't know if we want to keep in draft until more complete or merge and iterate. I also did not make auto the default yet. |
| if (COMET_EXEC_ENABLED | ||
| .get() && schemaSupported && partitionSchemaSupported && | ||
| !scanExec.bucketedScan && !knownIssues) { | ||
| scanImpl = SCAN_NATIVE_ICEBERG_COMPAT |
There was a problem hiding this comment.
native_iceberg_compat should be able to handle bucketed scans
There was a problem hiding this comment.
thanks, I will update this
| } | ||
|
|
||
| if (scanImpl == SCAN_AUTO) { | ||
| scanImpl = SCAN_NATIVE_COMET |
There was a problem hiding this comment.
We would never choose native_datafusion?
There was a problem hiding this comment.
I figured that we have more Spark SQL tests passing with native_iceberg_compat so we should start with that.
|
Not sure why this would cause the ci failures that we see here. Maybe defer this until some more of the known issues are fixed? |
Some of the tests need updating because they make assumptions based on the configured default scan and have not been updated to handle the new "auto" option. |
andygrove
changed the title
COMET_PARQUET_SCAN_IMPLCOMET_PARQUET_SCAN_IMPL
|
@parthchandra @mbutrovich Could I get a review? I changed the scope to adding the "auto" option without changing the default. There is a manual workflow where we can run the Spark SQL tests using the new auto mode to see which tests fail (if any). |
Which issue does this PR close?
N/A
Follows on from #1746
Rationale for this change
Rather than require the user to manually configure the scan implementation, let's add an "auto" option so that we can just pick the best one based on whether the schema is supported.
What changes are included in this PR?
Add the new option "auto". The default value is not changed yet because that would involve rewriting many tests.
How are these changes tested?
For now, there is a manual workflow where we can run the Spark SQL tests with auto enabled.