Skip to content

refactor: unify logical operators and scalar expressions - #528

Draft
Selvomega wants to merge 24 commits into
mainfrom
refactor/operator-flattening
Draft

Selvomega wants to merge 24 commits into
mainfrom
refactor/operator-flattening

Conversation

@Selvomega

@Selvomega Selvomega commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Revised priority (2026-10-03)

This PR stays open as the #511 reference implementation until Phase A merges, then closes.

Status (2026-10-03, evening): Phase A is rebuilt and pushed as drafts: #567 → #560 → #537 → #539 → #540 → #541 → #542 → #543. Each tip passes clippy and the workspace tests, independently re-run. The design doc split out of #567 is #573, against main. Phase C is up as drafts on #543: #561 (Pass 1) → #575 (Stage 1 workload candidates + stage_pipeline) → #576 (Stage 2/3) → #577 (Example 1 acceptance tests), plus the viewer #574. Phase B is complete as drafts: #578 (B1 types modules) → #579 (top-k readout consistency) → #581 (D1: stage-pipeline facade with tree-DP selection, MajorPass retired) → #582 (D2: Stage 1 without the cost model) → #583 (B2 logical-optimizer) → #584 (B3 physical-optimizer) → #585 (B4 plan-selection) → #586 (B4b facade in planner, asap-aware-mapping removed) → #587 (B5 executor). Follow-up: #580 (Pass 2 sharing and coverage parity).

Phase A — finish #511 operator sharing

Phase B — #572 crate/module reorganization: five pure-move PRs (types modules → logical-optimizer → physical-optimizer → plan-selection → executor).

Phase C — #509 end to end: Example 1 through LogicalDAG → LogicalASAPDAG → PhysicalASAPDAG, with cost estimation, shown in a three-lane DAG viewer and checked by an e2e test. Example 4 (window materialization) comes next. Stage 1 starts from #561.

Parked: #552–#569 and #561 are now drafts, because they sit on the old stack/528-legacy-physical-base chain and Phase B moves their files. Their content is re-scoped in Phase C. #551 is closed because it conflicts with the #572 accuracy decision.


Implements the unified operator/scalar representation in #511 and resolves the main-branch merge.

Before this PR: ordinary and summary plans used separate node types and KeepPreAsap wrappers; scalar computation could become operator nodes. For example, scalar(sum(up)) + 1 required bridge nodes, and SQL scalar-subquery rewrites could lose empty-input/cardinality semantics.

After this PR: both logical stages share OperatorNode with Operator::{NonASAP, ASAP} payloads, common typed schemas and explicit scalar producer edges. ScalarExpr owns literals, arithmetic, evaluation context, conversions and subquery reads; scalar roots need no fabricated relation. KeepPreAsap is removed, and unchanged work is retained directly. Native compilation and flat wire export consume this representation.

  • PromQL math/date operations use scalar projections, including dynamic scalar parameters. Unary minus preserves the metric name; arithmetic/math operations remove it as required.
  • SQL coercions, nullable SUM/window results, scalar-subquery cardinality and NOT IN are preserved. Unsafe scalar-subquery cross joins and ROW_NUMBER rewrites are removed. Unsupported signatures/native histogram samples fail explicitly instead of receiving placeholder semantics.
  • Structural and phase-dependency validation covers real planner candidates. SQL/PromQL document examples now run through lowering/export or native execution, including SQL empty/all-NULL input and scalar(sum(up)) + 1.

The acceptance matrix maps all four requested criteria and the proposal's language comparison tables to implementation/tests. It records parser/runtime gaps and compatibility changes (notably rejected ClickHouse stub signatures, native histograms and older persisted SUM payloads). This implements the proposal's representation scope; it does not claim full current DataFusion/Prometheus execution support. Generalizing analytical evidence to at-rest workloads was split into and merged separately as #532; this branch incorporates the updated main. Physical placement policy remains #520/#530.

Review follow-up: one multi-root workload DAG is explicit in PlanOutput; replacements/CSE use sub-DAG terminology; shared operator properties, schema derivation and errors have separate modules. SketchStatistic and summary “evaluation” replace ambiguous query/readout names. Wire version 7 requires regenerated exports/native programs. A real SQL batch acceptance test selects a shared summary and executes both results; exact-count fixtures now use valid finalizers. Bulk retained evidence cannot hide any ASAP descendant (regression reproduced and fixed).

