[MINOR][CORE] Make ColumnarInputAdapter inherit from GlutenPlan - #12973
[MINOR][CORE] Make ColumnarInputAdapter inherit from GlutenPlan#12973yikf wants to merge 1 commit into
Conversation
aece885 to
93f0c63
Compare
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🟢 Approval recommended
The change is a small, behavior-preserving refactor that aligns ColumnarInputAdapter with existing GlutenPlan convention/support mechanisms without introducing functional differences.
Pull request overview
This PR refactors ColumnarInputAdapter (used by ColumnarCollapseTransformStages) to inherit from GlutenPlan, relying on GlutenPlan’s bundled convention traits and its derived supportsColumnar / supportsRowBased behavior, and removing the now-redundant explicit convention plumbing.
Changes:
- Make
ColumnarInputAdapterextendGlutenPlaninstead of directly mixing inConvention.*/ConventionReq.*traits. - Remove redundant
supportsColumnar/supportsRowBasedoverrides and the explicitrequiredChildConvention()override. - Simplify imports by dropping the now-unused
ConventionReqimport.
File summaries
| File | Description |
|---|---|
| gluten-substrait/src/main/scala/org/apache/spark/sql/execution/ColumnarCollapseTransformStages.scala | Refactors ColumnarInputAdapter to inherit from GlutenPlan and removes redundant convention/support overrides accordingly. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
93f0c63 to
39b4bb9
Compare
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🟡 Changes recommended
The ClickHouse TPCDS metrics suite references ColumnarInputAdapter without it being in scope, causing a likely compilation failure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| case g: GlutenPlan if !g.isInstanceOf[InputIteratorTransformer] => g | ||
| case g: GlutenPlan | ||
| if !g.isInstanceOf[InputIteratorTransformer] && | ||
| !g.isInstanceOf[ColumnarInputAdapter] => |
39b4bb9 to
fc52b77
Compare
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🟢 Approval recommended
The change correctly aligns ColumnarInputAdapter with GlutenPlan’s established conventions, and the impacted unit tests are updated accordingly to reflect the new type relationship.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
What changes are proposed in this pull request?
ColumnarInputAdapter previously mixed in Convention.KnownBatchType, Convention.KnownRowType and ConventionReq.KnownChildConvention directly.
GlutenPlanalready bundles those three traits and derivessupportsColumnar/supportsRowBasedfrombatchType()/rowType(). This PR makesColumnarInputAdapterinherit fromGlutenPlanand removes the now-redundant overrides.How was this patch tested?
Exists UTs.
Was this patch authored or co-authored using generative AI tooling?
Yes.