GH-50186: [C++][Gandiva] REPLACE throws "Buffer overflow for output string" for results larger than 64 KB - #50187
Conversation
|
|
github-actions
Bot
added
Component: C++
Component: Gandiva
awaiting review
Gandiva's REPLACE hardcoded a 65535-byte output cap, throwing "Buffer overflow for output string" whenever the result exceeded 64 KB. Size the output buffer to the exact result instead, by counting non-overlapping matches of from_str: text_len + num_matches * (to_str_len - from_str_len). Removes the arbitrary cap; the internal replace_with_max_len variant and its bounds checks are unchanged. Gandiva variable-length output uses int32 offsets, so a single output string cannot exceed INT_MAX (2 GB). Guard that boundary explicitly with a clear error message instead of letting the int32 size cast wrap silently (which could otherwise lead to under-allocation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
awaiting committer review
akravchukdremio
left a comment
akravchukdremio
left a comment
There was a problem hiding this comment.
PR looks good, left minor comments
akravchukdremio
left a comment
akravchukdremio
left a comment
There was a problem hiding this comment.
Looks good to me, thanks!
…nstead of eagerly allocating a potentially gigantic buffer
|
@kou any chance this can be merged? |
There was a problem hiding this comment.
Pull request overview
Removes Gandiva REPLACE’s hardcoded 64KB output cap by computing a safe output buffer size (exact in the counting path; bounded upper bound in the eager path) while preserving the existing max-length helper as a backstop, and adds tests/benchmarks to validate and measure the change.
Changes:
- Compute
max_lengthforreplace_utf8_utf8_utf8based on input sizes and (when needed) a non-overlapping match-counting pass, instead of using65535. - Harden overflow checks in
replace_with_max_len_utf8_utf8_utf8by doing length arithmetic ingdv_int64. - Add regression tests for >64KB growth/shrink and INT_MAX boundary behavior; add a targeted microbenchmark and build plumbing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cpp/src/gandiva/precompiled/string_ops.cc | Computes dynamic output sizing for REPLACE and uses 64-bit arithmetic for overflow guards. |
| cpp/src/gandiva/precompiled/string_ops_test.cc | Adds regression coverage for large outputs and explicit INT_MAX boundary errors. |
| cpp/src/gandiva/tests/CMakeLists.txt | Adds a Gandiva benchmark target to measure REPLACE sizing overhead. |
| cpp/src/gandiva/tests/string_ops_benchmark.cc | New microbenchmark comparing wrapper sizing behavior vs direct max-len helper calls. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Calls the precompiled REPLACE functions directly, so it compiles | ||
| # string_ops.cc/context_helper.cc with GANDIVA_UNIT_TEST=1 (which exposes them | ||
| # as linkable symbols). Only built when ARROW_BUILD_BENCHMARKS is ON. |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit b38b5c5. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 55 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Gandiva's REPLACE hardcodes a 65535-byte output buffer, throwing Buffer overflow for output string whenever the result exceeds 64 KB. The cap is arbitrary: Gandiva's variable-length output column already grows dynamically and is only bounded by the int32 offset width (~2 GB). Real queries that replace into large concatenated/aggregated strings fail unnecessarily.
What changes are included in this PR
replace_utf8_utf8_utf8 now sizes the output buffer to the exact result instead of using a fixed cap. The output length of a replace is deterministic:
out_len = text_len + num_matches * (to_str_len - from_str_len)
The wrapper does a single counting pass over the input to find the number of non-overlapping matches of from_str (mirroring the match loop already used in the implementation), computes the exact size in gdv_int64 to avoid intermediate overflow, and passes that as max_length.
The internal replace_with_max_len_utf8_utf8_utf8 is unchanged — its bounds checks now act purely as a correctness backstop (they should never fire with an exact bound), and its explicit-max-length signature remains for the existing unit tests.
When to is shorter than from, the result shrinks and max_length <= text_len, so the shrinking path is sized correctly too.
Are these changes tested?
Yes. Added regression cases to TestStringOps.TestReplace in string_ops_test.cc:
A 35000-char 'X' input with X → XY, producing a 70000-byte result (previously overflowed at 65535) — asserts no error and exact length/content.
A 70000-char shrinking case (XX → X) to cover the shrink path on a >64 KB input.
Full precompiled suite passes locally (132/132), including the existing explicit-max_len overflow tests, which call the internal function directly and are unaffected.
Are there any user-facing changes?
REPLACE now succeeds on results larger than 64 KB instead of erroring. No API or signature changes.