andygrove opened a new pull request, #5029: URL: https://github.com/apache/datafusion-comet/pull/5029
## Which issue does this PR close? There is no dedicated issue. This is part of the ongoing effort to provide native implementations for expressions that are currently handled through JVM codegen dispatch rather than native execution. Related to the expression coverage epic #240. ## Rationale for this change `pmod` was previously handled by the JVM codegen dispatcher, which keeps the projection on the native pipeline but still evaluates the expression as Spark-generated Java per batch (Arrow to Spark rows to eval to Arrow). It also could not honour ANSI mode: the upstream `datafusion-spark` `pmod` UDF reads `enable_ansi_mode` from the session config, which Comet never sets, so a zero divisor would return NULL instead of raising an error under `spark.sql.ansi.enabled=true`. This PR replaces the dispatch fallback with a native DataFusion implementation that mirrors the existing `Remainder` path, so ANSI behaviour is honoured natively and the per-batch JVM round trip is removed. ## What changes are included in this PR? - New `MathExpr pmod` protobuf message and `PmodBuilder`, mirroring `Remainder`, so the eval mode reaches the native planner. - `spark_pmod` kernel and `create_pmod_expr` in `modulo_expr.rs`. The kernel computes `((left % right) + right) % right`, returning NULL on a zero divisor in legacy mode and raising a remainder-by-zero error in ANSI mode. Wide decimals use a Decimal256 intermediate, matching the modulo path. - `CometPmod` serde now serializes to the new proto message instead of extending `CometCodegenDispatch`. All numeric input types are supported, including decimal. - Audit notes for `pmod` added to the `math_funcs` expression-audit page. The implementation was scaffolded with the `implement-comet-expression` skill and reviewed with the `audit-comet-expression` skill, which confirmed the semantics are identical across Spark 3.4.3, 3.5.8, 4.0.1, and 4.1.1. The 4.1.1 `NumericEvalContext` constructor change and `remainderByZeroError` rename are both handled without a shim. ## How are these changes tested? - New `spark_pmod` Rust unit tests cover integer and decimal inputs, all sign combinations, and zero-divisor handling in both legacy and ANSI modes. - New Comet SQL file tests `pmod.sql` (legacy) and `pmod_ansi.sql` (ANSI) exercise column and literal arguments, every numeric type, negative operands, NaN and Infinity, float precision, INT_MIN and Long boundaries, mixed-scale decimals, the wide-decimal Decimal256 path, legacy NULL-on-zero, and the ANSI zero-divisor error. -- 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]
