Speed up the offsets checking #1684

HaoYang670 · 2022-05-10T08:13:46Z

Which issue does this PR close?

Rationale for this change

Originally, in each iteration, we fully check start_offset and end_offset, which means that all elements (except the first and the last) in the offsets_buffer are checked twice. This is somewhat wasteful.

What changes are included in this PR?

Hoist some checking outside the window iteration.
Update some error messages.
Add docs and tests for empty binary-like array. (As Arrow tends to be ambiguous in this case. I keep the current implementation. We can change it in the future, or in Ensure there is a single zero in the offsets buffer for an empty ListArray. #1620).
Add a benchmark for offsets checking.

Are there any user-facing changes?

No.

Benchmark result

Gnuplot not found, using plotters backend
validate_binary_array_data 20000                                                                             
                        time:   [35.595 us 35.650 us 35.712 us]
                        change: [-17.373% -17.034% -16.724%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

Signed-off-by: remzi <[email protected]>

codecov-commenter · 2022-05-10T08:34:29Z

Codecov Report

Merging #1684 (8f07d64) into master (e0a527b) will increase coverage by 0.01%.
The diff coverage is 93.44%.

@@            Coverage Diff             @@
##           master    #1684      +/-   ##
==========================================
+ Coverage   83.12%   83.14%   +0.01%     
==========================================
  Files         193      193              
  Lines       55850    56206     +356     
==========================================
+ Hits        46426    46733     +307     
- Misses       9424     9473      +49

Impacted Files	Coverage Δ
arrow/src/array/data.rs	`84.35% <93.44%> (+1.29%)`	⬆️
parquet/src/arrow/array_reader/test_util.rs	`81.08% <0.00%> (-2.26%)`	⬇️
arrow/src/datatypes/datatype.rs	`65.45% <0.00%> (-1.35%)`	⬇️
arrow/src/compute/kernels/length.rs	`99.04% <0.00%> (-0.96%)`	⬇️
arrow/src/compute/kernels/substring.rs	`99.36% <0.00%> (-0.64%)`	⬇️
parquet_derive/src/parquet_field.rs	`65.98% <0.00%> (-0.23%)`	⬇️
arrow/src/ipc/reader.rs	`88.80% <0.00%> (-0.23%)`	⬇️
arrow/src/compute/kernels/take.rs	`95.27% <0.00%> (-0.14%)`	⬇️
parquet/src/arrow/array_reader.rs	`89.76% <0.00%> (-0.07%)`	⬇️
arrow-flight/src/utils.rs	`0.00% <0.00%> (ø)`
... and 13 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 e0a527b...8f07d64. Read the comment docs.

rename to offsets buffer Signed-off-by: remzi <[email protected]>

Signed-off-by: remzi <[email protected]>

HaoYang670 · 2022-05-10T09:20:19Z

arrow/src/array/data.rs

                }
+            })
+            .collect::<Result<Vec<usize>>>()?;


collect the result here, because I want to use the windows method

The downside of calling collect() is that it will buffer the intermediate result (aka allocate memory and save the offsets in another buffer)

Has replaced with iter::scan, no collect now!

HaoYang670 · 2022-05-10T09:21:44Z

arrow/src/array/data.rs

+    #[test]
+    fn test_empty_utf8_array_with_non_zero_offset() {
+        let data_buffer = Buffer::from_slice_ref(&"abcdef".as_bytes());
+        let offsets_buffer = Buffer::from_slice_ref(&[0i32, 2, 6, 0]);


I think the offsets are weird but reasonable.

yeah, I agree this is strange but ok

alamb

Thanks @HaoYang670

This code has lots of test coverage so I feel good about the change functionally

I know you mentioned that one of the rationales for the change was speeding things up, but given the new collect() and allocation I wonder if this is actually faster or if it could actually be slower. It might be worth some measurements

alamb · 2022-05-10T20:14:57Z

arrow/src/array/data.rs

+    #[test]
+    fn test_empty_utf8_array_with_non_zero_offset() {
+        let data_buffer = Buffer::from_slice_ref(&"abcdef".as_bytes());
+        let offsets_buffer = Buffer::from_slice_ref(&[0i32, 2, 6, 0]);


yeah, I agree this is strange but ok

alamb · 2022-05-10T20:16:37Z

arrow/src/array/data.rs

                }
+            })
+            .collect::<Result<Vec<usize>>>()?;


The downside of calling collect() is that it will buffer the intermediate result (aka allocate memory and save the offsets in another buffer)

viirya

The change looks reasonable. Agree with @alamb that a performance measurement may be good as performance is one motivation.

arrow/src/array/data.rs

HaoYang670 · 2022-05-11T00:45:58Z

Thank you for your review @alamb @viirya. I will add a benchmark for offsets checking. Also, I find that using iter::scan is a better way than using slice::windows, which can avoid collecting.

fix a nit in docs. Co-authored-by: Liang-Chi Hsieh <[email protected]>

Signed-off-by: remzi <[email protected]>

HaoYang670 · 2022-05-11T06:03:22Z

