Skip to content

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

Merged
zzylol merged 3 commits into
mainfrom
stack/528-07b-promote-roots
Oct 13, 2026
Merged

zzylol merged 3 commits into
mainfrom
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 use 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

The unified modules replace the legacy ones at the same paths: the legacy frontend modules and their tests are deleted, and the unified files are moved into their place with imports updated. In the merged diff, the physical planner, sources and test files show as renames; the frontend crates show as edits of the root modules plus deletion of unified/. The external-consumer script, the README and test wording are fixed as listed above. No stage logic changes; this only gives the Stage 0 frontends and the physical planner their final paths.

Key code interfaces added / modified

crates/frontend-sql/src/lib.rs, error.rs, sql/mod.rs

// removed: pub mod unified;  (its contents are now the crate root, replacing the legacy lowering)
/// The SQL front end's error type (parse, planning, unsupported feature or dialect, wrong language, resolution).
pub use error::SqlError;
/// `SqlCatalog` holds the table schemas; `SqlLowerer` lowers SQL to the name-based tree for callers that resolve it themselves.
pub use sql::{SqlCatalog, SqlLowerer};

/// Lowers one DataFusion-dialect SQL query to the resolved operator DAG, with `accuracy` on every approximate intent.
pub async fn lower_sql(query: &str, catalog: &SqlCatalog, accuracy: AccuracyTarget)
    -> Result<Rc<OperatorNode>, SqlError>;              // was Result<QueryExpr, SqlError>
/// Like `lower_sql` for a chosen dialect; `ElasticSQL` always returns `UnsupportedDialect`.
pub async fn lower_sql_dialect(query: &str, catalog: &SqlCatalog, dialect: SqlDialect, accuracy: AccuracyTarget)
    -> Result<Rc<OperatorNode>, SqlError>;              // was Result<QueryExpr, SqlError>
/// Lowers every SQL batch entry; one `Result` per entry, so one bad query does not fail the batch.
pub async fn lower_sql_batch(workload: &QueryWorkload, catalog: &SqlCatalog)
    -> Vec<Result<Rc<OperatorNode>, SqlError>>;         // was Vec<Result<QueryExpr, SqlError>>

/// Why SQL lowering failed. `Convert`: resolving names against the bound schema, or deriving a schema, failed.
pub enum SqlError { /* ... */ Convert(ResolveDAGError) }  // ResolveDAGError now from asap_frontend_common

crates/frontend-promql/src/lib.rs, error.rs, histogram.rs

// removed: pub mod unified;  (its contents are now the crate root)
/// The PromQL front end's error type.
pub use error::PromqlError;                             // moved from unified::error
/// Per-metric declared histogram sample types that drive how `histogram_quantile` lowers.
pub use histogram::{HistogramCatalog, HistogramKind};   // moved from unified::histogram

/// Lowers every PromQL workload entry to an operator DAG; needs a non-zero `data_ingestion_interval` valid at `now_ms`.
pub fn lower_promql_workload(workload: &PlanningWorkload, now_ms: u64)
    -> Result<Vec<Rc<OperatorNode>>, PromqlError>;      // was Result<Vec<QueryExpr>, PromqlError>
/// Like `lower_promql_workload`, using `histograms` to choose bucket interpolation vs a sketchable quantile.
pub fn lower_promql_workload_with_histograms(workload: &PlanningWorkload, histograms: HistogramCatalog, now_ms: u64)
    -> Result<Vec<Rc<OperatorNode>>, PromqlError>;      // was Result<Vec<QueryExpr>, PromqlError>
/// Lowers every entry to a `QueryRoot`, keeping scalar roots as scalars instead of constant operators.
pub fn lower_promql_query_workload(workload: &PlanningWorkload, now_ms: u64)
    -> Result<Vec<asap_types::ir::QueryRoot>, PromqlError>;   // moved from unified::
/// Like `lower_promql_query_workload`, with a histogram catalog.
pub fn lower_promql_query_workload_with_histograms(workload: &PlanningWorkload, histograms: HistogramCatalog, now_ms: u64)
    -> Result<Vec<asap_types::ir::QueryRoot>, PromqlError>;   // moved from unified::

/// The sample type behind a histogram metric: classic `le` buckets, native histograms (rejected for now), or raw samples.
pub enum HistogramKind { ClassicBucket, Native, RawSamples }
impl HistogramKind {
    /// Whether `histogram_quantile` over this kind lowers to a sketchable `Quantile` rather than exact bucket interpolation.
    pub fn is_sketchable(self) -> bool;   // now only RawSamples (was every kind but ClassicBucket)
}

