Skip to content

perf: optimize spark_ceil (3x faster)#4926

Open
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:auto-opt/spark_ceil-datafusion-comet-20260714-125829
Open

perf: optimize spark_ceil (3x faster)#4926
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:auto-opt/spark_ceil-datafusion-comet-20260714-125829

Conversation

@andygrove

@andygrove andygrove commented Jul 14, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

N/A

Rationale for this change

Optimize existing expression.

What changes are included in this PR?

Decimal ceil now uses 64-bit hardware division for values/divisors that fit in i64 instead of the 128-bit division intrinsic, and the per-element rounding closure is monomorphized rather than called through a &dyn Fn vtable.

How are these changes tested?

Existing tests.

Benchmark (criterion):

  • ceil_decimal_18_4: 68.795% faster (base 23908ns -> cand 7460ns)
  • ceil_decimal_38_6: -2.613% faster (base 37323ns -> cand 38298ns)

Full criterion output:

ceil/ceil_decimal_18_4  time:   [7.3997 µs 7.4042 µs 7.4101 µs]
                        change: [−68.905% −68.795% −68.675%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 16 outliers among 100 measurements (16.00%)
  4 (4.00%) high mild
  12 (12.00%) high severe
ceil/ceil_decimal_38_6  time:   [38.243 µs 38.268 µs 38.302 µs]
                        change: [+2.4580% +2.6128% +2.7361%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high severe

@andygrove andygrove changed the title perf: optimize spark_ceil in datafusion-comet-spark-expr perf: optimize spark_ceil (3x faster) Jul 15, 2026
@andygrove andygrove marked this pull request as ready for review July 15, 2026 14:23

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass, thanks @andygrove!

_ => div_ceil(x, div),
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ceil.rs:97-254 tests only small unscaled values (max 12999), which all take the new i64 fast path. The entire point of this PR is the branch at ceil.rs:85, and the fallback arm (_ => div_ceil(x, div) at ceil.rs:93) has zero direct coverage. A regression that broke only the wide path would pass CI.

Suggested change: add an array test with a Decimal128(38, s) input whose unscaled values exceed i64::MAX (positive and negative), asserting the ceil result. Example: unscaled 20_000_000_000_000_000_000 (> i64::MAX) at scale 6 targeting Decimal128(38, 0), plus a negative sibling and an exact multiple, so both the fast and fallback arms plus the negative-remainder branch are covered in one suite.

precision: u8,
scale: i8,
f: &dyn Fn(i128) -> i128,
f: impl Fn(i128) -> i128,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utils.rs:60 now takes f: impl Fn(i128) -> i128, but the array call site at ceil.rs:51 still passes &f. This compiles because &F: Fn via the blanket impl, so F monomorphizes to &closure and dispatch is static, but it is now a stray reference that obscures the intent of the change and adds a pointer indirection the PR was trying to remove.

Suggested change: pass the closure by value at ceil.rs:51: make_decimal_array(array, precision, scale, f).

a: &Option<i128>,
precision: u8,
scale: i8,
f: &dyn Fn(i128) -> i128,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utils.rs:49 still declares f: &dyn Fn(i128) -> i128 and ceil.rs:68 passes &f. The PR's stated rationale is "the per-element rounding closure is monomorphized rather than called through a &dyn Fn vtable," but that only happened for the array path. The scalar path is a single element so the perf cost is irrelevant, however the two helpers now have inconsistent signatures for no reason, which is a maintenance trap.

Suggested change: change make_decimal_scalar at utils.rs:49 to f: impl Fn(i128) -> i128 and pass f by value at ceil.rs:68, matching make_decimal_array. This keeps the two helpers symmetric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants