[MINOR][CORE] Seal GlutenCost and drop unreachable case in LongCostModel - #12586
Closed
LuciferYang wants to merge 1 commit into
Closed
[MINOR][CORE] Seal GlutenCost and drop unreachable case in LongCostModel#12586LuciferYang wants to merge 1 commit into
GlutenCost and drop unreachable case in LongCostModel#12586LuciferYang wants to merge 1 commit into
Conversation
…Model Follow-up dead-code cleanup missed by apache#12522 (L3 planner-core review R6): * `GlutenCost` in `extension/columnar/cost/` has always had exactly one implementation (`LongCost`). Turn the trait into a `sealed trait` and co-locate `LongCost` in the same file so the sealed hierarchy is enforced at compile time. Scala 2 requires sealed subclasses to live in the same compilation unit. * Delete `LongCost.scala` (contents merged into `GlutenCost.scala`). * Remove the unreachable `case _ => throw new IllegalStateException(...)` branch in `LongCostModel.costComparator`; after sealing, the compiler proves exhaustiveness on `case LongCost(value) => value`. The two other `match` expressions in `LongCostModel` (`sum` and `diff`) already destructure `(LongCost(_), LongCost(_))` and remain exhaustive after sealing. Downstream call sites of `GlutenCost` (imports listed across `gluten-core`, `backends-velox`, `backends-clickhouse`) are unaffected — this is a compile-time-only tightening with no behavior change.
|
Run Gluten Clickhouse CI on x86 |
LuciferYang
marked this pull request as draft
July 21, 2026 05:39
Contributor
There was a problem hiding this comment.
Pull request overview
Tightens the columnar planner cost type hierarchy by sealing GlutenCost (which is effectively a single-implementation ADT) and cleaning up a now-unnecessary fallback branch in LongCostModel’s comparator.
Changes:
- Change
GlutenCostfrom an opentraitto asealed trait. - Co-locate the sole implementation
LongCostintoGlutenCost.scalaand deleteLongCost.scala. - Remove the unreachable default match case in
LongCostModel.costComparatornow thatLongCostmatching is exhaustive.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| gluten-core/src/main/scala/org/apache/gluten/extension/columnar/cost/LongCostModel.scala | Removes the unreachable fallback branch in the cost comparator match. |
| gluten-core/src/main/scala/org/apache/gluten/extension/columnar/cost/GlutenCost.scala | Seals GlutenCost and defines LongCost in the same compilation unit. |
| gluten-core/src/main/scala/org/apache/gluten/extension/columnar/cost/LongCost.scala | Deleted; implementation moved into GlutenCost.scala. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Follow-up dead-code cleanup missed by #12522.
GlutenCostinextension/columnar/cost/has always had exactly one implementation (LongCost), yet was declared as an opentraitand thecostComparatorfell back through acase _ => throw new IllegalStateException("Unexpected cost type")branch that was unreachable in practice.Changes
trait GlutenCost→sealed trait GlutenCost. Scala 2 requires sealed subclasses to live in the same compilation unit, soLongCostis co-located inGlutenCost.scala.LongCost.scala(its contents were merged intoGlutenCost.scala).case _ => throw new IllegalStateException("Unexpected cost type")branch inLongCostModel.costComparator. After sealing,case LongCost(value) => valueis exhaustive by construction.The two other
matchexpressions inLongCostModel—sumanddiff— already destructure(LongCost(_), LongCost(_))and remain exhaustive after sealing. Downstream imports ofGlutenCostacrossgluten-core,backends-velox, andbackends-clickhouseare unaffected. This is a compile-time-only tightening with no behavior change.How was this patch tested?
./build/mvn -Pbackends-velox -Pspark-3.5 -pl gluten-core -am test-compile -DskipTests: SUCCESS./build/mvn -Pbackends-velox -Pspark-3.5 -pl backends-velox -am test-compile -DskipTests: SUCCESS./build/mvn -Pbackends-clickhouse -Pspark-3.3 -Pdelta -pl backends-clickhouse -am test-compile -DskipTests: SUCCESS./dev/format-scala-code.sh: no additional diff-Wconf:any:e(fatal warnings), so exhaustiveness of the two remainingLongCost(_), LongCost(_)destructure matches is compiler-verified after the seal.grep -rn 'extends GlutenCost\|with GlutenCost'thatLongCostis the sole implementation across the whole repository.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-4-7