Repository navigation
chore: reconcile stale stubs + docs with the landed code - #99
Merged
Merged
Conversation
Hygiene pass from the whole-repo review. No behavior changes — the full workspace suite and clippy are untouched-green. Stubs (crates/plan): - Delete src/canonicalize.rs: its TODO(#34) asked for exactly what already shipped — canonicalize() landed in asap-l2 (run inside the shared convert_root, cross-language tests in crates/lower/tests) and #34 is closed. - Re-point src/boundary.rs from the dead TODO(#34) to #98 (the L3→L4 binding + sketch-vs-exact boundary tracking issue), spelling out its inputs (AccuracyTarget, agg_is_exact/agg_is_mergeable, HistogramKind sketchability). - lib.rs: note canonicalization lives in asap-l2, drop the deleted module. crates/e2e: fix the doc claim that it owns cross-language equivalence tests — it is PromQL-only; the cross-language tests live in crates/lower. Docs: - README: L2 row pointed at asap-ir (it is asap-l2, incl. canonicalize); SketchExpr → the shipped SummaryExpr/L4Node naming (+ binding pass = #98); plan stub list, directory tree, LOC counts, histogram.rs, e2e label. - design.md: the "AggIntent core is 9" claim updated to the shipped ~40 with the growth rationale and agg_intent.rs as source of truth; the three "HistogramQuantile was removed from L3" sites revised to the landed two-lowering design (#43/#79, intent not node); SketchExpr↔SummaryExpr naming note at the §6 definition + glossary; Source enum matched to the shipped thin form (no Source::Join); QueryExpr sketch annotated with the shipped node additions; workspace-members listing gains crates/l2. - promql-lowering.md: dead paths (front end is crates/frontend-promql, binder/ column_resolution are crates/l2), convert_root snippet now shows the canonicalize step, moved test path. - intent-algebra-reconciliation.md: post-reorg banner (crates/core→ir/l2/plan map; the CP-intent gap since absorbed by #43-#51 — tables are historical evidence), front-end paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Hygiene pass from the whole-repo review (no behavior changes; workspace suite + clippy untouched-green). Pairs with the new tracking issue #98 (L3→L4 binding + boundary decision).
Stubs (
crates/plan)src/canonicalize.rs— itsTODO(#34)asked for exactly what already shipped:canonicalize()landed inasap-l2(run inside the sharedconvert_root; cross-language tests incrates/lower/tests/cross_language.rs) and L3 IR standardization: cross-language correctness and canonicalization #34 is closed. The stub was a duplicate signpost pointing at done work.src/boundary.rsfrom the deadTODO(#34)to L4: implement the L3→L4 binding (AggIntent→SummaryKind) and the sketch-vs-exact boundary decision #98, spelling out the decision's inputs (AccuracyTarget, the currently caller-lessagg_is_exact/agg_is_mergeable, andHistogramKindsketchability from PromQL: histogram_quantile classic-bucket vs sketch-able quantile is a structural heuristic — drive it from sample type/metadata #79).lib.rs: canonicalization's real home noted; module list updated.crates/e2eFix the crate-doc claim that it owns cross-language equivalence tests — it is PromQL-only by design (depends only on the PromQL front end); the cross-language tests live in
crates/lower.Docs
Reconciled against the shipped code (the review found the staleness concentrated in L3/L4 vocabulary):
asap-ir::intent_algebra::relationalasap-l2(incl. the sharedcanonicalizepass)SketchExpr(README, design §6, glossary)SummaryExpr/L4Node— naming note at the definition + fixed references; binding pass = #98AggIntentcore is 9"agg_intent.rsnamed source of truthHistogramQuantileremoved from L3"AggIntent::HistogramQuantile(not re-sketchable), native/raw → genericQuantile; intent, not nodeSourcehasJoin/time/labels/columnsSource::Joinmarked unimplemented-draftQueryExprsketchScalar/EvalTime/bridges/Relabel/Sample/InfoJoin/TimeRange)crates/l2was missingcrates/frontend-promql, binder/resolution →crates/l2);convert_rootsnippet now shows the canonicalize step; moved test pathcrates/core→ ir/l2/plan map; the "CP has 11 intents ASAP lacks" table marked historical (absorbed by #43–#51)The §5.2 target-layout tree and migration-plan.md keep their original
core::<module>names — both already carry explicit reconciliation banners mapping them to the landed split, so they were left as intended.