You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/09/12 06:51:16 UTC
[PR] [GLUTEN-13003][CORE] Move two more SparkShims methods onto the trait [gluten]
LuciferYang opened a new pull request, #13006:
URL: https://github.com/apache/gluten/pull/13006
## What changes are proposed in this pull request?
#12954 removed 19 `SparkShims` methods whose four implementations had become identical once Spark 3.3 left. Two more belong in that set, and they were easy to miss then because the four bodies are not textually equal.
`structFromAttributes` and `attributesFromStruct` are declared abstract on the trait. spark34 spells the bodies out; spark35, spark40 and spark41 delegate to `DataTypeUtils.fromAttributes` and `DataTypeUtils.toAttributes`, whose implementations are spark34's bodies verbatim:
```scala
def fromAttributes(attributes: Seq[Attribute]): StructType =
StructType(attributes.map(a => StructField(a.name, a.dataType, a.nullable, a.metadata)))
def toAttribute(field: StructField): AttributeReference =
AttributeReference(field.name, field.dataType, field.nullable, field.metadata)()
def toAttributes(schema: StructType): Seq[AttributeReference] = schema.map(toAttribute)
```
So the two become concrete trait methods and the eight overrides go. The trait body has to be spark34's form rather than the delegation, because `DataTypeUtils` does not exist in the 3.4 catalyst jar. That asymmetry is the whole reason these two sat behind the shim.
One difference is worth naming even though nothing depends on it: on 3.5, 4.0 and 4.1 `attributesFromStruct` used to come back list-backed via `schema.map`, and now comes back array-backed via `structType.fields.map`. Same elements in the same order, and every caller takes it as a `Seq[Attribute]`; the declared return type was already `Seq[Attribute]` rather than `DataTypeUtils`' narrower `Seq[AttributeReference]`, so no signature moves either.
`invalidBucketFile` comes along as a `protected def`. It was a fourfold-duplicated `private def` that needs only `SparkException`, called from `filesGroupedToBuckets`, which has to stay per-module because it calls the per-module `PartitionedFileUtilShim`. No signature changes, so no caller moves.
Two others look collapsible and are not, recorded here so nobody retries. `createParquetFilters` has four byte-identical bodies, but `LegacyBehaviorPolicy` sits under `SQLConf` on 3.4 and stands alone from 3.5 on, so no single spelling compiles everywhere. `getShuffleBlockFetcherIterator` constructs `GlutenShuffleBlockFetcherIterator`, which exists once per shim module and is invisible to `shims/common`, since the dependency runs spark3x to common rather than the other way.
Worth knowing for whoever picks up the next round: of the 27 abstract members left, 14 are identical across 3.5, 4.0 and 4.1 with 3.4 as the only outlier. Dropping 3.4 would collapse most of the trait; continuing to mine 3.3 residue will not.
## How was this patch tested?
`clean test-compile` on Spark 3.4, 3.5, 4.0 and 4.1 with `-Pbackends-velox -Pspark-ut -Piceberg -Pdelta`, Scala 2.13, and the 3.5 run adds `-Pbackends-clickhouse`, that being the only profile clickhouse builds on.
That is the check that matters here. The change is a compile-time move, and the `DataTypeUtils` calls it replaces exist on three of the four versions but not the fourth, so picking the wrong body fails to build rather than misbehaving at runtime.
`spotless:apply` produced no changes beyond the edits. No suite was run: same expressions, same signatures, callers untouched.
## Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-5
Related issue: #13003
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move two more SparkShims methods onto the trait [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13006:
URL: https://github.com/apache/gluten/pull/13006#issuecomment-5644301300
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent helpers out of SparkShims into utils [gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch merged PR #13006:
URL: https://github.com/apache/gluten/pull/13006
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent shim helpers to ExpressionUtils and dedup invalidBucketFile [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #13006:
URL: https://github.com/apache/gluten/pull/13006#discussion_r4003039199
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -122,9 +122,20 @@ trait SparkShims {
partitionValues: InternalRow,
metadata: Map[String, Any] = Map.empty): Seq[PartitionedFile]
- def structFromAttributes(attrs: Seq[Attribute]): StructType
+ def structFromAttributes(attrs: Seq[Attribute]): StructType =
Review Comment:
Done — moved both to `ExpressionUtils` and repointed the call sites. They don't vary across the supported versions, so you're right that `SparkShims` was the wrong home.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move two more SparkShims methods onto the trait [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13006:
URL: https://github.com/apache/gluten/pull/13006#issuecomment-5658297486
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent shim helpers to ExpressionUtils and dedup invalidBucketFile [gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on code in PR #13006:
URL: https://github.com/apache/gluten/pull/13006#discussion_r4003787359
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -122,9 +122,20 @@ trait SparkShims {
partitionValues: InternalRow,
metadata: Map[String, Any] = Map.empty): Seq[PartitionedFile]
- def structFromAttributes(attrs: Seq[Attribute]): StructType
+ def structFromAttributes(attrs: Seq[Attribute]): StructType =
+ StructType(attrs.map(a => StructField(a.name, a.dataType, a.nullable, a.metadata)))
- def attributesFromStruct(structType: StructType): Seq[Attribute]
+ def attributesFromStruct(structType: StructType): Seq[Attribute] =
+ structType.fields.map {
+ field => AttributeReference(field.name, field.dataType, field.nullable, field.metadata)()
+ }
+
+ // https://issues.apache.org/jira/browse/SPARK-40400
Review Comment:
Thanks for the clarification. Since `invalidBucketFile` is no longer version-specific, maybe we can move it out of `SparkShims` as well.
How about putting it in `GlutenExceptionUtils` for now? It seems a better fit for shared exception construction logic.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent helpers out of SparkShims into utils [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13006:
URL: https://github.com/apache/gluten/pull/13006#issuecomment-5709406941
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent shim helpers to ExpressionUtils and dedup invalidBucketFile [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #13006:
URL: https://github.com/apache/gluten/pull/13006#discussion_r4003041030
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -122,9 +122,20 @@ trait SparkShims {
partitionValues: InternalRow,
metadata: Map[String, Any] = Map.empty): Seq[PartitionedFile]
- def structFromAttributes(attrs: Seq[Attribute]): StructType
+ def structFromAttributes(attrs: Seq[Attribute]): StructType =
+ StructType(attrs.map(a => StructField(a.name, a.dataType, a.nullable, a.metadata)))
- def attributesFromStruct(structType: StructType): Seq[Attribute]
+ def attributesFromStruct(structType: StructType): Seq[Attribute] =
+ structType.fields.map {
+ field => AttributeReference(field.name, field.dataType, field.nullable, field.metadata)()
+ }
+
+ // https://issues.apache.org/jira/browse/SPARK-40400
Review Comment:
I tried this and it doesn't compile from here: `QueryExecutionErrors` is `private[sql]`, so `object QueryExecutionErrors ... cannot be accessed ... from class Spark34Shims in package spark34`. Only Gluten classes placed under `org.apache.spark.sql.*`, like `AbstractFileSourceScanExec`, can reach it, which is the actual reason for the local copy; SPARK-40400 fixing the error framework doesn't change the visibility. I did dedup the four copies into one `protected def` on the trait.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent helpers out of SparkShims into utils [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #13006:
URL: https://github.com/apache/gluten/pull/13006#discussion_r4004054611
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -122,9 +122,20 @@ trait SparkShims {
partitionValues: InternalRow,
metadata: Map[String, Any] = Map.empty): Seq[PartitionedFile]
- def structFromAttributes(attrs: Seq[Attribute]): StructType
+ def structFromAttributes(attrs: Seq[Attribute]): StructType =
+ StructType(attrs.map(a => StructField(a.name, a.dataType, a.nullable, a.metadata)))
- def attributesFromStruct(structType: StructType): Seq[Attribute]
+ def attributesFromStruct(structType: StructType): Seq[Attribute] =
+ structType.fields.map {
+ field => AttributeReference(field.name, field.dataType, field.nullable, field.metadata)()
+ }
+
+ // https://issues.apache.org/jira/browse/SPARK-40400
Review Comment:
Makes sense. Moved it to the existing `org.apache.gluten.utils.ExceptionUtils` in `shims/common` (there is no `GlutenExceptionUtils` in the tree, and that is where the shared exception helpers already live, e.g. `hasCause`). The four shims now call `ExceptionUtils.invalidBucketFile(...)`, and it is off the trait entirely.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent helpers out of SparkShims into utils [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #13006:
URL: https://github.com/apache/gluten/pull/13006#issuecomment-5755833847
@jackylee-ch can we merge this one?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent shim helpers to ExpressionUtils and dedup invalidBucketFile [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13006:
URL: https://github.com/apache/gluten/pull/13006#issuecomment-5660599794
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent helpers out of SparkShims into utils [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13006:
URL: https://github.com/apache/gluten/pull/13006#issuecomment-5662300369
Run Gluten Clickhouse CI on x86
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move two more SparkShims methods onto the trait [gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on code in PR #13006:
URL: https://github.com/apache/gluten/pull/13006#discussion_r4002334335
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -122,9 +122,20 @@ trait SparkShims {
partitionValues: InternalRow,
metadata: Map[String, Any] = Map.empty): Seq[PartitionedFile]
- def structFromAttributes(attrs: Seq[Attribute]): StructType
+ def structFromAttributes(attrs: Seq[Attribute]): StructType =
+ StructType(attrs.map(a => StructField(a.name, a.dataType, a.nullable, a.metadata)))
- def attributesFromStruct(structType: StructType): Seq[Attribute]
+ def attributesFromStruct(structType: StructType): Seq[Attribute] =
+ structType.fields.map {
+ field => AttributeReference(field.name, field.dataType, field.nullable, field.metadata)()
+ }
+
+ // https://issues.apache.org/jira/browse/SPARK-40400
Review Comment:
[SPARK-40400](https://issues.apache.org/jira/browse/SPARK-40400) is fixed in Spark 3.4.0, can we also remove this?
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -122,9 +122,20 @@ trait SparkShims {
partitionValues: InternalRow,
metadata: Map[String, Any] = Map.empty): Seq[PartitionedFile]
- def structFromAttributes(attrs: Seq[Attribute]): StructType
+ def structFromAttributes(attrs: Seq[Attribute]): StructType =
+ StructType(attrs.map(a => StructField(a.name, a.dataType, a.nullable, a.metadata)))
- def attributesFromStruct(structType: StructType): Seq[Attribute]
+ def attributesFromStruct(structType: StructType): Seq[Attribute] =
Review Comment:
ditto
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -122,9 +122,20 @@ trait SparkShims {
partitionValues: InternalRow,
metadata: Map[String, Any] = Map.empty): Seq[PartitionedFile]
- def structFromAttributes(attrs: Seq[Attribute]): StructType
+ def structFromAttributes(attrs: Seq[Attribute]): StructType =
Review Comment:
Can we move this func out of `SparkShims`? We can defined it in `ExpressionUtils`.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move two more SparkShims methods onto the trait [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #13006:
URL: https://github.com/apache/gluten/pull/13006#issuecomment-5658314539
cc @jackylee-ch
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent helpers out of SparkShims into utils [gluten]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #13006:
URL: https://github.com/apache/gluten/pull/13006#discussion_r4013523280
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -122,9 +122,20 @@ trait SparkShims {
partitionValues: InternalRow,
metadata: Map[String, Any] = Map.empty): Seq[PartitionedFile]
- def structFromAttributes(attrs: Seq[Attribute]): StructType
+ def structFromAttributes(attrs: Seq[Attribute]): StructType =
+ StructType(attrs.map(a => StructField(a.name, a.dataType, a.nullable, a.metadata)))
- def attributesFromStruct(structType: StructType): Seq[Attribute]
+ def attributesFromStruct(structType: StructType): Seq[Attribute] =
+ structType.fields.map {
+ field => AttributeReference(field.name, field.dataType, field.nullable, field.metadata)()
+ }
+
+ // https://issues.apache.org/jira/browse/SPARK-40400
Review Comment:
Thanks for catching the name. The blocker is the module layer: `GlutenExceptionUtil` lives in `gluten-substrait`, and `gluten-substrait` depends on the shims (`shims-common` plus the active version shim), not the other way around. The callers here are the four `Spark*Shims` classes, so having them call `GlutenExceptionUtil` would require the shims to depend on `gluten-substrait`, which is a circular dependency and won't compile.
`ExceptionUtils` in `shims-common` is the exception helper all four shim modules can actually reach, so I'll keep `invalidBucketFile` there.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-13003][CORE] Move version-independent helpers out of SparkShims into utils [gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on code in PR #13006:
URL: https://github.com/apache/gluten/pull/13006#discussion_r4013448285
##########
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala:
##########
@@ -122,9 +122,20 @@ trait SparkShims {
partitionValues: InternalRow,
metadata: Map[String, Any] = Map.empty): Seq[PartitionedFile]
- def structFromAttributes(attrs: Seq[Attribute]): StructType
+ def structFromAttributes(attrs: Seq[Attribute]): StructType =
+ StructType(attrs.map(a => StructField(a.name, a.dataType, a.nullable, a.metadata)))
- def attributesFromStruct(structType: StructType): Seq[Attribute]
+ def attributesFromStruct(structType: StructType): Seq[Attribute] =
+ structType.fields.map {
+ field => AttributeReference(field.name, field.dataType, field.nullable, field.metadata)()
+ }
+
+ // https://issues.apache.org/jira/browse/SPARK-40400
Review Comment:
Sorry for my mistake. There is a `org.apache.gluten.exception.GlutenExceptionUtil`, we can put the `invalidBucketFile ` in it.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org