chore: Enable CI checks for native_datafusion scan#1479
chore: Enable CI checks for native_datafusion scan#1479andygrove merged 6 commits intoapache:mainfrom
native_datafusion scan#1479Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1479 +/- ##
============================================
+ Coverage 56.12% 58.77% +2.64%
- Complexity 976 1025 +49
============================================
Files 119 122 +3
Lines 11743 12279 +536
Branches 2251 2311 +60
============================================
+ Hits 6591 7217 +626
+ Misses 4012 3904 -108
- Partials 1140 1158 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mbutrovich could you review when you have time |
|
|
||
| linux-test-native-datafusion-scan: | ||
| env: | ||
| COMET_PARQUET_SCAN_IMPL: "native_datafusion" |
There was a problem hiding this comment.
This job is a copy of the regular job but enabling this env var
kazuyukitanimura
left a comment
kazuyukitanimura
left a comment
There was a problem hiding this comment.
I think we will need spark sql tests as well
| spark-version: ['3.5'] | ||
| scala-version: ['2.12'] |
There was a problem hiding this comment.
What about other versions? Spark 3.4, 4.0, Scala 2.13?
There was a problem hiding this comment.
I was thinking that this would be enough for now to prevent regressions while this is still an experimental feature.
We could increase the matrix here but it will add overhead to all PRs. What do you think @mbutrovich?
There was a problem hiding this comment.
I have not tested Spark 3.5, Spark 4.0, Scala 2.13 with native_datafusion and would prefer not to get spread too thin if we get a deluge of issues. I propose walking before we run and get this pipeline in great shape and then add more.
There was a problem hiding this comment.
@kazuyukitanimura Could we merge this PR to add these initial checks. We currently have almost zero tests running in CI for native_datafusion, so this is a big step forward. It is just the first step though.
There was a problem hiding this comment.
Sure, would you mind filing an issue to track this? I will go ahead and approve
Eventually, yes. I don't think we can support that yet, though. |
4 participants
Which issue does this PR close?
Part of #1486
Rationale for this change
To prevent regressions and track progress with supporting
native_datafusion, I would like to start enabling CI runs with this scan as the default.What changes are included in this PR?
native_datafusionenabledassume(!CometConf.isExperimentalNativeScan)and link to Experimental native scan test failures #1441How are these changes tested?