GH-50994: [C++][Compute] Implement casting from ListView to List with zero-copy fast-path - #50976
GH-50994: [C++][Compute] Implement casting from ListView to List with zero-copy fast-path#50976Jay846 wants to merge 11 commits into
Conversation
|
Thanks for opening a pull request! This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format. If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or After updating the title, you can mark the pull request as ready for review. See also: |
Reranko05
left a comment
Reranko05
left a comment
There was a problem hiding this comment.
Could you use the Arrow PR title template:-
GH-<Issue Number>: [<Component>] <Title>
Jay846
changed the title
Jay846
changed the title
|
|
github-actions
Bot
added
awaiting committer review
|
You should use issue id but not the PR's id. If there isn't an existed issue, you can create a new one. |
Jay846
changed the title
|
|
|
Hi @pitrou, I've updated the PR title to match the tracking issue (GH-50994), removed the unused compile variables, and applied clang-format styling. The failures in macOS GLib and Windows/Conda seem to be flaky Flight and S3FS/MinIO test failures. Could we please trigger a re-run of the checks? Thanks! |
Some CI tests are currently failing to run, but this is unrelated to the changes in this PR. Don't worry. |
|
Also pushed a quick style formatting update to satisfy the pre-commit linter check. Kindly approve for re-run. Thanks |
|
https://www.githubstatus.com/ shows that GitHub Actions is currently experiencing some issues. Let's wait for it to recover. |
HuaHuaY
left a comment
HuaHuaY
left a comment
There was a problem hiding this comment.
Left a few comments. I'm not sure about the check for in_array.length == 0. The rest looks good to me.
| const ArraySpan& in_array = batch[0].array; | ||
| ArrayData* out_array = out->array_data().get(); | ||
|
|
||
| if (in_array.length == 0) { |
There was a problem hiding this comment.
I may have a mistake during the previous review. I am not sure whether this if condition will always evaluate to false due to the check at cpp/src/arrow/compute/exec.cc:786; perhaps we can assume here that in_array.length is never 0. Let's wait for comments from a reviewer who is more familiar with Arrow Compute.
There was a problem hiding this comment.
Got it, using if constexpr makes perfect sense here. I'll update those two checks.
For the length == 0 condition, I'll leave the check in place for now as a safeguard and wait for input from other maintainers on whether empty batches can reach this execution path. Thanks
There was a problem hiding this comment.
I think we can replace this with a DCHECK_NE(in_array.length, 0) and ensure that the tests exercise zero-length arrays and chunked arrays.
|
Good morning, Is any more changes from my side needed, do respond whenever you have time. Thanks! |
|
Hi everyone, It is an follow up regarding previous message of any more changes needed. Thanks |
|
Hi @pitrou, I would genuinely love to help incase of any error present from my side before merging it as currently according to my best knowledge all issues are addressed. Thank you |
|
@Jay846 Sorry for the delay. I'll take a look when I have time, but please also read our policy on AI generated code and prose. |
|
Sure @pitrou, I'll have a look into the policies. Thanks |
As you've probably read by now, these guidelines ask that you disclose usage of AI in your submissions, so that we understand what has been produced and/or vetted by a human. Can you please do so? |
|
Hi @pitrou, thanks for the reminder. I'll be transparent: I used an AI coding assistant (specifically Google's Antigravity) as a pair-programming tool throughout this PR where needed. What the AI helped with: initial kernel structure and formatting in suggestions. What I personally owned: understanding the bug (corrupted offsets from ignored sizes buffer), verifying the zero-copy fast-path logic and buffer pointer equality test, running and validating the unit tests locally after each change, and reviewing every iteration of the code before pushing. I reviewed and understood every line before committing. Happy to answer any specific questions about the implementation. |
|
Thank you @Jay846 ! |
| static bool IsContiguous(const ArraySpan& in_array) { | ||
| const auto* offsets = in_array.GetValues<src_offset_type>(1); | ||
| const auto* sizes = in_array.GetValues<src_offset_type>(2); | ||
| for (int64_t i = 0; i < in_array.length - 1; ++i) { |
There was a problem hiding this comment.
You could perhaps use SetBitRunReader to speed up walking the validity bitmap (individual IsNull calls are more expensive), though that's not necessary either.
| const ArraySpan& in_array = batch[0].array; | ||
| ArrayData* out_array = out->array_data().get(); | ||
|
|
||
| if (in_array.length == 0) { |
There was a problem hiding this comment.
I think we can replace this with a DCHECK_NE(in_array.length, 0) and ensure that the tests exercise zero-length arrays and chunked arrays.
| DCHECK_OK(func->AddKernel(SrcType::type_id, std::move(kernel))); | ||
| } | ||
|
|
||
| template <typename SrcType, typename DestType> |
There was a problem hiding this comment.
Let's add a comment summarizing this:
| template <typename SrcType, typename DestType> | |
| // (Large)ListView<T> -> (Large)List<U> | |
| template <typename SrcType, typename DestType> |
There was a problem hiding this comment.
Added // (Large)ListView -> (Large)List comment before the template
| // Zero-copy fast-path: shift offsets and slice child values | ||
| ARROW_ASSIGN_OR_RAISE( | ||
| out_array->buffers[1], | ||
| ctx->Allocate(sizeof(dest_offset_type) * (in_array.length + 1))); | ||
| auto* dest_offsets = out_array->GetMutableValues<dest_offset_type>(1); |
There was a problem hiding this comment.
This is the same as below and can be factored out of the if/else branch.
There was a problem hiding this comment.
Factored out offset buffer allocation above the if/else branch
| if (in_array.IsNull(i) && sizes[i] != 0) { | ||
| return false; | ||
| } | ||
| if (offsets[i] + sizes[i] != offsets[i + 1]) { |
There was a problem hiding this comment.
If the entry is null or zero-sized, then the exact value of offsets[i] shouldn't matter and we can instead keep the value of the last non-null non-zero entry?
(this is not a bug of course, just an additional optimization opportunity)
| src_offset_type current_offset = 0; | ||
| dest_offsets[0] = 0; | ||
| for (int64_t i = 0; i < in_array.length; ++i) { | ||
| if (in_array.IsNull(i)) { | ||
| dest_offsets[i + 1] = static_cast<dest_offset_type>(current_offset); | ||
| } else { | ||
| current_offset += sizes[i]; | ||
| dest_offsets[i + 1] = static_cast<dest_offset_type>(current_offset); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you could simplify the implementation by having the same loop offsets for both branches. That loop would compute all destination offsets and compute whether the source entries are contiguous, all in one go.
The contiguity information is mostly useful to know how to compute values afterwards. It needn't affect the computation of destination offsets, which has roughly the same costs in both cases.
| } | ||
|
|
||
| template <typename SrcType, typename DestType> | ||
| struct CastListView { |
There was a problem hiding this comment.
Call this CallListViewToVarList to make sure it's not used for casting to another list-view type?
| CheckCast(contiguous_src, large_contiguous_expected); | ||
| CheckCast(large_contiguous_src, contiguous_expected); | ||
|
|
||
| // 5. Null Propagation |
There was a problem hiding this comment.
Can you add nulls in the examples above? This will probably stress more situations.
| CheckCast(contiguous_src, contiguous_expected); | ||
|
|
||
| // Assert zero-copy for contiguous values | ||
| ASSERT_OK_AND_ASSIGN(auto cast_result, Cast(contiguous_src, list(int16()))); |
There was a problem hiding this comment.
Can you call ValidateFull on the cast result?
| *null_val_src_values)); | ||
| auto null_val_src_masked = MaskArrayWithNullsAt(null_val_src, {1}); | ||
| auto null_val_expected = ArrayFromJSON(list(int16()), "[[10], null]"); | ||
| CheckCast(null_val_src_masked, null_val_expected); |
There was a problem hiding this comment.
Can you add a test with zero-length inputs?
There was a problem hiding this comment.
Thanks for the detailed review @pitrou!
I'll address all the mandatory items:
- Replace the length == 0 check with DCHECK_NE and add zero-length test coverage
- Add the template comment // (Large)ListView -> (Large)List
- Rename the struct to CastListViewToVarList
- Factor out the offset allocation above the if/else branch
- Simplify the end_offset calculation and the slice call
- Add ValidateFull and null entries to the tests
- Added null entries to the contiguous test examples
- Added a zero-length input test case
The SetBitRunReader optimization and unified loop are noted as future improvements. Will push the fixes very shortly!
…oc, fix end_offset, expand tests
|
As informed prior I have pushed the updates incorporating all of @pitrou's feedback in commit 6bd6303 (renamed struct to CastListViewToVarList, added template documentation, added DCHECK_NE, factored out offset allocation, simplified end_offset, and expanded test cases with nulls and zero-length arrays). I noticed the Dev / Lint check failed due to a minor clang-format formatting preference. I can push a quick formatting-only commit to make Dev / Lint green whenever you'd like me to, along to that other 4 are Flaky / Infrastructure Failures as of my best knowledge. Please let me know if any other adjustments are needed and also correct me too if I am wrong anywhere! |
Please push a commit to fix Dev / Lint failure. Any CI failures introduced by this PR needs to be fixed, excluding the existed CI failures. |
|
I've cleaned up the code formatting in the latest commit (5d7c759). All linter and code formatting checks are ready for final review. Thanks! |
I think you might have forgotten to submit the commits related to these? |
|
Hi @HuaHuaY apologies for the confusion! Since mentioned in chat SetBitRunReader wasn't strictly necessary, I saved those optimizations for a separate follow-up PR to keep this one focused. But as mentioned now , I am doing it shortly. |
|
I've pushed commit b312f28 to address the Dev / Lint formatting check. Summary of changes:
|
4 participants
Rationale for this change
This PR implements missing type-casting compute kernels to convert
ListViewTypeandLargeListViewTypearrays to standardListTypeandLargeListTypearrays.Previously, these casts routed to
CastList, which ignored the sizes buffer and read offsets out-of-bounds, resulting in corrupted output arrays. This PR introduces a dedicatedCastListViewfunctor to perform correct conversions.What changes are included in this PR?
To maximize performance and optimize memory layouts, a dual-execution path was implemented in the
CastListViewexecution functor insidescalar_cast_nested.cc:offsets[i] + sizes[i] == offsets[i+1]). It avoids copying the child values array entirely, allocating the new output offset buffer, shifting offsets relative to the start, and slicing the child array directly to preserve zero-copy pointer semantics.Int64Builder, and invokes Arrow's internaltakecompute kernel to reconstruct a new contiguous child values array.Are these changes tested?
Yes, added comprehensive unit test suites in
scalar_cast_test.ccpassing all cases. Tests explicitly cover:ListView<int16>toList<int32>).Are there any user-facing changes?
No public API contracts were broken. This adds correct, declarative casting support natively to the existing internal compute framework.