Skip to content

Change LIST column offsets from size_type to int32 in benchmarks and gtests - #23682

Open
davidwendt wants to merge 2 commits into
NVIDIA:mainfrom
davidwendt:lists-gtests-offsets-type
Open

Change LIST column offsets from size_type to int32 in benchmarks and gtests#23682
davidwendt wants to merge 2 commits into
NVIDIA:mainfrom
davidwendt:lists-gtests-offsets-type

Conversation

@davidwendt

Copy link
Copy Markdown
Contributor

Description

Follow on work to #23607 includes changes to cpp/benchmarks and cpp/tests where cudf::size_type is used for LIST column offsets and replaced with int32_t instead.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@davidwendt davidwendt self-assigned this Aug 17, 2026
@davidwendt
davidwendt requested a review from a team as a code owner August 17, 2026 18:36
@davidwendt
davidwendt requested review from vyasr and wence- August 17, 2026 18:36
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 17, 2026
@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 992cf619-cfb7-49aa-926f-8b29813b6d96

📥 Commits

Reviewing files that changed from the base of the PR and between 20fef07 and e0df26a.

📒 Files selected for processing (22)
  • cpp/benchmarks/lists/copying/scatter_lists.cu
  • cpp/tests/copying/concatenate_tests.cpp
  • cpp/tests/copying/get_value_tests.cpp
  • cpp/tests/copying/scatter_list_tests.cpp
  • cpp/tests/groupby/collect_list_tests.cpp
  • cpp/tests/hashing/murmurhash3_x86_32_test.cpp
  • cpp/tests/io/cudftable_test.cpp
  • cpp/tests/io/json/json_test.cpp
  • cpp/tests/io/parquet_chunked_reader_test.cu
  • cpp/tests/io/parquet_chunked_writer_test.cpp
  • cpp/tests/io/parquet_reader_dict_test.cpp
  • cpp/tests/io/parquet_reader_test.cpp
  • cpp/tests/io/parquet_v2_test.cpp
  • cpp/tests/lists/contains_tests.cpp
  • cpp/tests/reductions/list_rank_test.cpp
  • cpp/tests/reshape/byte_cast_tests.cpp
  • cpp/tests/rolling/collect_ops_test.cpp
  • cpp/tests/sort/top_k_tests.cpp
  • cpp/tests/structs/structs_column_tests.cpp
  • cpp/tests/transform/row_bit_count_test.cu
  • cpp/tests/utilities/column_utilities.cu
  • cpp/tests/utilities_tests/lists_column_wrapper_tests.cpp

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Summary by CodeRabbit

  • Tests

    • Updated list-column test fixtures and utilities to use explicit 32-bit offset values.
    • Preserved existing test scenarios, expected results, and overflow assertions across copying, grouping, hashing, I/O, lists, reductions, sorting, structs, and transforms.
    • Improved consistency between test offsets and list-column storage behavior.
  • Chores

    • Refreshed copyright years and attribution notices where applicable.

Walkthrough

The change standardizes list-column offset construction and access on explicit INT32 and int32_t types across benchmark code, utility code, and tests. Copyright notices and one empty-column type construction are also updated.

Changes

List offset alignment

Layer / File(s) Summary
Offset access and benchmark construction
cpp/benchmarks/lists/copying/scatter_lists.cu, cpp/tests/utilities/column_utilities.cu
Benchmark offsets and utility iterators now use explicit INT32 and int32_t types.
List construction and copying tests
cpp/tests/copying/*, cpp/tests/groupby/collect_list_tests.cpp, cpp/tests/hashing/*, cpp/tests/structs/*, cpp/tests/utilities_tests/*
List, nested-list, and struct-list fixtures now construct offsets with int32_t.
Serialization and table IO fixtures
cpp/tests/io/*
JSON, Parquet, and cudf-table fixtures now use int32_t list and map offsets.
Algorithm and transformation fixtures
cpp/tests/lists/*, cpp/tests/reductions/*, cpp/tests/reshape/*, cpp/tests/rolling/*, cpp/tests/sort/*, cpp/tests/transform/*
Expected and constructed list offsets now use int32_t. Copyright notices are updated in affected files.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to e0df2

This change updates LIST offset types in benchmarks and tests without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review.

Possibly related PRs

Suggested reviewers: pointkernel, bdice

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.52% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: replacing LIST column offsets from cudf::size_type with int32 in benchmarks and tests.
Description check ✅ Passed The description directly explains the offset type changes, affected directories, and relationship to PR #23607.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants