andygrove commented on PR #5050: URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-5441590325
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. A panic in debug and a silent wrong answer in release is exactly the kind of thing worth closing off quickly, and doing it at the serde boundary is a reasonable conservative first move. The comment on `CometMultiply` explaining why it deliberately has no guard is the sort of thing that saves the next person a lot of time. Four questions. **Where does `Unsupported` actually route these casts?** Since #4728, `Unsupported` routes through the JVM codegen dispatcher rather than straight to Spark row execution. The dispatcher builds Arrow vectors for its inputs and outputs. Does `CometBatchKernelCodegen` accept a `DecimalType` with negative scale, and can Arrow's `DecimalVector` represent one? If the dispatcher accepts it and then fails or produces garbage, this fix would move the problem rather than remove it. If the dispatcher does handle it correctly, a test asserting that a negative-scale cast runs through the dispatcher and matches Spark would be worth having. If it does not, these need `withFallbackReason` plus something that keeps them out of the dispatcher too. **The native kernel is still unsafe** Guarding in Scala stops the Spark planner from reaching the kernel, but `10_i128.pow(scale as u32)` with a negative scale is still a live panic in the Rust code, reachable from any other caller: the Iceberg native scan, a UDF, a future serde. Should the kernel also return an error rather than compute `10^4294967295`? Even a `debug_assert!` plus an explicit `Err` would turn a silent wrap in release into something diagnosable. Is there an issue tracking the real fix, so this does not stay a permanent fallback? **Only `BinaryArithmetic` is guarded** `negScaleDecimalRejection` takes a `BinaryArithmetic`, so it covers `Add`, `Subtract`, `Divide`, `IntegralDivide`, and `Remainder`. What about the other places that scale-align decimals? `Round` and `BRound` compute a scale delta, `Sum` and `Average` rescale their buffers, `UnaryMinus` and `Abs` at least touch the type, and `CheckOverflow` / `DecimalRescaleCheckOverflow` do explicit rescaling. Did you audit those for the same `10^|scale|` pattern? If they are safe, saying so in the description would close the question. If they are not, they belong in this PR. **Is the `Multiply` exemption pinned by the right test?** The comment says multiply does not scale-align, so negative scale is safe. `WideDecimalBinaryExpr`'s multiply branch does compute a scale adjustment against the output precision and scale, so I would want the regression test to cover a negative *output* scale specifically, not just negative input scales. Does the test in `CometExpressionSuite` do that? If so, referencing it by name in the comment would make the exemption self-verifying. -- 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]
