perf: avoid repeated decimal promotion in expression serialization

[peterxcli]

Which issue does this PR close?

Closes #5248.

Rationale for this change

exprToProto promotes the complete decimal expression tree before serialization. Array and bitwise serdes re-entered that public method for children of the promoted tree, repeating traversal and allocation. Promotion is already idempotent, but that does not eliminate the redundant work.

What changes are included in this PR?

• Route 40 child, literal, and non-arithmetic wrapper calls in array and bitwise serdes through exprToProtoInternal, including the casts in slice and null guard in array_join.
• Preserve public entry points for aggregate arguments and filters, operator roots, and synthesized arithmetic.
• Document which entry point owns decimal promotion.
• Add a nested-decimal regression covering array and bitwise recursion, aggregate arguments and filters, LEGACY/ANSI/TRY modes, and bound/unbound attributes. It checks that each arithmetic node retains one overflow wrapper with the correct decimal type and error behavior, and verifies the bound index or unbound name. Explicitly re-serialize an already-promoted tree to retain the idempotency regression.
• Add value-level overflow checks for 46 expressions per ANSI mode, using DECIMAL(38,0) multiply and DECIMAL(38,6) divide over native Parquet scans, with codegen dispatch disabled.

How are these changes tested?

• Native build: make core.
• JVM/test compilation and formatting checks: ./mvnw test-compile -DskipTests.
• Focused suites: SPARK_LOCAL_IP=127.0.0.1 ./mvnw test -Dtest=none -Dsuites=org.apache.spark.sql.comet.CometDecimalPromotionSuite,org.apache.comet.CometArrayExpressionSuite — 64 tests passed.
• Mutation check: temporarily bypassing promotion in exprToProto fails three tests, including a wrong LEGACY result and a missing ANSI error. Promotion was restored before the final passing run.

Serialization microbenchmark

Measured before rebasing, against 75fdddc9285ec61c0cd326977c61dd41fca39a8b. The rebase onto 7e1984399 does not change the measured serializer paths. The baseline uses the original arrays.scala and bitwise.scala compiled into a classpath overlay; the patched runs use this change, with the same dependencies and benchmark harness. Class origins were verified in each process.

Spark 4.1.3, Scala 2.13.17, Zulu JDK 21.0.6, macOS aarch64; JVM flags -Xms1g -Xmx1g -XX:ActiveProcessorCount=2. Three JVM runs per version in baseline/patched/patched/baseline/baseline/patched order. Each case warms up for two seconds, then measures seven batches of 1,000 serializations. Results are the median of the three per-process medians, per serialization. Allocations use the current thread's ThreadMXBean counter; a volatile sink consumes the protobuf output.

Every case contains an eight-add decimal chain. Array cases wrap it in CreateArray and the indicated number of Reverse nodes, then ArrayContains. Bitwise cases cast it to integer and nest BitwiseNot. The control serializes only the decimal arithmetic. Inputs are reused; expression construction and query execution are outside the measured region.

Case	Before (µs)	After (µs)	Speedup	Before (bytes)	After (bytes)	Allocation reduction
Decimal control	9.12	8.99	1.01×	83,080	82,824	0.3%
Array depth 1	26.06	20.05	1.30×	212,032	175,536	17.2%
Array depth 8	36.93	23.25	1.59×	311,977	195,064	37.5%
Array depth 32	90.83	37.18	2.44×	671,785	261,304	61.1%
Bitwise depth 8	25.29	14.99	1.69×	217,632	125,976	42.1%
Bitwise depth 32	79.86	29.50	2.71×	626,401	242,328	61.3%

These are synthetic serializer measurements on a shared development machine, not end-to-end SQL speedups. The unchanged control's per-process time medians range from 8.71–10.53 µs before and 8.76–9.29 µs after, so small timing differences should be treated as noise. Small allocation differences in the control can reflect JVM optimization differences.

[andygrove]

I spent some time on this one because a pure serde refactor is exactly the kind of change where reading the diff does not tell you much. Rather than reason about whether promotion is idempotent, I dumped the serialized protobuf for a corpus of 47 expressions across LEGACY/ANSI and bound/unbound, with decimal arithmetic children under every serde you touched plus deep chains and cross-family nesting, and diffed it against your parent commit. It comes out byte-identical once you normalize the codegen-dispatch blobs, which embed a per-JVM random ExprId.jvmId. So I am satisfied the refactor preserves behavior.

