Add Arrow fixed-size-list ingress - #23583
Conversation
github-actions
Bot
added
libcudf
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Arrow interop layer detects fixed-size-list schemas, converts them to cuDF ChangesFixed-size-list Arrow interoperability
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to The PR adds fixed-size-list Arrow ingress with focused and interoperability validation; remaining items are limited to optional test coverage, allocator consistency, and test-helper cleanup, with no actionable merge-blocking risk at the current head. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/interop/from_arrow_host.cu (1)
608-717: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd boundary and multi-block fixed-size-list cases.
The tests cover empty, null, and sliced input. They do not cover boundary or multi-block row counts. Add fixed-size-list cases that cross the relevant execution-size boundary.
As per coding guidelines, “Tests must cover empty inputs, nulls, sliced columns, boundary and multi-block sizes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/interop/from_arrow_host.cu` around lines 608 - 717, Add fixed-size-list test cases covering row counts at the relevant execution-size boundary and counts large enough to span multiple blocks. Extend the existing tests for empty, null, and sliced inputs, using the from_arrow_column conversion path and validating both conversion success and resulting values.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/interop/from_arrow_host.cu`:
- Around line 297-317: Update get_fixed_size_list_offsets to validate
input->offset and input->length are non-negative before arithmetic, handle width
== 0 explicitly, and verify input->length + 1 fits cudf::size_type. Check each
multiplication for int64_t overflow before computing offset and length, then
retain the existing child-element limit validation and only cast after all
bounds checks pass.
In `@cpp/tests/interop/from_arrow_device_test.cpp`:
- Around line 310-388: Expand the fixed-size-list coverage in
FixedSizeListColumn and FixedSizeListColumnSliced in
cpp/tests/interop/from_arrow_device_test.cpp:310-388 to include empty inputs,
nullable list rows, boundary sizes, and multi-block sizes while preserving
existing full and sliced cases. Extend the corresponding fixed-size-list tests
in cpp/tests/interop/from_arrow_host_test.cpp:608-717 with boundary-size and
multi-block-size cases; no empty or nullable cases are requested there.
In `@cpp/tests/interop/from_arrow_host_test.cpp`:
- Around line 608-717: Extend the fixed-size-list tests around
FixedSizeListColumn with a case at the relevant conversion boundary and another
whose row count exceeds one execution block. Cover both valid and nullable
inputs as appropriate, and assert the resulting offsets, child values, and null
masks (including nonempty null rows where applicable) rather than only table
equivalence.
In `@cpp/tests/interop/from_arrow_stream_test.cpp`:
- Around line 174-209: Expand the fixed-size-list coverage in
cpp/tests/interop/from_arrow_stream_test.cpp at lines 174-209 by adding
null-row, nonzero-offset sliced, boundary-size, and multi-block-size stream
cases alongside FixedSizeListEmptyTest and FixedSizeListChunkedTest. Expand the
related coverage in python/pylibcudf/tests/test_table.py at lines 110-120 with
empty, all-null, and single-row arrays, plus a mixed-type table containing a
fixed-size-list column; no other sites require changes.
---
Outside diff comments:
In `@cpp/src/interop/from_arrow_host.cu`:
- Around line 608-717: Add fixed-size-list test cases covering row counts at the
relevant execution-size boundary and counts large enough to span multiple
blocks. Extend the existing tests for empty, null, and sliced inputs, using the
from_arrow_column conversion path and validating both conversion success and
resulting values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a1a9d9ea-0721-4ce2-b20b-09819fa50260
📒 Files selected for processing (11)
cpp/src/interop/arrow_data_structures.cppcpp/src/interop/arrow_utilities.cppcpp/src/interop/arrow_utilities.hppcpp/src/interop/from_arrow_device.cucpp/src/interop/from_arrow_host.cucpp/src/interop/from_arrow_host.hppcpp/tests/interop/arrow_data_structures_test.cppcpp/tests/interop/from_arrow_device_test.cppcpp/tests/interop/from_arrow_host_test.cppcpp/tests/interop/from_arrow_stream_test.cpppython/pylibcudf/tests/test_table.py
|
Addressed in fdf87a7: added overflow-safe bounds validation, including negative metadata, zero width, multiplication overflow, and short-child checks |
|
Ready for maintainer vetting. Could someone add feature request and non-breaking labels and approve NVIDIA runner workflows? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/interop/arrow_utilities.cpp`:
- Around line 111-121: In the fixed-size-list bounds validation before
constructing the returned range, add a check that row_end is at most max_size /
width after the existing int64_t overflow check. Ensure this rejects absolute
child ranges exceeding cudf::size_type before the child_length validation and
return in the surrounding conversion function.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ae361867-000a-429d-8c1b-9343c94183eb
📒 Files selected for processing (8)
cpp/src/interop/arrow_utilities.cppcpp/src/interop/arrow_utilities.hppcpp/src/interop/from_arrow_device.cucpp/src/interop/from_arrow_host.cucpp/tests/interop/from_arrow_device_test.cppcpp/tests/interop/from_arrow_host_test.cppcpp/tests/interop/from_arrow_stream_test.cpppython/pylibcudf/tests/test_table.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/pylibcudf/tests/test_table.py
- cpp/src/interop/from_arrow_device.cu
- cpp/src/interop/from_arrow_host.cu
fdf87a7 to
625a3af
Compare
GregoryKimball
added
feature request
625a3af to
8599f6f
Compare
|
|
ddf9508 to
5d8fc7b
Compare
| "Owning Arrow device wrappers do not support fixed-size-list input until " | ||
| "fixed-size-list egress is implemented", | ||
| cudf::data_type_error); |
There was a problem hiding this comment.
Please remove the "TODO" from the error message.
| owned.emplace_back(make_fixed_size_list_offsets( | ||
| static_cast<size_type>(layout.row_end) + 1, layout.width, stream, mr)); | ||
| offsets_view = owned.back()->view(); |
There was a problem hiding this comment.
question: is it worth introducing this helper function that is only used here and could be inlined, I think.
| { | ||
| constexpr int32_t width = 2; | ||
|
|
||
| for (auto const num_rows : {cudf::size_type{1024}, cudf::size_type{1025}}) { |
There was a problem hiding this comment.
Why do we need to test two different numbers of rows here?
| * @return INT32 offsets column | ||
| */ | ||
| std::unique_ptr<column> make_fixed_size_list_offsets(size_type size, | ||
| int32_t width, |
There was a problem hiding this comment.
question: Should the width value also be size_type?
| * @param width Number of child elements per list row | ||
| * @param stream CUDA stream used for device memory operations | ||
| * @param mr Device memory resource to use for all device memory allocations | ||
| * @return INT32 offsets column |
There was a problem hiding this comment.
nit: This is correct today, but becomes out of date if we move to 64bit offsets.
| * Arrow fixed-size-list arrays carry no offsets buffer; the offsets are implicit. | ||
| * This generates `size` offsets of the form `{0, width, 2*width, ...}`. | ||
| * | ||
| * @param size Number of offsets to generate (normally num_rows + 1) |
There was a problem hiding this comment.
When would it not be num_rows + 1?
| * @brief Validated physical bounds for an Arrow fixed-size-list array | ||
| */ | ||
| struct fixed_size_list_layout { | ||
| int32_t width; ///< Child elements per row and LIST offset increment |
There was a problem hiding this comment.
question: size_type? Or does the arrow spec mandate int32?
|
/ok to test 5d8fc7b |
5d8fc7b to
b2fe766
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cpp/src/interop/from_arrow_host.cu (1)
443-444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the current device resource for the Thrust execution policy.
The policy passed to
thrust::sequencesupplies the allocator for temporary allocations. The output resourcemrshould own only the returned offsets column. Passcudf::get_current_device_resource_ref()tormm::exec_policy_nosyncinstead.thrust::sequencerarely allocates, so the practical impact is small.As per coding guidelines, "Temporary memory not using
cudf::get_current_device_resource_ref()" is a review concern forcpp/**/*.{cpp,cu,hpp,cuh}.♻️ Proposed change
- thrust::sequence( - rmm::exec_policy_nosync(stream, mr), d_offsets, d_offsets + num_offsets, int32_t{0}, width); + thrust::sequence(rmm::exec_policy_nosync(stream, cudf::get_current_device_resource_ref()), + d_offsets, + d_offsets + num_offsets, + int32_t{0}, + width);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/interop/from_arrow_host.cu` around lines 443 - 444, Update the execution policy used by thrust::sequence to pass cudf::get_current_device_resource_ref() instead of the output resource mr, keeping mr responsible only for the returned offsets column.Source: Coding guidelines
cpp/tests/interop/from_arrow_stream_test.cpp (1)
128-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the fixed-size-list Arrow test helpers. The fixed-size-list schema builder and array builder are duplicated almost verbatim across the interop test files.
cudf_test/nanoarrow_utils.hppis already included by these tests and is the natural home for them.
cpp/tests/interop/from_arrow_stream_test.cpp#L128-L189: Movemake_fixed_size_list_stream_schemaandmake_fixed_size_list_chunkintocudf_test/nanoarrow_utils.hppunder one shared name, then call the shared helpers here.cpp/tests/interop/from_arrow_host_test.cpp#L535-L610: Replacemake_fixed_size_list_schemaandmake_fixed_size_list_arraywith the shared helpers, and keepas_host_device_arraylocal if it stays host-specific.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/interop/from_arrow_stream_test.cpp` around lines 128 - 189, Move the duplicated fixed-size-list schema and array builders into the shared nanoarrow utility header under common helper names, preserving their current behavior and signatures as needed. In cpp/tests/interop/from_arrow_stream_test.cpp lines 128-189, remove the local make_fixed_size_list_stream_schema and make_fixed_size_list_chunk definitions and update callers to use the shared helpers. In cpp/tests/interop/from_arrow_host_test.cpp lines 535-610, replace make_fixed_size_list_schema and make_fixed_size_list_array with the shared helpers; leave as_host_device_array local if it remains host-specific.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/interop/arrow_data_structures_test.cpp`:
- Around line 324-333: Extend Arrow interop tests near
NestedFixedSizeListDeviceInputRejected with a schema whose dictionary value type
is a fixed-size list, then construct the corresponding arrow_column or
arrow_table and assert it throws cudf::data_type_error. Reuse the existing
fixed-size-list and device-input helpers where applicable, while preserving the
current direct-schema and struct-child coverage.
---
Nitpick comments:
In `@cpp/src/interop/from_arrow_host.cu`:
- Around line 443-444: Update the execution policy used by thrust::sequence to
pass cudf::get_current_device_resource_ref() instead of the output resource mr,
keeping mr responsible only for the returned offsets column.
In `@cpp/tests/interop/from_arrow_stream_test.cpp`:
- Around line 128-189: Move the duplicated fixed-size-list schema and array
builders into the shared nanoarrow utility header under common helper names,
preserving their current behavior and signatures as needed. In
cpp/tests/interop/from_arrow_stream_test.cpp lines 128-189, remove the local
make_fixed_size_list_stream_schema and make_fixed_size_list_chunk definitions
and update callers to use the shared helpers. In
cpp/tests/interop/from_arrow_host_test.cpp lines 535-610, replace
make_fixed_size_list_schema and make_fixed_size_list_array with the shared
helpers; leave as_host_device_array local if it remains host-specific.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 889a9cb2-074c-4dd0-9902-b8508e11dde6
📒 Files selected for processing (11)
cpp/src/interop/arrow_data_structures.cppcpp/src/interop/arrow_utilities.cppcpp/src/interop/arrow_utilities.hppcpp/src/interop/from_arrow_device.cucpp/src/interop/from_arrow_host.cucpp/src/interop/from_arrow_host.hppcpp/tests/interop/arrow_data_structures_test.cppcpp/tests/interop/from_arrow_device_test.cppcpp/tests/interop/from_arrow_host_test.cppcpp/tests/interop/from_arrow_stream_test.cpppython/pylibcudf/tests/test_table.py
|
|
||
| TEST_F(ArrowTableTest, NestedFixedSizeListDeviceInputRejected) | ||
| { | ||
| auto schema = make_fixed_size_list_schema(true); | ||
| auto array = make_empty_device_array(); | ||
|
|
||
| EXPECT_THROW( | ||
| { static_cast<void>(cudf::interop::arrow_table(std::move(*schema.get()), std::move(array))); }, | ||
| cudf::data_type_error); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add coverage for dictionary-held fixed-size lists.
Line 36 in cpp/src/interop/arrow_data_structures.cpp traverses schema.dictionary. These tests cover a direct schema and a struct child only. Add a dictionary schema whose dictionary value is a fixed-size list. Assert that the owning arrow_column or arrow_table constructor throws cudf::data_type_error.
As per coding guidelines, **/*: “Add unit tests and unit benchmarks.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/tests/interop/arrow_data_structures_test.cpp` around lines 324 - 333,
Extend Arrow interop tests near NestedFixedSizeListDeviceInputRejected with a
schema whose dictionary value type is a fixed-size list, then construct the
corresponding arrow_column or arrow_table and assert it throws
cudf::data_type_error. Reuse the existing fixed-size-list and device-input
helpers where applicable, while preserving the current direct-schema and
struct-child coverage.
Source: Coding guidelines
5 participants
Closes #23545
Validation on RTX A6000, CUDA 13.3: