Util to get packed table buffer size by nirandaperera · Pull Request #21322 · rapidsai/cudf

nirandaperera · 2026-02-03T23:58:02Z

Description

Closes #21321

Checklist

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

Signed-off-by: niranda perera <niranda.perera@gmail.com>

copy-pr-bot · 2026-02-03T23:58:05Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

wence- · 2026-02-04T09:49:56Z

cpp/src/copying/contiguous_split.cu

+  // Handle empty table cases
+  if (input.num_columns() == 0 || input.num_rows() == 0) { return 0; }
+
+  auto const num_partitions = std::size_t{1};  // no splits, just computing size for pack
+  auto const num_src_bufs   = count_src_bufs(input.begin(), input.end());
+  auto const num_bufs       = static_cast<std::size_t>(num_src_bufs) * num_partitions;
+
+  // Compute splits to get buffer sizes
+  auto partition_buf_size_and_dst_buf_info =
+    compute_splits(input, {} /* no splits */, num_partitions, num_src_bufs, num_bufs, stream, mr);


This part replicates the set up of the contiguous_split_state struct. Can we make it a static method of that struct so that the code is not repeated and can be used in both places.

I added a method here 5d304ae
Not the most elegant TBH 😇

Argh! This change conflicts with make_empty_packed_table() method. Fixing now

davidwendt · 2026-02-04T17:49:45Z

cpp/include/cudf/contiguous_split.hpp

+ *
+ * @param input View of the table to compute the packed size for
+ * @param stream CUDA stream used for device memory operations and kernel launches
+ * @param mr An optional memory resource to use for temporary allocations


The mr parameter is generally not intended for this kind of use in public APIs in libcudf.
I recommend removing the parameter.

@davidwendt we need a temp mr for the compute_splits method. What should we use there?

Just use cudf::get_current_device_resource_ref() at the compute_splits() call directly:

auto partition_buf_size_and_dst_buf_info = compute_splits(input, {} /* no splits */, num_partitions, num_src_bufs, num_bufs, stream, cudf::get_current_device_resource_ref());

I understand that this would be good enough for this PR and the new allocations are minimal. But I thought cudf was heading towards accepting an mr rather than using current device mr, in all methods?

Not quite. Currently we are talking about passing in both mr's in a container class in order to help make it easier to override on individual calls without having to modify the global one. That will be a large effort.
Having said that, I think you are right that eventually this mr would end up back here.
The doxygen in the .hpp clearly stated this was a temporary mr so I'm ok with leaving it but perhaps call the parameter temp_mr.

@davidwendt I renamed it to temp_mr

Signed-off-by: niranda perera <niranda.perera@gmail.com>

… cudf_packed_size

wence- · 2026-02-05T09:06:42Z

cpp/src/copying/contiguous_split.cu

+                                const_cast<size_type&>(num_src_bufs),
+                                const_cast<std::size_t&>(num_bufs),


This const_cast is needed because you now explicitly initialise the const num_bufs/num_src_bufs?

I am leery, because const_casting away constness always feels like a bad idea.

Yes, that was the reason. But, I removed the const from those 2 class members now. I think it should be okay. Do you think they should be initialized to INT::MAX like number so that it fails quickly in case of a future mistake?

cpp/src/copying/contiguous_split.cu

Signed-off-by: niranda perera <niranda.perera@gmail.com>

cpp/include/cudf/detail/contiguous_split.hpp

Signed-off-by: niranda perera <niranda.perera@gmail.com>

wence- · 2026-02-06T16:45:13Z

cpp/src/copying/contiguous_split.cu

-    // First pass over the source tables to generate a `dst_buf_info` per split and column buffer
-    // (`num_bufs`). After this, contiguous_split uses `dst_buf_info` to further subdivide the work
-    // into 1MB batches in `compute_batches`
-    partition_buf_size_and_dst_buf_info = std::move(
-      compute_splits(input, splits, num_partitions, num_src_bufs, num_bufs, stream, temp_mr));
+    auto result  = compute_num_bufs_and_splits(input, splits, stream, temp_mr);
+    num_src_bufs = std::get<0>(result);
+    num_bufs     = std::get<1>(result);
+    partition_buf_size_and_dst_buf_info = std::move(std::get<2>(result));