I also checked the performance claim independently, since the harness is not in the repo. My absolute numbers are much higher than yours because I timed inside a full test session with a debug libcomet, but the shape matches: the baseline grows superlinearly with nesting depth (array 76 -> 97 -> 211 us, bitwise 65 -> 193) while this branch stays near-linear (68 -> 80 -> 108, 43 -> 69). At depth 32 I measured 1.95x for array and 2.82x for bitwise against your 2.44x and 2.71x. The untouched control moved 0.80-0.90x between runs, so treat anything under roughly 20% as noise, but the complexity change is real.

Comments below. Nothing blocks. The two I care most about are the remaining public calls in arrays.scala and the test comment that got deleted rather than acted on.

[sunchao]

Correctness

Reviewed d3c44512 against 7e198439. I found no remaining P1/P2 issue. The public serializer already promotes the complete expression tree. Re-entering it for array and bitwise children therefore repeated an idempotent traversal. The 40 replacements retain that initial promotion and serialize its children internally, preserving the overflow wrappers, decimal result types, null handling, binding and evaluation modes.

The maintained Spark 3.5 and 4.0 sources confirm that decimal arithmetic uses its computed result precision and scale, with ANSI overflow errors and null-on-overflow behavior outside ANSI. The unchanged Comet promotion and CheckOverflow paths preserve those distinctions here. Aggregate arguments and Partial-mode filters still enter through the public serializer as independent roots. The synthesized Slice casts and ArrayJoin null guard add no decimal arithmetic, and the Atan2 arithmetic already uses Double inputs.

The expanded tests retain explicit re-promotion coverage and check nested wrappers, decimal types, LEGACY/ANSI/TRY flags, and bound/unbound attributes. The value tests exercise overflowing multiply and divide in 46 contexts per ANSI setting over native Parquet scans with codegen dispatch disabled. The idempotency, nested/aggregate and both value tests passed in the inspected Spark 3.5 and Spark 4.0 execution jobs. Both expression jobs, including CometArrayExpressionSuite, also passed.

Those jobs ran merge c41f8773, which includes this head on a later base. All four changed files and the 13 additional reviewed Comet source units have identical blobs there, but the complete tree differs, so this is source-equivalent CI coverage rather than execution of the exact assigned pair. Main CI succeeded. CodeQL's Actions analysis was cancelled and its separate check is neutral. I did not run a local build or tests. Maintained Spark 3.4 and 4.1 source branches were unavailable for comparison.

Performance

The change removes repeated tree walks and the associated temporary expressions while retaining one promotion at each independent root. A static audit found 37 public calls removed from arrays and three from bitwise. After normalizing those entry-point substitutions and the explicit existing binding=true literal default, the executable source is unchanged. This supports the narrow implementation scope without claiming runtime protobuf equivalence from a text audit.

The author reports 1.30x to 2.71x faster serialization and lower allocation for nested array and bitwise cases. I verified that the measured serializer and promotion files at benchmark baseline 75fdddc9 match the assigned base. The reported harness reuses inputs, consumes results, measures allocation, and alternates separate JVM runs. The nearly unchanged decimal control and its process-to-process variation appropriately limit small timing claims. The earlier review independently reports the same direction for deep cases, but I did not reproduce either experiment. These synthetic results do not establish SQL query speedups. Sequence's restricted literal/reference path is a consistency change, not evidence of an additional measured gain.

Design

The existing public-root/internal-child split is a suitable place to own promotion. The revised documentation now explains the aggregate exception directly, and the implementation preserves public calls where arguments and filters have not yet been promoted. The safe synthesized wrappers reuse already-promoted children, so they do not need a second walk.

The current revision also addresses the earlier requests to sweep the remaining calls and strengthen the regression tests. The proposed testing-only guard against future synthesized arithmetic is already discussed as a follow-up. I found no current caller that violates the contract, so that proposal does not block this change.

Abstraction & complexity

This uses existing entry points and adds no cache, traversal framework or production state. The shared value-test matrix complements the focused protobuf assertions without changing runtime behavior. Keeping the distinction between independently promoted roots and already-promoted children explicit is simpler than adding checked/unchecked layers in this patch. I have no additional change request.

[sunchao]

Correctness

Re-reviewed e2897ed9f6b8509127851635165169ed9ee4ad20 against 17f54da8ca5cb0ad5dbe8357b6e037fef8a0db2c, including the merge since the prior approval on d3c44512. No new correctness finding. The merge-specific interaction is ArrayAppend passing the outputs of widenElementInLockstep to the internal serializer: that helper adds only casts around already-promoted children, so nested decimal overflow wrappers remain present. The inherited Slice, ElementAt null guard, and ApplyFunctionExpression registration preserve that entry-point contract. Aggregate arguments and filters still enter through the public method.

