Copilot commented on code in PR #12499:
URL: https://github.com/apache/gluten/pull/12499#discussion_r3570429315
##########
cpp/velox/memory/GpuBufferColumnarBatch.cc:
##########
@@ -67,16 +67,20 @@ std::shared_ptr<GpuBufferColumnarBatch>
GpuBufferColumnarBatch::compose(
GLUTEN_CHECK(!batches.empty(), "No batches to compose");
// Compute the returned GpuBufferColumnarBatch buffers.
auto& type = batches[0]->getRowType();
- const auto bufferSize = batches[0]->buffers().size();
+ const auto numBuffers = batches[0]->buffers().size();
std::vector<size_t> bufferSizes;
- bufferSizes.resize(bufferSize);
+ bufferSizes.resize(numBuffers);
std::vector<BufferType> bufferTypes;
- bufferTypes.reserve(bufferSize);
+ bufferTypes.reserve(numBuffers);
for (const auto& colType : type->children()) {
bufferSizes[bufferTypes.size()] = arrow::bit_util::BytesForBits(numRows);
bufferTypes.push_back(BufferType::kNull);
Review Comment:
The validity/null bitmap buffer is currently sized with
arrow::bit_util::BytesForBits(numRows). cuDF null-mask operations require
allocation rounded up to the cuDF bitmask word size; using the Arrow byte-sized
buffer can lead to out-of-bounds reads/writes when this batch is converted to
cuDF (e.g., via GpuBufferBatchResizer/makeCudfTable). Consider sizing the null
buffer using cuDF’s allocation rule (equivalent to
cudf::bitmask_allocation_size_bytes(numRows)).
##########
cpp/velox/memory/GpuBufferColumnarBatch.cc:
##########
@@ -86,27 +90,37 @@ std::shared_ptr<GpuBufferColumnarBatch>
GpuBufferColumnarBatch::compose(
bufferTypes.push_back(BufferType::kValue);
}
}
- VELOX_CHECK_EQ(bufferTypes.size(), bufferSize);
+ VELOX_CHECK_EQ(bufferTypes.size(), numBuffers);
// This buffer may be more than the actual reauired buffer for null buffer.
for (const auto& batch : batches) {
if (batch->numRows() == 0) {
continue;
}
- for (auto i = 0; i < bufferSize; ++i) {
+ for (auto i = 0; i < numBuffers; ++i) {
// The null buffer may be null or length = 0.
// Maybe optimize later, detect if the null buffer is all true. And set
the return null buffer to 0.
if (bufferTypes[i] == BufferType::kNull || bufferTypes[i] ==
BufferType::kLength) {
continue;
}
+
auto& buffer = batch->bufferAt(i);
VELOX_CHECK_NOT_NULL(buffer);
- bufferSizes[i] += buffer->size();
+
+ if (bufferTypes[i] == BufferType::kTimestampValue) {
+ // Velox Timestamp value is 16 bytes. CUDF Timestamp value is 8 bytes.
+ bufferSizes[i] += buffer->size() >> 1;
+ } else if (bufferTypes[i] == BufferType::kBooleanValue) {
Review Comment:
Timestamp buffer sizing uses `buffer->size() >> 1`, which implicitly assumes
the source buffer is exactly 16 bytes/row. This is fragile if the upstream
buffer size includes padding/capacity or if the timestamp representation
changes; sizing based on `batch->numRows()` keeps the allocation rule explicit
(8 bytes/row output).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
