Skip to content

perf: in-place fast path for ScalarValue::sub / sub_checked - #23203

Closed
Dandandan wants to merge 1 commit into
apache:mainfrom
Dandandan:perf/scalarvalue-sub-fast-path
Closed

perf: in-place fast path for ScalarValue::sub / sub_checked#23203
Dandandan wants to merge 1 commit into
apache:mainfrom
Dandandan:perf/scalarvalue-sub-fast-path

Conversation

@Dandandan

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • Closes #.

Rationale for this change

ScalarValue::add / add_checked already short-circuit the common primitive and decimal cases through an in-place try_add_*_in_place path, avoiding the to_scalar() → Arrow kernel → try_from_array round-trip (≈3 heap allocations + a kernel dispatch per call). sub / sub_checked did not — they always took the slow kernel path.

This matters for window functions: WindowFrameStateRange::calculate_index_of_row calls ScalarValue::sub / sub_checked once per row to compute RANGE frame boundaries for PRECEDING-on-ascending / FOLLOWING-on-descending offsets, so every row in such a partition paid the allocation + dispatch cost. This brings sub to parity with the already-optimized add.

What changes are included in this PR?

  • Add sub_optional, sub_decimal_values, and try_sub_{wrapping,checked}_in_place, mirroring the existing add helpers.
  • Route sub / sub_checked through the in-place fast path, gated by the whitelist (renamed can_use_direct_addcan_use_direct_arithmetic since it now serves both ops), with the Arrow kernel path retained as the fallback for non-whitelisted types.
  • The decimal path stays always-checked (mul_checked + sub_checked) to match the Arrow decimal kernels exactly, regardless of the wrapping vs. checked variant.

No public API or behavior change — sub/sub_checked produce identical results (and identical overflow errors) to the kernel path.

Are these changes tested?

Yes. Existing sub tests (including the exact overflow error messages) continue to pass — the fast path uses the same ArrowNativeTypeOp::sub_checked the Arrow kernel calls internally, so messages are unchanged. Added parity tests covering:

  • primitive wrapping subtraction incl. signed wrap-around and unsigned underflow,
  • checked overflow/underflow (errors) and the no-overflow case,
  • NULL propagation on either operand,
  • decimal scale reconciliation for Decimal128/Decimal256.

Are there any user-facing changes?

No.

`ScalarValue::add`/`add_checked` already short-circuit the common
primitive/decimal cases through an in-place `try_add_*_in_place` path,
avoiding the `to_scalar() -> arrow kernel -> try_from_array` round-trip
(three allocations + a kernel dispatch per call). `sub`/`sub_checked`
did not, so RANGE-mode window frame boundary computation
(`WindowFrameStateRange::calculate_index_of_row`), which calls
`ScalarValue::sub`/`sub_checked` once per row for PRECEDING-on-ascending
/ FOLLOWING-on-descending offsets, paid that cost on every row.

This mirrors the existing add fast path for sub: `sub_optional`,
`sub_decimal_values`, and `try_sub_{wrapping,checked}_in_place`, gated by
the (renamed) `can_use_direct_arithmetic` whitelist, with the kernel path
retained as the fallback. The decimal path stays always-checked
(`mul_checked` + `sub_checked`) to match the Arrow decimal kernels
exactly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions github-actions Bot added the common Related to common crate label Jun 26, 2026
@Dandandan Dandandan closed this Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant