LuciferYang commented on code in PR #12954: URL: https://github.com/apache/gluten/pull/12954#discussion_r3931960851
########## shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala: ########## Review Comment: You are right about the two pass-throughs, and the reason I put in #12953 §2 does not hold: it says the callers sit outside `shims/`, but `isFinalAdaptivePlan`'s callers were outside too (`GenerateTransformStageId`, `GlutenImplicits`) and I inlined it anyway. The real line is which modules. Those two are in `gluten-substrait`, already in this diff, whereas `getBatchScanExecTable` / `getKeyGroupedPartitioning` reach into `gluten-iceberg` and `gluten-paimon`, which this PR does not touch and which are profile-gated. I would rather widen into two extra optional modules in the follow-up than here, and I will fix the issue's wording. Your visibility point checks out on 3.4/3.5/4.0/4.1 in both source and bytecode; one note for whoever does it, `keyGroupedPartitioning` is a constructor `val` on 3.4 but a `def` over `spjParams` on 3.5+, so reads port cleanly and a `copy(keyGroupedPartitioning = ...)` would not. §2 lists `generatePartitionedFile` as a third of the same shape. `generateFileScanRDD`: the dead-caller half is right, `CHAggAndShuffleBenchmark.scala:334` is the only reference in the tree. It is not the same case otherwise, though. It has a real body, and the four shim copies were byte-identical before this PR, so lifting was de-duplication rather than a choice against inlining. It did bridge a difference until recently: 3.2 took the 3-arg constructor, and 3.3 passed `metadataColumns` where 3.4+ passes `fileConstantMetadataColumns`. Whether a shim method should survive for one benchmark caller is a fair question, so I will add it to #12953. `ExpressionUtils`: the file is there, `UnaryExpressionTransformer` is in that package, and one of the two callers sits in it, so the move is cheap; all four call sites are in modules this diff already touches. I still think the shim is the right home, and the reason is the history. The 3.3 shim implemented `withAnsiEvalMode` as `case c: Cast => c.ansiEnabled` and carried no `withTryEvalMode` at all, because `EvalMode` does not exist before 3.4. These two predicates are about a Spark API whose shape has already diverged by version once; being uniform across 3.4 to 4.1 today is not the same as being version-independent, and `ExpressionUtils` would have to hand them back the next time `evalMode` moves. On the precedent itself, #11687 is not quite that shape: it did not move `genDecimalRoundExpressionOutput` into `SparkPlanExecApi`, because that default was already there byte-identical. What the commit deleted was the duplicate in `shims/common` plus a ClickHouse override that only forwarded to it, and its stated reason was that the shim API existed for a 3.2-vs-later difference that no longer does, so the implementation goes back to the caller side. That reasoning does support pulling a method out of the shim once the difference is gone; it just is not an example of an `*Utils` object as the landing spot, and here the difference is gone only as of 3.3's removal. If you read the tradeoff the other way I will move them, but that is why I left them. -- 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]
