LuciferYang opened a new pull request, #12954:
URL: https://github.com/apache/gluten/pull/12954
## What changes are proposed in this pull request?
With Spark 3.3 gone, a group of `SparkShims` members no longer varies by
version. This collapses them four ways. Nothing here is meant to change
behaviour on 3.4 through 4.1.
Seven methods had byte-identical overrides in all four remaining shims, so
the implementation moved into the trait and the overrides went away:
`generateFileScanRDD`, `generatePartitionedFile`, `getBatchScanExecTable`,
`generateMetadataColumns`, `getKeyGroupedPartitioning`, `withAnsiEvalMode` and
`withTryEvalMode`. `generateMetadataColumns` is the one that needed care: its
override began by calling `super.generateMetadataColumns(...)`, and there is no
`super` to call from the trait itself, so the two halves are merged into one
body that computes the same three base entries and then applies the same loop.
Two others went to their call sites rather than into the trait, because the
indirection cost more than it saved. `isFinalAdaptivePlan` was a one-line
wrapper over `AdaptiveSparkPlanExec.isFinalPlan` with two callers, both already
in `org.apache.spark.sql.execution`. `extractExpressionTimestampDiffUnit` had a
single caller, `VeloxSparkPlanExecApi.genTimestampDiffTransformer`, which now
matches on `TimestampDiff` directly and throws from the `case _` branch instead
of testing an `Option` for emptiness.
`getExtendedColumnarPostRules` is deleted outright. Spark 3.3 was the only
shim that returned a non-empty list; 3.4 through 4.1 all return `List()`, so
with 3.3 gone the method was constant-empty and the register-these-rules blocks
in `VeloxRuleApi` and `CHRuleApi` could never register anything.
`GlutenFormatFactory.getExtendedColumnarPostRule` goes with them, having been
their only callee.
Twelve trait methods carried a default implementation that all four shims
override, which means the default was only ever reached on 3.3. Those become
abstract, so the compiler will ask for an implementation when a new shim lands
instead of silently handing it a value that does not apply:
`broadcastInternal`, `enableNativeWriteFilesByDefault`,
`extractExpressionArrayInsert`, `extractExpressionTimestampAddUnit`,
`getCollectLimitOffset`, `getCommonPartitionValues`,
`getLimitAndOffsetFromGlobalLimit`, `getLimitAndOffsetFromTopK`,
`getV1WriteRequiredOrdering`, `orderPartitions`, `unBase64FunctionFailsOnError`
and `writeFilesExecuteTask`. This settles both remaining `drop Spark 3.3` TODOs
in `SparkShims.scala`, on `getV1WriteRequiredOrdering` and on
`broadcastInternal`.
The twelve are every method that fits the rule, which is worth stating
because it is easy to catch only some of them. The rule is: a concrete default
in the trait, plus an override in all four shims, so the default can only ever
be reached by a shim that forgets one. After this change no concrete default in
the trait is overridden by all four shims; the twenty-two that remain are
overridden by nobody, or by one, two or three of them, and each of those still
needs its default.
Each shim ends up with ten fewer overrides: 37 to 27 in `Spark34Shims`, 44
to 34 in `Spark35Shims`, 51 to 41 in `Spark40Shims`, 53 to 43 in `Spark41Shims`.
### What stayed, and why
`createParquetFilters` looks liftable and is not. All four bodies are
identical, but the `LegacyBehaviorPolicy` they name is not the same class:
| Spark | where `LegacyBehaviorPolicy` lives |
|-|-|
| 3.4.4 | `org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy`, in
`spark-catalyst` |
| 3.5.5, 4.0.2, 4.1.1 |
`org.apache.spark.sql.internal.LegacyBehaviorPolicy`, in `spark-sql-api` |
Neither form exists in the other versions, so no single import in
`shims/common` compiles against all four. Falling back to `RebaseSpec()` does
not help either: on 3.4.4 the class has an `apply$default$2` for `timeZone` and
no default for `mode`. It becomes liftable once 3.4 is dropped.
`getShuffleBlockFetcherIterator` is untouched for the reason it always was:
its body constructs `GlutenShuffleBlockFetcherIterator`, which exists once per
shim module under `org.apache.spark.storage` with the same name and constructor
but different bodies, and `shims/common` cannot see it. `broadcastInternal` has
the same shape, so it became abstract rather than concrete: `SparkContextUtils`
is also per-shim.
Nine of the twelve abstracted methods also have byte-identical bodies in all
four shims, so lifting them was an option too, and I did that only for
`withTryEvalMode`, whose twin `withAnsiEvalMode` is lifted here and sits on the
adjacent line with the same shape and the same imports. Leaving the other eight
abstract is the design choice this series started from: for a value that
genuinely differed by version, making a new shim state an answer is worth more
than saving the duplication. #12953 records the seven where that choice is
still open, along with two other candidates this PR leaves alone.
`broadcastInternal` and `writeFilesExecuteTask` could not have been lifted in
any case, since `SparkContextUtils` and `GlutenFileFormatWriter` exist once per
shim module.
One loose end is worth naming, and it predates this PR rather than being
created by it. `postRuleFactory` in `GlutenFormatWriterInjects` is registered
by `CHListenerApi` unconditionally and by `VeloxListenerApi` inside `if
(SparkShimLoader.getSparkVersion.startsWith("3.3"))`, and the only path from
there to `injector.injectPost` ran through `getExtendedColumnarPostRules`,
which has returned `List()` on every supported shim since 3.3 left. So the
ClickHouse registration has been inert for as long as 3.4 has been the floor;
deleting the reader here makes that visible rather than causing it. The
registrations and the `NativeWritePostRule` they name belong to the follow-up
PR that clears the residual version checks, which already owns the Velox guard.
`shims/common` is a published artifact (`gluten-shims-common`) and both
`trait SparkShims` and `object GlutenFormatFactory` are public, so this is a
source and binary incompatible change for anything outside the repo: four
public members are gone and twelve trait methods no longer have a default.
Every in-repo caller and implementor is accounted for, so the build is
unaffected, but an out-of-tree shim would need the twelve implemented, and a
pre-compiled third-party shim jar would fail at load with `AbstractMethodError`.
## How was this patch tested?
Compile-only. No test was added or changed, and the shim methods keep their
signatures, so the callers are unchanged apart from the two inlined ones.
| build | result |
|-|-|
| `install -Pbackends-velox -Pspark-ut -Pdelta` on 3.4, 3.5, 4.0, 4.1 | all
four exit 0 |
| `install -Pbackends-clickhouse -Pspark-3.5 -Pdelta -Piceberg` | exit 0 |
| `spotless:check` for the touched modules under each of the four Spark
profiles, plus `backends-clickhouse` | clean |
| `.github/workflows/util/check.sh` against main | OK for all 11 changed
files |
The ClickHouse build is the one that matters for `generateMetadataColumns`
and for the `CHRuleApi` edit, since `CHIteratorApi` is a caller of the first.
The whole matrix ran twice, once for the first pass and again after a
self-review added the three missing abstractions and moved `withTryEvalMode`
into the trait.
Two things worth naming about the local runs rather than leaving them
implicit. Spark 3.5 was built with `-Pscala-2.13` because this machine's 2.12
artifacts for 3.5.5 are unusable; CI covers 3.5 on 2.12. And the compiler
settings here treat an unused import as an error, not a warning, which is how
the leftover imports in all five files were found rather than left behind.
Before lifting anything I checked that every symbol the moved bodies name
resolves to the same fully qualified class in 3.4.4, 3.5.5, 4.0.2 and 4.1.1, by
listing class entries in the published jars: `EvalMode`, `IntegralDivide`,
`SparkPath`, `TimestampFormatter` and `RebaseSpec` all do.
`LegacyBehaviorPolicy` was the only one that does not, which is what excluded
`createParquetFilters`.
## Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-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]
