feat(control_plane): Phase 2 step 1 -- merge schema.rs onto asap-ir - #393
Merged
Merged
Conversation
Column/ColumnId/DataType/Schema/CseError/cse_reuse_is_legal are now
re-exported from asap_ir::intent_algebra::schema instead of defined
locally. Unlike AggIntent's merge (Phase 1b), this needed no boundary-
conversion layer: fresh diff showed asap_ir's version is a purely
additive, backward-compatible superset of the pre-merge local type --
- Column gains `table: Option<String>` (SQL join qualifier) +
Column::new()/with_table() constructors.
- Schema gains `closed: bool` (schema-on-read completeness flag) +
column_id_qualified().
- DataType is byte-identical, no changes.
Both new fields are #[serde(default)], confirmed backward-compatible by
asap_ir's own tests. This is what made a full swap the right call here
instead of Phase 1b's boundary-conversion approach for Column/DataType --
deleted agg_intent.rs's to_asap_column/from_asap_column/to_asap_dtype/
from_asap_dtype helpers, no longer needed once there's only one
Column/DataType type.
Blast radius was much smaller than the migration plan's stale ~38-site
estimate: 15 real Column{}/Schema{} struct-literal construction sites
across 9 files needed the new field added (table: None / closed: false)
-- most of the original grep hits were field references or doc comments,
not constructions.
Verified: full workspace builds clean; control_plane's 820-test suite
passes unchanged (same 1 pre-existing failure as #392, confirmed
unrelated).
Remaining Phase 2 work (docs/migration-plan-backend-plan.md): expr_ir.rs,
query_expr.rs/relational.rs fresh diff, binder.rs/column_resolution.rs
fresh diff, cse.rs move to L4, lower.rs, PromQL/SQL frontend retarget.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #392 (base branch set accordingly -- merge #392 first, then
this). First step of Phase 2 (
docs/migration-plan-backend-plan.md):schema.rs.Column/ColumnId/DataType/Schema/CseError/cse_reuse_is_legalare now re-exported from
asap_ir::intent_algebra::schemainstead ofdefined locally. Unlike
AggIntent's merge in #392 (which needed aboundary-conversion layer because
AccuracyTarget's wire shape actuallychanged), this one is a clean full swap -- a fresh diff showed asap_ir's
version is a purely additive, backward-compatible superset:
Columngainstable: Option<String>(SQL join qualifier) +Column::new()/with_table()constructors.Schemagainsclosed: bool(schema-on-read completeness flag) +column_id_qualified().DataTypeis byte-identical, no changes at all.Both new fields are
#[serde(default)], confirmed backward-compatible byasap_ir's own tests (
schema_closed_defaults_to_open_when_absent,column_table_defaults_to_none_when_absent). That's what made a fullswap the right call here instead of #392's boundary-conversion approach
for
Column/DataType-- and it let me delete theto_asap_column/from_asap_column/to_asap_dtype/from_asap_dtypehelpers
agg_intent.rsneeded in #392, since there's only oneColumn/DataTypetype now.Blast radius was smaller than expected
The migration plan flagged ~38
Column{}literals as the risk here.Turned out most of those grep hits were field references or doc comments,
not constructions -- only 15 real struct-literal sites across 9 files
needed the new field added (
table: None/closed: false).Verification
cargo build --workspace-- cleancargo test -p control_plane --lib-- 820 passed, same 1 pre-existingfailure as feat(control_plane): Phase 1b -- depend on asap-ir for real, not a copy #392 (confirmed unrelated)
rustfmtappliedRemaining Phase 2 work
Per the migration plan:
expr_ir.rs(new file, D2 decision), fresh diffon
query_expr.rs/relational.rs, fresh diff onbinder.rs/column_resolution.rs,cse.rsmove to L4 (with theregression pass that requires),
lower.rs, then retargeting thePromQL/SQL frontends.
Test plan
cargo build --workspacecargo test -p control_plane --lib🤖 Generated with Claude Code