-
Notifications
You must be signed in to change notification settings - Fork 29.4k
[SPARK-50593][SQL] SPJ: Support truncate transform via generalized ReducibleFunction API #55885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
d0f43af
c225cc1
f0d8088
aeab763
721f576
9a70695
28368ca
9d92888
7ea6de9
8614b01
551fe55
6ee8de8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,6 @@ | |
|
|
||
| package org.apache.spark.sql.catalyst.plans.physical | ||
|
|
||
| import scala.annotation.tailrec | ||
| import scala.collection.mutable | ||
|
|
||
| import org.apache.spark.{SparkException, SparkUnsupportedOperationException} | ||
|
|
@@ -641,19 +640,15 @@ object KeyedPartitioning { | |
|
|
||
| def supportsExpressions(expressions: Seq[Expression]): Boolean = { | ||
| def isSupportedTransform(transform: TransformExpression): Boolean = { | ||
| transform.children.size == 1 && isReference(transform.children.head) | ||
| } | ||
|
|
||
| @tailrec | ||
| def isReference(e: Expression): Boolean = e match { | ||
| case _: Attribute => true | ||
| case g: GetStructField => isReference(g.child) | ||
| case _ => false | ||
| // Should only consider column references, not literals. | ||
| val nonLiteralChildren = transform.children.filterNot(_.isInstanceOf[Literal]) | ||
| // We need exactly one column reference per transform. | ||
| nonLiteralChildren.size == 1 && TransformExpression.isColumnRef(nonLiteralChildren.head) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Reducer-compatible transforms must not take the raw partition-key equality fast path. This widened gate now admits parameterized transforms such as Please restrict the raw-key-equality fast path to identical transform semantics, or force compatible-but-different transforms through reduced-key reconciliation. Add end-to-end SPJ regressions for identity-vs-parameterized and parameterized-vs-parameterized transforms whose raw key arrays coincide but whose physical partitions differ. |
||
| } | ||
|
|
||
| expressions.forall { | ||
| case t: TransformExpression if isSupportedTransform(t) => true | ||
| case e: Expression if isReference(e) => true | ||
| case e: Expression if TransformExpression.isColumnRef(e) => true | ||
| case _ => false | ||
| } | ||
| } | ||
|
|
@@ -1302,24 +1297,58 @@ case class KeyedShuffleSpec( | |
| } | ||
| } | ||
|
|
||
| private def isExpressionCompatible(left: Expression, right: Expression): Boolean = | ||
| /** | ||
| * The reducer mapping a raw identity column `col` onto transform `t` (it applies `t` to the | ||
| * identity values), or None if not reducible. Single source of the identity-vs-transform | ||
| * decision: [[isExpressionCompatible]] derives the gate from it (`.isDefined`) and [[reducers]] | ||
| * returns it, so the two cannot drift (a divergence would keep raw keys -> mis-join). | ||
| * | ||
| * The reducer evals the transform with `col` substituted for its column, so it validates the | ||
| * SUBSTITUTED expression's arg types ([[TransformExpression.argsMatchInputTypes]]) -- not t's own | ||
| * -- keeping a type mismatch (e.g. a `ShortType` `col` at an `IntegerType` slot) from reaching | ||
| * eval and raising a `ClassCastException`. | ||
| */ | ||
| private def identityReducer( | ||
| col: AttributeReference, t: TransformExpression): Option[Reducer[_, _]] = { | ||
| // `transform` preserves the root node type, so this is always a TransformExpression; the only | ||
| // real gate is argsMatchInputTypes on the substituted (identity) column -- see the doc above. | ||
| val reducerExpr = | ||
| t.transform { case _: AttributeReference => col }.asInstanceOf[TransformExpression] | ||
| if (reducerExpr.argsMatchInputTypes) { | ||
| val boundExpr = BindReferences.bindReference(reducerExpr, AttributeSeq(Seq(col))) | ||
| Some(new Reducer[Any, Any] { | ||
| override def reduce(v: Any): Any = boundExpr.eval(new GenericInternalRow(Array[Any](v))) | ||
| override def resultType(): DataType = reducerExpr.dataType | ||
| override def displayName(): String = reducerExpr.toString | ||
| }) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| private def isExpressionCompatible(left: Expression, right: Expression): Boolean = { | ||
| def compatibleTransformsAllowed: Boolean = | ||
| SQLConf.get.v2BucketingPushPartValuesEnabled && | ||
| !SQLConf.get.v2BucketingPartiallyClusteredDistributionEnabled && | ||
| SQLConf.get.v2BucketingAllowCompatibleTransforms | ||
| (left, right) match { | ||
| case (_: LeafExpression, _: LeafExpression) => true | ||
| case (left: TransformExpression, right: TransformExpression) => | ||
| if (SQLConf.get.v2BucketingPushPartValuesEnabled && | ||
| !SQLConf.get.v2BucketingPartiallyClusteredDistributionEnabled && | ||
| SQLConf.get.v2BucketingAllowCompatibleTransforms) { | ||
| if (compatibleTransformsAllowed) { | ||
| left.isCompatible(right) | ||
| } else { | ||
| left.isSameFunction(right) | ||
| } | ||
| case (_: AttributeReference, _: TransformExpression) | | ||
| (_: TransformExpression, _: AttributeReference) => | ||
| SQLConf.get.v2BucketingPushPartValuesEnabled && | ||
| !SQLConf.get.v2BucketingPartiallyClusteredDistributionEnabled && | ||
| SQLConf.get.v2BucketingAllowCompatibleTransforms | ||
| // Identity transform on one side, arbitrary transform on the other. Derive the gate from the | ||
| // producer (identityReducer): the pair is compatible only if a reducer can actually be built, | ||
| // so the gate and reducers cannot drift (a divergence would keep raw keys -> mis-join). | ||
| case (col: AttributeReference, t: TransformExpression) => | ||
| compatibleTransformsAllowed && identityReducer(col, t).isDefined | ||
| case (t: TransformExpression, col: AttributeReference) => | ||
| compatibleTransformsAllowed && identityReducer(col, t).isDefined | ||
| case _ => false | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Return a set of [[Reducer]] for the partition expressions of this shuffle spec, | ||
|
|
@@ -1341,19 +1370,11 @@ case class KeyedShuffleSpec( | |
| val results = partitioning.expressions.zip(other.partitioning.expressions).map { | ||
| case (e1: TransformExpression, e2: TransformExpression) => e1.reducers(e2) | ||
|
|
||
| // Identity transform on this side, arbitrary transform on the other side: create a reducer | ||
| // that applies the other's transform to the raw identity values. The symmetric case | ||
| // Identity transform on this side, arbitrary transform on the other side. The symmetric case | ||
| // (TransformExpression, AttributeReference) is handled when the other side calls reducers. | ||
| // Each partition expression is guaranteed to have exactly one leaf child (asserted in | ||
| // keyPositions), so `a` lives at position 0 in the row we construct. | ||
| case (a: AttributeReference, t: TransformExpression) => | ||
| val reducerExpr = t.transform { case _: AttributeReference => a } | ||
| val boundExpr = BindReferences.bindReference(reducerExpr, AttributeSeq(Seq(a))) | ||
| Some(new Reducer[Any, Any] { | ||
| override def reduce(v: Any): Any = boundExpr.eval(new GenericInternalRow(Array[Any](v))) | ||
| override def resultType(): DataType = reducerExpr.dataType | ||
| override def displayName(): String = reducerExpr.toString | ||
| }) | ||
| // identityReducer is the shared decision the compatibility gate also consults, so the two | ||
| // cannot drift. | ||
| case (col: AttributeReference, t: TransformExpression) => identityReducer(col, t) | ||
|
|
||
| case (_, _) => None | ||
| } | ||
|
|
@@ -1375,7 +1396,13 @@ case class KeyedShuffleSpec( | |
|
|
||
| val newExpressions = partitioning.expressions.zip(keyPositions).map { | ||
| case (te: TransformExpression, positionSet) => | ||
| te.copy(children = te.children.map(_ => clustering(positionSet.head))) | ||
| // Preserve literal parameters (e.g., numBuckets, truncate width) | ||
| // while replacing only column references with the new clustering expression | ||
| val newChildren = te.children.map { | ||
| case l: Literal => l // Keep literals as-is | ||
| case _ => clustering(positionSet.head) // Replace column references | ||
| } | ||
| te.copy(children = newChildren) | ||
| case (_, positionSet) => clustering(positionSet.head) | ||
| } | ||
| KeyedPartitioning(newExpressions, partitioning.partitionKeys, partitioning.isGrouped) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Apply bound-function input casts before evaluating parameterized transforms through SPJ
This gate now admits parameterized transforms whose bound function requests legal implicit casts.
BoundFunction.inputTypes()explicitly allows types to differ from those passed tobind, with Spark responsible for casting. However, the scan path stores the raw Catalyst children inTransformExpression, and the identity-vs-transform reducer later binds and directly evaluates that expression without running Analyzer type coercion.For a concrete case, a connector can report
truncate(str_col, Expressions.literal(2.toShort)); binding(StringType, ShortType)to a scalar function that declares(StringType, IntegerType)is legal. This transform passes the new gate, but the synthetic reducer evaluates the rawShortliteral throughApplyFunctionExpression. ItsSpecificInternalRow(IntegerType)then attempts to cast the boxedShorttoInt, raisingClassCastException. Before parameterized transforms were admitted here, this query would fall back to a shuffle.Please coerce the transform children to
function.inputTypes()before direct reducer evaluation (or reject mismatched types from this SPJ path), and add an identity-vs-parameterized-transform regression covering an implicitly cast literal.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sunchao Thanks. I was able to reproduce this issue which raises ClassCastException.
I'll work on a fix, and let you know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sunchao I went with the second option you mentioned, rejecting "mismatched types from this SPJ path" rather than coercing, since coercion (even up-cast-only) added a family of edge cases (lossy casts,
null→0, overflow→ClassCastException, eval-vs-reducer strategy drift) for a narrow optimization.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Thanks—rejecting the mismatched literal fixes that reproducer, but this still checks only
Literalchildren.BoundFunction.inputTypes()applies to every argument, andidentityReducerdirectly evaluates every raw child through the same unanalyzed path. For example, a connector can legally bind(ShortType, IntegerType)and return a scalar function whose declared inputs are(IntegerType, IntegerType). Because the literal is already an exactIntegerType,literalParamsMatchInputTypespasses; thenidentityReducer.reducefeeds the boxedShortthroughApplyFunctionExpressionintoSpecificInternalRow(IntegerType), which throwsClassCastException. Please validate every child against its corresponding declared input type before this direct evaluation (or insert the required casts), and add an identity-vs-transform regression with aShortTypecolumn and exact-typed integer literal. The current tests cover only the literal slot.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sunchao Fixed! identityReducer now validates every child (not just literals) against the declared input types via
argsMatchInputTypes(+ exact-arity)