Change `ArrayDataBuilder::null_bit_buffer` to accept `Option<Buffer>` rather than `Buffer` #1739

HaoYang670 · 2022-05-24T14:34:27Z

Signed-off-by: remzi 13716567376yh@gmail.com

Which issue does this PR close?

Closes #1737.

Rationale for this change

More user-friendly: expose the type of null_bit_buffer, so that users know null_bit_buffer is an optional field.
Avoid writing code like this:

let mut builder = ArrayDataBuilder::new(...).add_buffer(...);
if let Some(null_bit_buffer) = null_bit_buffer {
   builder.null_bit_buffer(null_bit_buffer.clone())
}
...

Instead, we can directly write like this now:

let builder = ArrayDataBuilder::new(...).add_buffer(...).null_bit_buffer(null_bit_buffer.cloned());

Maybe faster: avoid some useless pattern matching.
Fewer muts are safer. Less code is better.

What changes are included in this PR?

Change the type of buf from Buffer to Option<Buffer>

Are there any user-facing changes?

Yes!

Signed-off-by: remzi <13716567376yh@gmail.com>

codecov-commenter · 2022-05-24T15:16:35Z

Codecov Report

Merging #1739 (1020fce) into master (b265780) will decrease coverage by 0.00%.
The diff coverage is 96.04%.

@@            Coverage Diff             @@
##           master    #1739      +/-   ##
==========================================
- Coverage   83.36%   83.35%   -0.01%     
==========================================
  Files         196      196              
  Lines       56147    56122      -25     
==========================================
- Hits        46805    46782      -23     
+ Misses       9342     9340       -2

