Skip to content

Commit 7564501

Browse files
committed
T1.7c review fixes: generated HOST-2 list, check branches, no quoted literals
The type families still refused are generated from the admission tests' family table and compared with the families HOST-2 names, whose wording now says the reference may read them under an explicit dtype. Fault tests cover check (b)'s required metadata and nulls and check (a)'s nulls in a non-nullable column. A deferred date literal's refusal gives its position as fields instead of quoting it, and a validated constant's read error no longer carries the reader's text. Canaries now ride in Arrow values and a credential too, and the adapter-stack panic happens inside the interim parser, behind its own test-ops feature. Signed-off-by: Adi Wadaskar <awadaskar@nvidia.com>
1 parent 2712b35 commit 7564501

16 files changed

Lines changed: 207 additions & 90 deletions

File tree

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ packages/structured-data-models-client/src/sdm_client/
226226
- The validated-query format is versioned and locked apart from the contract (E8), and is provisional until the PQL asks are answered.
227227
- Today's ANTLR grammar is the CI reference through the reference observations and the corpus recorded on it; Phase 1 adds no separate grammar check.
228228
- Every tree level counts against `MAX_DEPTH`, AND and OR links included; the adapter grows the stack once and builds its result and drops the tree inside it; the validated-query format is read back with `serde_stacker`, so the deepest accepted tree round-trips and the first rejected depth is refused (H27).
229-
As built at T1.7c: a chain's first condition takes a level of its own, so 999 nested NOTs or parentheses are the deepest accepted and 1,000 refused, while an AND or OR chain of 998 links is accepted and 999 refused; 10,000 links are refused at the limit on a 256 KB thread. `sdm_core::pql::on_adapter_stack` is the one place a stack grows. `KNOWN_DIFFERING` is `sdm-core/tests/pql/known-differing.json`, naming per case the parts that differ (answer, validity, tree, readings, errors or warnings), so the D4 case is pinned for its warning only; `corpus.json` (format 2) carries the reference's date readings, recorded by `record_readings.py`, and `journey.json` (format 2) the six skipped inventory IDs.
229+
As built at T1.7c: the parser bounds both its own nesting and the height of every condition tree it builds (a comparison one level, a NOT or parenthesis one more than what it holds, a link one more than its taller side, a filtered aggregation's condition and nested arrays below their comparison), so no mix of NOT, parentheses, AND, OR and arrays builds a tree past `MAX_DEPTH`; 999 nested NOTs or parentheses are the deepest accepted and 1,000 refused, an AND or OR chain of 998 links is accepted and 999 refused, and 10,000 links, NOTs under a long chain, or chains nested in groups are refused at the limit in bounded time. A deferred date literal's refusal gives its position as fields, never its text. `sdm_core::pql::on_adapter_stack` is the one place a stack grows. `KNOWN_DIFFERING` is `sdm-core/tests/pql/known-differing.json`, naming per case the parts that differ (answer, validity, tree, readings, errors or warnings), so the D4 case is pinned for its warning only; `corpus.json` (format 2) carries the reference's date readings, recorded by `record_readings.py`, and `journey.json` (format 2) the six skipped inventory IDs.
230230
- Only `sdm-core` has a production dependency on the crate, and only its adapter uses it; the PQL team's crate later goes behind the same function.
231231
- `scripts/rust_core/emulation-allowlist.txt` holds exactly those three modules; emulation is banned everywhere else, each addition needs an owner-approved decisions row, and the list expires with the crate (H21).
232232
- Its header states the line: porting the reference's own defined algorithms (`DateOffset` arithmetic in `dor.rs`, `str.isspace` in `syntax.rs`, the `query[:-1]` quirk in `visitor.rs`, and T1.25's `text_shapes`) is a port with a row in `dependencies.md`; emulation is copying a library's incidental behavior, such as a codec or a repr.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ A task's checks are active from the moment it lands, as listed in the design's t
1616
| T1.6a Interim parser port and adapter | 1a | done | c01d79e, dbb10d8, 7a425a3, 23acca0; CI run 37802846877 green (`rust-lint` checks both schema locks; `rust-test`: the 417-query corpus through the adapter, 416 agreeing, the 34 journey queries recorded on the reference, the adapter's own tests) | The archive's parser and seven validators behind `sdm_core::pql`, capability checks in `sdm-core` (the pre-resolution `MODEL_NO_TEMPORAL_TARGET` check; the two filter capabilities consulted in resolution, E11); contract version 2 with PQL's row and column spans (E7); the validated-query format, version 1, locked apart (E8); the allowlist holds `pyint`, `float_repr` and `escapes` (E9); an interim date reader without D4's warning and D8 (E10); the corpus and journey queries live in `sdm-core/tests/pql` (E12). The one corpus difference is D4's warning; message text is not promised and differs |
1717
| T1.7 `sdm-host` | 1a | done | a7a5a02, 4ebb1ae, 1b7b68e; CI run 37804761504 green, every job (`rust-test`: `sdm-host` codec 7, admission 8, runs 12 tests; `sdm-core` `Run::end`) | Codecs and the 64 KB cap; the admission table, with `VALUE_OUT_OF_RANGE` for a value the admitted type cannot hold; `HostRun` with bytes in and out, contained calls, `RUN_BUSY`, E2's attachment rules, admission refusals ending the run (`Run::end`), and zones per run from the host facts (E13). The batch 2 code review's blockers (an uncatchable stack overflow on long AND chains; refusals of types the reference accepts) and should-fix items are T1.7c's exit (H27); the exit refuses only families that cannot convert losslessly |
1818
| T1.7b Contract v3 | 1a | in progress | Topic branch `rust-core-phase-1-t1.7b` at 305c73c (after the review fixes), one package, CI run 37830953836 green on it, every job; `BASE_SHA` c17d51891b2720e931309f55b9738bcd05d625ef (the phase tip at package start), `CHECKPOINT_SHA` the same (the candidate's merge base with it); before T1.14 the candidate's own harness runs and no acceptance is claimed. Exit tests: `sdm-host` `contract_v3` (11: another contract version refused first; the 5,000-column schema with a query past 64 KB, the start envelope and attachments either side of the cap, the wide `Fetch` step, the `validate_query` result, a real validation past the cap, the error and the warnings, each arriving whole; the run-numbered attachment; retryability; POSIX local zones), `codec` (the codec-level `Done` with a `SourceDescription` either side of the cap); `sdm-contract` `messages` (seed `2**70`, `Span` rows from 1, `ErrorCode`, provider errors without text) and `schema_lock` (the annotation-free hash); `sdm-core` `dispatch` (start attachments, the negative seed); `tests/test_rust_contract.py` (working entry, merged entries, the projection) | Contract version 3 (E15): input-sized fields as attachments (H5); exhaustive `ErrorCode` with `retryable` (H6); the hex seed, the annotation-free hash, over-cap core messages as `attached` (H7); errors from `HostRun` as control messages; the lock collapsed to one working entry (H4); test-only operations behind `test-ops`. Awaiting independent review and landing; owner check-in after it (H12) |
19-
| T1.7c Batch 1 and 2 code fixes | 1a | in progress | Topic branch `rust-core-phase-1-t1.7c`, one package, stacked on T1.7b's candidate 305c73c because its exit needs v3 (over-cap output, test-only operations through `HostRun::start`); `BASE_SHA` c17d51891b2720e931309f55b9738bcd05d625ef and `CHECKPOINT_SHA` the same until T1.7b lands, then rebased onto that landing with both reset. Exit tests: `sdm-host` `faults` (check (a): an extra column, swapped same-typed columns, a decimal scale, a timestamp unit, metadata, a later batch; check (b): a column lacking, added, renamed, reordered or of another type; panics at start, in the body and on the adapter's stack; `fields.panic` not poisoning; canaries in errors and, in a child process, stderr), `widening` (each widened type's values, validity and received type; split and single-value refusal at a test-only capacity; the still-refused list against HOST-2), `admission` (refusals name the type received), `runs`; `sdm-core` `pql_adapter` (10,000-link chains, depth boundaries, DT-9 pinned, E11 precedence), `pql_stack` (one stack per call, the deepest validated query round-trips), `pql_corpus` (`KNOWN_DIFFERING`, readings, multiset errors), `pql_journey` (34 cases, six skipped by name) | H16, H17 and H27 as built (design, "Core, host and binding" and "Interim parser"). Before T1.14 the candidate's own harness runs; no acceptance is claimed. Awaiting independent review and landing |
19+
| T1.7c Batch 1 and 2 code fixes | 1a | in progress | Topic branch `rust-core-phase-1-t1.7c`, one package, stacked on T1.7b's candidate 305c73c because its exit needs v3 (over-cap output, test-only operations through `HostRun::start`); `BASE_SHA` c17d51891b2720e931309f55b9738bcd05d625ef and `CHECKPOINT_SHA` the same until T1.7b lands, then rebased onto that landing with both reset. Exit tests: `sdm-host` `faults` (check (a): an extra column, swapped same-typed columns, a decimal scale, a timestamp unit, metadata, a later batch; nulls in a non-nullable column; check (b): a column lacking, added, renamed, reordered, of another type, without required metadata or with nulls the request does not take; panics at start, in the body and on the adapter's stack; `fields.panic` not poisoning; canaries in malformed messages and attachments, Arrow values and a credential, and panic payloads, in errors and, in a child process, stderr; the adapter-stack panic is inside the interim parser), `widening` (each widened type's values, validity and received type; split and single-value refusal at a test-only capacity; ), `admission` (refusals name the type received; the still-refused families, generated from the family table, against HOST-2), `runs`; `sdm-core` `pql_adapter` (10,000-link chains, trees tall by NOTs under a chain or nested groups refused in bounded time, depth boundaries, DT-9 pinned without quoting the literal, E11 precedence), `pql_stack` (one stack per call, the tallest accepted query of each shape and mix round-trips), `pql_corpus` (`KNOWN_DIFFERING`, readings, multiset errors), `pql_journey` (34 cases, six skipped by name) | H16, H17 and H27 as built (design, "Core, host and binding" and "Interim parser"). Before T1.14 the candidate's own harness runs; no acceptance is claimed. Awaiting independent review and landing |
2020
| T1.8 `sdm-py`, the client wheel and CI rewiring | 1a | not started | | From T1.7: bind `HostRun` (bytes in and out, attachments by id) and raise `_native.CoreError` from the error it returns; containment is already in `sdm-host`. From re-review: the `pyo3-log` bridge with `tracing`'s `log` feature, GIL, clean-import and abort tests, and a delivered log record (H8, F5); the local macOS build for the reference comparisons, its SHA-256 in identity (owner answer 5) |
2121
| T1.9 Python host and routing seam | 1a | not started | | The exhaustive error mapping (H6); the level-cache reset; no chained driver exception (H17); a wide-schema `materialize()` through the installed wheel (H5) |
2222
| T1.10 First S0 case on every runner | 1a | not started | | Guard name list updated on `rust-core-reference` before any reference observation is recorded; records through `run_python.py` in `python-ref` mode until T1.14 lands `gate.sh`; `python-old` is a smoke runner (H10); a recorded `RuntimeError` is an outcome, not a runner failure |

‎docs/design/rust-core/register.yaml‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -845,16 +845,18 @@ entries:
845845
ms, date64 (as timestamp[us]), view types, fixed_size_binary and any
846846
dictionary are admitted, keeping the received type; a batch whose values
847847
pass int32 offsets is split between rows. Proposed: what still cannot
848-
convert is refused by name. Generated at T1.7c by the admission tests
849-
(sdm-host tests/widening.rs, which fail if this list and the code
850-
disagree), the type families still refused are `interval`,
851-
`run_end_encoded` and `union`; interval and union are refused by the
852-
reference too (inventory cases time.feature-interval and nested.union),
853-
so run_end_encoded is the one type family that needs the owner. The
854-
values still refused are a timestamp, duration or date64 at s or ms
855-
whose value passes int64 microseconds, and a single string, binary or
856-
list value past int32 offsets; any type T1.33 finds no lossless row for
857-
joins them.
848+
convert is refused by name. Generated at T1.7c from the admission
849+
tests' family table (sdm-host tests/admission.rs, which fails if this
850+
list and the code disagree), the type families still refused are
851+
`interval`, `run_end_encoded` and `union`. The reference refuses
852+
interval and union where it infers a column's type (inventory cases
853+
time.feature-interval and nested.union), but a column given an
854+
explicit dtype skips that inference (table.py), so the reference may
855+
read all three there, and each needs the owner. The values still
856+
refused are a timestamp, duration or date64 at s or ms whose value
857+
passes int64 microseconds, and a single string, binary or list value
858+
past int32 offsets; any type T1.33 finds no lossless row for joins
859+
them.
858860
affected_inputs: Sources returning Arrow types outside the closed set with no lossless conversion.
859861
cases: [T1.7c, T1.33]
860862
status: proposed

‎rust/crates/sdm-core/Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ publish.workspace = true
1414

1515
[features]
1616
# Operations only tests reach; a shipped build leaves it off.
17-
test-ops = ["sdm-contract/test-ops"]
17+
test-ops = ["sdm-contract/test-ops", "sdm-pql-interim/test-ops"]
1818

1919
[dependencies]
2020
arrow-array.workspace = true

‎rust/crates/sdm-core/src/dispatch.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ pub fn start(
8989
Run::new(|_| async { Err(not_yet("predict")) })
9090
}
9191
#[cfg(feature = "test-ops")]
92-
Operation::Test(operation) => crate::test_ops::run(operation, &mut attachments)?,
92+
Operation::Test(operation) => crate::test_ops::run(operation, &mut attachments, facts)?,
9393
})
9494
}
9595

‎rust/crates/sdm-core/src/pql.rs‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -154,15 +154,26 @@ fn is_error(diagnostic: &PqlDiagnostic) -> bool {
154154
diagnostic.severity == Severity::Error
155155
}
156156

157+
/// The refusal of a query the interim parser cannot read yet: where the
158+
/// literal is, as fields, never the literal itself (decision H17).
157159
fn unsupported(interrupted: interim::Interrupted) -> CoreError {
158-
CoreError::new(
160+
let mut error = CoreError::new(
159161
ErrorKind::Unsupported,
160162
ErrorCode::UnsupportedRequest,
161163
format!(
162164
"{}; the interim PQL parser cannot validate this query",
163-
interrupted.0
165+
interim::ast::DEFERRED
164166
),
165-
)
167+
);
168+
if let Some(at) = interrupted.0 {
169+
let count = |n: usize| Detail::Scalar(Scalar::UInt64(u64::try_from(n).unwrap_or(u64::MAX)));
170+
error = error
171+
.with_field("start_row", count(at.start_row))
172+
.with_field("start_col", count(at.start_col))
173+
.with_field("end_row", count(at.end_row))
174+
.with_field("end_col", count(at.end_col));
175+
}
176+
error
166177
}
167178

