mbutrovich opened a new issue, #99: URL: https://github.com/apache/datafusion-spark/issues/99
## Why this matters These functions divide every value in a batch by one divisor that is known only at run time. For `Decimal128` that is an `i128` division, which compiles to a `__divti3` call even for a literal divisor. apache/arrow-rs#11421 adds precomputed divisors in `arrow-buffer` for this, and a prototype downscale by `10^18` on 38-digit values ran 3.0x faster than the current code. This issue depends on the `i128` divisor in apache/arrow-rs#12167 (PR apache/arrow-rs#12232) reaching an arrow release that datafusion-spark builds against. Comet has its own `ceil` and `floor`, so the gain here goes to other users of the crate and not to Comet. ## Sites | Function | Code | What it does per value | |---|---|---| | `floor` on `Decimal128` | [`floor.rs` L134-L136](https://github.com/apache/datafusion-spark/blob/dd802a821111e14462c2d5ca4692baa6ab464735/crates/spark/src/function/math/floor.rs#L134-L136) | `x / divisor` and `x % divisor` on `i128` | | `ceil` on `Decimal128` | [`ceil.rs` L99-L101](https://github.com/apache/datafusion-spark/blob/dd802a821111e14462c2d5ca4692baa6ab464735/crates/spark/src/function/math/ceil.rs#L99-L101) | `10_i128.pow_wrapping(scale)`, then `/` and `%` | | `round` on integers and decimals | [`round.rs` L228-L232](https://github.com/apache/datafusion-spark/blob/dd802a821111e14462c2d5ca4692baa6ab464735/crates/spark/src/function/math/round.rs#L228-L232) and [L328](https://github.com/apache/datafusion-spark/blob/dd802a821111e14462c2d5ca4692baa6ab464735/crates/spark/src/function/math/round.rs#L328) | `checked_pow` or `pow_checked`, then `/` and `%` by `factor` | | `unix_timestamp` on timestamps | [`unix.rs` L116-L132](https://github.com/apache/datafusion-spark/blob/dd802a821111e14462c2d5ca4692baa6ab464735/crates/spark/src/function/datetime/unix.rs#L116-L132) and [L168-L173](https://github.com/apache/datafusion-spark/blob/dd802a821111e14462c2d5ca4692baa6ab464735/crates/spark/src/function/datetime/unix.rs#L168-L173) | `div_euclid` by 1_000, 1_000_000, or 1_000_000_000, chosen at run time | ## Proposed change The power of ten is the same for every row, so `ceil` and `round` can compute it once per batch with no arrow release. That is a small first PR. After the `i128` divisor is released, build one divisor per batch for `floor`, `ceil`, and `round`, and divide with it. Spark's `floor` and `ceil` need a floor or ceiling quotient, so each applies its sign correction to the divisor's truncated quotient and remainder. For `i128`, the epic asks for a per-batch check that the values fit in `i64`, and a batch that does divides with the `i64` divisor, because the wide divisor was as slow as or slower than `/` there. `unix_timestamp` divides an `i64` by a positive divisor and floors the result. It needs the signed `i64` divisor, which the epic lists as an optional follow-up (draft PR apache/arrow-rs#11979). Leave it out of the first PRs. ## Overlap with open issues apache/datafusion-spark#37 changes the result type of `round` on a `Decimal128`, and apache/datafusion-spark#22 adds the two-argument `ceil(decimal, scale)`. Both edit the code in the table above, so the divisor change for `round` and `ceil` should follow them. Hoisting the power of ten out of the row loop is small enough to land first. ## Checks before a PR - Add sqllogictest cases for negative values, zero, `scale` 0, the maximum precision, nulls, and a result that rounds across a power of ten, so the output matches the current code. - Benchmark through `invoke_with_args` with an 8192-row batch, in the `crates/spark/benches` criterion setup. Cover values that fit in `i64` and values that need all 38 digits, and include a function the change doesn't touch to show the noise floor. -- 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]
