parthchandra commented on PR #6455: URL: https://github.com/apache/datafusion-comet/pull/6455#issuecomment-5940740462
Thanks for the depth-1 dispatch fix - the direct-parent case now reads the function's real value. But the detection in `QueryPlanSerde.scala:1176` (`readsDispatchedDsv2Decimal`) only looks one level deep, so a value-preserving decimal node between the dispatched call and a native consumer still returns a wrong result. Put `abs`, unary minus, `coalesce`, `if`, `case`, `least`, or `greatest` between the call and a native decimal consumer, and the intermediate node is the one that gets dispatched. It materializes a DECIMAL vector that is nulled-on-overflow at its own declared type, and the native consumer reads that null - while Spark keeps the over-wide value through those nodes and only overflow-checks at the later CheckOverflow / row write. Concrete repro with `i = 100000000`: - `SELECT abs(decfn.ns.as_money(i)) + 1 FROM t` - Comet dispatches `abs(call)` (its direct child is the call), writes 100000000 into DECIMAL(10,2), changePrecision overflows to null, native `+ 1` reads null -> null. Spark keeps Decimal(100000000), `+ 1` -> 100000001.00, CheckOverflow to DECIMAL(13,2) fits -> 100000001.00. - Same gap on the aggregate side, since the aggregate fallback at `QueryPlanSerde.scala:844` also only checks `fn.children`: `SELECT sum(abs(decfn.ns.as_money(i)))` (or `max(abs(...))`) aggregates the nulled column and diverges from Spark. This is the same cross-boundary composition the existing `"ScalaUDF as a child of a native Spark expression"` test (`CometCodegenSuite.scala:1041`) covers for strings - harmless there because there's no lossy rescale at the boundary, but the decimal rescale-and-null this PR adds is exactly what makes the boundary lossy for the over-wide values #6425 is about. Could you either make the detection transitive (dispatch the maximal decimal subtree that transitively reads such a call through type-preserving decimal nodes as one unit - over-dispatching is safe because the dispatcher is Spark-exact), or restrict/fall back those shapes and document the limitation with a tracking issue? The current depth-1 tests all stop at the direct parent, so `abs(fn(i)) + 1` and `sum(abs(fn(i)))` would be good to add either way. -- 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]