168179
fn internal(message: impl Into<String>) -> CoreError {
@@ -449,7 +460,9 @@ fn literal(n: &NodeRef, value: &ConstValue) -> Result<PqlLiteral, CoreError> {
449460
}
450461
ConstValue::Text(text) => text,
451462
};
452-
let read = ast::typed_value(n).map_err(|e| internal(format!("a validated constant: {e}")))?;
463+
let read = ast::typed_value(n)
464+
// The reader's text may quote the constant, which is input.
465+
.map_err(|_| internal("a validated constant could not be read as its type"))?;
453466
Ok(match read {
454467
Literal::Null => PqlLiteral::Null,
455468
Literal::Bool(value) => PqlLiteral::Bool { value },

‎rust/crates/sdm-core/src/test_ops.rs‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,19 @@
55
//! The bodies of the test-only operations (`sdm_contract::test_ops`), built
66
//! only with the `test-ops` feature, which a shipped build leaves off.
77
8-
use sdm_contract::ops::OperationResult;
8+
use sdm_contract::ops::{OperationResult, ProfileName, ProfileSpec, QuerySchema};
99
use sdm_contract::test_ops::{PanicAt, TestFetch, TestOperation, TestResult};
10-
use sdm_contract::{CoreError, CoreWarning};
10+
use sdm_contract::{CoreError, CoreWarning, HostFacts};
1111

1212
use crate::attached::take_json;
13+
use crate::profiles::Profile;
1314
use crate::run::{Attachments, FetchResult, Reply, Run, internal};
1415

1516
/// The run of a test-only operation.
1617
pub(crate) fn run(
1718
operation: TestOperation,
1819
attachments: &mut Attachments,
20+
facts: HostFacts,
1921
) -> Result<Run, CoreError> {
2022
Ok(match operation {
2123
TestOperation::Fetch { fetch } => {
@@ -51,10 +53,20 @@ pub(crate) fn run(
5153
TestOperation::Panic { at, payload } => match at {
5254
PanicAt::Start => panic!("{payload}"),
5355
PanicAt::Body => Run::new(move |_| async move { panic!("{payload}") }),
56+
// Inside the interim parser, on the adapter's grown stack.
5457
PanicAt::AdapterStack => Run::new(move |_| async move {
55-
crate::pql::on_adapter_stack(move || -> Result<OperationResult, CoreError> {
56-
panic!("{payload}")
57-
})
58+
sdm_pql_interim::parser::panic_in_next_parse(payload);
59+
let profile = Profile::resolve(&ProfileSpec {
60+
name: ProfileName::Relational,
61+
model: None,
62+
capabilities: Default::default(),
63+
});
64+
let schema = QuerySchema {
65+
tables: vec![],
66+
links: vec![],
67+
};
68+
crate::pql::validate("PREDICT t.c FOR EACH t.id", &schema, &profile, &facts)
69+
.map(OperationResult::ValidateQuery)
5870
}),
5971
},
6072
})

‎rust/crates/sdm-core/tests/pql_adapter.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -412,5 +412,17 @@ fn a_date_the_reference_reads_and_the_interim_reader_does_not_is_pinned_as_refus
412412
(ErrorKind::Unsupported, ErrorCode::UnsupportedRequest),
413413
"{literal}"
414414
);
415+
// The literal is input: the refusal gives its position, never its
416+
// text (decision H17).
417+
let written = format!("{} {err:?}", serde_json::to_string(&err).unwrap());
418+
assert!(!written.contains(literal), "{written}");
419+
assert!(!written.contains(&literal[..8]), "{written}");
420+
assert_eq!(
421+
err.fields.get("start_row"),
422+
Some(&sdm_contract::Detail::Scalar(sdm_contract::Scalar::UInt64(
423+
1
424+
)))
425+
);
426+
assert!(err.fields.contains_key("start_col"));
415427
}
416428
}

‎rust/crates/sdm-host/tests/admission.rs‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,3 +504,35 @@ fn a_refused_child_is_named_by_its_path() {
504504
let err = admitted_type("col", &dictionary).unwrap_err();
505505
assert_eq!(text(&err, "arrow_type"), interval.to_string());
506506
}
507+
508+
/// The backticked names in register HOST-2's entry: the type families it
509+
/// lists as still refused.
510+
fn host_2_families() -> BTreeSet<String> {
511+
let register = std::fs::read_to_string(
512+
std::path::Path::new(env!("CARGO_MANIFEST_DIR"))
513+
.join("../../../docs/design/rust-core/register.yaml"),
514+
)
515+
.unwrap();
516+
let start = register.find("- id: HOST-2\n").expect("register HOST-2");
517+
let entry = &register[start + 1..];
518+
let entry = &entry[..entry.find("\n- id: ").unwrap_or(entry.len())];
519+
entry
520+
.split('`')
521+
.skip(1)
522+
.step_by(2)
523+
.map(str::to_owned)
524+
.collect()
525+
}
526+
527+
#[test]
528+
fn the_families_still_refused_are_the_ones_register_host_2_lists() {
529+
// Generated from the family table above, so a widening or a new
530+
// refusal changes it, and then HOST-2 must say so.
531+
let refused: BTreeSet<String> = every_family()
532+
.iter()
533+
.map(family)
534+
.filter(|(_, taken)| *taken == Family::Outside)
535+
.map(|(name, _)| name.to_owned())
536+
.collect();
537+
assert_eq!(refused, host_2_families());
538+
}

0 commit comments

Comments
 (0)