-
Notifications
You must be signed in to change notification settings - Fork 853
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
Better construction of RecordBatchOptions #2729
Conversation
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 @askoa -- this is looking good. I had a few comments
arrow/src/record_batch.rs
Outdated
self.match_field_names = match_field_names; | ||
self | ||
} | ||
pub fn row_count(mut self, row_count: usize) -> Self { |
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.
pub fn row_count(mut self, row_count: usize) -> Self { | |
pub fn row_count(mut self, row_count: Option<usize>) -> Self { |
I recommend having this parameter take an Option, otherwise there is no way to set it to None
.
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.
The parameter is changed to Option
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.
I think it looks great. Thank you @askoa
https://github.com/apache/arrow-rs/actions/runs/3060939456/jobs/4940078478 was caused by
So I have restarted the check and hopefully it will pass on a subsequent run |
Co-authored-by: Andrew Lamb <[email protected]>
Co-authored-by: Andrew Lamb <[email protected]>
Docs failure is unrelated #2733 |
Benchmark runs are scheduled for baseline = 0ebd71e and contender = 43d912c. 43d912c is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2728
What changes are included in this PR?
Removed the current option of initializing using
Default
as the approach will work only within the crate.Default
does not work outside the crate due to non-exhaustive option.Are there any user-facing changes?
Nope.