Unbind cudf::size_type from offsets used by list columns - #23607
Open
davidwendt wants to merge 8 commits into
Open
Unbind cudf::size_type from offsets used by list columns#23607davidwendt wants to merge 8 commits into
davidwendt wants to merge 8 commits into
Conversation
davidwendt
added
improvement
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR standardizes list, string, JSON, and TDigest offsets on fixed-width ChangesFixed-width offset migration
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/groupby/sort/group_collect.cu (1)
66-70: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDeclare explicit return types on the changed device lambdas.
Both changed lambdas are passed to device algorithms. Their return types should be explicit to prevent type deduction from becoming part of the CUDA algorithm contract.
cpp/src/groupby/sort/group_collect.cu#L66-L70: declare the lambda return type assize_type.cpp/src/rolling/detail/rolling_collect_list.cu#L61-L65: declare the lambda return type asbool.As per coding guidelines, extended device lambdas passed to device algorithms must declare explicit return types.
🤖 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/groupby/sort/group_collect.cu` around lines 66 - 70, Declare explicit return types on both changed device lambdas passed to device algorithms: in cpp/src/groupby/sort/group_collect.cu lines 66-70, make the lambda returning the null-count result return size_type; in cpp/src/rolling/detail/rolling_collect_list.cu lines 61-65, make the corresponding lambda return bool.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/include/cudf/detail/sizes_to_offsets_iterator.cuh`:
- Around line 289-292: Update the final STRING-offset construction in
ngrams_tokenize to call cudf::strings::detail::make_offsets_child_column instead
of the generic sizes-to-offsets helper, preserving the configured dynamic offset
type and allowing large-string totals. Keep the existing tokenization and offset
values unchanged.
- Around line 321-322: Prevent oversized cudf::size_type values from wrapping
before validation: in the generic path around the device lambda in
sizes_to_offsets_iterator.cuh, keep the scan input wide and perform the checked
int32_t conversion only after validating against total_elements; in
cpp/include/cudf/lists/detail/scatter.cuh lines 234-239, replace the direct
lv->view().size() narrowing with a checked int32_t step and preserve the
existing range-validation behavior.
In `@cpp/src/groupby/sort/group_collect.cu`:
- Around line 88-94: Enforce checked INT32 range handling for LIST offsets at
all four sites: in cpp/src/groupby/sort/group_collect.cu lines 88-94, validate
every group_offsets value before copying into the INT32 offsets column; in
cpp/include/cudf_test/column_wrapper.hpp lines 1614-1629, validate cumulative
nested child sizes before int32_t conversion; in
cpp/include/cudf_test/column_wrapper.hpp lines 1669-1675, validate c->size()
before constructing the INT32 offsets column; and in
cpp/src/io/utilities/column_buffer.cpp lines 107-110, reject or safely
checked-convert oversized string offsets before creating the LIST column. Ensure
no conversion can wrap or produce invalid memory access.
In `@cpp/src/lists/combine/concatenate_rows.cu`:
- Around line 108-116: Guard every size_type-to-int32_t LIST offset conversion:
in cpp/src/lists/combine/concatenate_rows.cu lines 108-116, accumulate into a
wider temporary and reject cumulative child counts above INT32_MAX before the
exclusive scan; in cpp/src/lists/copying/concatenate.cu lines 66-72, accumulate
shift in a wide type, validate its range, and return an explicit int32_t from
the device transform; in cpp/src/io/json/host_tree_algorithms.cu line 216,
validate row_offsets before scattering and use an int32-compatible scan; in
cpp/src/lists/utilities.cu lines 35-38, ensure labels_to_offsets rejects
label-derived values outside the int32_t range.
In `@cpp/src/lists/copying/copying.cu`:
- Around line 43-45: Keep LIST offset storage explicitly int32_t at all affected
sites: in cpp/src/lists/copying/copying.cu lines 43-45, allocate out_offsets as
rmm::device_uvector<int32_t>; in
cpp/src/lists/combine/concatenate_list_elements.cu lines 50-52, create the
empty-inner-child fallback with int32_t zero offsets; and in
cpp/src/io/json/parser_features.cpp line 78, use a LIST-specific int32_t
zero-offset helper while leaving string offsets unchanged.
In `@cpp/src/lists/sequences.cu`:
- Line 159: Update the internal LIST offset pointer declarations in
tabulator::offsets, sequences_dispatcher::operator(), and
sequences_functor::invoke from size_type const* to int32_t const*. Preserve
size_type for row counts and element indices so LIST offsets remain independent
of cudf::size_type.
In `@java/src/main/native/src/ColumnViewJni.cu`:
- Around line 182-183: Update the generate_list_offsets implementation to read
the validated INT32 list_length input using int32_t iterators, replacing the
cudf::size_type begin/end iterator types while preserving the existing
offset-generation logic.
---
Outside diff comments:
In `@cpp/src/groupby/sort/group_collect.cu`:
- Around line 66-70: Declare explicit return types on both changed device
lambdas passed to device algorithms: in cpp/src/groupby/sort/group_collect.cu
lines 66-70, make the lambda returning the null-count result return size_type;
in cpp/src/rolling/detail/rolling_collect_list.cu lines 61-65, make the
corresponding lambda return bool.
🪄 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: 32ae0536-4e3a-478e-ba27-395feed032d0
📒 Files selected for processing (46)
cpp/include/cudf/column/column_factories.hppcpp/include/cudf/detail/sizes_to_offsets_iterator.cuhcpp/include/cudf/lists/detail/gather.cuhcpp/include/cudf/lists/detail/scatter.cuhcpp/include/cudf/lists/list_device_view.cuhcpp/include/cudf/lists/lists_column_device_view.cuhcpp/include/cudf/lists/lists_column_view.hppcpp/include/cudf/tdigest/tdigest_column_view.hppcpp/include/cudf_test/column_wrapper.hppcpp/src/groupby/groupby.cucpp/src/groupby/sort/group_collect.cucpp/src/groupby/sort/group_merge_lists.cucpp/src/hash/md5_hash.cucpp/src/io/json/host_tree_algorithms.cucpp/src/io/json/nested_json.hppcpp/src/io/json/parser_features.cppcpp/src/io/orc/stripe_enc.cucpp/src/io/orc/writer_impl.cucpp/src/io/statistics/statistics.cuhcpp/src/io/utilities/column_buffer.cppcpp/src/io/utilities/column_buffer_strings.cucpp/src/lists/combine/concatenate_list_elements.cucpp/src/lists/combine/concatenate_rows.cucpp/src/lists/copying/concatenate.cucpp/src/lists/copying/copying.cucpp/src/lists/copying/gather.cucpp/src/lists/copying/scatter_helper.cucpp/src/lists/dremel.cucpp/src/lists/extract.cucpp/src/lists/interleave_columns.cucpp/src/lists/lists_column_factories.cucpp/src/lists/lists_column_view.cucpp/src/lists/reverse.cucpp/src/lists/segmented_sort.cucpp/src/lists/sequences.cucpp/src/lists/stream_compaction/apply_boolean_mask.cucpp/src/lists/utilities.cucpp/src/rolling/detail/rolling_collect_list.cucpp/src/rolling/detail/rolling_collect_list.cuhcpp/src/rolling/detail/rolling_operators.cuhcpp/src/strings/convert/convert_lists.cucpp/src/strings/repeat_strings.cucpp/src/transform/row_bit_count.cucpp/tests/groupby/collect_list_tests.cppjava/src/main/native/src/ColumnViewJni.cppjava/src/main/native/src/ColumnViewJni.cu
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/quantiles/tdigest/tdigest_util.cuh (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
<cstdint>directly in this header.This header now declares
int32_t, but it does not include<cstdint>. Add the direct include instead of relying on transitive includes.As per coding guidelines, include headers directly for every used symbol.
Proposed include
`#pragma` once +#include <cstdint> + `#include` <cudf/detail/iterator.cuh>🤖 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/quantiles/tdigest/tdigest_util.cuh` at line 18, Update the header containing the t-digest offsets declaration to directly include the standard <cstdint> header before using int32_t, without relying on transitive includes.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/quantiles/tdigest/tdigest.cu`:
- Line 362: In cpp/src/quantiles/tdigest/tdigest.cu at lines 362-362, reject
input.size() == INT32_MAX before allocating input.size() + 1 offsets; for
non-empty output, compute input.size() * percentiles.size() in a widened type
and reject values above INT32_MAX before num_output_values, grid sizing, and
exclusive scan. In cpp/src/strings/search/find_multiple.cu at lines 72-73,
reject strings_count == INT32_MAX before evaluating strings_count + 1, while
preserving the existing product check for the final offset.
---
Nitpick comments:
In `@cpp/src/quantiles/tdigest/tdigest_util.cuh`:
- Line 18: Update the header containing the t-digest offsets declaration to
directly include the standard <cstdint> header before using int32_t, without
relying on transitive includes.
🪄 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: 09ad97ba-5310-4520-99d2-f94c3b951c60
📒 Files selected for processing (5)
cpp/src/quantiles/tdigest/tdigest.cucpp/src/quantiles/tdigest/tdigest_aggregation.cucpp/src/quantiles/tdigest/tdigest_util.cuhcpp/src/strings/search/find_multiple.cucpp/src/text/minhash.cu
3 tasks
PointKernel
approved these changes
Aug 10, 2026
PointKernel
left a comment
PointKernel
left a comment
Member
There was a problem hiding this comment.
Changes look good to me. For my own learning, @davidwendt, how did you find all the places where size_type was being misused as an offset type? Or was it mostly AI effort?
bdice
approved these changes
Aug 11, 2026
ttnghia
approved these changes
Aug 12, 2026
Contributor
|
The This is addressed in NVIDIA/cudf-spark-jni#4990. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/src/lists/extract.cu (1)
102-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an explicit
INT32_MAXbound fornum_lists.
cudf::detail::sequencedoes not reject indices aboveINT32_MAX; it casts each index to the output type. Sincemake_index_offsetscreatesnum_lists + 1int32_toffsets, rejectnum_lists > INT32_MAXbefore the call.🤖 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/lists/extract.cu` around lines 102 - 108, Update make_index_offsets to validate that num_lists does not exceed INT32_MAX before calling cudf::detail::sequence, and reject the input using the surrounding code’s established validation/error mechanism. Preserve the existing sequence construction for valid values.java/src/main/native/src/ColumnViewJni.cu (1)
41-50: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce the INT32 range in every LIST-offset producer.
These paths can produce cumulative LIST offsets outside the
INT32contract. A 64-bitcudf::size_typemakes the existing size checks insufficient. Reject out-of-range totals before narrowing or scanning.
java/src/main/native/src/ColumnViewJni.cu#L41-L50: validate the cumulativelist_lengthtotal before the INT32 inclusive scan, or scan with a wider accumulator and reject overflow.cpp/src/strings/search/find_multiple.cu#L70-L75: validatestrings_count * targets_countagainstINT32_MAXwith overflow-safe arithmetic before constructingnumeric_scalar<int32_t>.cpp/src/text/minhash.cu#L197-L199: updatebuild_list_resultat Lines 615-619 to use INT32 scalars and validateinput.size() * seeds_sizeagainstINT32_MAX.🤖 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 `@java/src/main/native/src/ColumnViewJni.cu` around lines 41 - 50, Enforce the INT32 offset contract in all three LIST-offset producers: in java/src/main/native/src/ColumnViewJni.cu lines 41-50, validate the cumulative list_length total before the INT32 scan or use a wider scan accumulator and reject overflow; in cpp/src/strings/search/find_multiple.cu lines 70-75, use overflow-safe arithmetic to reject strings_count * targets_count above INT32_MAX before constructing numeric_scalar<int32_t>; and in cpp/src/text/minhash.cu lines 197-199, update build_list_result at lines 615-619 to use INT32 scalars and validate input.size() * seeds_size against INT32_MAX.
🤖 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/hash/md5_hash.cu`:
- Around line 326-327: Update the offset lookups in the md5() boundary-reading
code to add col.offset() to both row_index accesses: use row_index +
col.offset() for the start and row_index + col.offset() + 1 for the end,
preserving the existing offset element type and hashing flow.
In `@cpp/src/io/json/nested_json.hpp`:
- Around line 151-152: Validate every JSON LIST child offset against INT32_MAX
before constructing, allocating, or scattering into int32_t child-offset
storage. Apply this to both host-side child offset construction at
cpp/src/io/json/nested_json.hpp lines 75-76 and device-side storage at lines
151-152, preserving the int32_t contract only after validation and rejecting or
handling overflow explicitly.
In `@cpp/src/io/utilities/column_buffer.cpp`:
- Line 55: Update the temporary offset allocation in the ColumnBuffer
constructor to use the dynamic string offset width required by the large-string
path, rather than always using INT32. Preserve INT32 allocation for the
standard-string output path, ensuring the buffer remains correctly sized when
size_type is 64-bit.
In `@cpp/src/lists/interleave_columns.cu`:
- Around line 52-53: Validate the accumulated output offset before storing the
exclusive-scan result in the INT32 `list_offsets` column. In the
interleave-columns implementation around `d_offsets`, reject counts exceeding
`INT32_MAX`, or perform the scan in a wider temporary type and validate before
converting; preserve the existing offset output for valid inputs.
In `@cpp/src/quantiles/tdigest/tdigest_aggregation.cu`:
- Around line 1613-1614: Update compute_tdigests to convert generated
cluster_info::cluster_start offsets to int32_t with range validation before
make_tdigest_column, preserving the TDigest LIST offset type. In
build_output_column, replace size_type-based offset access with int32_t access
for both reads and writes, while retaining the existing output construction
behavior.
---
Outside diff comments:
In `@cpp/src/lists/extract.cu`:
- Around line 102-108: Update make_index_offsets to validate that num_lists does
not exceed INT32_MAX before calling cudf::detail::sequence, and reject the input
using the surrounding code’s established validation/error mechanism. Preserve
the existing sequence construction for valid values.
In `@java/src/main/native/src/ColumnViewJni.cu`:
- Around line 41-50: Enforce the INT32 offset contract in all three LIST-offset
producers: in java/src/main/native/src/ColumnViewJni.cu lines 41-50, validate
the cumulative list_length total before the INT32 scan or use a wider scan
accumulator and reject overflow; in cpp/src/strings/search/find_multiple.cu
lines 70-75, use overflow-safe arithmetic to reject strings_count *
targets_count above INT32_MAX before constructing numeric_scalar<int32_t>; and
in cpp/src/text/minhash.cu lines 197-199, update build_list_result at lines
615-619 to use INT32 scalars and validate input.size() * seeds_size against
INT32_MAX.
🪄 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: acea5496-3125-402e-8380-0616a2288370
📒 Files selected for processing (50)
cpp/include/cudf/column/column_factories.hppcpp/include/cudf/detail/sizes_to_offsets_iterator.cuhcpp/include/cudf/lists/detail/gather.cuhcpp/include/cudf/lists/detail/scatter.cuhcpp/include/cudf/lists/list_device_view.cuhcpp/include/cudf/lists/lists_column_device_view.cuhcpp/include/cudf/lists/lists_column_view.hppcpp/include/cudf/tdigest/tdigest_column_view.hppcpp/include/cudf_test/column_wrapper.hppcpp/src/groupby/groupby.cucpp/src/groupby/sort/group_collect.cucpp/src/groupby/sort/group_merge_lists.cucpp/src/hash/md5_hash.cucpp/src/io/json/host_tree_algorithms.cucpp/src/io/json/nested_json.hppcpp/src/io/json/parser_features.cppcpp/src/io/orc/stripe_enc.cucpp/src/io/orc/writer_impl.cucpp/src/io/statistics/statistics.cuhcpp/src/io/utilities/column_buffer.cppcpp/src/io/utilities/column_buffer_strings.cucpp/src/lists/combine/concatenate_list_elements.cucpp/src/lists/combine/concatenate_rows.cucpp/src/lists/copying/concatenate.cucpp/src/lists/copying/copying.cucpp/src/lists/copying/gather.cucpp/src/lists/copying/scatter_helper.cucpp/src/lists/dremel.cucpp/src/lists/extract.cucpp/src/lists/interleave_columns.cucpp/src/lists/lists_column_factories.cucpp/src/lists/lists_column_view.cucpp/src/lists/reverse.cucpp/src/lists/segmented_sort.cucpp/src/lists/sequences.cucpp/src/lists/stream_compaction/apply_boolean_mask.cucpp/src/lists/utilities.cucpp/src/quantiles/tdigest/tdigest.cucpp/src/quantiles/tdigest/tdigest_aggregation.cucpp/src/quantiles/tdigest/tdigest_util.cuhcpp/src/rolling/detail/rolling_collect_list.cucpp/src/rolling/detail/rolling_collect_list.cuhcpp/src/rolling/detail/rolling_operators.cuhcpp/src/strings/convert/convert_lists.cucpp/src/strings/repeat_strings.cucpp/src/strings/search/find_multiple.cucpp/src/text/minhash.cucpp/src/transform/row_bit_count.cujava/src/main/native/src/ColumnViewJni.cppjava/src/main/native/src/ColumnViewJni.cu
Comment on lines
+326
to
+327
| auto const offset_begin = offsets.element<int32_t>(row_index); | ||
| auto const offset_end = offsets.element<int32_t>(row_index + 1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target context ---'
sed -n '280,350p' cpp/src/hash/md5_hash.cu
printf '%s\n' '--- related offset accesses ---'
rg -n -C 3 'offsets\.element|col\.offset\(\)|column_device_view' \
cpp/src/hash/md5_hash.cu cpp/src/io/orc/writer_impl.cu cpp/src/strings/convert/convert_lists.cu \
| head -n 260
printf '%s\n' '--- declarations and uses of md5 hash entry points ---'
rg -n -C 3 'md5|md5_hash' cpp/src/hash/md5_hash.cu cpp/src/hash cpp/include \
| head -n 260Repository: NVIDIA/cudf
Length of output: 30882
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- column_device_view declarations and offset implementation ---'
rg -n -C 5 'size_type offset\(\)|offset\(\) const|class column_device_view|struct column_device_view|create\(.*table|table_device_view' \
cpp/include cpp/src | head -n 320
printf '%s\n' '--- md5 tests and sliced LIST coverage ---'
rg -n -C 5 'md5|slice|LIST|lists_column_view' \
cpp/tests cpp/src/hash | head -n 360
printf '%s\n' '--- list column construction and offset invariants ---'
rg -n -C 4 'make_lists_column|lists_column_view.*offset|offsets.*offset|offset.*offset' \
cpp/include cpp/src cpp/tests | head -n 360Repository: NVIDIA/cudf
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- column_device_view base: offset, child, element, validity ---'
sed -n '1,280p' cpp/include/cudf/column/column_device_view_base.cuh
printf '%s\n' '--- column_device_view: child and element APIs ---'
rg -n -C 8 'child\(|element\(|column_device_view\(' cpp/include/cudf/column/column_device_view.cuh | head -n 260
printf '%s\n' '--- table_device_view creation and indexing ---'
fd -t f 'table_device_view' cpp/include cpp/src | xargs -r -n1 sh -c 'echo "--- $0"; rg -n -C 8 "create\\(|column\\(|table_device_view" "$0" | head -n 180'
printf '%s\n' '--- md5 list tests ---'
sed -n '100,235p' cpp/tests/hashing/md5_test.cpp
printf '%s\n' '--- list slicing examples ---'
rg -n -C 8 'slice\(.*list|lists_column_wrapper.*slice|column_view.*slice|cudf::slice' cpp/tests/hashing cpp/tests/lists cpp/src | head -n 260Repository: NVIDIA/cudf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- device-view construction ---'
rg -n -C 12 'column_device_view::column_device_view|column_device_view_core|create_column_device_views|extent\(column_view' \
cpp/src/column cpp/src/table cpp/include/cudf/column/column_device_view.cuh \
| head -n 300
printf '%s\n' '--- column view child and slicing semantics ---'
rg -n -C 10 'column_view::child|column_view::slice|column_view::offset|lists_column_view::parent|get_sliced_child' \
cpp/src/column cpp/src/lists cpp/include/cudf/column cpp/include/cudf/lists \
| head -n 320
printf '%s\n' '--- list view API documentation ---'
rg -n -C 10 'class lists_column_view|offsets\(\)|parent\(\)|child\(\)' \
cpp/include/cudf/lists/lists_column_view.hpp cpp/src/lists/lists_column_view.cpp cpp/include/cudf/column/column_view.hpp \
| head -n 300Repository: NVIDIA/cudf
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
# Read-only model of a LIST column whose parent view starts at row 1.
offsets = [0, 2, 2, 5] # underlying LIST offsets
row_index = 0 # first row in the sliced view
parent_offset = 1
unadjusted = offsets[row_index], offsets[row_index + 1]
adjusted = offsets[row_index + parent_offset], offsets[row_index + parent_offset + 1]
assert unadjusted == (0, 2)
assert adjusted == (2, 5 - 3) if False else adjusted == (2, 5)
assert unadjusted != adjusted
print({
"unadjusted_boundaries": unadjusted,
"adjusted_boundaries": adjusted,
"result": "parent offset is required",
})
PYRepository: NVIDIA/cudf
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
# Underlying LIST offsets represent rows [0, 1, 2].
offsets = [0, 2, 2, 5]
row_index = 0
parent_offset = 1 # the sliced view starts at underlying row 1
unadjusted = (offsets[row_index], offsets[row_index + 1])
adjusted = (
offsets[row_index + parent_offset],
offsets[row_index + parent_offset + 1],
)
assert unadjusted == (0, 2)
assert adjusted == (2, 2)
assert unadjusted != adjusted
print({
"unadjusted_boundaries": unadjusted,
"adjusted_boundaries": adjusted,
"result": "parent offset is required",
})
PYRepository: NVIDIA/cudf
Length of output: 253
Add the parent LIST offset when reading boundaries.
For sliced LIST input, read both boundaries at row_index + col.offset() and row_index + col.offset() + 1. Otherwise, md5() hashes the wrong elements.
🤖 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/hash/md5_hash.cu` around lines 326 - 327, Update the offset lookups
in the md5() boundary-reading code to add col.offset() to both row_index
accesses: use row_index + col.offset() for the start and row_index +
col.offset() + 1 for the end, preserving the existing offset element type and
hashing flow.
| CUDF_EXPECTS(type.id() == type_id::STRING, "allocate_strings_data called for non-string column"); | ||
| // size + 1 for final offset. _string_data will be initialized later. | ||
| _data = create_data(data_type{type_to_id<size_type>()}, size + 1, memset_data, stream, _mr); | ||
| _data = create_data(data_type{type_id::INT32}, size + 1, memset_data, stream, _mr); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve wide temporary offsets for large STRING columns.
Line 55 always allocates four-byte offsets. cpp/src/io/utilities/column_buffer_strings.cu Line 24 reads the same buffer as size_type* in the large-string path and produces INT64 offsets. When size_type becomes 64-bit, that path accesses an underallocated buffer.
Allocate temporary offsets with the dynamic string offset width, or resize and convert the buffer before the large-string path. Keep INT32 only for the standard-string output path.
🤖 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/io/utilities/column_buffer.cpp` at line 55, Update the temporary
offset allocation in the ColumnBuffer constructor to use the dynamic string
offset width required by the large-string path, rather than always using INT32.
Preserve INT32 allocation for the standard-string output path, ensuring the
buffer remains correctly sized when size_type is 64-bit.
Comment on lines
+52
to
+53
| data_type{type_id::INT32}, num_output_lists + 1, mask_state::UNALLOCATED, stream, mr); | ||
| auto const d_offsets = list_offsets->mutable_view().template begin<int32_t>(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the accumulated output offset before writing INT32 storage.
Lines 52-53 store the exclusive-scan result in an INT32 column. When the combined list-entry count exceeds INT32_MAX, the scan can wrap before Line 349 reads the final offset. Reject this input, or calculate in a wider temporary type and validate before conversion to INT32.
🤖 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/lists/interleave_columns.cu` around lines 52 - 53, Validate the
accumulated output offset before storing the exclusive-scan result in the INT32
`list_offsets` column. In the interleave-columns implementation around
`d_offsets`, reject counts exceeding `INT32_MAX`, or perform the scan in a wider
temporary type and validate before converting; preserve the existing offset
output for valid inputs.
Comment on lines
+1613
to
+1614
| cuda::std::span<int32_t const>{tdigest_offsets.begin<int32_t>(), | ||
| static_cast<size_t>(tdigest_offsets.size())}}, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep generated TDigest LIST offsets on int32_t.
Although Line 1613-1614 correctly passes an int32_t span for input offsets, compute_tdigests still constructs output offsets from cluster_info::cluster_start, which is an rmm::device_uvector<size_type>. build_output_column also reads and writes this column with begin<size_type>() at Line 861-919. When size_type becomes int64_t, the output TDigest will carry INT64 offsets, while the public TDigest view requires int32_t offsets. Convert and range-check the generated offsets before make_tdigest_column, then use int32_t access in build_output_column.
🤖 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/quantiles/tdigest/tdigest_aggregation.cu` around lines 1613 - 1614,
Update compute_tdigests to convert generated cluster_info::cluster_start offsets
to int32_t with range validation before make_tdigest_column, preserving the
TDigest LIST offset type. In build_output_column, replace size_type-based offset
access with int32_t access for both reads and writes, while retaining the
existing output construction behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
In preparation for evaluating a change of
cudf::size_typefromint32_ttoint64_t, this PR decouples LIST column offsets fromsize_type.Per the Apache Arrow columnar format, the offsets buffer width is part of the type, not an implementation detail —
List<T>hasint32offsets andLargeList<T>hasint64offsets, and these are distinct types:and from format/Schema.fbs, on
LargeList:The same split appears in the C Data Interface format strings:
+lis a list with int32 offsets,+Lis a large list withint64offsets.libcudf has
type_id::LISTand noLARGE_LIST. So aLISTcolumn's offsets child isint32by definition of the type — not "int32because that happens to besize_typetoday."Many places in the code, however, spelled that type as
cudf::size_type, viatype_to_id<size_type>(),data<size_type>(),element<size_type>(),size_type const*, etc. Those uses are correct only by the coincidence thatsize_type == int32_t. Under a 64-bitsize_typethey would either allocate INT64 offsets children — producing columns cuDF cannot export to Arrow, and thatcontiguous_splitand the JNI layer would misinterpret — or reinterpret anint32buffer asint64. Both are silent data corruption rather than a compile error, which is what makes them worth flushing out ahead of anysize_typechange rather than during one.This PR replaces those with explicit
int32_t / type_id::INT32at LIST offsets creation and read sites.STRING offsets are deliberately untouched: they are already dynamically
int32/int64and are handled through theoffsetalator.Why this isn't a new constraint: The codebase already depends on 32-bit list offsets everywhere it is forced to be explicit.
This PR makes that existing, already-relied-upon invariant explicit at the sites that were spelling it as
size_type.Follow-on work would now allow support of
LARGE_LISTin libcudf without explicitly adding a new type_id by simply checking the column's child offset type much like howLARGE_STRINGis supported in interop today. Theoffsetalatorwould similarly be employed to read/write the offset values correctly without need a special dispatch for the column type.Checklist