apache / apache/gluten

[CORE] Further SparkShims dedup left out of the Spark 3.3 shim cleanup

Open
#12,953 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Scala
Stars
1.6k
Forks
657
Avg merge
2d 21h
Merged PRs (30d)
85

Description

## 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` call `SparkContextUtils` and `GlutenFileFormatWriter`, each of which exists once per shim module under `shims/spark*` and is therefore invisible from `shims/common`. That rules out lifting a body into the trait, but not removal: `gluten-substrait` declares `${sparkshim.artifactId}` at compile scope, so a caller in that module can reference those classes directly, as `BasicPhysicalOperatorTransformer` already does for `SparkContextUtils.createPartitioningAwareUnionRDD`. #12954 removed `broadcastInternal` on that basis. `writeFilesExecuteTask` is the same shape and is still open.

`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 which modules the callers sit in, not that they sit outside `shims/`; `isFinalAdaptivePlan`, which that PR did inline, had its callers outside too (`GenerateTransformStageId` and `GlutenImplicits`, both in `gluten-substrait`). For the first two the callers are `ScanTransformerFactory` in `gluten-substrait`, `IcebergScanTransformer` in `gluten-iceberg` and `PaimonScanTransformer` in `gluten-paimon`, so inlining them would pull that diff into two profile-gated modules it does not otherwise touch. For the third they are `SoftAffinitySuite` / `SoftAffinityWithRDDInfoSuite`.

## 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+.

## 4. `generateFileScanRDD` has no production caller

The only reference in the tree is `backends-clickhouse/src/test/scala/org/apache/spark/sql/execution/benchmarks/CHAggAndShuffleBenchmark.scala:334`. The four shim bodies were byte-identical before #12954 lifted the method into the trait, so that lift was de-duplication rather than a choice against inlining, and the body is a plain five-argument `new FileScanRDD(...)`.

It did bridge a real difference until recently: Spark 3.2 took the three-argument constructor, and 3.3 passed `metadataColumns` where 3.4+ passes `fileConstantMetadataColumns`. Both of those versions are gone.

The question is whether a shim method should exist for a single benchmark call site, or whether the benchmark should construct `FileScanRDD` itself. The first five constructor parameters are identical on 3.4 through 4.1 and the later ones have defaults, so the direct construction compiles on every supported version.

Contributor guide

Open the contributing guide

Research direction

Start with the SparkShims trait and the corresponding Spark34Shims.scala through Spark41Shims.scala files under shims/, then inspect the callers named in gluten-substrait, gluten-iceberg, gluten-paimon, and the SoftAffinity suites. Review the benchmark reference at backends-clickhouse/src/test/scala/org/apache/spark/sql/execution/benchmarks/CHAggAndShuffleBenchmark.scala. Done means the selected deduplication and removal decisions are applied consistently and all supported Spark shim builds compile.

Written by the indexing model from the issue text.

Assessment

Tech stack
scala, spark
Domain
backend, build-system
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.