crates/frontend-metricsql/src/lib.rs

// removed: pub mod unified;  (its contents are now the crate root)
/// The parsed MetricsQL AST, re-exported from the parser.
pub use metricsql_parser::ast::Expr as MetricsqlExpr;

/// Why MetricsQL lowering failed.
pub enum MetricsqlError {
    /// The query text does not parse (bad input).
    Parse(String),
    /// The workload is not a MetricsQL workload (bad input).
    WrongLanguage(String),                 // new at the root (from unified)
    /// The workload fails validation, e.g. a missing ingestion interval (bad input).
    InvalidWorkload(WorkloadError),        // new at the root (from unified)
    /// The query uses a MetricsQL feature the lowering does not support.
    UnsupportedFeature(String),
    /// Column resolution of the lowered tree failed.
    Resolve(String),
}

/// Lowers one MetricsQL query to an operator DAG; a scalar root is an `UnsupportedFeature` error (use `lower_metricsql_query`).
pub fn lower_metricsql(query: &str, accuracy: AccuracyTarget, ingestion_interval: Duration)
    -> Result<Rc<OperatorNode>, MetricsqlError>;   // was (query, accuracy) -> Result<QueryExpr, MetricsqlError>
/// Lowers one query to a `QueryRoot`; a bare instant selector looks back `ingestion_interval` for its latest sample.
pub fn lower_metricsql_query(query: &str, accuracy: AccuracyTarget, ingestion_interval: Duration)
    -> Result<asap_types::ir::QueryRoot, MetricsqlError>;        // moved from unified::
/// Lowers every MetricsQL workload entry, as `lower_promql_query_workload` does for PromQL.
pub fn lower_metricsql_query_workload(workload: &PlanningWorkload, now_ms: u64)
    -> Result<Vec<asap_types::ir::QueryRoot>, MetricsqlError>;   // moved from unified::

crates/asap-physical-operators/src/lib.rs, dag/mod.rs, expressions/mod.rs, expressions/planner.rs

/// Compiles logical computation to native operators; a node without a native lowering fails compilation.
pub mod physical_planner;   // renamed from unified_physical_planner
/// Raw data access: connectors provide rows, and Scan applies planner predicates.
pub mod sources;            // renamed from unified_sources

// dag/mod.rs
/// Compatibility re-exports under the old `planner` / `scan` names; new code uses the modules directly.
pub use crate::{expressions, operators, physical_planner as planner, sources as scan, values};
    // was unified_physical_planner as planner, unified_sources as scan

// expressions/mod.rs
mod planner;                          // was pub mod unified_planner
/// A planner scalar expression compiled for evaluation over native typed rows.
pub use planner::CompiledExpression;

pub enum Expression {
    /* ... */
    /// A compiled planner scalar expression, evaluated per row.
    Planner(Box<CompiledExpression>), // renamed from UnifiedPlanner
}
impl Expression {
    /// Wraps a compiled planner expression as an `Expression`.
    pub fn planner(expression: CompiledExpression) -> Self;   // renamed from unified_planner
}

crates/devtools/src/lib.rs

/// PromQL lowering re-exported by the devtools facade.
pub use asap_frontend_promql::{
    lower_promql_workload, lower_promql_workload_with_histograms, PromqlError,
};   // was asap_frontend_promql::unified::{...}
/// SQL lowering re-exported by the devtools facade.
pub use asap_frontend_sql::{lower_sql, lower_sql_batch, SqlCatalog, SqlError, SqlLowerer};
     // was asap_frontend_sql::unified::{...}

Usage:

let sql_root: Rc<OperatorNode> = asap_frontend_sql::lower_sql(query, &catalog, accuracy).await?;
let promql_roots: Vec<QueryRoot> = asap_frontend_promql::lower_promql_query_workload(&workload, now_ms)?;
let metricsql_root = asap_frontend_metricsql::lower_metricsql(query, accuracy, ingestion_interval)?;

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).

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,726 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:

zzylol and others added 3 commits October 13, 2026 01:53
…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
@zzylol
zzylol force-pushed the stack/528-07b-promote-roots branch from e6c4216 to b26bfdc Compare October 13, 2026 01:54
@zzylol
zzylol changed the base branch from stack/528-07-planner to main October 13, 2026 01:54
@zzylol
zzylol merged commit 2f94b58 into main Oct 13, 2026
1 check passed
@zzylol zzylol mentioned this pull request Oct 13, 2026
2 tasks done
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