[GLUTEN-12807][CORE] Clean up Spark shims APIs after Spark 3.3 deprecation - #12954
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 | ||
| } |
There was a problem hiding this comment.
Still true, and still deliberate. Nothing reads postRuleFactory now that getExtendedColumnarPostRules is gone.
I re-checked the cascade before leaving it: dropping the setter orphans GlutenWriterColumnarRules.NativeWritePostRule, which orphans the private getNativeFormat, which orphans BackendSettingsApi.skipNativeCtas and skipNativeInsertInto plus their two VeloxBackend overrides. Six files, and two of them are members of a backend-facing trait. VeloxListenerApi still carries an Only register NativeWritePostRule for Spark 3.3 guard right next to the registration, so that whole path belongs to the follow-up that clears the residual 3.3 version checks, and it goes as one piece there rather than half here.
|
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 |
| @@ -89,33 +91,40 @@ trait SparkShims { | |||
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.
Done, all of it. Fifteen methods leave the trait now (64 down to 46): fourteen inlined at the call site, and withTryEvalMode / withAnsiEvalMode / generateMetadataColumns moved into gluten-substrait utils since each has two callers. The PR description has the full list and the per-method reasoning; individual replies are on your comments above.
Two of them look removable and are not, so they stay with a note in #12953: createParquetFilters needs LegacyBehaviorPolicy, which is nested in SQLConf on 3.4 and top-level from 3.5 on, so no single import in a shared module compiles against all four versions; and widerDecimalType forwards to DecimalPrecision on 3.4/3.5 and DecimalPrecisionTypeCoercion on 4.0/4.1. Both become removable when 3.4 goes.
One behavior change worth your eye: ExpressionConverter matched ArrayInsert through getClass.getSimpleName because the class did not exist on 3.3. With the cast now inlined next to it that guard was self-contradictory, so it is a plain type match. A third-party class whose simple name is also ArrayInsert used to enter the arm and fail in the cast; it now falls through.
Verification is compile-only, no suites: substrait including test sources on all four versions, velox on 3.5/4.0/4.1, iceberg on 3.4/3.5, paimon on 3.5, clickhouse on 3.5, and spotless:check on all four. The gaps and why they are gaps are in the description.
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.
There was a problem hiding this comment.
Removed. ScanTransformerFactory, IcebergScanTransformer and PaimonScanTransformer now read batchScan.table directly; the field is a public case-class val on 3.4 through 4.1.
| } | ||
|
|
||
| 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.
There was a problem hiding this comment.
Both moved to gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionUtils.scala, which already holds pure expression predicates, and the two callers (UnaryExpressionTransformer, VeloxSparkPlanExecApi) call it there.
I argued for keeping them in the shim earlier in this thread, on the grounds that EvalMode did not exist before 3.4 so the predicates had already diverged by version once. Your point stands: they do not vary across the versions we support now, and moving them back if evalMode changes again is cheap.
|
|
||
| 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.
There was a problem hiding this comment.
Removed the same way, with the three callers reading batchScan.keyGroupedPartitioning. Worth noting for anyone reading the diff: it is a constructor val on 3.4 and a def over spjParams on 3.5+, so reads port cleanly while a copy(keyGroupedPartitioning = ...) would not. getCommonPartitionValues stays, since 3.4 reads batchScan.commonPartitionValues and 3.5+ reads it off spjParams.
| 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.
There was a problem hiding this comment.
Moved to a new gluten-substrait/src/main/scala/org/apache/gluten/utils/FileMetadataUtil.scala, called from VeloxIteratorApi and CHIteratorApi. The body is unchanged. I went with a new file rather than folding it into an existing util because none of the seven in that package is about per-file metadata: FileIndexUtil takes a FileIndex, PartitionsUtil groups files into partitions.
| // 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.
There was a problem hiding this comment.
Removed. ColumnarBroadcastExchangeExec now calls SparkContextUtils.broadcastInternal(sparkContext, ...) directly, the same way BasicPhysicalOperatorTransformer already calls SparkContextUtils.createPartitioningAwareUnionRDD.
A correction while I am here: #12953 §1 said this one "cannot be lifted at all" because SparkContextUtils exists once per shim module and is invisible from shims/common. The first half is true, the conclusion was not, and it is what made me leave this alone the first time. gluten-substrait declares ${sparkshim.artifactId} at compile scope, so the caller can see that class even though shims/common cannot. I will fix the issue text.
| 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.
There was a problem hiding this comment.
Both removed. They shared a private getLimit helper that was duplicated byte-for-byte in all four shims; since both callers are in OffloadSingleNodeRules, that is now one private limitAndOffset there and the four copies are gone.
| 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.
There was a problem hiding this comment.
Removed. The callers are SoftAffinitySuite and SoftAffinityWithRDDInfoSuite, 12 sites between them, so per the second half of your criterion each suite got a small private partitionedFile(path, start, length, locations) rather than 12 copies of the constructor call. PartitionsUtilSuite in the same tree already uses that pattern.
| 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.
There was a problem hiding this comment.
Removed, and it turned out to have no production caller at all: the only reference in the tree was CHAggAndShuffleBenchmark, which now builds the FileScanRDD itself. The first five constructor parameters are identical on 3.4 through 4.1, so the direct construction is version-safe; 3.5+ inserts metadataExtractors at position six, which is worth knowing before anyone adds a sixth positional argument.
| @@ -89,33 +91,40 @@ trait SparkShims { | |||
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.
Fifteen of them: the callers read the underlying Spark API directly, and the two eval-mode predicates plus the file-metadata helper move to gluten-substrait utils. createParquetFilters and widerDecimalType stay, since LegacyBehaviorPolicy and DecimalPrecision are named differently on 3.4 than on 3.5+.
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🔵 Needs a closer look
Broad cross-version shim API refactoring with compile-only validation and multiple call-site inlines warrants final human review for behavioral equivalence.
Review details
Suppressed comments (1)
shims/common/src/main/scala/org/apache/gluten/execution/datasource/GlutenFormatWriterInjects.scala:77
injectPostRuleFactorynow only assigns topostRuleFactory, butpostRuleFactoryis no longer read anywhere after removinggetExtendedColumnarPostRule, so these registrations become inert and can mislead future changes. Consider removingpostRuleFactory/injectPostRuleFactoryentirely (and updating the remaining call sites), or replacing this hook with whatever the new post-rule registration mechanism is meant to be.
def injectPostRuleFactory(factory: SparkSession => Rule[SparkPlan]): Unit = {
postRuleFactory = factory
}
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
|
||
| def getExtendedColumnarPostRules(): List[SparkSession => Rule[SparkPlan]] | ||
|
|
||
| def writeFilesExecuteTask( |
There was a problem hiding this comment.
The writeFilesExecuteTask seems can also be removed
There was a problem hiding this comment.
Removed in bd79ec9. The four overrides were identical forwards, and the only caller is ColumnarWriteFilesExec, which lives in gluten-substrait and in the same package as GlutenFileFormatWriter, so calling it directly needs no import.
The only caller is in gluten-substrait, which depends on the active shim module, so it can call GlutenFileFormatWriter directly.
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🔵 Needs a closer look
The new limitAndOffset helper will throw on valid limit == offset cases (e.g., LIMIT 0), which needs a correctness fix before merging.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/offload/OffloadSingleNodeRules.scala:194
limitAndOffsetassertslimit > offset, which will throw for valid plans likeLIMIT 0(wherelimit == offset == 0). This should return a zero row limit rather than failing, and it should validate with>=instead of strict>.
gluten-substrait/src/main/scala/org/apache/gluten/utils/FileMetadataUtil.scala:40FileMetadataUtil.generateMetadataColumnsintroduces non-trivial formatting/metadata-resolution logic (including modification time formatting). Adding a focused unit test would help prevent subtle regressions across Spark versions (e.g., requested-column filtering and_metadatavsinput_file_*names).
- Files reviewed: 31/31 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Run Gluten Clickhouse CI on x86 |
|
Thank you @jackylee-ch and @philo-he |
What changes are proposed in this pull request?
With Spark 3.3 gone, most of
SparkShimsno longer varies by version. Following review feedback this deletes those methods rather than lifting their bodies into the trait: the caller reads the underlying Spark API directly, or the logic moves to a util object where more than one caller needs it.Nineteen methods leave the trait, 64 down to 45.
Fifteen are inlined at the call site, each a single field read or a single forward:
getBatchScanExecTable,getKeyGroupedPartitioning,getCollectLimitOffset,unBase64FunctionFailsOnError,enableNativeWriteFilesByDefault,isFinalAdaptivePlan,broadcastInternal,getV1WriteRequiredOrdering,writeFilesExecuteTask,extractExpressionArrayInsert,extractExpressionTimestampDiffUnit,generatePartitionedFile,generateFileScanRDD,getLimitAndOffsetFromGlobalLimitandgetLimitAndOffsetFromTopK. Three of those land in a private helper instead of a raw inline.getLimitAndOffsetFromGlobalLimitandgetLimitAndOffsetFromTopKshared a privategetLimitduplicated in all four shims, and both callers are inOffloadSingleNodeRules, which now has one private helper.generatePartitionedFilehad twelve call sites across two soft-affinity suites, so each suite got a privatepartitionedFile.Three move into
gluten-substraitbecause they have two callers each:withTryEvalModeandwithAnsiEvalModetoExpressionUtils, andgenerateMetadataColumnsto a newFileMetadataUtil.getExtendedColumnarPostRulesis deleted outright. It returnedList()in every remaining shim, so the register-these-rules blocks inVeloxRuleApiandCHRuleApiand their only callee,GlutenFormatFactory.getExtendedColumnarPostRule, were dead.Three surviving defaults become abstract (
extractExpressionTimestampAddUnit,getCommonPartitionValues,orderPartitions), so a new shim has to state an answer rather than inherit a value only 3.3 needed. Bothdrop Spark 3.3TODOs inSparkShims.scalaare now gone.Two look removable and are not.
createParquetFiltersneedsLegacyBehaviorPolicy, which is nested inSQLConfon 3.4 and top-level from 3.5 on, so no single import in a shared module compiles against all four versions.widerDecimalTypeforwards toDecimalPrecisionon 3.4/3.5 andDecimalPrecisionTypeCoercionon 4.0/4.1. Both become removable when 3.4 goes; #12953 tracks them.One behavior change to flag:
ExpressionConvertermatchedArrayInsertthroughgetClass.getSimpleNamebecause the class did not exist on 3.3, and now uses a plain type match. A third-party class whose simple name is alsoArrayInsertused to enter that arm and fail in the cast; it now falls through to the following arms.How was this patch tested?
Compile only, no suites were run.
gluten-substraitincl. test sources, velox, iceberg, delta, packagepaimon-spark-3.5_2.13is not published upstream)test-compilegluten-substraitincl. test sources, iceberg, shimgluten-substrait, velox, shimspotless:checkis clean on 3.4, 3.5, 4.0 and 4.1.Not covered locally: velox on 3.4, where the untouched
FlushableHashAggregateRuledoes not compile against the Spark 2.12 artifacts this machine resolves; iceberg on 4.0/4.1; clickhouse on 3.4. What this PR changes in those modules is either a call into agluten-substraitutil or a read of a public Spark field, neither of which varies by version.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-5
Related issue: #12807