-
Notifications
You must be signed in to change notification settings - Fork 246
chore: Pass Comet configs to native createPlan
#2543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
createPlan
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2543 +/- ##
============================================
+ Coverage 56.12% 58.91% +2.79%
- Complexity 976 1457 +481
============================================
Files 119 147 +28
Lines 11743 13644 +1901
Branches 2251 2369 +118
============================================
+ Hits 6591 8039 +1448
- Misses 4012 4382 +370
- Partials 1140 1223 +83 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @andygrove
memoryLimit, | ||
memoryLimitPerTask, | ||
taskAttemptId, | ||
debug = COMET_DEBUG_ENABLED.get(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} | ||
|
||
impl SparkConfig for HashMap<String, String> { | ||
fn get_bool(&self, name: &str) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps in future work we can generialize this methods using macros
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I was also wondering if we can generate Rust code from CometConf.scala somehow, to make all the configs and defaults available
Which issue does this PR close?
N/A
Rationale for this change
These changes are in #2521, but I would like them reviewed and merged with higher priority than #2521.
We were already passing Spark configs to native code, but not actually using them. This PR completes this work and simplifies the
createPlan
call because we no longer have to keep adding more arguments as we add more configs.What changes are included in this PR?
Refactoring. No functional changes.
How are these changes tested?
These changes have been manually tested as part of #2521