Skip to content

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

Description

@LuciferYang

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

  // 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions