GH-50275: [C++][CSV] avoid int32 overflow in block parser value counts#50074
Conversation
|
Pushed a clang-format fix for the C++ Format lint failure. The macOS Python job looks like an unrelated teardown issue, not from this change. |
|
|
|
Good point, the offset is only 31 bits so widening the products doesn't actually save us once the value count passes int32. I've switched to erroring out before presizing when rows x cols would exceed INT32_MAX, and added a test that drives num_cols high enough to trip it. Kept the two multiplications in int64 as well, since the capacity's |
|
@metsw24-max Thanks for the update! This is looking good, but can you open an issue for this? This is not a MINOR issue. |
|
|
|
Opened #50275 and re-titled the PR to reference it. Cheers. |
|
CI failures are unrelated, I'll merge. |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit d8890e0. There was 1 benchmark result indicating a performance regression:
The full Conbench report has more details. |
Rationale for this change
The CSV block parser sizes its per-chunk value array from
num_cols, the column count inferred from the first line of the input, times the rows-in-chunk count.PresizedValueDescWritercomputes2 + num_rows * num_cols, andParseSpecializedcomputesnum_cols_ * (num_rows_ - start) * 10, both inint32_t. A CSV whose first line carries a few million fields pushes these products pastINT32_MAX, which is signed-integer-overflow UB (UBSan flags both expressions).What changes are included in this PR?
Raise an error if the
num_colswould prevent the block size from fitting in a 32 bits integer.Are these changes tested?
With a new unit test.
Are there any user-facing changes?
No.