Convert batch_size to config option #2771
Conversation
| ), | ||
| ConfigDefinition::new_u32( | ||
| OPT_BATCH_SIZE, | ||
| "Default batch size while creating new batches, it's especially useful for \ |
There was a problem hiding this comment.
I think this description can be improved but that seemed outside the scope of this PR
andygrove
added
api change
There was a problem hiding this comment.
I think the basic pattern looks good to me 👍
I had some questions about type (u32 vs u64) and seeing if we an avoid having to repeat option names so many times but otherwise 👍
Thank you @andygrove
| /// Customize batch size | ||
| pub fn with_batch_size(mut self, n: usize) -> Self { | ||
| pub fn with_batch_size(self, n: usize) -> Self { |
There was a problem hiding this comment.
I like this approach (with a named function with_batch_size that calls into the generic implementation below). Seems like a good idea to me
👍
| // batch size must be greater than zero | ||
| assert!(n > 0); | ||
| self.batch_size = n; | ||
| self | ||
| self.set_u32(OPT_BATCH_SIZE, n as u32) |
There was a problem hiding this comment.
Why not use u64 as batch size (why are we storing as a u32 and then casting back and forth to usize)?
There was a problem hiding this comment.
I changed to u64 but it still requires casting back and forth to usize.
| map.insert( | ||
| BATCH_SIZE.to_owned(), | ||
| format!("{}", self.config_options.get_u32(OPT_BATCH_SIZE)), | ||
| ); |
There was a problem hiding this comment.
I wonder if it is possible to avoid having to enumerate all options again in this function -- what about maybe converting all entries in self.config_options?
There was a problem hiding this comment.
Should we consolidate BATCH_SIZE with OPT_BATCH_SIZE?
There was a problem hiding this comment.
We should. Ballista currently relies on this method and the keys used here. Once all the properties are converted I will update Ballista to use the new config_option method and delete this method,
There was a problem hiding this comment.
So we use these names internally as well so I ended up removing the duplicate option. This will require Ballista to look for the new name but that is a small change, I filed apache/datafusion-ballista#73 to track this.
| map.insert( | ||
| BATCH_SIZE.to_owned(), | ||
| format!("{}", self.config_options.get_u32(OPT_BATCH_SIZE)), | ||
| ); |
There was a problem hiding this comment.
Should we consolidate BATCH_SIZE with OPT_BATCH_SIZE?
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Codecov Report
@@ Coverage Diff @@
## master #2771 +/- ##
==========================================
+ Coverage 84.95% 85.15% +0.20%
==========================================
Files 272 272
Lines 48221 48096 -125
==========================================
- Hits 40964 40958 -6
+ Misses 7257 7138 -119
Continue to review full report at Codecov.
|
| pub fn to_props(&self) -> HashMap<String, String> { | ||
| let mut map = HashMap::new(); | ||
| map.insert(BATCH_SIZE.to_owned(), format!("{}", self.batch_size)); | ||
| // copy configs from config_options |
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
Part of #2756
This just moves a single config but I wanted to get reviews on this approach before moving all of them.
Rationale for this change
Start moving existing configs to the new config mechanism
What changes are included in this PR?
batch_sizeto new config mechanismAre there any user-facing changes?
SessionConfig::batch_sizeis now a method rather than an attribute.