I rechecked decimal result types, captured LEGACY/ANSI/TRY modes, overflow/null handling and bottom-up tree rewriting against the maintained Spark 3.5 and 4.0 branches. The promotion rule and native overflow handlers are unchanged from the prior review. Both bound and unbound serialization retain their inputs, and the public root still collects fallback/coverage tags from rewritten descendants. Maintained Spark 3.4 and 4.1 branches remain unavailable, so the CI results below are separate evidence rather than a source-compatibility claim for those branches.

Validation

All five named CometDecimalPromotionSuite tests passed in the current Spark 3.4, 3.5, 4.0, and 4.1 execution jobs, including repeated promotion, nested/aggregate wrappers, and actual decimal overflow values with ANSI off/on. The corresponding expression jobs also passed. The eight inspected consumers and native producer ran merge 1809fd32, with the exact assigned base/head parents and a tracked tree identical to the head. Their native artifact ID and SHA256 match the producer and artifact metadata.

At September 9, 05:31:50 UTC, CI has 57 successful, seven running, seven skipped and one failed check. The macOS shuffle failure is a Maven Central connection reset fetching surefire-junit4:3.5.4, not a reported test assertion. This is a code-review approval, with CI still incomplete. I ran no new local build or runtime test.

Performance

The merge retains all 40 public-to-internal call substitutions. A fresh source audit found no executable difference from the assigned base beyond those substitutions and explicit default binding for literals. Repeated subtree promotion remains removed. The author's reported serializer benchmarks show 1.30–2.44x for nested arrays and 1.69–2.71x for bitwise cases, with reduced allocation and a roughly unchanged bare-decimal control. The measured handler/method bodies remain equivalent across the merge, while unrelated handlers and the dispatch registry advanced. These are attributed synthetic serializer measurements with eight-add decimal chains and increasing wrapper depth, not a new measurement or an end-to-end query speedup. Literal/reference-only Sequence changes provide consistency rather than a decimal performance gain. No new material cost was introduced by the merge.

Design

The existing division of responsibility remains sound: public entry points promote independent roots. Recursive serializers consume those promoted children. The new array widening helper fits that contract because it synthesizes only casts. The retained public aggregate paths and nested-wrapper regression tests make the boundary reviewable. No additional design change is needed for this follow-up.

Abstraction & complexity

No new abstraction is introduced by the PR contribution. The documentation now states the caller's promotion responsibility and covers synthesized arithmetic explicitly. A checked/unchecked API redesign would be a separate change. The merge does not require it, and a blanket assertion that every internal node is already wrapped would incorrectly reject the deliberate child of CometCheckOverflow. All prior discussion threads are resolved. No duplicate inline comment is warranted.

[andygrove]

Checked the latest revision against what I raised earlier rather than taking the thread's word for it. arrays.scala and bitwise.scala have no public exprToProto calls left, so the Slice casts and the ArrayJoin null guard I flagged are both on exprToProtoInternal now, same as the rest of the file. aggExprToProto has a real scaladoc block instead of a plain comment. The test file re-points the shape check at promoted instead of expression, which matters because CometArrayContains no longer double-promotes, so the old target couldn't exercise that path anymore. The new 46-case value-level suite over native Parquet decimal columns is what actually closes the gap between a proto-shape assertion and a wrong answer, and the bound/unbound loop now asserts something different in each branch instead of repeating the same check twice.

The mutation testing in this PR, bypassing promotion and watching the nested-shape and value tests fail, is good evidence that every serde touched here holds to the contract. What's still open is a way to catch the next serde that doesn't. You mentioned an exprToProtoInternalUnchecked prototype that ran clean across 80 tests and caught an injected Add(child, child) violation. Could you file an issue for it? Right now the contract lives in three scaladoc comments, and nothing else points at the idea once this thread closes.

Checked this against my own #5216 as well, since both touch DecimalPrecision.promote. They don't conflict. #5216 makes promote itself cheaper when a tree has no decimal arithmetic anywhere in it. This PR instead removes the call to promote entirely for array and bitwise children that were already promoted at the root. They touch different files with no shared call sites, and #5216 still pays off at every exprToProto entry point this PR leaves alone, aggregate arguments and filters included.

On the performance numbers, I reproduced the depth-scaling shape independently earlier in this review, a superlinear baseline against a near-linear one here, and the calls swept since then (Slice, ArrayJoin, the literal calls) sit outside the benchmarked CreateArray/Reverse/BitwiseNot chains, so the numbers in the description still hold.

[peterxcli]

Thanks all for the review!