Skip to content

refactor: move the unified frontends and physical planner to the crate roots - #665

Open
zzylol wants to merge 3 commits into
stack/528-07-plannerfrom
stack/528-07b-promote-roots
Open

zzylol wants to merge 3 commits into
stack/528-07-plannerfrom
stack/528-07b-promote-roots

Conversation

@zzylol

@zzylol zzylol commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Why

After #542 the planner calls the unified modules through their temporary unified paths, and the replaced legacy frontend modules are dead. This PR gives the unified code its final paths, so the crates read as they will stay.

What

Moves, import updates and the documentation/tooling that names the moved APIs. No planner or execution behavior change.

  • asap_frontend_{sql,promql,metricsql}::unified become the crate roots; the replaced legacy frontend modules and their tests are deleted.
  • unified_physical_planner / unified_sources become physical_planner / sources; expressions/unified_planner.rs becomes the private expressions/planner.rs (re-exported as expressions::CompiledExpression), and Expression::UnifiedPlanner becomes Expression::Planner.
  • The unified_* tests take the names of the tests they replace (sql_lowering.rs, promql_lowering.rs, promql_conformance.rs, histogram_metadata.rs, scalar_design.rs, support.rs, common/, precompute.rs, node_ids.rs; MetricsQL lowering_semantics.rs).
  • crates/asap-physical-operators/README.md is corrected: physical_planner::compile takes a Planner PhysicalASAPDAG (not PostAsapDAG), the separate unified_physical_planner paragraph is folded in, and the module list says evaluation instead of readout.
  • tools/verify_metricsql_external_consumer.sh calls lower_metricsql with the new ingestion-interval argument and removes its temporary crate on exit.
  • Tests: the moved files take the stack's wording ("tree" → "DAG", "readout" → "evaluation"); the MetricsQL legacy lowering test is ported to the unified API with the ingestion-interval parameter; the duplicate default_rollup-without-range test is removed (rollups_without_an_explicit_range_are_rejected covers it).

How

Three commits:

  1. Remove the legacy frontend modules and tests that the unified ones replace (pure deletion). This commit does not build on its own; skip it when bisecting.
  2. Move with git mv and update imports. Viewed per commit, GitHub shows the moved files as renames (R100 or near).
  3. Fix the external-consumer script, the README and test wording listed above.

Before this PR

The unified code lives at …::unified, unified_physical_planner, unified_sources, next to dead legacy frontend modules whose root functions still return QueryExpr.

After this PR

One SQL, PromQL and MetricsQL frontend at each crate root, and one physical_planner. Callers import from the roots. Public API changes at the crate roots (the root functions now have the former unified signatures):

  • SQL: lower_sql, lower_sql_dialect return Rc<OperatorNode>; lower_sql_batch returns Vec<Result<Rc<OperatorNode>, SqlError>> (previously QueryExpr).
  • PromQL: lower_promql_workload and lower_promql_workload_with_histograms return Vec<Rc<OperatorNode>>; lower_promql_query_workload and lower_promql_query_workload_with_histograms (returning Vec<QueryRoot>) are new at the root.
  • MetricsQL: lower_metricsql(query, accuracy, ingestion_interval: Duration) returns Rc<OperatorNode> (previously (query, accuracy) → QueryExpr); lower_metricsql_query (→ QueryRoot) and lower_metricsql_query_workload (→ Vec<QueryRoot>) are new at the root.
  • Physical operators: unified_physical_planner → physical_planner, unified_sources → sources, Expression::UnifiedPlanner → Expression::Planner.
  • The unified modules no longer exist.

Evidence

Not applicable (no behavior change). The commit-by-commit diff shows the renames.

Verification

  • Unit, integration and end-to-end tests: unchanged apart from paths, wording and the one removed duplicate.
  • Other checks on the PR head: cargo fmt --check; workspace all-target Clippy with -D warnings; cargo test --workspace (1,714 passed, 0 failed, 2 ignored); dag-viewer test_render (31 passed); tools/verify_metricsql_external_consumer.sh OK.

Architectural decisions

Limitations and follow-up

Stack: tracker #528 · main → #542 → #665 → #666 → #543 → #667 → #561.

Human review — do not complete with an agent

  • The MVP boundary is correct.
  • New conceptual layers or public interfaces are necessary.
  • The before/after description matches the intended product behavior.
  • Human reviewer:
  • Decision and rationale:

🤖 Generated with Claude Code

https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW

zzylol and others added 3 commits October 12, 2026 21:51
…ones

Since the previous commit nothing calls the legacy SQL, PromQL and
MetricsQL lowerings or their tests; the unified modules take their paths
in the next commit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
…e roots

Moves plus import updates: `asap_frontend_{sql,promql,metricsql}::unified`
become the crate roots, `unified_physical_planner` / `unified_sources` /
`expressions::unified_planner` become `physical_planner` / `sources` /
`expressions::planner`, and the `unified_*` tests take the names of the
tests they replace. The moved files also take the DAG wording ("tree" ->
"DAG", "readout" -> "evaluation") the rest of the stack uses, and the
MetricsQL frontend's legacy lowering test is ported to the unified API
with the ingestion-interval parameter.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
…iew cleanups

The external-consumer script calls lower_metricsql with the ingestion
interval it now takes, and removes its temp crate on exit. Also: the
physical-operators README names the input a Planner PhysicalASAPDAG,
expressions/mod.rs names CompiledExpression directly, lowering test locals
and prose say dag/DAG, and the default_rollup-without-range test is dropped
since rollups_without_an_explicit_range_are_rejected covers it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W7qG9aFyPij5uWsyAJCxDW
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant