Repository navigation
fix(functions): prevent panic on ceil and floor for decimal scalar with 0 rows (#26950) - #26955
Open
XiaoMingFa wants to merge 1 commit into
Open
XiaoMingFa wants to merge 1 commit into
XiaoMingFa wants to merge 1 commit into
Conversation
…th 0 rows (apache#26950) When invoking `ceil` or `floor` on a decimal scalar argument, the function converts the scalar into an array to reuse the decimal operator logic, then extracts the resulting scalar from index 0. Previously, it converted using `args.number_rows`. If `args.number_rows` was 0, an empty array of length 0 was created, and reading row 0 panicked with: "Trying to access an element at index 0 from a PrimitiveArray of length 0". This change: - Converts decimal scalars to an array of size 1 via `arg.to_array(1)` instead of `arg.to_array(args.number_rows)` when `is_scalar` is true, matching the architectural design comment and avoiding unnecessary array expansion or empty arrays. - Preserves the array path when `is_scalar` is false. - Adds regression unit tests for both `ceil` and `floor` verifying decimal scalar evaluation with `number_rows: 0`. Closes apache#26950. Signed-off-by: Mingfa Lim <346975989+XiaoMingFa@users.noreply.github.com>
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?
Closes #26950.
Rationale for this change
When evaluating
ceilorflooron a decimal scalar argument, the scalar is converted into an array to reuse the decimal operator and precision/scale calculations, and the resulting scalar is then extracted from index 0.Previously,
arg.to_array(args.number_rows)was used. Whenargs.number_rows == 0, this produced an emptyPrimitiveArrayof length 0, causingScalarValue::try_from_array(&result, 0)to panic:Furthermore, the architectural comment in both functions explicitly noted:
Using
args.number_rowsinstead of 1 defeated this optimization whenargs.number_rows > 1and panicked whenargs.number_rows == 0.What changes are included in this PR?
ceil.rsandfloor.rs, whenis_scalaris true, convert the scalar to an array of size 1 (arg.to_array(1)).arg.to_array(args.number_rows)for array inputs (is_scalar == false).ceilandfloorcovering decimal scalars evaluated withnumber_rows: 0.Are these changes tested?
Yes, new unit tests
test_ceil_decimal_scalar_zero_rowsandtest_floor_decimal_scalar_zero_rowsverify this behavior and pass. All existing math tests continue to pass.Are there any user-facing changes?
No breaking changes. Fixes panics when
ceilorflooris called with decimal scalars under zero rows.