Refine FixedSizeListBuilder
#1988
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: remzi <[email protected]>
HaoYang670
commented
Jul 2, 2022
| @@ -230,22 +219,20 @@ mod tests { | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_fixed_size_binary_builder() { | |||
There was a problem hiding this comment.
Move this test to fixed_size_binary_builder.rs
HaoYang670
commented
Jul 2, 2022
| pub fn new(values_builder: T, length: i32) -> Self { | ||
| /// Creates a new [`FixedSizeListBuilder`] from a given values array builder | ||
| /// `value_length` is the number of values within each array | ||
| pub fn new(values_builder: T, value_length: i32) -> Self { |
There was a problem hiding this comment.
value_length is less confusing
HaoYang670
commented
Jul 2, 2022
| #[derive(Debug)] | ||
| pub struct FixedSizeListBuilder<T: ArrayBuilder> { | ||
| bitmap_builder: BooleanBufferBuilder, | ||
| values_builder: T, | ||
| len: usize, |
There was a problem hiding this comment.
Remove this field because self.len is always equal to self.bitmap_builder.len
HaoYang670
commented
Jul 2, 2022
| // check that values_data length is multiple of len if we have data | ||
| if len != 0 { | ||
| assert!( | ||
| values_data.len() / len == self.list_len as usize, |
There was a problem hiding this comment.
There was a bug when values_data.len() / len == self.list_len but values_data.len() % len != 0.
Checking values_data.len() == len * self.list_len is more straightforward.
HaoYang670
commented
Jul 2, 2022
| #[should_panic( | ||
| expected = "Length of the child array (10) must be the multiple of the value length (3) and the array length (3)." | ||
| )] | ||
| fn test_fixed_size_list_array_builder_fail() { |
There was a problem hiding this comment.
Test the checking in finish
HaoYang670
commented
Jul 2, 2022
| /// `capacity` is the number of items to pre-allocate space for in this builder | ||
| pub fn with_capacity(values_builder: T, length: i32, capacity: usize) -> Self { | ||
| let mut offsets_builder = Int32BufferBuilder::new(capacity + 1); | ||
| offsets_builder.append(0); |
Signed-off-by: remzi <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1988 +/- ##
=======================================
Coverage 83.58% 83.58%
=======================================
Files 222 222
Lines 57495 57557 +62
=======================================
+ Hits 48056 48109 +53
- Misses 9439 9448 +9
Continue to review full report at Codecov.
|
Signed-off-by: remzi <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: remzi [email protected]
Which issue does this PR close?
None.
Fixed some minor things when I read the code of
FixedSizeListBuilder.What changes are included in this PR?
Commented in the PR.
Are there any user-facing changes?
Yes. Renaming
lengthtovalue_lengthwhich I think is minor.