0lai0 commented on issue #5095: URL: https://github.com/apache/datafusion-comet/issues/5095#issuecomment-5301084893
Hi @andygrove, following up on this and my earlier comment. I implemented the int arm delegation on a branch and benchmarked it against the current open-coded version at the same call site. The delegation is byte-identical to `cast_int_to_decimal128_internal` in the safe positive-scale case. Verified against `arrow-cast-58.4.0/src/cast/mod.rs:381-394`, which does the same `checked_mul(10^scale)` plus `D::is_valid_decimal_precision` filter and wraps with `with_precision_and_scale`. The ANSI rescan continues to raise the same `SparkError::NumericValueOutOfRange` payload. All 26 numeric tests pass. The performance is a regression on every common shape. **Bench setup** - Comet release profile (`lto = "thin"`, `codegen-units = 1`) - Criterion 100 samples per bench, 5s measurement, single machine, one run - Shapes extended in `native/spark-expr/benches/cast_int_to_decimal.rs`: all non-null, sparse null (every 10, 10%), dense null (every 2, 50%), plus ANSI over an all-overflow batch **Results** (`main` vs delegated branch) | Shape | main | branch | delta | | --- | --- | --- | --- | | i32 → dec(15,4), no nulls | ~5.8 µs | 8.44 µs | +46% | | i32 → dec(15,4), sparse nulls | 7.44 µs | 15.88 µs | +111% | | i32 → dec(15,4), dense nulls | 5.24 µs | 9.33 µs | +81% | | i64 → dec(38,4), no nulls | 5.80 µs | 8.29 µs | +45% | | i32 → dec(15,4), ANSI, no overflow | 5.74 µs | 8.59 µs | +49% | | i64 → dec(15,4), legacy, all overflow | 15.15 µs | 15.60 µs | +2% (noise) | | i64 → dec(15,4), ANSI, all overflow | 14.83 µs | 15.49 µs | +7% | Criterion flagged all seven as `Performance has regressed` at `p < 0.05`. The overflow shapes look near-neutral only because the rescan already dominates them. **Root cause** The `unary_opt` algorithm is the same `checked_mul(10^scale)` plus precision filter. Going through `cast_with_options` changes two things at once, and I did not isolate them: 1. Per-batch dispatch. `cast_with_options` → `cast_to_decimal` → `cast_integer_to_decimal`. The first is a large runtime `(from_type, to_type)` match and is not marked `#[inline]`. `cast_integer_to_decimal` itself is crate-private, so the public API cannot skip that match. 2. A heavier per-row closure. Arrow uses `ArrowNativeTypeOp::mul_checked` (`arrow-array-58.4.0/src/arithmetic.rs:185`), which returns `Result<T, ArrowError>` and then `.ok()`. Comet's version is `i128::checked_mul(...).filter(...)` returning `Option<i128>`. `format!` sits in `ok_or_else`, so it does not run on the success path. I have not looked at assembly, so I do not know how much of the `Result` / `ArrowError` shape survives inlining under thin LTO. `unary_opt` uses a dense `(0..len)` iterator when there are no nulls and `BitIndexIterator` otherwise (`arrow-buffer-58.4.0/src/util/bit_iterator.rs:405-408`). That matches the sparse-null shape being the largest regression, but it does not tell me whether (1) or (2) dominates. Either way, the public Arrow cast API is a regression on the shapes Comet actually runs, so I am keeping the open-coded `cast_int_to_decimal128_internal`. **Related findings from this investigation** - Boolean arm has a pre-existing bug independent of the delegation question. `CAST(true AS DECIMAL(3,3))` returns an unscaled 1000 at type `Decimal128(3, 3)`, whose valid range is `[-999, 999]`. `with_precision_and_scale` at `arrow-array-58.4.0/src/array/primitive_array.rs:1615` only validates metadata, not values, so the current `.map_err` in `cast_boolean_to_decimal` never fires. Filed as #5334. - Float arm remains blocked on #1371 / #5136. **Recommendation** Keep the current open-coded `cast_int_to_decimal128_internal`. With int staying as-is and float blocked, #5095 is boolean-only after #5334, or it can be closed with links to those issues. Leaving the decision -- 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]
