andygrove commented on issue #4500: URL: https://github.com/apache/datafusion-comet/issues/4500#issuecomment-4835727028
**Items #5 (`ceil`/`floor` with scale = RoundCeil/RoundFloor) and #6 (BRound) — investigated together; no correctness defect, but native support is blocked on adding rounding modes to `spark_round`.** Verified current behavior on Spark 3.5 (parquet-backed columns so the projection can run natively): | Expression | Native today | Matches Spark | | --- | --- | --- | | `ceil(x, scale)` / `floor(x, scale)` (RoundCeil/RoundFloor) | no — always falls back to Spark | yes | | `bround(x, scale)` (BRound, HALF_EVEN) | yes, via `CometCodegenDispatch` (codegen flag on, the default) | yes | | `bround(...)` with `spark.comet.exec.scalaUDF.codegen.enabled=false` | no — falls back | yes | Neither is a correctness defect: RoundCeil/RoundFloor fall back to Spark, and BRound is handled correctly by the codegen dispatcher (which runs Spark's own `doGenCode` in a native kernel). **Blocker for true-native support.** The native `spark_round` (`native/spark-expr/src/math_funcs/round.rs`) implements **HALF_UP only** (the integer/decimal macros round half-away-from-zero; floats delegate to DataFusion's `RoundFunc`, also HALF_UP). So "wire via the same path as `Round`" is not viable as written — that path would silently apply HALF_UP and produce wrong `bround`/`ceil`/`floor` results. Native support requires adding **HALF_EVEN / CEILING / FLOOR** modes to `spark_round` for the integer and decimal paths. Float/Double would stay `Unsupported`, same BigDecimal-via-`toString` reason that already blocks `Round` on float/double (as #6 notes). **Nuance for #6.** BRound via codegen dispatch currently covers *all* numeric types incl. float/double. A pure-native int/decimal impl would gate float/double to fallback unless codegen dispatch is retained for those, i.e. a partial coverage regression for the codegen-on case. **Unified approach when this is picked up.** One native change serves both items: add a `RoundingMode` parameter (HALF_UP / HALF_EVEN / CEILING / FLOOR) to `spark_round` for int+decimal, thread it through proto, then add `CometRoundCeil` / `CometRoundFloor` serdes and optionally switch `CometBRound` to native, all gating float/double + negative-scale decimal as `Unsupported` to mirror `CometRound`. Net: #5 is a clean coverage win (zero native accel today); #6 is marginal (already correct via codegen dispatch). Left as-is for now per investigation; no code change. -- 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]
