[MINOR][CORE] Remove dead code across gluten-core#12590
Conversation
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
Removes several unused / unreachable code paths in gluten-core identified via Scala unused warnings and prior dead-code review themes, aiming to reduce maintenance surface area without changing behavior for reachable paths.
Changes:
- Removed unused
GlutenCoreConfiginstantiation/import fromGlutenInjector.applier. - Removed unreachable fallback
case _inTransition.Factory#findTransition(sealed 2×2 requirement product already fully covered). - Cleaned up unused locals/pattern vars and removed an unused regex
Patternconstant; removed an unused builder cache field inFloydWarshallGraph.Builder.Impl.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| gluten-core/src/main/scala/org/apache/gluten/extension/injector/GlutenInjector.scala | Drops unused GlutenCoreConfig import/instantiation in applier. |
| gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transition.scala | Removes unreachable case _ branch in findTransition requirement matching. |
| gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/package.scala | Replaces unused pattern vars with _ in canPropagateConvention. |
| gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/FloydWarshallGraph.scala | Removes never-read cached graph field and invalidation writes in builder. |
| gluten-core/src/main/scala/org/apache/gluten/config/ConfigBuilder.scala | Removes unused TIME_STRING_PATTERN and Pattern import; keeps delegation to JavaUtils.timeStringAs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| private def applier(session: SparkSession): ColumnarRuleApplier = { | ||
| val conf = new GlutenCoreConfig(session.sessionState.conf) |
There was a problem hiding this comment.
This should not be removed, as this would register the GlutenCoreConfigs.
There was a problem hiding this comment.
Reverted — kept the val conf instantiation. You're right that new GlutenCoreConfig(...) forces GlutenCoreConfig object initialization and its config registration, so removing it is not side-effect-free. Dropped this hunk from the PR.
| Transition.empty | ||
| case _ => | ||
| throw new UnsupportedOperationException( | ||
| s"Illegal convention requirement: $ConventionReq") |
There was a problem hiding this comment.
This is a protect code, we can leave it here.
There was a problem hiding this comment.
Reverted — kept the case _ guard. Agreed it's protective against future additions to the sealed RowType/BatchType hierarchy even though the current 2x2 is exhaustive. Dropped this hunk from the PR.
Cleanup pass over `gluten-core` that removes three dead code paths surfaced by the `Dead code / dead cases` cross-cutting theme from the earlier L1/L2/L3 review docs. No behavior change on reachable paths. 1. `FloydWarshallGraph.Builder.Impl`: drop the never-read `private var graph` field and its two `graph = None` writes in `addVertex` / `addEdge`. Remnant of an abandoned build-cache mechanism; the live cache is `Transition.factory#graphCache`. 2. `ConfigHelpers.TIME_STRING_PATTERN`: drop the compiled `Pattern` plus its `java.util.regex.Pattern` import. `timeFromString` delegates to `JavaUtils.timeStringAs`; the regex has no reference. 3. `canPropagateConvention`: rename five unused pattern vars `p` to `_` in the `case _: X => true` arms. Dropped from the initial version after review: - `GlutenInjector.applier` keeps `val conf = new GlutenCoreConfig(...)` since instantiating it forces `GlutenCoreConfig` registration. - `Transition.Factory#findTransition` keeps the `case _` branch as a guard against future additions to the sealed RowType / BatchType hierarchy.
cac5067 to
a7dedf6
Compare
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
What changes were proposed in this pull request?
Cleanup pass over
gluten-corethat removes three dead code paths surfaced by theDead code / dead casescross-cutting theme in the earlier L1/L2/L3 review docs. No behavior change on reachable paths.FloydWarshallGraph.Builder.Impl: drop the never-readprivate var graphfield and its twograph = Nonewrites inaddVertex/addEdge. Remnant of an abandoned build-cache mechanism; the live cache isTransition.factory#graphCache.ConfigHelpers.TIME_STRING_PATTERN: drop the compiledPatternplus itsjava.util.regex.Patternimport.timeFromStringdelegates toJavaUtils.timeStringAs; the regex has no reference.canPropagateConvention: rename five unused pattern varspto_in thecase _: X => truearms.Two changes from the initial version were dropped after review:
GlutenInjector.applierkeepsval conf = new GlutenCoreConfig(...): instantiating it forcesGlutenCoreConfigregistration, so it is not safe to remove.Transition.Factory#findTransitionkeeps thecase _branch as a guard against future additions to the sealedRowType/BatchTypehierarchy.How was this patch tested?
-Wconf:any:e(fatal warnings):./build/mvn -Pbackends-velox -Pspark-3.5 -pl gluten-core -am install -DskipTests../dev/format-scala-code.sh: no additional diff.grepverified zero references toTIME_STRING_PATTERNand to the removedFloydWarshallGraph.Builder.Impl.graphfield.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude claude-opus-4-7