[GLUTEN-12807][CORE] Clean up Spark shims APIs after Spark 3.3 deprecation - #12954
[GLUTEN-12807][CORE] Clean up Spark shims APIs after Spark 3.3 deprecation#12954LuciferYang wants to merge 6 commits into
Conversation
Six SparkShims methods have byte-identical overrides in all four remaining shims. Moved the implementation into the trait and dropped the overrides. createParquetFilters stays in the shims: LegacyBehaviorPolicy sits inside SQLConf on 3.4 and at top level from 3.5 on, so no single import in shims/common satisfies all four versions.
isFinalAdaptivePlan was a one-line wrapper over AdaptiveSparkPlanExec.isFinalPlan; extractExpressionTimestampDiffUnit had a single caller. Both go straight to the call sites rather than into the trait.
…ults abstract getExtendedColumnarPostRules returned List() in every remaining shim, so the method and the register-these-rules blocks in VeloxRuleApi and CHRuleApi were dead. GlutenFormatFactory.getExtendedColumnarPostRule went with them, being their only callee. Ten trait methods carried a default that all four shims override, so the default was only ever reached on 3.3. Making them abstract means the compiler requires an implementation when a new shim lands. This settles both remaining "drop Spark 3.3" TODOs in SparkShims.scala.
…alMode Three trait defaults met the same rule as the ten already abstracted but were missed: getCommonPartitionValues, orderPartitions and extractExpressionTimestampAddUnit. The last one is the twin of extractExpressionTimestampDiffUnit, which this branch inlines. withTryEvalMode moves into the trait instead of becoming abstract: its twin withAnsiEvalMode is lifted on the adjacent line with the same shape, and the imports it needs are already there.
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🟡 Changes recommended
It leaves a now-effectless post-rule injection hook (injectPostRuleFactory / postRuleFactory) and includes an avoidable, unclear error message in updated code that should be cleaned up before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Cleans up the SparkShims API surface after dropping Spark 3.3, consolidating identical shim implementations into the shared trait and removing now-dead shim indirections and rule-registration hooks, without intended behavior changes for Spark 3.4–4.1.
Changes:
- Lifted several byte-identical shim methods into
shims/commonand removed redundant per-version overrides across Spark 3.4/3.5/4.0/4.1. - Inlined a couple of one-callsite / one-line shim wrappers at call sites (
isFinalAdaptivePlan,extractExpressionTimestampDiffUnit). - Removed
getExtendedColumnarPostRulesplumbing and the associated factory reader that had become dead code.
File summaries
| File | Description |
|---|---|
| shims/spark34/src/main/scala/org/apache/gluten/sql/shims/spark34/Spark34Shims.scala | Drops overrides now provided by the shared SparkShims trait (Spark 3.4 shim simplification). |
| shims/spark35/src/main/scala/org/apache/gluten/sql/shims/spark35/Spark35Shims.scala | Drops overrides now provided by the shared SparkShims trait (Spark 3.5 shim simplification). |
| shims/spark40/src/main/scala/org/apache/gluten/sql/shims/spark40/Spark40Shims.scala | Drops overrides now provided by the shared SparkShims trait (Spark 4.0 shim simplification). |
| shims/spark41/src/main/scala/org/apache/gluten/sql/shims/spark41/Spark41Shims.scala | Drops overrides now provided by the shared SparkShims trait (Spark 4.1 shim simplification). |
| shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala | Centralizes previously duplicated shim implementations; makes several previously-defaulted methods abstract to force explicit answers in future shims. |
| shims/common/src/main/scala/org/apache/gluten/execution/datasource/GlutenFormatWriterInjects.scala | Removes the (now-dead) extended post-rule reader from GlutenFormatFactory. |
| gluten-substrait/src/main/scala/org/apache/spark/sql/execution/GlutenImplicits.scala | Inlines final-AQE detection to AdaptiveSparkPlanExec.isFinalPlan. |
| gluten-substrait/src/main/scala/org/apache/spark/sql/execution/GenerateTransformStageId.scala | Inlines final-AQE detection and removes shim-loader indirection. |
| backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala | Inlines TimestampDiff unit extraction at the only call site. |
| backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxRuleApi.scala | Removes injection of extended columnar post rules (previously always empty for supported Spark versions). |
| backends-clickhouse/src/main/scala/org/apache/gluten/backendsapi/clickhouse/CHRuleApi.scala | Removes injection of extended columnar post rules (previously always empty for supported Spark versions). |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def injectPostRuleFactory(factory: SparkSession => Rule[SparkPlan]): Unit = { | ||
| postRuleFactory = factory | ||
| } |
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🟢 Approval recommended
The refactor cleanly removes dead shim APIs and deduplicates identical implementations, and repository-wide references to deleted members appear fully updated.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@jackylee-ch Could you please re‑trigger the failed tasks? Thanks |
|
Thanks. Fixed the first one; the second is real but I would rather not do it here, and the reason is a cascade I did not expect. Error message. Dropped the pointless
I tried removing the setter and both registrations, and the diff does not stop there. So it stays as is here, and the follow-up PR that clears the residual 3.3 version checks takes the path as a unit. It already owns the |
|
|
||
| def runtimeReplaceableExpressionMappings: Seq[Sig] | ||
|
|
||
| def generateFileScanRDD( |
There was a problem hiding this 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/.../expression/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.
There was a problem hiding this comment.
You are right about the two pass-throughs, and the reason I put in #12953 §2 does not hold: it says the callers sit outside shims/, but isFinalAdaptivePlan's callers were outside too (GenerateTransformStageId, GlutenImplicits) and I inlined it anyway. The real line is which modules. Those two are in gluten-substrait, already in this diff, whereas getBatchScanExecTable / getKeyGroupedPartitioning reach into gluten-iceberg and gluten-paimon, which this PR does not touch and which are profile-gated. I would rather widen into two extra optional modules in the follow-up than here, and I will fix the issue's wording. Your visibility point checks out on 3.4/3.5/4.0/4.1 in both source and bytecode; one note for whoever does it, keyGroupedPartitioning is a constructor val on 3.4 but a def over spjParams on 3.5+, so reads port cleanly and a copy(keyGroupedPartitioning = ...) would not. §2 lists generatePartitionedFile as a third of the same shape.
generateFileScanRDD: the dead-caller half is right, CHAggAndShuffleBenchmark.scala:334 is the only reference in the tree. It is not the same case otherwise, though. It has a real body, and the four shim copies were byte-identical before this PR, so lifting was de-duplication rather than a choice against inlining. It did bridge a difference until recently: 3.2 took the 3-arg constructor, and 3.3 passed metadataColumns where 3.4+ passes fileConstantMetadataColumns. Whether a shim method should survive for one benchmark caller is a fair question, so I will add it to #12953.
ExpressionUtils: the file is there, UnaryExpressionTransformer is in that package, and one of the two callers sits in it, so the move is cheap; all four call sites are in modules this diff already touches. I still think the shim is the right home, and the reason is the history. The 3.3 shim implemented withAnsiEvalMode as case c: Cast => c.ansiEnabled and carried no withTryEvalMode at all, because EvalMode does not exist before 3.4. These two predicates are about a Spark API whose shape has already diverged by version once; being uniform across 3.4 to 4.1 today is not the same as being version-independent, and ExpressionUtils would have to hand them back the next time evalMode moves.
On the precedent itself, #11687 is not quite that shape: it did not move genDecimalRoundExpressionOutput into SparkPlanExecApi, because that default was already there byte-identical. What the commit deleted was the duplicate in shims/common plus a ClickHouse override that only forwarded to it, and its stated reason was that the shim API existed for a 3.2-vs-later difference that no longer does, so the implementation goes back to the caller side. That reasoning does support pulling a method out of the shim once the difference is gone; it just is not an example of an *Utils object as the landing spot, and here the difference is gone only as of 3.3's removal. If you read the tradeoff the other way I will move them, but that is why I left them.
There was a problem hiding this comment.
I posted some comments before being aware of this comment. I would recommend to do the further refactor to remove those shim APIs if their implementations are consistent across the supported Spark versions. We can move them to the caller side or a proper module (if the shim API implementation is a bit complex and they are called from two or more places) for meeting the dependency requirement.
There was a problem hiding this comment.
Since the s prefix came off the TimestampDiff copy 11 lines below, the TimestampAdd copy here is now the odd one out — same for CHSparkPlanExecApi.scala:1059. Worth dropping this one at least, it is in a file the PR already touches.
There was a problem hiding this comment.
Fair, and it's my own change that created the mismatch. Dropped the s on the TimestampAdd copy at :1460 as well.
Left CHSparkPlanExecApi.scala:1059 as it is, since this PR does not touch that file; the only ClickHouse file in the diff is CHRuleApi.scala.
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🔵 Needs a closer look
It leaves behind newly dead/inert post-rule injection state (postRuleFactory) and introduces avoidable overhead in a hot-path metadata helper that should be addressed before merging.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:1460
- The exception message here is ungrammatical and the nearby comment still references Spark 3.3 even though Spark 3.3 support has been removed in this PR series. Updating both improves debuggability and keeps the comment accurate.
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:187 generateMetadataColumnsdoes non-trivial work (building a mutable map andnew Path(...)) even whenmetadataColumnNamesis empty (the default). Adding a fast-path for the empty case and lazily constructingPathavoids unnecessary per-call overhead.
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:1474
- The thrown exception message is ungrammatical; consider aligning it with typical Spark/Gluten wording to make logs clearer.
val unit = original match {
case timestampDiff: TimestampDiff => timestampDiff.unit
case _ =>
throw new UnsupportedOperationException("Not support expression TimestampDiff.")
}
shims/common/src/main/scala/org/apache/gluten/execution/datasource/GlutenFormatWriterInjects.scala:77
- After removing
getExtendedColumnarPostRule,postRuleFactoryis now write-only andinjectPostRuleFactory(...)has no effect (no reads remain). This leaves inert initialization code at call sites (e.g., listener APIs) and a misleading API surface; consider removing the field/method and updating the callers in the same PR to avoid dead code.
def injectPostRuleFactory(factory: SparkSession => Rule[SparkPlan]): Unit = {
postRuleFactory = factory
}
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
philo-he
left a comment
philo-he
left a comment
There was a problem hiding this comment.
Thanks for the work. Please check if my comments make sense.
|
|
||
| // Spark3.4 new add table parameter in BatchScanExec. | ||
| def getBatchScanExecTable(batchScan: BatchScanExec): Table | ||
| def getBatchScanExecTable(batchScan: BatchScanExec): Table = batchScan.table |
There was a problem hiding this comment.
Should this shim API be removed? Then, directly call batchScan.table on the caller side.
| } | ||
|
|
||
| def withAnsiEvalMode(expr: Expression): Boolean = false | ||
| def withAnsiEvalMode(expr: Expression): Boolean = { |
There was a problem hiding this comment.
It seems both withAnsiEvalMode and withTryEvalMode can be moved from this shim class, since no divergence among the supported Spark versions with Spark 3.3 removed. Perhaps, it would be better to move them to an existing or new util class.
|
|
||
| def getKeyGroupedPartitioning(batchScan: BatchScanExec): Option[Seq[Expression]] = Option(Seq()) | ||
| def getKeyGroupedPartitioning(batchScan: BatchScanExec): Option[Seq[Expression]] = { | ||
| batchScan.keyGroupedPartitioning |
There was a problem hiding this comment.
It looks like this method can be removed, and let batchScan.keyGroupedPartitioning be directly called on the caller side.
| case _ => | ||
| } | ||
| } | ||
| metadataColumn.toMap |
There was a problem hiding this comment.
Perhaps, it would be better to move this shim API to other existing or new util class.
| // TODO, remove this shim once we drop Spark3.3 and previous | ||
| sc.broadcast(value) | ||
| } | ||
| def broadcastInternal[T: ClassTag](sc: SparkContext, value: T): Broadcast[T] |
There was a problem hiding this comment.
Seems we can remove this shim API also.
| def getLimitAndOffsetFromGlobalLimit(plan: GlobalLimitExec): (Int, Int) | ||
|
|
||
| def getExtendedColumnarPostRules(): List[SparkSession => Rule[SparkPlan]] | ||
| def getLimitAndOffsetFromTopK(plan: TakeOrderedAndProjectExec): (Int, Int) |
There was a problem hiding this comment.
It seems that the above two shims APIs can be removed now. And the implementation for them are consistent for Spark 3.4 and later versions now. Consider to use their implementations on the caller side.
| length: Long, | ||
| @transient locations: Array[String] = Array.empty): PartitionedFile | ||
| @transient locations: Array[String] = Array.empty): PartitionedFile = | ||
| PartitionedFile(partitionValues, SparkPath.fromPathString(filePath), start, length, locations) |
There was a problem hiding this comment.
Ditto to directly call the implementation on the call side and remove this shim API.
| new StructType( | ||
| fileSourceScanExec.requiredSchema.fields ++ | ||
| fileSourceScanExec.relation.partitionSchema.fields), | ||
| fileSourceScanExec.fileConstantMetadataColumns |
There was a problem hiding this comment.
Ditto to directly use the implementation on the caller side or move to a util class or method if it is called from two or more places.
|
|
||
| def runtimeReplaceableExpressionMappings: Seq[Sig] | ||
|
|
||
| def generateFileScanRDD( |
There was a problem hiding this comment.
I posted some comments before being aware of this comment. I would recommend to do the further refactor to remove those shim APIs if their implementations are consistent across the supported Spark versions. We can move them to the caller side or a proper module (if the shim API implementation is a bit complex and they are called from two or more places) for meeting the dependency requirement.
What changes are proposed in this pull request?
With Spark 3.3 gone, a batch of
SparkShimsmembers no longer varies by version. Four changes, none of them meant to alter behaviour on 3.4 through 4.1:generateFileScanRDD,generatePartitionedFile,getBatchScanExecTable,generateMetadataColumns,getKeyGroupedPartitioning,withAnsiEvalMode,withTryEvalMode.generateMetadataColumnsused to start by callingsuper, so its two halves are merged into one body.isFinalAdaptivePlanwas a one-line wrapper overAdaptiveSparkPlanExec.isFinalPlan, andextractExpressionTimestampDiffUnithad a single caller.getBatchScanExecTableisbatchScan.table,getKeyGroupedPartitioningisbatchScan.keyGroupedPartitioning, and both fields are public on 3.4 through 4.1. They stay because their call sites reach intogluten-icebergandgluten-paimon, which this PR does not otherwise touch. [CORE] Further SparkShims dedup left out of the Spark 3.3 shim cleanup #12953 §2 records them together withgeneratePartitionedFile, the third of the same shape.getExtendedColumnarPostRulesis deleted. It returnedList()in every remaining shim, so the register-these-rules blocks inVeloxRuleApiandCHRuleApiand their only callee,GlutenFormatFactory.getExtendedColumnarPostRule, were dead.drop Spark 3.3TODOs inSparkShims.scala.Each shim loses ten overrides: 37 to 27, 44 to 34, 51 to 41, 53 to 43. After the change no concrete default left in the trait is overridden by all four shims.
createParquetFilterslooks liftable and is not:LegacyBehaviorPolicysits insideSQLConfon 3.4 and at top level from 3.5 on, so no single import inshims/commoncompiles against all four versions. It becomes liftable once 3.4 is dropped. #12953 tracks the seven abstract methods whose four bodies are also identical, plus two smaller candidates.shims/commonis a published artifact, so this is source and binary incompatible 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.One thing to read correctly rather than as a regression.
postRuleFactoryinGlutenFormatWriterInjectsis now written and never read, and the ClickHouse registration ofNativeWritePostRulehas been inert since 3.4 became the floor, becausegetExtendedColumnarPostRulesalready returnedList()there. Deleting the reader makes that visible instead of causing it. The two registrations belong to the follow-up PR that clears the residual version checks, which already owns the Velox one.How was this patch tested?
Compile-only. The lifted methods keep their signatures, so callers are unchanged apart from the two inlined ones.
install -Pbackends-velox -Pspark-ut -Pdeltaon 3.4, 3.5, 4.0, 4.1install -Pbackends-clickhouse -Pspark-3.5 -Pdelta -Picebergspotless:checkon the touched modules under all four Spark profiles, plusbackends-clickhouse.github/workflows/util/check.shagainst mainThe ClickHouse build is the one that matters for
generateMetadataColumnsand theCHRuleApiedit. Spark 3.5 was built with-Pscala-2.13because this machine's 2.12 artifacts for 3.5.5 are unusable; CI covers 3.5 on 2.12.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-5
Related issue: #12807