Skip to content

[EPIC] Finish moving PhysicalPlanner::create_plan and create_expr onto the operator and expression registries #6979

Open

Description

@andygrove

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): 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): 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) 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, 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 7d8d9b1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions