LuciferYang opened a new issue, #12953:
URL: https://github.com/apache/gluten/issues/12953
## Context
Reviewing the shim cleanup for #12807 turned up three groups of further
candidates. All three are mechanical and independent of each other, and none
belongs in that PR: two of them re-open a design choice the series already
made, and the third touches call sites outside `shims/`. Filing them here so
the decisions are recorded rather than rediscovered.
## 1. Seven methods are abstract in the trait with four byte-identical
implementations
The cleanup turned twelve trait defaults into abstract methods, on the
grounds that a default only Spark 3.3 ever reached should not be inherited
silently by a future shim. For nine of the twelve the four shim bodies are
byte-identical, so lifting the body into the trait was equally available. Only
`withTryEvalMode` was lifted, because its twin `withAnsiEvalMode` was being
lifted in the same commit and leaving them different would have been
indefensible.
That leaves seven where the choice is still open:
| method | body |
|-|-|
| `enableNativeWriteFilesByDefault` | `true` |
| `getCollectLimitOffset` | `plan.offset` |
| `unBase64FunctionFailsOnError` | `unBase64.failOnError` |
| `extractExpressionArrayInsert` | needs `ArrayInsert` and `Literal` adding
to the trait's `catalyst.expressions.{...}` selector list |
| `getV1WriteRequiredOrdering` | one call to `V1WritesUtils.getSortOrder`;
that is Spark's own class and the trait's `datasources._` wildcard already
covers it |
| `getLimitAndOffsetFromGlobalLimit` | must move together with the private
`getLimit` helper, itself byte-identical in all four shims |
| `getLimitAndOffsetFromTopK` | same helper |
The tradeoff to settle: abstract makes a new shim state an answer, lifting
removes four copies of the answer. Whichever way it goes, the seven should go
the same way.
`broadcastInternal` and `writeFilesExecuteTask` are the two of the nine that
cannot be lifted at all: they call `SparkContextUtils` and
`GlutenFileFormatWriter`, each of which exists once per shim module under
`shims/spark*` and is therefore invisible from `shims/common`.
`createParquetFilters` is a separate case, already noted in the PR: its four
bodies are identical too, but `LegacyBehaviorPolicy` lives inside `SQLConf` on
3.4 and at top level from 3.5 on, so no single import in `shims/common`
compiles against all four versions. It becomes liftable when 3.4 is dropped.
## 2. Three lifted methods are now pass-throughs with no override anywhere
After the cleanup these three have a body in the trait and no override in
any shim:
- `getBatchScanExecTable`, which is `batchScan.table`
- `getKeyGroupedPartitioning`, which is `batchScan.keyGroupedPartitioning`
- `generatePartitionedFile`, which is one `PartitionedFile(...)` construction
The argument the same PR used to delete `isFinalAdaptivePlan` (a one-line
wrapper is not worth an indirection) applies here as well: the callers could
read the field directly and the shim methods could go. The reason it was not
done there is that the callers sit outside `shims/`: `IcebergScanTransformer`,
`PaimonScanTransformer` and `ScanTransformerFactory` for the first two, and
`SoftAffinitySuite` / `SoftAffinityWithRDDInfoSuite` for the third.
## 3. A stale JIRA link, four copies
```scala
// https://issues.apache.org/jira/browse/SPARK-40400
private def invalidBucketFile(path: String): Throwable = {
```
`shims/spark34/.../Spark34Shims.scala:104` and the 35/40/41 equivalents.
SPARK-40400 ("Pass error message parameters to exceptions as a map") landed in
3.4.0, and the deleted `Spark33Shims` had the same helper with the pre-3.4
signature and no comment. The link only ever marked why the 3.4+ copy diverged
from the 3.3 one, so with 3.3 gone it explains nothing. The helper itself is
identical in all four and is another lift candidate, though its caller
`filesGroupedToBuckets` still differs between 3.4 and 3.5+.
--
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]
