[MINOR][CORE] Fail fast in InsertTransitions when a child has no recognizable Convention#12442
Conversation
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
This PR makes transition insertion fail fast (at planning time) when a plan (or a child plan) has an unrecognizable Convention (rowType=None and batchType=None), replacing silent no-ops / assertion failures with a GlutenException that identifies the offending plan(s).
Changes:
- In
InsertTransitions.applyForNode, throw aGlutenExceptionwhen a child’s derivedConventionisisNoneinstead of returning the child unchanged. - In
Transitions.enforceReq, add a symmetric root-level pre-check to throw aGlutenExceptionrather than hitting an internalassert(!from.isNone)inTransition.Factory#findTransition. - Add
Transition.notExecutable(...)helpers plus UT fixtures and tests to validate the new failure mode and error framing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| gluten-substrait/src/test/scala/org/apache/gluten/extension/columnar/transition/TransitionSuiteBase.scala | Adds a NoConventionLeaf test fixture representing rowType=None, batchType=None. |
| gluten-substrait/src/test/scala/org/apache/gluten/extension/columnar/transition/TransitionSuite.scala | Adds tests asserting planning-time failures and message framing for child and root cases. |
| gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transitions.scala | Replaces silent handling of from.isNone with Transition.notExecutable and adds a root pre-check in enforceReq. |
| gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transition.scala | Introduces notExecutable exception constructors used by both entry points. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…gnizable Convention
2935ce7 to
3bb2845
Compare
|
Run Gluten Clickhouse CI on x86 |
Do you mean the error is thrown from Spark executor rather than at planning time? |
|
Yes. If this branch were hit, no transition would be inserted for a child whose |
What changes were proposed in this pull request?
InsertTransitions.applyForNodehas a branch that handles the case where a child'sConventionisisNone(row type and batch type bothNone) by returning the child unchanged. The pre-existing comment already notes that such a plan is not executable. No production plan reaches this branch today, but the silent fall-through would defer the failure to task execution. This PR replaces the silent no-op with aGlutenExceptionthat names the offending parent and child. The same latent hole exists inTransitions.enforceReq, which currently reachesassert(!from.isNone, ...)insideTransition.Factory#findTransitionand raises anAssertionErrorwithout plan identity; the fix adds a symmetric pre-check there so both entry points behave the same way.Why are the changes needed?
Defense-in-depth: keeps a code path that is currently unreachable from silently degrading if it ever does become reachable (e.g. a future
GlutenPlanthat inadvertently declares both types asNone). No behavior change for any currently-shipping plan.Does this PR introduce any user-facing change?
No.
How was this patch tested?
mvn -pl gluten-core,gluten-substrait -am -Pspark-3.5 spotless:checkpasses.TransitionSuitegains three tests covering (1) an unexecutable child at position 0, (2) an unexecutable child at a non-first position under a binary parent, and (3) an unexecutable root plan routed throughenforceReq. Each was verified against the pre-fix code and observed to fail with the expected signal. A newNoConventionLeaffixture inTransitionSuiteBasesupplies therowType=None, batchType=Nonestate.