[GLUTEN-12809][VL] Fix short decimal / timestamp width in hash shuffle partition buffer sizing

[zhouyuan]

What changes are proposed in this pull request?

calculateSimpleColumnBytes() derived the per-row width from arrow::bit_width(arrowColumnTypes_[colIdx]->id()), which disagrees with the width the writer actually stores for two types:

• short decimal: split as int64 (splitFixedWidthValueBuffer, case 128:) and allocated as 8 bytes by valueBufferSizeForFixedWidthArray(), but arrow::bit_width(Decimal128) is 128, so it was counted as 16 bytes.
• timestamp: split as int128 and allocated as byteSize() = 16 bytes, but arrow::bit_width(Timestamp) is 64, so it was counted as 8.

fixes: #12809

How was this patch tested?

existing unit tests

Was this patch authored or co-authored using generative AI tooling?

Claude Opus 5

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Aligns the “simple column” per-row fixed-width size estimate with the actual partition buffer allocation logic to avoid under/over-sizing for certain Arrow types (short decimal, timestamp).

Changes:

• Replace arrow::bit_width-based per-column byte sizing with valueBufferSizeForFixedWidthArray(..., 1) to match real allocation behavior.
• Add clarifying comments explaining why Arrow bit width diverges from the writer’s allocated width for specific types.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cpp/velox/shuffle/VeloxHashShuffleWriter.cc:902

• Using valueBufferSizeForFixedWidthArray(colIdx, 1) to derive a per-row byte width relies on the implicit assumption that this helper is linear in numRows and does not include any minimum-allocation rounding/padding that would distort a per-row estimate. Consider adding a small dedicated helper that returns the per-element byte width used by allocation (or document/guarantee in the helper contract that size is strictly numRows * elementWidth for fixed-width types). This makes the intent clearer and avoids future drift if allocation logic changes to include alignment or minimum capacity behavior.

    const auto colIdx = simpleColumnIndices_[col];
    // Reuse the same per-column sizing as the actual buffer allocation, otherwise this estimate can
    // drift from it: `arrow::bit_width` mis-counts the types whose Arrow bit width differs from the
    // width the partition buffer allocates, i.e. short decimal (allocated as int64, 8 bytes not 16)
    // and timestamp (allocated as int128, 16 bytes not 8). Note bool is still rounded up to one byte
    // per row.
    fixedWidthBufferBytes_ += valueBufferSizeForFixedWidthArray(colIdx, 1);

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

cpp/velox/tests/VeloxHashShuffleWriterBufferSizingTest.cc:47

• The header comment claims this test verifies the fixed-width estimate matches the actual per-row allocation for each fixed-width column, but the estimate intentionally rounds BOOL up to 1 byte/row (see calculateSimpleColumnBytes) and doesn’t account for conditional validity buffers. Consider tightening the wording to focus on the short-decimal and timestamp sizing mismatch this PR fixes, to avoid misleading future readers.

// Verifies that calculateSimpleColumnBytes() estimates each fixed-width column with the same
// per-row width the partition buffers are actually allocated with (see
// valueBufferSizeForFixedWidthArray). Short decimal is stored as int64 (8 bytes, not the 16 of
// arrow::bit_width(Decimal128)), and timestamp is stored as int128 (16 bytes, not the 8 of
// arrow::bit_width(Timestamp)).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[marin-ma]

LGTM. Thanks!

[philo-he]

Just one very trivial comment. Thanks.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.