LuciferYang commented on issue #12991: URL: https://github.com/apache/gluten/issues/12991#issuecomment-5645182229
Looked into it. Everything here holds, and the removal is up as #13002. What I checked beyond what the issue says. The strongest argument for the three rewrites being identity does not need Spark's behaviour at all: since 3.4 `PromotePrecision` is Gluten's own class, and nothing in the repository constructs it. The only `withNewChildren` on it sits inside `rescaleCastForOneSide`, which is reachable only from a match on `PromotePrecision` itself, so the rewrites cannot fire on any supported version regardless of what `DecimalPrecision` does. For the `CheckOverflow(BinaryArithmetic, ...)` case I checked the construction sites on v3.4.4, v3.5.5, v4.0.2 and v4.1.1: there are three, all in `SerializerBuildHelper`, where an encoder converts a JVM `BigDecimal` through a `StaticInvoke`. Nothing wraps it around an arithmetic node. On the first thing to watch, the explicit rejection stays. I did try the fallback test you suggested and then dropped it: asserting that the plan holds no `ProjectExecTransformer` passes whenever the stage falls back for any reason, and `velox_backend_ansi.yml` runs that suite with ANSI on, where everything falls back, so the test would have gone green while proving nothing. It is a unit test on `getResultType` instead, which pins the rejection directly and runs on all four versions. One correction to the issue text, because it affects where the guard actually bites. `getResultType` has a single caller, and it sits in the branch taken when `transformCheckOverflow` is set. Velox and bolt set it; clickhouse overrides it to false and takes the generic arm in the same `case`, which offloads `decimal % decimal` as a plain `modulus`. So the throw is what keeps `%` and `pmod` off the native path on velox and bolt, not everywhere. Both removed `ExpressionConverter` arms are behind `!transformCheckOverflow`, so that half of the diff is clickhouse-only and I could not exercise it. There is no clickhouse native library on this machine, and `-Pbackends-clickhouse` only compiles on 3.5 at all, since `CHRuleApi` and `src-delta/main` reference Delta 3.3 APIs that only the `spark-3.5` profile selects. A `Run Gluten Clickhouse CI` pass on #13002 would be worth having before it merges. cc @rui-mo @jiangjiangtian -- 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]
