andygrove opened a new issue, #6979: URL: https://github.com/apache/datafusion-comet/issues/6979
### What is the problem the feature request solves? `native/core/src/execution/planner.rs` is about 7,150 lines and has had 166 commits in the last year, making it the most frequently edited file in the native code and a regular source of merge conflicts. Most of that weight is in two functions: - [`PhysicalPlanner::create_plan` (L1711-L3030)](https://github.com/apache/datafusion-comet/blob/7d8d9b1544b734c85a93cca2cc9d5380ad70accd/native/core/src/execution/planner.rs#L1711-L3030): 1,081 lines, cyclomatic complexity 91, cognitive complexity 167. Its arms inline full operator construction: HashAgg (~230 lines), Explode (~160), MergeRows (~140), Window (~110), HashJoin (~95), plus Filter, BroadcastNestedLoopJoin, Limit, Sort, scans, Expand, SortMergeJoin and others. - [`PhysicalPlanner::create_expr` (L753-L1342)](https://github.com/apache/datafusion-comet/blob/7d8d9b1544b734c85a93cca2cc9d5380ad70accd/native/core/src/execution/planner.rs#L753-L1342): 532 lines, cyclomatic complexity 83. The Literal arm alone is a large nested match independent of planner state, and Cast and CheckOverflow duplicate the query-context lookup. Both functions already start by consulting a registry (`OperatorRegistry` / `ExpressionRegistry`) and fall back to what the code itself calls the "original monolithic match", so the migration path exists but is unfinished. The match arms are independent of each other (each recurses into children and builds one plan or expression), so their size is accidental rather than inherent. This is related to #5639 (splitting the `core` crate), but separate: even after a crate split, these two functions would still be the conflict hotspot. ### Describe the potential solution Finish the registry migration, one operator or expression family per PR so each change stays reviewable. Proposed sub-tasks: - [ ] Move the largest `OpStruct` arms into registered builders: MergeRows, Explode, HashAgg, HashJoin and SortMergeJoin (sharing a join-key helper), Window. - [ ] Move the remaining `OpStruct` arms (Filter, Limit, Sort, Expand, scans, BroadcastNestedLoopJoin, ...), leaving `create_plan` as a thin dispatcher. - [ ] Move `ExprStruct` arms of `create_expr` into registered builders by family. As first steps, extract `literal_to_scalar(literal) -> Result<ScalarValue>` as a free function and a `query_context_for(expr_id)` helper. - [ ] Extract window frame conversion from [`create_window_expr` (L3666-L3880)](https://github.com/apache/datafusion-comet/blob/7d8d9b1544b734c85a93cca2cc9d5380ad70accd/native/core/src/execution/planner.rs#L3666-L3880) into `convert_window_frame`. The lower and upper bound conversions are near-parallel copies, the `WindowFrameUnits::Groups` "not supported" error is repeated five or more times and could be rejected once up front, and the two copies already differ: a Range offset uses `offset.offset.abs()` for the lower bound (L3781) but plain `offset.offset` for the upper bound (L3840). That may be intended, but it should be pinned with a comment or test. - [ ] Move Iceberg task parsing ([`parse_file_scan_tasks_from_common`, L4621-L4984](https://github.com/apache/datafusion-comet/blob/7d8d9b1544b734c85a93cca2cc9d5380ad70accd/native/core/src/execution/planner.rs#L4621-L4984), 273 lines) out of planner.rs into its own module, split into pool-parsing helpers, and replace the six repeated `get(idx).ok_or_else(Invalid X_idx ...)` constructions with one `pool_get` helper. - [ ] Move the ~2,500 lines of `mod tests` into a separate file. ### Additional context Line references are pinned to commit 7d8d9b1544b7. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
