Skip to content

Commit 55a4c88

Browse files
committed
Docs: fold the batch 2 code review into T1.7c
The batch 2 review of T1.6a and T1.7 found two blockers: a long AND or OR chain escapes MAX_DEPTH and can overflow the stack outside containment, and admission refuses Arrow types the reference accepts. Decision H27 makes its blockers, should-fix items and nits exit checks of T1.7c: depth-bounded chains with the tree dropped inside one grown stack, admission widened losslessly with HOST-2 for anything still refused, pinned differing corpus IDs, reference date readings compared, E10's refusal registered as DT-9 with a test T1.6b must flip, the validated-query format read with serde_stacker, over-cap results carried by contract v3's attachment, panics reported out of band, and a test-only panicking operation through HostRun::start. T1.7 goes back to in progress until T1.7c lands. Signed-off-by: Adi Wadaskar <awadaskar@nvidia.com>
1 parent 32b26ed commit 55a4c88

5 files changed

Lines changed: 70 additions & 15 deletions

File tree

‎docs/design/rust-core/phase-1/decisions.md‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ Decisions made for Phase 1, newest last.
44
Rows marked "orchestrator, owner to confirm" were made by the orchestrator; implementation proceeds on them, and the owner confirms or changes them at the next owner check-in (design, "Owner check-ins"; decision H12); work past a check-in waits for the owner's answer on the rows it lists.
55
Rows marked "approved by owner 2026-10-08" record the owner's written answers to re-review round 1; no other row is owner-approved unless its status says so.
66
Only these wait for the owner: the 1b go or no-go, any new refusal of an input Python accepts (P1B-1), relaxing a comparison rule, adding an exclusion or downgrading a field to unavailable, a new acceptance capability, an addition to `emulation-allowlist.txt`, the Q2 narrowing fallback, and a stop-rule counter reaching 2.
7-
Rows marked "revised R1" or "revised R2" were changed after that review round; rows C1 to C9 come from round 1, D1 from round 2, E1 to E14 from implementation, and H1 to H26 and F1 to F7 from re-review round 1.
7+
Rows marked "revised R1" or "revised R2" were changed after that review round; rows C1 to C9 come from round 1, D1 from round 2, E1 to E14 from implementation, and H1 to H26 and F1 to F7 from re-review round 1, H27 from the batch 2 code review.
88
Each row that changes the design is listed in the design's "Changes since lock" table and written into the section it names, in the same change.
99
Standing decisions are in [migration-plan.md](../migration-plan.md) section 2 and [phase-0/decisions.md](../phase-0/decisions.md).
1010

@@ -58,7 +58,7 @@ Standing decisions are in [migration-plan.md](../migration-plan.md) section 2 an
5858
| H4 | Contract version bumps: within a phase each bump is a decisions row (E7 for version 2, H5 for version 3); the lock compares against the merge base with `rust-core`, and a phase lands as one bump per merge into `rust-core` | Plan section 6 asks for an owner-approved bump; per-bump owner approval inside an unmerged phase stalls work the merge already gates | approved by owner 2026-10-08 (answer 3) |
5959
| H5 | Contract v3 (T1.7b): the 64 KB cap bounds the envelope only; every field whose size grows with user input (query text, `QuerySchema`, `SourceDescription` in a `start` or a `Done`, later view definitions) travels as a bytes attachment holding UTF-8 JSON of the same serde type, named by `AttachmentId`; `sdm_core::start` and `HostRun::start` take attachments. Supersedes HOST-1 and closes E14 | The cap refused wide schemas and long queries the reference accepts, on every core route, and T1.10 and T1.11 would otherwise build on that shape; JSON of the same type adds no second encoding | approved by owner 2026-10-08 |
6060
| H6 | Errors: `CoreError.code` becomes a `#[non_exhaustive]` `ErrorCode` enum with SCREAMING_SNAKE names (PQL diagnostic codes stay strings in `PqlDiagnostic`); `retryable: bool`, default false, carried from provider and transport errors; `_rust/errors.py` maps every `(kind, ErrorCode)` to one `StructuredDataError` subclass, a test iterates the enum, and mapping never reads message text. Every code has a register or decisions row (`VALUE_OUT_OF_RANGE`: E13; `PROVIDER_*`: this row). Not adopted: another product's Data, Config, System and Corruption categories | A string code set cannot be listed or locked; retryability was lost between provider errors and `CoreError`; the host table had no exit check | approved by owner 2026-10-08 |
61-
| H7 | Also in contract v3: `materialize`'s seed is a non-negative integer of any size as decimal text, with RS-7 applied in the core; lock entries added from v3 hash a description-free projection (existing entries untouched), closing the open doc-comment question; an error whose encoding passes the cap travels whole as an attachment named by a minimal control message, instead of losing its cause, fields and message | RS-1 and RS-2 keep seeds of `2**64` and above, which a `u64` refuses on SQLite and Postgres; doc comments forced bumps; `encode_error` dropped promised fields and causes | orchestrator, owner to confirm |
61+
| H7 | Also in contract v3: `materialize`'s seed is a non-negative integer of any size as decimal text, with RS-7 applied in the core; lock entries added from v3 hash a description-free projection (existing entries untouched), closing the open doc-comment question; any result or error whose encoding passes the cap travels whole as an attachment named by a minimal control message, instead of being lost or losing its cause, fields and message | RS-1 and RS-2 keep seeds of `2**64` and above, which a `u64` refuses on SQLite and Postgres; doc comments forced bumps; `encode_error` dropped promised fields and causes | orchestrator, owner to confirm |
6262
| H8 | Logging: `tracing` in Rust; `sdm-py` bridges it to Python `logging` with `pyo3-log`; the binding releases the GIL during core calls, and a GIL-deadlock regression test guards it; the Python host resets `pyo3-log`'s level cache at each run start so a logging configuration change takes effect; Java's SLF4J bridge is decided in the Java phase. Drained log collection from `HostRun` is not adopted | The plan names a `tracing` bridge; a known deadlock arises when a blocking call holds the GIL while another thread logs through the bridge | approved by owner 2026-10-08 (answer 4) |
6363
| H9 | SQL comparison: against the reference, gate on statement count, typed parameters and consumed batches; the reference's raw SQL text and bindings are diagnostic only; the core's SQL text is pinned by the DuckDB goldens. Replaces Q2's temporary-binding canonicalization of SQL text; Q2's capture stays for the gated fields | Comparing text ties the swappable data layer to the old SQL; canonicalization was machinery for a field that should not gate | approved by owner 2026-10-08 (answer 5) |
6464
| H10 | `python-old` is a smoke runner: it completes every case, is the comparison base in the non-reference cells (H1), and gets no determinism fixes, so fields they affect are compared there by named invariants; `python-ref` is the oracle | `rust-core` does not contain the reference's determinism fixes, so comparing `python-old` with the reference measured those fixes, not the core | approved by owner 2026-10-08 (answer 6) |
@@ -67,8 +67,8 @@ Standing decisions are in [migration-plan.md](../migration-plan.md) section 2 an
6767
| H13 | Locked-check edits: (a) checks 1 and 4 use the runtime cells of H1, with `python-old` as the base outside `ref-pandas3` (H10); (b) a field marked unavailable fails a locked check that requires it; (c) check 4 compares SQL as H9 says; (d) check 1 adds that no S0 case may refuse an input the reference accepts without an owner-approved register entry | Re-review round 1 found checks 1 and 4 ill-defined on pandas 2, passable by marking fields unavailable, and silent on S0 refusals | approved by owner 2026-10-08 (answer 9) |
6868
| H14 | Data port: `sdm-contract::data` defines `DataLayer::plan(request, capabilities)`; `CompositionRoot` holds one `Arc<dyn DataLayer>` built by `sdm_host::default_root()`; dialects are internal to `sdm-data`; domain crates do no I/O and hold no `Asker`, and `Asker::fetch` takes a `DataRequest`; no crate below `sdm-core` depends on it; `sdm_core::start` takes `Arc<dyn Zones>`; a fetch attachment is the complete result, drained by the host (T1.11a) | `SqlCompiler` in `sdm-core` would make `sdm-data` depend on the core and inverts the plan's graph; a non-SQL data layer could not register; `HostRun` kept the zones from the core | orchestrator, owner to confirm |
6969
| H15 | `Neighbors { table, key, columns, seeds, time, per_seed, order }` (seeds a key-set attachment with key, batch and optional anchor; `TimeRule { column, inclusive, nulls }`; explicit `TieBreak`) and `Lookup` join the data vocabulary in T1.11a, with `OptionalRequest::Neighbors`; a provider without it gets `Scan` with a key filter and the core applies window, limit and order in memory; every field is honored or refused | The reference's sampler fetches latest neighbors per batch and anchor, which the vocabulary could not express; T1.26 would otherwise invent it under deadline | approved by owner 2026-10-08 (the request, in place of a sampler trait, F1); its fields orchestrator, owner to confirm at T1.11e |
70-
| H16 | Boundary schema match and admission: each batch matches its attachment's declared schema before admission and each admitted fetch result matches the outstanding request's expected schema, stored with the request, or the run gets `PROVIDER_SCHEMA_MISMATCH`; admission adds lossless rows (`duration` and `time32` at `s`/`ms`, `date64` of whole days, `utf8_view`, `binary_view`, `float16`, `decimal32`, `decimal64`) and splits a batch whose offsets pass int32 instead of refusing; `AGREEING_AT_LEAST` becomes a shrinking `KNOWN_DIFFERING` id list (T1.7c) | Admission zipped batch columns against the envelope schema, so an extra column was dropped and same-typed columns relabelled; the fetch result was never checked against its request; refusing lossless types refuses inputs; a count floor lets one regression take another's slot | orchestrator, owner to confirm |
71-
| H17 | Diagnostics carry no input values: `contain()` keeps the panic flag and location, not the payload text; codec errors name position and category, not the offending text; provider errors keep SQLSTATE and vendor codes as fields and leave the driver's message out of `CoreError`, while the Python host may chain the driver's exception as `__cause__`; canary tests in T1.7c and T1.11d | `contain()` copied the panic payload into the message and the codec embedded raw deserializer text; DuckDB messages quote cell values | orchestrator, owner to confirm; the canary tests are owner-approved exit checks (F5) |
70+
| H16 | Boundary schema match and admission: each batch matches its attachment's declared schema before admission and each admitted fetch result matches the outstanding request's expected schema, stored with the request, or the run gets `PROVIDER_SCHEMA_MISMATCH`; admission widens losslessly (H27) and splits a batch whose offsets pass int32 instead of refusing; `AGREEING_AT_LEAST` becomes a shrinking `KNOWN_DIFFERING` id list (T1.7c) | Admission zipped batch columns against the envelope schema, so an extra column was dropped and same-typed columns relabelled; the fetch result was never checked against its request; refusing lossless types refuses inputs; a count floor lets one regression take another's slot | orchestrator, owner to confirm |
71+
| H17 | Diagnostics carry no input values: `contain()` keeps the panic location, not the payload text, and reports the panic out of band rather than through an error field a host can set; codec errors name position and category, not the offending text; provider errors keep SQLSTATE and vendor codes as fields and leave the driver's message out of `CoreError`, while the Python host may chain the driver's exception as `__cause__`; canary tests in T1.7c and T1.11d | `contain()` copied the panic payload into the message and the codec embedded raw deserializer text; DuckDB messages quote cell values | orchestrator, owner to confirm; the canary tests are owner-approved exit checks (F5) |
7272
| H18 | Task order: T1.7b (contract v3) and T1.7c (code fixes) before T1.8; T1.10c's L0 guards before the domain crates; T1.11 split into a to e, with T1.12 to T1.19 continuing on topic branches and a T1.11 sub-task failing its exit after two fix rounds going to the owner; the four test-the-tests faults before the first promotion in T1.14; reference immutability activates in T1.14; T1.21 to T1.28 name `base.regression`, T1.29 the other two; a task is done when its exit holds and the design and tracker say what changed | T1.11 held most of 1b behind one row; guards landed after the code they guard; the bootstrap preceded the failure tests; several exits named things that did not exist yet | orchestrator, owner to confirm |
7373
| H19 | A semantic or SQL deviation is recorded and independently approved in the change that first admits its case to acceptance; an owner-only refusal waits for the owner; T1.33 reconciles completeness and is not the first approval point | T1.29 and T1.30 needed approvals the task order scheduled only at T1.33 | orchestrator, owner to confirm |
7474
| H20 | The tracker keeps stop-rule counters (same divergence, fix rounds, same-way failures); one reaching 2 is escalated to the owner in the commit that records it; the design's "except where decisions.md records otherwise" clause on review rounds is deleted | The design's stop rules had no detector, and the exception clause let a row override the round limit | orchestrator, owner to confirm |
@@ -78,6 +78,7 @@ Standing decisions are in [migration-plan.md](../migration-plan.md) section 2 an
7878
| H24 | Deferred, each with its destination: common-input comparison of nondeterministic downstream fields (Phase 2 for PostgreSQL, Phase 3 for live warehouses; unproven until then); DT-3's public in-memory proof (Phase 4); PD-1's NaN ranking (Phase 2 ranking); seeding the failure families of retired tests (Phase 5). T1.28 adds a PD-2 payload-limit check | Phase 0 named these obligations; Phase 1's exits implied them without testing them | orchestrator, owner to confirm |
7979
| H25 | Version review covers behavior as well as JSON shape; host and core ship together and refuse any other contract version before execution; immutable historical schemas do not promise historical decoders; persisted `RequestState` follows H23; a Java binding will check its packaged contract version | E4 changed acceptance without a schema change, while a doc comment forced a bump | orchestrator, owner to confirm |
8080
| H26 | Locked checks are executable: `gate.sh --locked 1a` and `--locked 1b` exit non-zero on failure, print the check table and write `evidence/gate/<sha>.json`; before T1.20 each check is shown passing on a stub or the reference and failing on a seeded defect; a CI step bans `#[ignore]`, `skip`, `xfail` and `continue-on-error` in the Rust and contract code; T1.36 runs both on the exact merge SHA | Checks were locked without a passing run, and "no check is disabled" rested on prose | orchestrator, owner to confirm |
81+
| H27 | The batch 2 code review is folded into T1.7c as exit checks: AND and OR links count against `MAX_DEPTH` and the tree is built and dropped inside one grown stack, with a long-chain test; admission widens losslessly instead of refusing reference-accepted types (float16, decimal32 and decimal64, `duration` and `time32` by checked multiplication, views, `fixed_size_binary`, any dictionary to its value type), with register HOST-2 for anything still refused; pinned differing corpus IDs; the reference's date readings compared; E10's refusal registered as DT-9 with a reference-accepted test T1.6b must flip; the validated-query format read with `serde_stacker` so `MAX_DEPTH` round-trips; over-cap output carried by v3's attachment (H7); panics reported out of band; a test-only panicking operation through `HostRun::start`; the review's nits as a checklist. T1.7 is not done until T1.7c lands | The review found an uncatchable stack overflow on long AND chains and new refusals of types the reference accepts, both blockers under the owner's rule that refusing an accepted input is a regression | orchestrator, owner to confirm |
8182
| F1 | Graph-sampler audit: no `SubgraphSampler` trait; the `Neighbors` request (H15) carries explicit time and tie rules instead; a sampler seam is added only when a second sampler implementation is approved | A trait with one in-scope implementation and many semantic knobs, each needing refusals and vectors, for a backend the plan rules out | approved by owner 2026-10-08 |
8283
| F2 | The reference's SQL and in-memory samplers disagree on NULL times (dropped against kept) and emit order (newest against oldest first): L1 vectors on the DuckDB path record the SQL policy now (T1.26, register ORD-2); settling the disagreement belongs to S3 in Phase 4 | Phase 1 runs only the SQL sampler | approved by owner 2026-10-08 |
8384
| F3 | Time: one internal `Instant(i128)` and `Span(i128)` in nanoseconds; columns stay Arrow arrays; conversions are checked at the boundaries and return `VALUE_OUT_OF_RANGE`; no unit-generic family; civil arithmetic past `jiff`'s range (E6). Corrects `dependencies.md`, which said zone conversions refuse there | Unit confusion was the audited engine's largest bug class; one `i64` cannot hold both units across pandas' range, and `i128` keeps the core independent of the time library | approved by owner 2026-10-08 |

‎docs/design/rust-core/phase-1/dependencies.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ A row changes with the dependency, in the same commit.
2626
| `proptest` | 1 (`std` only) | contract tests | Use a library | The bounded, property-based fuzz test of the contract's decoders |
2727
| `sha2` | 0.10 | contract tests | Use a library | Hashes the committed schema for the lock check |
2828
| `tracing` | planned (T1.8) | first-party crates | Use a library | Logging in Rust (decision H8) |
29+
| `serde_stacker` | planned (T1.7c) | contract | Use a library | Reads the validated-query format back at `MAX_DEPTH`, past `serde_json`'s recursion limit (H27) |
2930
| `pyo3-log` | planned (T1.8) | `sdm-py` only | Use a library | Bridges Rust log records to Python `logging`; its level cache is reset at each run start (H8) |
3031

3132
## Ported algorithms and host concerns

0 commit comments

Comments
 (0)