std::tie(num_src_bufs, num_buffs, partition_buf_size_and_dst_buf_info) = compute_num_bufs_and_splits(...);

Then we can reconstify the num_src_bufs and num_bufs I think.

partition_buf_size_and_dst_buf_info is unique_ptr. So, I think we cant use tie. That's one reason why I initially went with the out-ref approach.

Looks like it should work: https://godbolt.org/z/TPWdYce7z

Sorry, I was mistaken. I think I mistook the error with the const assignment errors.
Changed this to std::tie now

nirandaperera · 2026-02-06T17:13:07Z

@davidwendt I think these python test failures are unrelated. Is the CI broken currently?

davidwendt · 2026-02-06T17:13:54Z

@davidwendt I think these python test failures are unrelated. Is the CI broken currently?

Yes

Signed-off-by: niranda perera <niranda.perera@gmail.com>

adding packed_size impl

1e99756

Signed-off-by: niranda perera <niranda.perera@gmail.com>

github-actions bot assigned nirandaperera Feb 3, 2026

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Feb 3, 2026

nirandaperera mentioned this pull request Feb 3, 2026

[FEA] Util to get packed table buffer size #21321

Open

wence- reviewed Feb 4, 2026

View reviewed changes

davidwendt reviewed Feb 4, 2026

View reviewed changes

adding a util method

5d304ae

nirandaperera marked this pull request as ready for review February 4, 2026 20:48

nirandaperera requested a review from a team as a code owner February 4, 2026 20:48

nirandaperera requested review from devavret and ttnghia February 4, 2026 20:48

nirandaperera added 4 commits February 4, 2026 20:35

fixing test

5b62906

Merge branch 'main' into cudf_packed_size

46fb631

addressing comments

b8c8e94

Signed-off-by: niranda perera <niranda.perera@gmail.com>

Merge branch 'cudf_packed_size' of github.com:nirandaperera/cudf into…

08232e6

… cudf_packed_size

nirandaperera requested review from davidwendt and wence- February 5, 2026 04:47

wence- reviewed Feb 5, 2026

View reviewed changes

davidwendt requested changes Feb 5, 2026

View reviewed changes

cpp/src/copying/contiguous_split.cu Outdated Show resolved Hide resolved

cpp/src/copying/contiguous_split.cu Outdated Show resolved Hide resolved

nirandaperera added 2 commits February 5, 2026 07:34

use a tuple

afd7374

Signed-off-by: niranda perera <niranda.perera@gmail.com>

Merge branch 'main' of github.com:rapidsai/cudf into cudf_packed_size

00e572d

nirandaperera requested review from davidwendt and wence- February 5, 2026 15:36

davidwendt added feature request New feature or request 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Feb 5, 2026

davidwendt reviewed Feb 5, 2026

View reviewed changes

cpp/include/cudf/detail/contiguous_split.hpp Outdated Show resolved Hide resolved

nirandaperera added 2 commits February 5, 2026 10:24

removing unncessary method

30ef1fa

Signed-off-by: niranda perera <niranda.perera@gmail.com>

Merge branch 'main' of github.com:rapidsai/cudf into cudf_packed_size

95610c9

nirandaperera requested a review from davidwendt February 5, 2026 18:26

davidwendt approved these changes Feb 5, 2026

View reviewed changes

Merge branch 'main' into cudf_packed_size

da4a841

wence- reviewed Feb 6, 2026

View reviewed changes

nirandaperera added 2 commits February 6, 2026 11:37

use std tie

8403b69

Signed-off-by: niranda perera <niranda.perera@gmail.com>

Merge branch 'main' into cudf_packed_size

82c6e64

		const_cast<size_type&>(num_src_bufs),
		const_cast<std::size_t&>(num_bufs),

Conversation

nirandaperera commented Feb 3, 2026

Description

Checklist

Uh oh!

copy-pr-bot bot commented Feb 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirandaperera commented Feb 6, 2026

Uh oh!

davidwendt commented Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants