GH-44183: [C++] Support run-end encoded struct, list (view), large list (view) and map values - #50534
Conversation
Adding the encode path for nested-value encoding for List, FixedSizeList and Struct arrays
Adding the decode path for nested-value encoding for List, FixedSizeList and Struct arrays
|
|
BenMagyar
changed the title
|
|
BenMagyar
changed the title
|
|
1 similar comment
|
|
BenMagyar
changed the title
|
|
BenMagyar
changed the title
|
|
1 similar comment
|
|
github-actions
Bot
added
awaiting committer review
| for (int64_t i = 0; i < run_length; ++i) { | ||
| RETURN_NOT_OK( | ||
| output_builder->AppendArraySlice(input_values, physical_index, 1)); | ||
| } |
There was a problem hiding this comment.
I wonder if it would be more performant to implement this as:
| for (int64_t i = 0; i < run_length; ++i) { | |
| RETURN_NOT_OK( | |
| output_builder->AppendArraySlice(input_values, physical_index, 1)); | |
| } | |
| ARROW_ASSIGN_OR_RAISE( | |
| auto scalar, input_values.GetScalar(physical_index)); | |
| RETURN_NOT_OK(output_builder->AppendScalar(scalar, run_length)); |
GetScalar adds its own cost, but it's amortized over the entire run.
@felipecrv What do you think?
There was a problem hiding this comment.
I ran some some mini benchmarks to test this:
| Value type | Run length 1 | Run length 64 | Run length 4096 | Run length 16384 |
|---|---|---|---|---|
List<Int32>[4] |
32.5x slower | 12.1x slower | 11.5x slower | 11.6x slower |
List<Int32>[64] |
126.3x slower | 116.3x slower | 115.9x slower | 115.2x slower |
LargeList<Int32>[8] |
41.9x slower | 22.3x slower | 21.8x slower | 22.1x slower |
ListView<Int32>[8] |
41.7x slower | 22.1x slower | 21.7x slower | 21.7x slower |
LargeListView<Int32>[8] |
42.3x slower | 22.2x slower | 21.9x slower | 21.6x slower |
FixedSizeList<Int32>[8] |
53.4x slower | 26.2x slower | 25.7x slower | 25.6x slower |
Map<Utf8, Int32>[4] |
130.5x slower | 107.9x slower | 109.6x slower | 107.0x slower |
Struct mixed |
58.9x slower | 16.5x slower | 15.6x slower | 15.7x slower |
Empty List |
24.2x slower | 0.75x | 0.35x | 0.35x |
List<Int32>[1] |
23.7x slower | 3.5x slower | 3.1x slower | 3.1x slower |
Empty Map |
28.0x slower | 1.0x | 0.52x | 0.51x |
Struct<Int32> |
34.9x slower | 1.4x slower | 0.84x | 0.77x |
Empty Struct |
30.6x slower | 1.1x slower | 0.56x | 0.56x |
There was a problem hiding this comment.
I see, sorry for the bad idea then :)
BenMagyar
changed the title
|
|
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thank you @BenMagyar ! This looks good to me now.
|
CI failures are unrelated, I'll merge. |
2 participants
Rationale for this change
From #44183 - creating run-end encoded cols that contain list or struct values is unsupported. Adding support for both the encode and decode paths.
What changes are included in this PR?
List,ListView,LargeList,LargeListView,Map,FixedSizeListandStructvalues in therun_end_encodeand therun_end_decodepaths.Are these changes tested?
Are there any user-facing changes?
There are but they are not breaking. Currently the encode path throws that it is unsupported on one of these types. We are adding support.