feat: Improve fallback reporting for native_datafusion scan#2879
Conversation
|
|
||
| if (encryptionEnabled(hadoopConf) && scanImpl != CometConf.SCAN_NATIVE_COMET) { | ||
| if (!isEncryptionConfigSupported(hadoopConf)) { | ||
| return withInfos(scanExec, fallbackReasons.toSet) |
There was a problem hiding this comment.
It looks like fallbackReasons was always an empty set here?
andygrove
changed the title
native_datafusion scannative_datafusion scan
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2879 +/- ##
============================================
+ Coverage 56.12% 59.40% +3.27%
- Complexity 976 1374 +398
============================================
Files 119 167 +48
Lines 11743 15337 +3594
Branches 2251 2549 +298
============================================
+ Hits 6591 9111 +2520
- Misses 4012 4939 +927
- Partials 1140 1287 +147 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| scanExec.relation match { | ||
| case r: HadoopFsRelation => | ||
| val fallbackReasons = new ListBuffer[String]() |
There was a problem hiding this comment.
The code no longer accumulates fallback reasons in a list, but just calls withInfo for each fallback so that we accumulate them directly on the operator.
| } | ||
|
|
||
| // Native DataFusion doesn't support subqueries/dynamic pruning | ||
| if (scanImpl == SCAN_NATIVE_DATAFUSION && |
There was a problem hiding this comment.
All of the checks for native_datafusion have moved into CometNativeScan.isSupported(scanExec).
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
We looked at this together earlier, LGTM! Thanks @andygrove!
3 participants
Which issue does this PR close?
N/A
We should merge #2877 before this one
Rationale for this change
Move
native_datafusionchecks intoCometNativeScanand improve fallback reporting. The current implementation does not report all fallback reasons because the code returns on hitting the first fallback reason.What changes are included in this PR?
CometNativeScanHow are these changes tested?
Existing tests