[GLUTEN-13003][CORE] Move version-independent helpers out of SparkShims into utils - #13006
LuciferYang wants to merge 5 commits into
Conversation
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review comments remain.
Pull request overview
This pull request centralizes shared Spark shim implementations across Spark 3.4–4.1, reducing duplication while preserving compatibility.
Changes:
- Centralizes struct/attribute conversion methods.
- Moves
invalidBucketFileinto the common trait. - Removes redundant per-version methods and imports.
File summaries
| File | Description |
|---|---|
shims/spark41/src/main/scala/org/apache/gluten/sql/shims/spark41/Spark41Shims.scala |
Removes duplicated overrides and imports. |
shims/spark40/src/main/scala/org/apache/gluten/sql/shims/spark40/Spark40Shims.scala |
Removes duplicated overrides and imports. |
shims/spark35/src/main/scala/org/apache/gluten/sql/shims/spark35/Spark35Shims.scala |
Removes duplicated overrides and imports. |
shims/spark34/src/main/scala/org/apache/gluten/sql/shims/spark34/Spark34Shims.scala |
Removes duplicated overrides. |
shims/common/src/main/scala/org/apache/gluten/sql/shims/SparkShims.scala |
Adds shared implementations. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Run Gluten Clickhouse CI on x86 |
|
cc @jackylee-ch |
| metadata: Map[String, Any] = Map.empty): Seq[PartitionedFile] | ||
|
|
||
| def structFromAttributes(attrs: Seq[Attribute]): StructType | ||
| def structFromAttributes(attrs: Seq[Attribute]): StructType = |
There was a problem hiding this comment.
Can we move this func out of SparkShims? We can defined it in ExpressionUtils.
There was a problem hiding this 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.
| 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] = |
| field => AttributeReference(field.name, field.dataType, field.nullable, field.metadata)() | ||
| } | ||
|
|
||
| // https://issues.apache.org/jira/browse/SPARK-40400 |
There was a problem hiding this comment.
SPARK-40400 is fixed in Spark 3.4.0, can we also remove this?
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this 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.
…tFile for Spark's
…private[sql]); drop duplicate imports
LuciferYang
changed the title
|
Run Gluten Clickhouse CI on x86 |
LuciferYang
changed the title
|
Run Gluten Clickhouse CI on x86 |
What changes are proposed in this pull request?
Three helpers on
SparkShimsno longer vary across the supported versions ({3.4, 3.5, 4.0, 4.1}), so they no longer need to live on the version-shim trait.structFromAttributesandattributesFromStructmove toExpressionUtils. They were abstract on the trait with four identical overrides (spark34 hand-written; 3.5/4.0/4.1 delegating toDataTypeUtils, whose body is spark34's verbatim). Since they don't depend on the Spark version,SparkShimsis the wrong home, andExpressionUtilsin gluten-substrait is reachable from every caller. The call sites move fromSparkShimLoader.getSparkShims.structFromAttributes(x)toExpressionUtils.structFromAttributes(x), and the trait declarations and the four overrides are gone.invalidBucketFilewas aprivate defcopied into all four shims; it moves toorg.apache.gluten.utils.ExceptionUtils, the shared exception helper in shims/common, and the four shims callExceptionUtils.invalidBucketFile(...). It buildsSparkException(INVALID_BUCKET_FILE, path=..., cause=null)by hand rather than calling Spark'sQueryExecutionErrors.invalidBucketFile, because that object isprivate[sql]and cannot be reached from Gluten's own packages. Only Gluten classes placed underorg.apache.spark.sql.*, such asAbstractFileSourceScanExec, can call it. A comment records that.Thanks to @jackylee-ch for the review that prompted moving all three off the trait.
How was this patch tested?
clean test-compileon Spark 3.4, 3.5, 4.0 and 4.1 with-Pbackends-velox -Pspark-ut -Piceberg -Pdelta, Scala 2.13; the 3.5 run adds-Pbackends-clickhouse, the only profile it builds on.spotless:applyclean. This is code motion with unchanged signatures, so no suite was run.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-5
Related issue: #13003