jackylee-ch commented on code in PR #12954:
URL: https://github.com/apache/gluten/pull/12954#discussion_r3922391461
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
Review Comment:
The lift/inline split reads well for five of the seven, but two look like
they fail the rule the PR itself states ("where the indirection cost more than
it saved"): `getBatchScanExecTable` is `batchScan.table` and
`getKeyGroupedPartitioning` is `batchScan.keyGroupedPartitioning` — the same
one-line-wrapper shape, and roughly the same call-site count (3 each), as the
`isFinalAdaptivePlan` you deleted. Both are public on 3.4/3.5/4.0/4.1 and the
trait bodies already compile from a non-Spark package, so
`ScanTransformerFactory.scala`:48-49, `IcebergScanTransformer.scala`:359-360
and `PaimonScanTransformer.scala`:224-225 could read them directly. #12953 §2
already records this, so mainly two follow-up questions: is
`generateFileScanRDD` meant to be in that list too? It has zero production
callers — the only reference in the tree is
`backends-clickhouse/src/test/.../CHAggAndShuffleBenchmark.scala`:334. And
should `withTryEvalMode`/`withAnsiEvalMode` land in `gluten-substrait/.../ex
pression/ExpressionUtils.scala` instead, the way #11687 relocated
`genDecimalRoundExpressionOutput` to SparkPlanExecApi? They are shared logic
rather than version logic, and `UnaryExpressionTransformer` is already in that
package.
--
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]
