Skip to content

Commit 315db02

Browse files
authored
Fix convert_to_rows dropping the trailing column at a tile boundary (#4493)
## Summary - `detail::determine_tiles` had an off-by-one in how it accounts for the first column of a new tile after the running tile is closed mid-loop. When the shmem-budget threshold happened to fire on the very last column of the table, the tail-close at the end of the loop was skipped and that column was never emitted to the kernel. `rmm::device_buffer` is not zero-initialized, so the dropped column read back whatever the pool allocator had there — typically 0, occasionally stale — yielding the non-deterministic `b = 0` failures tracked in NVIDIA/spark-rapids#10062. - Fix: set `current_tile_width = 1` after the reset. `col` is the first (and so far only) column of the new tile. - Add `ColumnToRowTests.PivotLikeLayout` that reproduces the failing schema (191 INT64 + 1 INT32). It reads the trailing INT32 from the raw JCUDF bytes directly rather than round-tripping through `convert_from_rows`, which would mask the bug by using the same layout code on the read side. The failing pivot schema packs to exactly 192 columns; with the default 48 KB shmem budget and `tile_height = 32`, the 191 Longs fit in one tile (`1528 * 32 = 48896 ≤ 49136`) and adding the trailing Int tips the estimate to `1536 * 32 = 49152 > 49136`, firing the threshold on the last column — the shape required to hit this bug. The existing `convert_to_rows` tests all use uniform-size schemas, so the threshold either fired early (leaving later columns to re-increment the width) or never fired; none reached the "threshold fires on the last column" path. Related to NVIDIA/spark-rapids#10062. ## Test plan - [x] `ROW_CONVERSION` gtest, including the new `ColumnToRowTests.PivotLikeLayout`, passes - [x] With this fix applied and the `isAcceleratedTransposeSupported` workaround reverted in spark-rapids, the `test_hash_multiple_grpby_pivot` integration test passes under `DATAGEN_SEED=1702610203` --------- Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
1 parent 83f4a14 commit 315db02

2 files changed

Lines changed: 62 additions & 2 deletions

File tree

src/main/cpp/src/row_conversion.cu

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1733,7 +1733,7 @@ void determine_tiles(std::vector<size_type> const& column_sizes,
17331733
row_size += col_size; // alignment required for shared memory tile boundary to match
17341734
// alignment of output row
17351735
current_tile_start_col = col;
1736-
current_tile_width = 0;
1736+
current_tile_width = 1;
17371737
} else {
17381738
row_size = row_size_with_this_col;
17391739
current_tile_width++;

src/main/cpp/tests/row_conversion.cpp

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2022-2024, NVIDIA CORPORATION.
2+
* Copyright (c) 2022-2026, NVIDIA CORPORATION.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -26,6 +26,8 @@
2626
#include <row_conversion.hpp>
2727
#include <utilities/iterator.cuh>
2828

29+
#include <cstddef>
30+
#include <cstring>
2931
#include <limits>
3032
#include <random>
3133

@@ -437,6 +439,64 @@ TEST_F(ColumnToRowTests, Biggest)
437439
}
438440
}
439441

442+
// Regression test for https://github.com/NVIDIA/spark-rapids/issues/10062.
443+
//
444+
// AcceleratedColumnarToRowIterator packs columns by size descending, so a
445+
// pivot-shaped schema of N INT64 columns plus one INT32 places the INT32
446+
// last. Choosing N so that the INT32 is the column that tips the estimated
447+
// shmem usage in detail::determine_tiles over the per-tile budget is what
448+
// triggers the bug: the tile containing only that column is never emitted,
449+
// leaving the column's output bytes at whatever rmm::device_buffer handed
450+
// back. With the 48 KB default shmem budget and tile_height = 32, 191 Longs
451+
// fit in one tile (1528 * 32 = 48896 <= 49136) and adding the trailing INT32
452+
// tips the estimate to 1536 * 32 = 49152 > 49136.
453+
//
454+
// We check the trailing INT32 directly against the raw JCUDF bytes rather
455+
// than round-tripping through convert_from_rows, which would mask the bug by
456+
// using the same layout code on the read side.
457+
TEST_F(ColumnToRowTests, PivotLikeLayout)
458+
{
459+
constexpr int num_longs = 191;
460+
constexpr int num_rows = 100;
461+
462+
std::vector<int64_t> long_data(num_rows, 0);
463+
std::vector<int32_t> int_data(num_rows);
464+
for (int r = 0; r < num_rows; ++r) {
465+
int_data[r] = 0x11223344 + r;
466+
}
467+
468+
std::vector<cudf::test::fixed_width_column_wrapper<int64_t>> long_cols;
469+
long_cols.reserve(num_longs);
470+
for (int i = 0; i < num_longs; ++i) {
471+
long_cols.emplace_back(long_data.begin(), long_data.end());
472+
}
473+
cudf::test::fixed_width_column_wrapper<int32_t> int_col(int_data.begin(), int_data.end());
474+
475+
std::vector<cudf::column_view> views;
476+
views.reserve(num_longs + 1);
477+
for (auto& c : long_cols) {
478+
views.emplace_back(c);
479+
}
480+
views.emplace_back(int_col);
481+
482+
auto rows = spark_rapids_jni::convert_to_rows(cudf::table_view(views));
483+
ASSERT_EQ(rows.size(), 1u);
484+
485+
// JCUDF row layout: [Longs][INT32][validity bits][pad to 8B].
486+
constexpr std::size_t int_offset = static_cast<std::size_t>(num_longs) * sizeof(int64_t);
487+
constexpr std::size_t data_end = int_offset + sizeof(int32_t);
488+
constexpr std::size_t validity_bytes = (num_longs + 1 + 7) / 8;
489+
constexpr std::size_t row_stride = (data_end + validity_bytes + 7) & ~std::size_t{7};
490+
491+
auto host_bytes = cudf::test::to_host<int8_t>(cudf::lists_column_view(*rows[0]).child()).first;
492+
ASSERT_GE(host_bytes.size(), row_stride * num_rows);
493+
for (int r = 0; r < num_rows; ++r) {
494+
int32_t actual = 0;
495+
std::memcpy(&actual, host_bytes.data() + r * row_stride + int_offset, sizeof(int32_t));
496+
EXPECT_EQ(actual, int_data[r]) << "row " << r;
497+
}
498+
}
499+
440500
TEST_F(RowToColumnTests, Single)
441501
{
442502
cudf::test::fixed_width_column_wrapper<int32_t> a({-1});

0 commit comments

Comments
 (0)