Benchmark result has been added!

arrow/src/array/data.rs

Co-authored-by: Liang-Chi Hsieh <[email protected]>

arrow/src/array/data.rs

Co-authored-by: Liang-Chi Hsieh <[email protected]>

HaoYang670 · 2022-05-11T07:22:00Z

arrow/src/array/data.rs

                        "Offset invariant failure: non-monotonic offset at slot {}: {} > {}",
-                        i, start_offset, end_offset))
-                    );
+                        i - 1, start, end))


The i - 1 here is a little ugly. I try to find a more elegant way

viirya · 2022-05-11T07:22:32Z

Thanks @HaoYang670. The performance numbers look good!

HaoYang670 · 2022-05-11T07:26:30Z

Actually, I think we could do more. Because there is some redundant checking (such as comparing each offset with offset_limit). However, as a number of tests rely on that checking, I do not remove it.

HaoYang670 · 2022-05-15T00:47:46Z

Could we merge this PR?

viirya · 2022-05-15T02:36:17Z

Merged, thanks @HaoYang670 @alamb

simplify offsets fully checking

880d683

Signed-off-by: remzi <[email protected]>

github-actions bot added the arrow Changes to the arrow crate label May 10, 2022

HaoYang670 added 3 commits May 10, 2022 16:38

update doc

cc8d82b

rename to offsets buffer Signed-off-by: remzi <[email protected]>

add more tests for empty binary-like array

f502da3

Signed-off-by: remzi <[email protected]>

add docs for empty binary-like array

ab203ac

Signed-off-by: remzi <[email protected]>

HaoYang670 commented May 10, 2022

View reviewed changes

alamb approved these changes May 10, 2022

View reviewed changes

viirya reviewed May 10, 2022

View reviewed changes

arrow/src/array/data.rs Outdated Show resolved Hide resolved

HaoYang670 and others added 3 commits May 11, 2022 10:52

Update arrow/src/array/data.rs

8f07d64

fix a nit in docs. Co-authored-by: Liang-Chi Hsieh <[email protected]>

add benchmark

25d3935

Signed-off-by: remzi <[email protected]>

replace windows with scan

0e61afa

Signed-off-by: remzi <[email protected]>

viirya reviewed May 11, 2022

View reviewed changes

arrow/src/array/data.rs Outdated Show resolved Hide resolved

Update arrow/src/array/data.rs

9e1602f

Co-authored-by: Liang-Chi Hsieh <[email protected]>

viirya reviewed May 11, 2022

View reviewed changes

arrow/src/array/data.rs Outdated Show resolved Hide resolved

Update arrow/src/array/data.rs

d86902b

Co-authored-by: Liang-Chi Hsieh <[email protected]>

viirya approved these changes May 11, 2022

View reviewed changes

HaoYang670 commented May 11, 2022

View reviewed changes

HaoYang670 changed the title ~~simplify offsets checking~~ Speed up the offsets checking May 15, 2022

viirya merged commit ff182f1 into apache:master May 15, 2022

HaoYang670 deleted the simplity_offsets_checking branch June 1, 2022 01:05

HaoYang670 mentioned this pull request Jun 1, 2022

Further simplify the offset validation #1770

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up the offsets checking #1684

Speed up the offsets checking #1684

HaoYang670 commented May 10, 2022 •

edited

Loading

codecov-commenter commented May 10, 2022 •

edited

Loading

HaoYang670 May 10, 2022 •

edited

Loading

alamb May 10, 2022

HaoYang670 May 11, 2022

HaoYang670 May 10, 2022

alamb May 10, 2022

alamb left a comment

alamb May 10, 2022

alamb May 10, 2022

viirya left a comment

HaoYang670 commented May 11, 2022

HaoYang670 commented May 11, 2022

HaoYang670 May 11, 2022

viirya commented May 11, 2022

HaoYang670 commented May 11, 2022

HaoYang670 commented May 15, 2022

viirya commented May 15, 2022

Speed up the offsets checking #1684

Speed up the offsets checking #1684

Conversation

HaoYang670 commented May 10, 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?

Benchmark result

codecov-commenter commented May 10, 2022 • edited Loading

Codecov Report

HaoYang670 May 10, 2022 • edited Loading

Choose a reason for hiding this comment

alamb May 10, 2022

Choose a reason for hiding this comment

HaoYang670 May 11, 2022

Choose a reason for hiding this comment

HaoYang670 May 10, 2022

Choose a reason for hiding this comment

alamb May 10, 2022

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb May 10, 2022

Choose a reason for hiding this comment

alamb May 10, 2022

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

HaoYang670 commented May 11, 2022

HaoYang670 commented May 11, 2022

HaoYang670 May 11, 2022

Choose a reason for hiding this comment

viirya commented May 11, 2022

HaoYang670 commented May 11, 2022

HaoYang670 commented May 15, 2022

viirya commented May 15, 2022

HaoYang670 commented May 10, 2022 •

edited

Loading

codecov-commenter commented May 10, 2022 •

edited

Loading

HaoYang670 May 10, 2022 •

edited

Loading