0lai0 commented on PR #5050: URL: https://github.com/apache/datafusion-comet/pull/5050#issuecomment-6203234050
@andygrove yes, finishing it, thanks for the nudge. Replied to @comphead's two questions and rewrote the description, which was still listing guards that have since been removed. 1.2.0 works for me. @sunchao thanks, precise catch. **The P2** is fixed with your first option. The reader now takes the unscaled-long path only when `precision <= 18 && scale >= 0`, at all three selection points plus the runtime branch, so a negative scale gets the `BigDecimal`-backed reader. Two notes from reproducing it. First, it is wider than TRY: plain `cast` failed the query rather than returning NULL. ``` try_cast(v as int) comet=[NULL,NULL,NULL] spark=[100,-100,0] cast(v as int) comet=[THREW SparkException] spark=[100,-100,0] ``` Second, the reader bug predates this PR. It is reachable on `main` with no cast guard involved, through any dispatched expression converting a negative-scale decimal to an integral type. A `ScalaUDF` with an `Int` parameter over a materialized `Decimal(10,-1)` column does it. Happy to split the reader fix into its own PR if you would prefer. The other two items are done: the materialized-input regression excludes `CollapseProject` so the decimal really is an input, and the float/double guard is removed since #5684 made those exact for negative scale. **Validation**: debug build, clippy clean, nine suites green with 600 passing and 0 failures, including four codegen suites because the reader change touches every dispatched decimal read. I have not run the full Spark SQL suite locally. This touches the serde, the planner and the dispatcher, so per the CI guide `run-spark-4.1-tests` is the one that would catch a behavior difference. I cannot add labels myself, so could one of you apply it? **One question back**: you noted the native TRY cast is already correct, which I confirmed, while LEGACY and ANSI still fail. That means the guard is scoped too widely and could skip TRY entirely rather than routing it through the dispatcher. Threading `evalMode` into `canCastFromDecimal` plus checking the reverse direction is more than I wanted to add here. Worth doing in this PR, or a follow-up? -- 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]