Impacted Files	Coverage Δ
integration-testing/src/lib.rs	`0.00% <0.00%> (ø)`
arrow/src/compute/kernels/filter.rs	`88.33% <83.33%> (ø)`
arrow/src/array/array.rs	`89.67% <100.00%> (ø)`
arrow/src/array/array_binary.rs	`93.27% <100.00%> (-0.03%)`	⬇️
arrow/src/array/array_dictionary.rs	`91.91% <100.00%> (ø)`
arrow/src/array/array_list.rs	`96.16% <100.00%> (ø)`
arrow/src/array/array_map.rs	`84.81% <100.00%> (ø)`
arrow/src/array/array_primitive.rs	`95.06% <100.00%> (ø)`
arrow/src/array/array_string.rs	`97.72% <100.00%> (-0.01%)`	⬇️
arrow/src/array/array_struct.rs	`88.40% <100.00%> (-0.05%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b265780...1020fce. Read the comment docs.

Signed-off-by: remzi <13716567376yh@gmail.com>

HaoYang670 · 2022-05-25T01:35:56Z

cc @alamb @tustvold @viirya
Please help to review. Thank you!

tustvold

I'm fine with this, but as this is a breaking change should probably get feedback from others.

Left a suggestion on how to make the null_count > 0 construction that appears in various places less verbose

tustvold · 2022-05-25T07:43:57Z

arrow/src/array/builder.rs

-            builder = builder.null_bit_buffer(null_bit_buffer);
-        }
+            .add_buffer(self.values_builder.finish())
+            .null_bit_buffer(if null_count > 0 {


This could be written as (null_count > 0).then(|| null_bit_buffer)

Signed-off-by: remzi <13716567376yh@gmail.com>

alamb

I think this looks like a very nice change to me. While it will result in some code churn I think the API is low enough level it should not be super widely used and the churn will result in cleaner code downstream 👍

Thank you @HaoYang670

cc @jhorstmann (whose project I think uses these lower level APIs)

alamb · 2022-05-26T19:45:18Z

arrow/src/compute/kernels/string.rs

@@ -74,15 +74,11 @@ pub fn string_concat<Offset: OffsetSizeTrait>(
        output_offsets.append(Offset::from_usize(output_values.len()).unwrap());
    }

-    let mut builder =


Since this code is moved, this PR now has a conflict sadly

alamb · 2022-05-26T19:45:47Z

arrow/src/ipc/reader.rs

@@ -287,24 +287,22 @@ fn create_primitive_array(
    let array_data = match data_type {
        Utf8 | Binary | LargeBinary | LargeUtf8 => {
            // read 3 buffers
-            let mut builder = ArrayData::builder(data_type.clone())
+            ArrayData::builder(data_type.clone())


👍 the new pattern certainly look nicer in my opinion

jhorstmann · 2022-05-26T22:26:26Z

Fully agree, this looks much more fluent than before.

tustvold · 2022-05-27T08:11:08Z

arrow/src/array/builder.rs

-            builder = builder.null_bit_buffer(null_bit_buffer.unwrap());
-        }
+            .add_buffer(self.values_builder.finish())
+            .null_bit_buffer(if null_count > 0 {


Missed a .then opportunity here FWIW. Same for line below

.then is not suitable here because the type of null_bit_buffer is Option<Buffer>. Using .then will get a result of Option<Option<Buffer>>

Previously this called .unwrap() which is probably safe given the null count, although I can't remember what NullArrays do. Either way you could then just call .flatten(). Not a big deal 😁

clean up

d07f2c7

Signed-off-by: remzi <13716567376yh@gmail.com>

HaoYang670 marked this pull request as draft May 24, 2022 14:34

github-actions bot added arrow Changes to the arrow crate parquet Changes to the parquet crate labels May 24, 2022

fix a nit

a5a15d0

Signed-off-by: remzi <13716567376yh@gmail.com>

HaoYang670 marked this pull request as ready for review May 25, 2022 01:35

tustvold approved these changes May 25, 2022

View reviewed changes

HaoYang670 added 3 commits May 25, 2022 17:23

use boolean::then

3d44a8a

Signed-off-by: remzi <13716567376yh@gmail.com>

Merge branch 'master' into refactor_null_bit_buffer

69c7f33

fix merge conflict

fb3b0e6

Signed-off-by: remzi <13716567376yh@gmail.com>

alamb added the api-change Changes to the arrow API label May 26, 2022

alamb approved these changes May 26, 2022

View reviewed changes

Merge branch 'master' into refactor_null_bit_buffer

1020fce

HaoYang670 requested review from tustvold and alamb May 27, 2022 01:36

tustvold reviewed May 27, 2022

View reviewed changes

tustvold merged commit 9722f06 into apache:master May 27, 2022

HaoYang670 deleted the refactor_null_bit_buffer branch May 27, 2022 10:16

alamb changed the title ~~Rewrite ArrayDataBuilder::null_bit_buffer~~ Change ArrayDataBuilder::null_bit_buffer to accept Option<Buffer> rather than Buffer May 27, 2022

alamb removed the parquet Changes to the parquet crate label May 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `ArrayDataBuilder::null_bit_buffer` to accept `Option<Buffer>` rather than `Buffer` #1739

Change `ArrayDataBuilder::null_bit_buffer` to accept `Option<Buffer>` rather than `Buffer` #1739

HaoYang670 commented May 24, 2022 •

edited

Loading

codecov-commenter commented May 24, 2022 •

edited

Loading

HaoYang670 commented May 25, 2022

tustvold left a comment

tustvold May 25, 2022

HaoYang670 May 25, 2022

alamb left a comment

alamb May 26, 2022

HaoYang670 May 27, 2022

alamb May 26, 2022

jhorstmann commented May 26, 2022

tustvold May 27, 2022 •

edited

Loading

HaoYang670 May 27, 2022

tustvold May 27, 2022

Change ArrayDataBuilder::null_bit_buffer to accept Option<Buffer> rather than Buffer #1739

Change ArrayDataBuilder::null_bit_buffer to accept Option<Buffer> rather than Buffer #1739

Conversation

HaoYang670 commented May 24, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented May 24, 2022 • edited Loading

Codecov Report

HaoYang670 commented May 25, 2022

tustvold left a comment

Choose a reason for hiding this comment

tustvold May 25, 2022

Choose a reason for hiding this comment

HaoYang670 May 25, 2022

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb May 26, 2022

Choose a reason for hiding this comment

HaoYang670 May 27, 2022

Choose a reason for hiding this comment

alamb May 26, 2022

Choose a reason for hiding this comment

jhorstmann commented May 26, 2022

tustvold May 27, 2022 • edited Loading

Choose a reason for hiding this comment

HaoYang670 May 27, 2022

Choose a reason for hiding this comment

tustvold May 27, 2022

Choose a reason for hiding this comment

Change `ArrayDataBuilder::null_bit_buffer` to accept `Option<Buffer>` rather than `Buffer` #1739

Change `ArrayDataBuilder::null_bit_buffer` to accept `Option<Buffer>` rather than `Buffer` #1739

HaoYang670 commented May 24, 2022 •

edited

Loading

codecov-commenter commented May 24, 2022 •

edited

Loading

tustvold May 27, 2022 •

edited

Loading