ARROW-13877: [C++] Support FixedSizeList in generic list kernels #11127
Conversation
| const uint8_t* bitmap = input->GetValues<uint8_t>(0, 0); | ||
| for (int32_t i = 0; i < input->length; i++) { | ||
| if (!bitmap || BitUtil::GetBit(bitmap, input->offset + i)) { | ||
| std::fill(out_indices, out_indices + slot_length, |
There was a problem hiding this comment.
Move the if (!bitmap) check outside of the loop.
There was a problem hiding this comment.
Also, you can batch copies by tracking the number of consecutive valid bits, and then perform a std::fill when an invalid bit is encountered.
There was a problem hiding this comment.
You can't batch copies because different slots have different values.
There was a problem hiding this comment.
Also I would expect the branch predictor to make the difference moot here anyways; if we move !bitmap outside the loop, we would have to add a separate loop to fill the output array.
There was a problem hiding this comment.
Also, if you wanted to do that, it would be much better to call VisitSetBitRuns...
| /// Note that it's different from `values()` in that it takes into | ||
| /// consideration null elements (they are skipped, thus copying may be needed). | ||
| Result<std::shared_ptr<Array>> Flatten( | ||
| MemoryPool* memory_pool = default_memory_pool()) const; |
There was a problem hiding this comment.
Can you add a test for this method?
| const uint8_t* bitmap = input->GetValues<uint8_t>(0, 0); | ||
| for (int32_t i = 0; i < input->length; i++) { | ||
| if (!bitmap || BitUtil::GetBit(bitmap, input->offset + i)) { | ||
| std::fill(out_indices, out_indices + slot_length, |
There was a problem hiding this comment.
Also, if you wanted to do that, it would be much better to call VisitSetBitRuns...
| auto expected = ArrayFromJSON(out_ty, "[0, null, 2, 3, 0, 42]"); | ||
| CheckVectorUnary("list_flatten", input, expected); | ||
|
|
||
| // Construct a list with a non-empty null slot |
There was a problem hiding this comment.
For a fixed size list, this doesn't test anything special ;-)
There was a problem hiding this comment.
D'oh, thanks for catching this!
| auto expected = ArrayFromJSON(int32(), "[0, 0, 2, 2, 3, 3, 4, 4]"); | ||
| CheckVectorUnary("list_parent_indices", input, expected); | ||
|
|
||
| // Construct a list with a non-empty null slot |
lidavidm
force-pushed
the
arrow-13877
branch
from
4ec0df7
to
acd8859
Compare
Closes apache#11127 from lidavidm/arrow-13877 Authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
No description provided.