Validation: after integrating #532, all 1,588 workspace tests pass with two existing ignores. Workspace/all-target/all-feature clippy passes with warnings denied. cargo test --workspace --locked, fmt, clippy with warnings denied, external MetricsQL consumer, and DAG viewer Python tests. The viewer's six Node-dependent tests were skipped because Node.js is unavailable. The vendored MetricsQL baseline passes on Rust 1.99 (CI), verifying its existing 21 library and 3 doctest failures without changing fingerprints. Formatting and clippy also pass locally on Rust 1.99.

Revised logical foundation (#509/#511)

The first five scopes are now ordered as:

  1. feat(ir): define compatible logical summary merges #560 — compatible logical summary merge structure.
  2. feat(ir): add logical sub-DAG sharing and export without execution timing #537 — canonicalization, common sub-DAG sharing and phase-free LogicalASAPDAG export.
  3. feat: lower SQL through unified operators and scalars #539 — SQL operator/scalar lowering.
  4. feat(promql): lower queries to unified operator and scalar IR #540 — PromQL/ MetricsQL operator/scalar lowering.
  5. feat(planner): enumerate local logical alternatives without execution timing #561 — unranked local logical realization inventory, preserving exact/specialized/universal choices.

Timing follows physical materialization; there is no intermediate timed-DAG stage. #541 and its existing descendants remain on the frozen stack/528-legacy-physical-base snapshot until the physical contract/materialization/selection scopes are reorganized. The first-five tip passes 1,961 workspace tests/doctests, formatting and full workspace Clippy.

Summary coverage prerequisite

#567 adds separate summary observation coverage metadata identified as missing in #535. Updated logical review order: #567 → #560 → #537 → #539 → #540 → #561. #560 now requires known disjoint coverage and derives its union; #537 preserves coverage in CSE and logical transport.

Selvomega and others added 11 commits October 2, 2026 00:02
…ieldDataType

Column -> Field, Schema.columns -> fields, SummaryFamilyType -> FieldDataType
(same variants); SummarySchema/SummaryField deleted. Post-ASAP node schemas
are built through Schema::lifted (no unique keys, closed). Adds the unified
operator IR module (ir::{OperatorNode, Operator, NonASAPOp, ASAPOp,
ScalarExpr}) and the lifecycle timing pass alongside the existing types.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…the unified IR; frontend-common resolver crate

- ir::canonicalize / ir::cse port the pre-ASAP passes to Rc<OperatorNode>;
  CSE serves NonASAP and ASAP nodes alike and follows scalar references.
- ir::export emits one post-ASAP node per operator (wire version 6):
  relational operators are Relational payloads, scalar references are
  ScalarRef edges, no embedded subtrees.
- ir::timing writes execution timings from a LifecycleAssignment and
  validates every edge; nodes carry no timing before that pass.
- asap-frontend-common owns the name-based tree front ends build
  (UnresolvedOp / UnresolvedScalar) and resolve_root, which binds names,
  derives schemas and canonicalizes into Rc<OperatorNode>.
- ParsedWorkload roots are Rc<OperatorNode>.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
One builder serves export / export_summary / export_post_asap; nodes are
deduplicated by pointer, scalar operator references render as scalar_ref
ids, every node carries structural_hash.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The SQL front end now emits ScalarExpr::{Exists, InSubquery, ScalarSubquery};
canonicalize rewrites them to the semi/anti/cross joins the planner saw before.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
SQL, PromQL and MetricsQL front ends build UnresolvedOp/UnresolvedScalar
and return Rc<OperatorNode> from resolve_root. New: SQL Values (SELECT
without FROM, VALUES), unary minus as Negative, ExprSemantics on every
scalar comparison/arithmetic, uncorrelated EXISTS / IN / scalar subqueries
as scalar expressions (lowered to joins by canonicalize); PromQL
TimeRange.kind (instant vs range selector), the bool modifier
(return_bool), scalar negation as Negative.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e unified IR (library)

Replacement::{Summary, Rewrite} collapse into Subtree(Rc<OperatorNode>);
assemble_residual becomes one rule (keep the operator, assemble its
children); nodes carry no timing, legality checks run through
ir::timing::validate_default and export through ir::export after
apply_lifecycle_timings. Test modules are migrated separately.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… kept subtree one node

keep_pre_asap memoizes its exact-guarantee copy per input node, so a Scan
kept by two candidates (AVG's SUM and COUNT branches) stays one Rc. The
viewer contract test moves to crates/devtools/tests/viewer_contract.rs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Developer and architecture docs, the user guide and the viewer README now
describe one OperatorNode DAG, lifecycle-assigned timing and wire version 6;
the viewer renderer reads schema fields.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gressions

Regressions caught by the migrated suites:
- accuracy-evidence and runtime-support checks now cover summary decisions
  rooted in a relational operator above their readouts;
- exact read boundaries keep the placement binding chose (query-time
  snapshot vs. maintenance feed); the timing pass honors it;
- generic assembly keeps a value-computing operator (aggregate, binary op,
  window) exact over an approximate input instead of passing a guarantee
  through it;
- table populations name their unified-IR input, so filtered SQL scans are
  recognized again.

Adds the #468 acceptance tests (integration-tests/tests/operator_sharing.rs).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QueryExpr, SummaryNode/SummaryExpr, ValueOperation, the wire-5 post-ASAP
DAG, the old resolver, canonicalize and both CSE passes are gone; the
unified ir module is the only operator language. pre_asap keeps the shared
vocabulary (query_expr.rs renamed vocabulary.rs); post_asap keeps summary
state and timing vocabulary. CSE compares a node's own fields with the
signed-zero-aware check. cargo fmt --all.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol zzylol changed the title refactor: operator flattening Design: operator sharing and separating operator and ScalarExpr in logical plan Oct 2, 2026
Merge main into refactor/operator-flattening while retaining the unified
OperatorNode graph proposed in #511. Port native physical compilation,
aggregate filters, shared summaries and lifecycle placement to that graph.

Remove ScalarBridge, preserve scalar workload roots as owned expressions,
and lower mixed PromQL arithmetic and comparisons to Project/Filter with
complete series identity and explicit vector/scalar conversions.

Preserve native execution coverage and correct cross-selector maintenance
eligibility and empty PromQL aggregation behavior.

Validation: 1,569 workspace tests pass; workspace all-target/all-feature
Clippy with warnings denied, rustfmt, and MetricsQL external consumer pass.
Vendored MetricsQL baseline retains its main-branch sources and checker:
lib baseline passes, but one existing doctest diagnostic hash differs under
rustc 1.98.1 (same expected failing test set).
@zzylol zzylol changed the title Design: operator sharing and separating operator and ScalarExpr in logical plan refactor: unify logical operators and scalar expressions Oct 2, 2026
Comment thread crates/asap-aware-mapping/src/accuracy/reconciliation.rs Outdated
Comment thread crates/asap-aware-mapping/src/accuracy/reconciliation.rs Outdated
use asap_types::pre_asap::query_expr::{GroupKeys, Source};
use asap_types::pre_asap::schema::{Column, ColumnId, DataType, Schema};
use asap_types::pre_asap::schema::{ColumnId, DataType, Field, Schema};
use asap_types::pre_asap::vocabulary::{GroupKeys, Source};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is called vocabulary

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it reflect the actual content?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“Vocabulary” was too vague and the module mixed different responsibilities. It contained operator parameter types (joins, windows, predicates, reductions, etc.), aggregate schema derivation, and IR errors. Split it into ir::operator_properties, ir::aggregate_schema, and ir::error; migrated imports and removed pre_asap::vocabulary. The modules now describe their actual contents and belong to the common IR. Change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ir::operator_properties, ir::aggregate_schema, and ir::error what does these three mean then?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For your follow-up, these modules separate three concrete responsibilities:

  • ir::operator_properties defines settings stored inside operator payloads: e.g. GroupKeys says which columns an aggregate groups by, JoinKind selects inner/left/etc., and WindowFrame describes a SQL window. It does not hold derived node metadata such as schema, guarantee, or timing.
  • ir::aggregate_schema computes an aggregate's output schema from its input schema and reduction. For example, grouping by host preserves that column and adds the aggregate result column with its derived type. It does not compute aggregate values.
  • ir::error defines QueryExprError for schema/type derivation failures: invalid grouping-column indices, scalar signatures, sample columns, or an empty concatenation. My previous module description was too broad: structural and execution-timing validation have separate errors.

fc328b12 adds these explanations and examples to the module docs, the IR module index, and the acceptance document. This makes the boundaries explicit without introducing another abstraction or changing behavior. Workspace tests and clippy pass.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that QueryExpr is removed, why QueryExprError still here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Selvomega needing your help on this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right: QueryExprError was a leftover name from the removed QueryExpr, not a separate concept that needed to survive the migration. Fixed in 150735f2: it is now ir::SchemaDerivationError, describing its actual role in deriving operator output schemas and scalar types. Updated signatures, imports, re-exports, error construction, and documentation throughout; no QueryExprError alias remains. Error variants and behavior are unchanged. Types/frontend-common tests, formatting, and workspace/all-target/all-feature clippy pass.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much stuffs is still pending on this thread?

Comment thread crates/asap-aware-mapping/src/accuracy/reconciliation.rs Outdated
Comment thread crates/asap-aware-mapping/src/accuracy/reconciliation.rs Outdated
Comment thread crates/asap-aware-mapping/src/summary_maintenance_cost/estimator.rs Outdated
Comment thread crates/asap-aware-mapping/src/summary_maintenance_cost/evidence.rs Outdated
Comment thread crates/asap-aware-mapping/src/summary_maintenance_cost/model.rs Outdated
Comment thread crates/asap-aware-mapping/src/summary_maintenance_cost/model.rs Outdated
Comment thread crates/asap-aware-mapping/src/summary_maintenance_cost/model.rs Outdated
@zzylol

zzylol commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

The eight-PR split is implemented and pushed. Each PR targets the preceding branch, so its Files changed tab shows the incremental review scope.

Order PR Review focus
1 #535 Common value and summary-state schema
2 #536 Unified operator/scalar IR and structural contracts
3 #537 Canonicalization, sharing, timing, scalar dependencies, wire export
4 #539 SQL lowering and shared frontend resolution
5 #540 PromQL and MetricsQL operator/scalar lowering
6 #541 Native compilation and execution of the new IR
7 #542 Production workload planner cutover, ASAP optimization, shared result roots
8 #543 Delete obsolete/transitional sources, migrate viewer, consolidate acceptance docs

The chain starts at main (69cc2f91, including #511 and #532). The final tree at 6c2c313d is byte-for-byte identical to #528 at 150735f2, verified with Git. This preserves the reviewed implementation while changing its review/merge boundaries. #530's general computation-placement work remains separate.

Each layer was checked locally. The first six retain the existing production path while adding the new representation, lowerers, and compiler. #542 switches the compiled callers together and proves real batch summary sharing and execution of both result roots. #543 deletes the now-unused sources. Review #542 as the main integration change; some of its frontend/runtime diff is promotion of code already reviewed in earlier layers.

Final validation: 1,588 workspace tests/doctests passed, formatting and workspace/all-target/all-feature Clippy passed, and viewer tests passed (24 passed; 6 Node-dependent tests skipped because Node is unavailable). GitHub CI is green for #535–#540; the latest layers are still running.

Please review/merge in the table order. After each merge, retarget the next PR to main and restack it if the merge method changes ancestry. Nothing has been merged or closed; #528 remains the reference/tracking PR until the stack lands.

@zzylol

zzylol commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

The complete stack (#535, #536, #537, #539, #540, #541, #542, #543) is now rebased and pushed onto latest main, 7734c68f (#544). Each PR still targets the preceding branch. The rebase preserves the new DAG API/wire names and the #535 schema JSON compatibility and ColumnId fixes.

#535 also updates the two #511 design documents to distinguish Schema/Field metadata from ColumnRef/ColumnId references, including the schema-position/runtime-value example.

Validation: all eight layers pass workspace/all-target/all-feature Clippy; final workspace tests/doctests: 1,590 passed; formatting passes; viewer tests: 25 passed, 6 Node-dependent skips. Remote CI has been retriggered. All eight branches were pushed atomically with explicit leases. No PRs were merged or closed.

This supersedes the earlier base/tree-equality statements in the initial stack index: the stack now includes newer main changes and review fixes.

@zzylol

zzylol commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Stack update: #535 has merged. The terminology-only SketchQuery → SketchStatistic and PopulationReadout → PopulationStatistic changes are now isolated in #549; #536 is based on it and excludes those renames from its incremental diff.

Current review order: #549 → #536 → #537 → #539 → #540 → #541 → #542 → #543. All dependent branches have been restacked and pushed, preserving the merged schema / SchemaRef changes.

Also addressed the #536 constructor review: both operator categories now use OperatorNode::new_shared(Operator), with explicit schema/guarantee builders where required. Updated callers throughout the stack. Validation: Clippy passed at every stack layer; final workspace tests passed (1,591 tests), as did formatting. Viewer tests passed earlier in this restack (25 passed, 6 Node-dependent skips).

@zzylol
zzylol marked this pull request as draft October 3, 2026 20:02
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.

2 participants