This is an automated email from the ASF dual-hosted git repository.
zhztheplayer pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gluten.git
The following commit(s) were added to refs/heads/main by this push:
new 0a16d3272c [MINOR][CORE] Fail fast in InsertTransitions when a child
has no recognizable Convention (#12442)
0a16d3272c is described below
commit 0a16d3272c7199fe7b4a28104f33df1cba8bbc4d
Author: YangJie <[email protected]>
AuthorDate: Mon Jul 6 21:15:44 2026 +0800
[MINOR][CORE] Fail fast in InsertTransitions when a child has no
recognizable Convention (#12442)
---
.../extension/columnar/transition/Transition.scala | 20 +++++++++
.../columnar/transition/Transitions.scala | 25 +++++++++--
.../columnar/transition/TransitionSuite.scala | 50 ++++++++++++++++++++++
.../columnar/transition/TransitionSuiteBase.scala | 15 +++++++
4 files changed, 106 insertions(+), 4 deletions(-)
diff --git
a/gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transition.scala
b/gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transition.scala
index 85b19d01de..79aa0d1e40 100644
---
a/gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transition.scala
+++
b/gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transition.scala
@@ -65,6 +65,26 @@ object Transition {
s"No viable transition to [$required] found for plan:
${plan.simpleString(maxFields)}")
}
+ /**
+ * Raised when `plan` has a child whose observed Convention has neither a
recognizable row type
+ * nor a recognizable batch type. Such a child cannot be executed and has no
`from` convention to
+ * search from, so callers should raise this at planning time instead of
accepting the child and
+ * letting the failure surface later at task execution.
+ */
+ def notExecutable(plan: SparkPlan, child: SparkPlan): GlutenException = {
+ new GlutenException(
+ "Child plan has no recognizable row type or batch type, so no transition
can be inserted " +
+ "for it. Parent: " +
+ s"${plan.simpleString(maxFields)}. Child:
${child.simpleString(maxFields)}.")
+ }
+
+ /** Single-argument variant for the root-plan case. See
[[notExecutable(plan, child)]]. */
+ def notExecutable(plan: SparkPlan): GlutenException = {
+ new GlutenException(
+ "Plan has no recognizable row type or batch type, so no transition can
be inserted for " +
+ s"it. Plan: ${plan.simpleString(maxFields)}.")
+ }
+
trait Factory {
final def findTransitionOrThrow(from: Convention, to: ConventionReq)(
otherwise: => Exception): Transition = {
diff --git
a/gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transitions.scala
b/gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transitions.scala
index 1607bc16ce..5402909fb5 100644
---
a/gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transitions.scala
+++
b/gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transitions.scala
@@ -44,9 +44,11 @@ case class InsertTransitions(convReq: ConventionReq) extends
Rule[SparkPlan] {
case (child, convReq) =>
val from = convFunc.conventionOf(child)
if (from.isNone) {
- // For example, a union op with row child and columnar child at the
same time,
- // The plan is actually not executable, and we cannot tell about its
convention.
- child
+ // Defensive check: a GlutenPlan that declares both rowType() and
batchType() as None
+ // is not executable. No production plan does this today, but the
pre-existing branch
+ // silently returned the child, which would defer the failure to
task execution with
+ // an unrelated error. Fail at planning time instead so the
offending pair is visible.
+ throw Transition.notExecutable(node, child)
} else {
val transition =
Transition.factory.findTransitionOrThrow(from,
convReq)(Transition.notFound(node))
@@ -94,11 +96,26 @@ object Transitions {
enforceReq(plan,
ConventionReq.ofBatch(ConventionReq.BatchType.Is(toBatchType)))
}
+ /**
+ * Wraps `plan` in the shortest transition chain that produces the given
[[ConventionReq]].
+ *
+ * Only validates the ROOT of `plan`. Callers that need to guarantee every
descendant is
+ * executable should route through [[InsertTransitions.apply]] first, which
visits each non-leaf
+ * node via `transformUp` and raises [[Transition.notExecutable]] on any
child whose `Convention`
+ * is `isNone`.
+ */
def enforceReq(plan: SparkPlan, req: ConventionReq): SparkPlan = {
val convFunc = ConventionFunc.create()
val removed = RemoveTransitions.removeForNode(plan)
+ val from = convFunc.conventionOf(removed)
+ if (from.isNone) {
+ // Symmetric with InsertTransitions.applyForNode; see the comment there.
Also replaces the
+ // internal assert(!from.isNone) buried in
Transition.Factory#findTransition so callers see
+ // a GlutenException with the plan spelled out instead of a bare
AssertionError.
+ throw Transition.notExecutable(removed)
+ }
val transition = Transition.factory
- .findTransitionOrThrow(convFunc.conventionOf(removed),
req)(Transition.notFound(removed, req))
+ .findTransitionOrThrow(from, req)(Transition.notFound(removed, req))
val out = transition.apply(removed)
out
}
diff --git
a/gluten-substrait/src/test/scala/org/apache/gluten/extension/columnar/transition/TransitionSuite.scala
b/gluten-substrait/src/test/scala/org/apache/gluten/extension/columnar/transition/TransitionSuite.scala
index eab0f66598..c30a6ef2e7 100644
---
a/gluten-substrait/src/test/scala/org/apache/gluten/extension/columnar/transition/TransitionSuite.scala
+++
b/gluten-substrait/src/test/scala/org/apache/gluten/extension/columnar/transition/TransitionSuite.scala
@@ -128,6 +128,56 @@ class TransitionSuite extends SharedSparkSession with
TransitionSuiteBase with W
insertTransitions(in,
ConventionReq.ofRow(ConventionReq.RowType.Is(RowTypeA)))
}
}
+
+ test("Fail at planning time when a child has no recognizable convention") {
+ // Child with rowType=None and batchType=None is not executable; the fix
throws at planning
+ // time with the offending parent / child spelled out.
+ val in = BatchUnary(BatchTypeA, NoConventionLeaf())
+ val ex = intercept[GlutenException] {
+ insertTransitions(in,
ConventionReq.ofRow(ConventionReq.RowType.Is(RowTypeA)))
+ }
+ // Positional labels: catches an accidental argument swap in
notExecutable(node, child) that
+ // would still contain both class names but with roles inverted.
+ assert(
+ ex.getMessage.contains("Parent: BatchUnary"),
+ s"error should identify BatchUnary as the parent, got: ${ex.getMessage}")
+ assert(
+ ex.getMessage.contains("Child: NoConventionLeaf"),
+ s"error should identify NoConventionLeaf as the offending child, got:
${ex.getMessage}")
+ }
+
+ test("Fail at planning time when a non-first child has no recognizable
convention") {
+ // Ensures the per-child check does not short-circuit on the first slot.
+ val in = BatchBinary(BatchTypeA, BatchLeaf(BatchTypeA), NoConventionLeaf())
+ val ex = intercept[GlutenException] {
+ insertTransitions(in,
ConventionReq.ofRow(ConventionReq.RowType.Is(RowTypeA)))
+ }
+ assert(
+ ex.getMessage.contains("Parent: BatchBinary"),
+ s"error should identify BatchBinary as the parent even for a non-first
bad child, got: " +
+ ex.getMessage
+ )
+ assert(
+ ex.getMessage.contains("Child: NoConventionLeaf"),
+ s"error should identify the offending child even in a non-first slot,
got: ${ex.getMessage}")
+ }
+
+ test("Fail at planning time when the root plan itself has no recognizable
convention") {
+ // Root goes through Transitions.enforceReq. Pre-fix that path raised the
internal
+ // assert(!from.isNone) in Transition.Factory#findTransition, giving a
bare AssertionError
+ // without plan identity. Post-fix raises a symmetric GlutenException.
+ val ex = intercept[GlutenException] {
+ insertTransitions(NoConventionLeaf(),
ConventionReq.ofRow(ConventionReq.RowType.Is(RowTypeA)))
+ }
+ // Single-arg overload uses "Plan:" and no "Parent:" / "Child:"; guard
against migrating to
+ // a self-referential notExecutable(removed, removed) call.
+ assert(
+ ex.getMessage.contains("Plan: NoConventionLeaf"),
+ s"root-plan error should use the single-arg 'Plan:' framing, got:
${ex.getMessage}")
+ assert(
+ !ex.getMessage.contains("Parent:"),
+ s"root-plan error must not use the two-arg 'Parent:/Child:' framing,
got: ${ex.getMessage}")
+ }
}
object TransitionSuite extends TransitionSuiteBase {
diff --git
a/gluten-substrait/src/test/scala/org/apache/gluten/extension/columnar/transition/TransitionSuiteBase.scala
b/gluten-substrait/src/test/scala/org/apache/gluten/extension/columnar/transition/TransitionSuiteBase.scala
index 4b3b8e42db..5314c2d906 100644
---
a/gluten-substrait/src/test/scala/org/apache/gluten/extension/columnar/transition/TransitionSuiteBase.scala
+++
b/gluten-substrait/src/test/scala/org/apache/gluten/extension/columnar/transition/TransitionSuiteBase.scala
@@ -77,6 +77,21 @@ object TransitionSuiteBase {
override def output: Seq[Attribute] = List.empty
}
+ /**
+ * Leaf with rowType=None and batchType=None. Used only to exercise
+ * [[InsertTransitions.applyForNode]]'s defensive fail-fast on unexecutable
children; no
+ * production plan declares itself this way.
+ */
+ case class NoConventionLeaf() extends LeafExecNode with GlutenPlan {
+ override def rowType(): Convention.RowType = Convention.RowType.None
+
+ override def batchType(): Convention.BatchType = Convention.BatchType.None
+
+ override protected def doExecute(): RDD[InternalRow] = throw new
UnsupportedOperationException()
+
+ override def output: Seq[Attribute] = List.empty
+ }
+
case class RowUnary(override val rowType: Convention.RowType, override val
child: SparkPlan)
extends UnaryExecNode
with GlutenPlan {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
