peterxcli opened a new pull request, #5736: URL: https://github.com/apache/datafusion-comet/pull/5736
## 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 32 existing-child calls in array and bitwise serdes through `exprToProtoInternal`. - Preserve public entry points for aggregate arguments and filters, operator roots, and synthesized expressions. - 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. ## 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` — 62 tests passed. ### 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
