Repository navigation
fix: return an error instead of aborting in repeat for huge LargeUtf8 results - #26957
Open
shahwazreza wants to merge 2 commits into
Open
shahwazreza wants to merge 2 commits into
shahwazreza wants to merge 2 commits into
Conversation
…f8 results `repeat` capped LargeUtf8 results at i64::MAX bytes, which admits sizes no allocation can satisfy, so the process aborted or panicked instead of returning an error. Cap LargeUtf8 results at u32::MAX bytes in both the scalar and array paths. Closes apache#26948
The per-row cap alone still let many rows add up to an allocation no system can satisfy, which panicked in the Arrow buffer instead of returning an error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
repeatonLargeUtf8aborts the process on a result size no allocation can satisfy #26948.Rationale for this change
repeaton a LargeUtf8 string with a huge count, e.g.repeat(arrow_cast('a', 'LargeUtf8'), 1152921504606846976), aborts the process (scalar case) or panics (array case) instead of returning an error. Utf8 and Utf8View already error abovei32::MAXbytes, but LargeUtf8 was capped ati64::MAX, which is way past anything that can actually be allocated.What changes are included in this PR?
LargeUtf8 now uses a fixed cap of
u32::MAXbytes (LARGE_UTF8_MAX_SIZE). In the array path the cap applies to each value and to the total size of the output, since the builder allocates the whole output up front. My first version only checked each value, so lots of rows could still add up to an allocation that panics. The second commit fixes that.The issue asked whether to use a cap or fallible allocation. I went with a cap because the builder has no fallible way to allocate, and changing that would be a bigger separate change. A cap also behaves the same way Utf8 already does.
u32::MAXmatches DuckDB's string size limit and still lets LargeUtf8 go past the Utf8 limit. Happy to change it, e.g. to Postgres' 1 GB, if it is preferred.This doesn't help when an allocation under the cap succeeds and then fails later because of overcommit, same as the issue says.
What is the testing strategy for this PR?
Unit tests in
repeat.rs:1 << 60(used to abort the test process)1 << 60(used to panic)Are there any user-facing changes?
repeatreturns an error for LargeUtf8 results overu32::MAXbytes instead of trying to allocate them.