-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Unbind cudf::size_type from offsets used by list columns #23607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6d369ec
b7f310a
9a5365a
3d4ca87
c1d1c53
dd326b2
598ae02
80e37ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION. | ||
| * SPDX-FileCopyrightText: Copyright (c) 2020-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
|
|
@@ -286,8 +286,10 @@ auto sizes_to_offsets(SizesIterator begin, | |
| * The return also includes the total number of elements -- the last element value from the | ||
| * scan. | ||
| * | ||
| * The returned column is always `type_id::INT32` since offsets children are 32-bit. | ||
| * | ||
| * @throw std::overflow_error if the total size of the scan (last element) greater than maximum | ||
| * value of `size_type` | ||
| * value of `int32_t` | ||
| * | ||
| * @tparam InputIterator Used as input to scan to set the offset values | ||
| * @param begin The beginning of the input sequence | ||
|
|
@@ -303,26 +305,34 @@ std::pair<std::unique_ptr<column>, size_type> make_offsets_child_column( | |
| rmm::cuda_stream_view stream, | ||
| rmm::device_async_resource_ref mr) | ||
| { | ||
| auto count = static_cast<size_type>(std::distance(begin, end)); | ||
| auto offsets_column = make_numeric_column( | ||
| data_type{type_to_id<size_type>()}, count + 1, mask_state::UNALLOCATED, stream, mr); | ||
| // Offsets children of compound (LIST/STRING) columns are 32-bit and so are | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not all STRING columns have 32-bit offsets, so this categorical statement is confusing. Can we state this in terms of functionality or supported values (e.g., "Only LIST columns and STRING columns with 32-bit offsets are supported, so this is deliberately not tied to `cudf::size_type`")? Also in line 289 above. |
||
| // deliberately not tied to `cudf::size_type`. | ||
| auto count = static_cast<size_type>(std::distance(begin, end)); | ||
| auto offsets_column = | ||
| make_numeric_column(data_type{type_id::INT32}, count + 1, mask_state::UNALLOCATED, stream, mr); | ||
| auto offsets_view = offsets_column->mutable_view(); | ||
| auto d_offsets = offsets_view.template data<size_type>(); | ||
| auto d_offsets = offsets_view.template data<int32_t>(); | ||
|
|
||
| // The number of offsets is count+1 so to build the offsets from the sizes | ||
| // using exclusive-scan technically requires count+1 input values even though | ||
| // the final input value is never used. | ||
| // The input iterator is wrapped here to allow the last value to be safely read. | ||
| // The input sizes are deliberately not narrowed to the 32-bit offsets type here -- doing so | ||
| // would corrupt individual sizes larger than `int32_t` before they reach the scan, so the | ||
| // overflow check below could no longer detect the overflow. Narrowing happens only on write | ||
| // to `d_offsets`, after the accumulated total has been validated. | ||
| using SizeType = cuda::std::iter_value_t<InputIterator>; | ||
| auto map_fn = | ||
| cuda::proclaim_return_type<size_type>([begin, count] __device__(size_type idx) -> size_type { | ||
| return idx < count ? static_cast<size_type>(begin[idx]) : size_type{0}; | ||
| cuda::proclaim_return_type<SizeType>([begin, count] __device__(size_type idx) -> SizeType { | ||
| return idx < count ? begin[idx] : SizeType{0}; | ||
| }); | ||
| auto input_itr = cudf::detail::make_counting_transform_iterator(0, map_fn); | ||
| // Use the sizes-to-offsets iterator to compute the total number of elements | ||
| auto const total_elements = | ||
| sizes_to_offsets(input_itr, input_itr + count + 1, d_offsets, 0, stream); | ||
| // the offsets are 32-bit so the total must fit in an int32_t | ||
| CUDF_EXPECTS( | ||
| total_elements <= static_cast<decltype(total_elements)>(std::numeric_limits<size_type>::max()), | ||
| total_elements <= static_cast<decltype(total_elements)>(std::numeric_limits<int32_t>::max()), | ||
| "Size of output exceeds the column size limit", | ||
| std::overflow_error); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -323,8 +323,8 @@ std::unique_ptr<column> md5(table_view const& input, | |
| if (data_col.type().id() == type_id::LIST) { | ||
| CUDF_UNREACHABLE("Nested list unsupported"); | ||
| } | ||
| auto const offset_begin = offsets.element<size_type>(row_index); | ||
| auto const offset_end = offsets.element<size_type>(row_index + 1); | ||
| auto const offset_begin = offsets.element<int32_t>(row_index); | ||
| auto const offset_end = offsets.element<int32_t>(row_index + 1); | ||
|
Comment on lines
+326
to
+327
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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 🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @davidwendt This is a real bug, but pre-existing and out of scope, so the only potential argument for addressing it in this PR is that the fix would be on the exact two lines you're already touching. Definitely deferrable to a follow-up. |
||
| cudf::type_dispatcher<dispatch_storage_type>( | ||
| data_col.type(), ListHasherDispatcher(&hasher, data_col), offset_begin, offset_end); | ||
| } else { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,8 +72,8 @@ struct json_column { | |
| std::vector<row_offset_t> string_offsets; | ||
| std::vector<row_offset_t> string_lengths; | ||
|
|
||
| // Row offsets | ||
| std::vector<row_offset_t> child_offsets; | ||
| // Row offsets (LIST offsets child data; always 32-bit) | ||
| std::vector<int32_t> child_offsets; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated to this file, but flagging here since |
||
|
|
||
| // Validity bitmap | ||
| std::vector<bitmask_type> validity; | ||
|
|
@@ -148,8 +148,8 @@ struct device_json_column { | |
| rmm::device_uvector<row_offset_t> string_offsets; | ||
| rmm::device_uvector<row_offset_t> string_lengths; | ||
|
|
||
| // Row offsets | ||
| rmm::device_uvector<row_offset_t> child_offsets; | ||
| // Row offsets (LIST offsets child data; always 32-bit) | ||
| rmm::device_uvector<int32_t> child_offsets; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| // Validity bitmap | ||
| rmm::device_buffer validity; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.