Repository navigation
feat(gqt): generate a step's params from a recipe - #927
Conversation
A `--- params generate: v1 seed: <u64>` section holds a YAML `params:` map
from each parameter to one column generator, evaluated at ordinal zero, in
place of a JSON body. A new `blob` column produces the `base64:` input of a
managed Blob of `length` repeated bytes, for parameters and generated loads.
The recipe is validated and bounded (256 parameters, 64 MiB of JSON) when the
case parses and generated when its step runs, so a case can pass a 32 MiB
Blob without carrying 44.7 MB of literal text, and reports keep the recipe.
It takes no `${` substitution.
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: request changes. One blocking defect: generated parameters accept Zipf endpoints, but every such value fails during execution. The inline comment gives the cause and a minimal GQT regression. I am submitting a COMMENT review because the authenticated account is the PR author.
Reviewed d84900c68de1334ecfc2d4abd741e9de8abe5d4e against base 76ce6a0d9cc1c4e4ad693ee25c67c11e01d093ce.
This PR lets a test describe large parameter values with a small recipe. It avoids checking in about 44.7 MB of base64 text for one 32 MiB Blob. The parser retains a validated recipe. Each step generates a JSON object, then uses the existing typed parameter conversion. The new Blob generator also works in generated loads. This supplies the test-harness feature for #903, but does not itself prove that PR's 32 MiB write fix.
The design addresses the general fixture-size problem at the GQT boundary. It reuses the column generators, strict YAML reader, and parameter binding. It adds no graph state, storage format, or deployment path. The worker reparses the frozen case text, so replay retains the recipe rather than an expanded input file. I traced Blob text through the compiler, the engine's append_blob_value, and the pinned Lance 11.0.0 BlobArrayBuilder at upstream commit ab6b5bbe46009ed78746b444df8db59a8bc5d842. It uses the existing managed-byte path.
The tradeoff is a new format variant and its compatibility obligations: generator semantics, deterministic streams, and size limits must remain aligned. The 64 MiB bound limits JSON size, not peak memory. Generation still allocates the bytes and encoded text, and parameter binding copies strings. This removes fixture bulk and the need for separate Blob-only test programs. It adds a modest harness maintenance cost. Five more consumers can reuse this design, provided generator preparation has one owner. The current Zipf failure shows where that reuse is incomplete: loads prepare distributions, while parameters skip that step. Fix that shared requirement rather than adding a special-case expected error or silently changing the distribution.
Validation:
- Local, exact head: the checked-in
generated_params_supply_scalar_and_blob_values.gqtpassed all five executions: direct filesystem, plus DST seeds 0 and 42 with one replay each. - Local reproduction, same executable: I temporarily extended that existing case with a one-element Zipf endpoint. Mutation and query variants each failed all five executions with
zipf distribution was not prepared. Both reports show full selection, zero unrun attempts, and matching DST replays. These are behavioral failures, not build or setup failures. All temporary test edits are restored, and the checkout is clean. - Exact-head CI: workspace checks and GQT checks passed. The logs show both new Rust tests and the new corpus case passing. Those tests do not cover a Zipf parameter.
- Local static checks: AGENTS links, all 232 Markdown files, and the diff whitespace check passed.
The local build used Rust 1.97.1, RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable', and the existing private build cache:
cargo run -p omnigraph-gqt --locked --features omnigraph/failpoints \
--bin omnigraph-gqt -- \
crates/omnigraph-gqt/cases/generated_params_supply_scalar_and_blob_values.gqt \
--artifacts /tmp/review-927/baseline-artifactsThe reproduction used that same built omnigraph-gqt binary and case path, with only the fixture edits described inline. There is no passing fixed commit for this finding yet. Keep the small GQT regression and require it to pass after the correction. I did not run the full workspace locally, cloud environments, or memory/performance measurements. No separate optional improvements.
| .iter() | ||
| .map(|(name, column)| { | ||
| column | ||
| .value(self.seed, PARAMS_TABLE, &format!("param.{name}"), 0, None) |
There was a problem hiding this comment.
[P2] Prepare Zipf distributions for generated parameters
This call always passes None as the prepared distribution. The parser admits every Column, including endpoint with distribution: {kind: zipf, exponent: 1.25}, but Column::value requires a prepared CDF for Zipf. Thus every valid Zipf parameter fails with zipf distribution was not prepared before the engine operation. This affects both query and mutation steps, including DST workers.
Minimal reproduction: in the new generated_params_supply_scalar_and_blob_values.gqt, replace only the slug recipe with:
slug: {kind: endpoint, prefix: doc-, width: 2, population: 1, distribution: {kind: zipf, exponent: 1.25}}The expected key remains doc-00. At this head, all five executions fail at the mutation. Adding the same parameter recipe to a query filtered by $slug also fails all five executions. The unmodified case passes.
Reuse the existing bounded Zipf preparation so every admitted generator can run through either consumer. Extend this existing GQT case with the one-element population and make that regression pass after the fix. Do not replace the generator failure with an expected error: the recipe is valid and must produce a value.
There was a problem hiding this comment.
Fixed in 2842afb. GeneratedParams::generate now prepares a Zipf endpoint's CDF with the same zipf_cdf a table recipe uses, through a Column::zipf helper that the table path (distributions, the parse-time entry count) now shares. GeneratedParams::parse counts Zipf entries under the table recipe's eight million entry bound (now one constant, MAX_ZIPF_ENTRIES), checked before any distribution is built. The case's slug is your one-value Zipf endpoint, on the mutation and on the read. On the parent, all five direct and DST executions fail at the mutation with zipf distribution was not prepared, and the served conformance fails both verdicts the same way. On this head the case passes everywhere. The unit test generated_params_supply_a_step_and_are_refused_outside_their_contract checks a generated Zipf value, and that nine one-million-entry Zipf params are refused at parse.
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: request changes. The server integration is coherent, but the reported Zipf defect remains. Please fix the shared generator and retain its minimal GQT regression. The inline comment also requests coverage for the new server path. No separate optional improvements. This is a COMMENT review because the authenticated account is the PR author.
Reviewed 6b655415fa4da8cc4637777b3c2e4ac9be2cf0b9, including the main merge and the server follow-up after review of d84900c68de1334ecfc2d4abd741e9de8abe5d4e.
This PR lets GQT tests describe large inputs with small recipes. Its first intended consumer is the 32 MiB Blob regression for #903. The parser retains a bounded recipe. The runner generates each step's JSON values when that step executes. The new follow-up sends those values through the ordinary query and mutation requests. It keeps type validation at the server boundary.
The shared wire_params function covers both HTTP callers. It calls the same generator as direct execution. This addresses transport integration at the correct owner, with no separate generation algorithm or durable state. Literal substitution keeps its existing behavior.
The remaining contract failure is broader than HTTP. Every admitted column generator must be executable wherever a recipe is accepted. Parameter generation still passes no prepared distribution. Loads prepare Zipf distributions through Table::distributions; parameters do not. Reuse bounded preparation at the common generator owner. A server-only workaround would leave direct execution and DST exposed.
The liability tradeoff remains favorable once that defect is fixed. Recipes remove large checked-in inputs and avoid separate test programs for each large value. They add format compatibility, deterministic-stream, preparation, and size-bound obligations. Five more consumers should reuse those rules, rather than maintain five partial adapters. The server follow-up adds one match over the existing parameter variants and no new API or coordination state.
The bounds have distinct owners. The 64 MiB recipe bound limits encoded JSON, not peak memory. Generation still allocates bytes and base64 text. The server retains its 1 MiB request limit, so the 32 MiB Blob case must use a direct-engine environment. The new documentation states this boundary correctly.
I checked the current repository guides and reused the complete relevant upstream reading from the earlier review. I rechecked the actual Blob path: compiler binding, append_blob_value, and Lance 11.0.0's BlobArrayBuilder::push_bytes at upstream commit ab6b5bbe46009ed78746b444df8db59a8bc5d842. The generated text uses the existing managed-byte representation. No storage protocol or production deployment boundary changes here.
Validation:
- Local, exact head: the generated-parameter Rust test and Blob encoding/bound test each passed. The unmodified served conformance case reported success but explicitly skipped execution. I do not count that skip as coverage.
- Local, temporary GQT extension: I added a server environment and a generated
$slugfilter to the existing case. One selected case passed, with bothserved=okandin-process=ok. It used the existing schema, one entity, and four-byte Blob. - Local defect confirmation: I changed only the mutation's key recipe to the one-element Zipf recipe from the original finding. Both runners failed at step 1 with
zipf distribution was not prepared. Moving that recipe to the query instead made both fail at step 2. These are executed behavioral failures during parameter preparation, before that step's engine call or HTTP request. - Before/after limitation: I attempted the same served regression at immediate parent
70d1f6d9b9018827439aef493fd90f821e03649d. Compilation failed with two E0308 errors:wire_paramsexpectedOption<&String>but receivedOption<&ParamsInput>. No GQT ran there. Thus the passing result at6b655415fa4da8cc4637777b3c2e4ac9be2cf0b9is not a behavioral fail-before/pass-after proof. The Zipf defect has no fixed passing commit yet. - CI: GQT and workspace checks are green for this head. The served job checked out merge commit
31a713677a37d3d197e98efe81e4fa82a8eb2885, whose tree differs from the head. Its log explicitly skips this generated-parameters case. These are CI merge results, separate from the local head results above. - Local static checks: formatting, AGENTS links, all 235 Markdown files, and diff whitespace passed.
The build and parent attempt used Rust 1.97.1, CARGO_TARGET_DIR=/tmp/review-928/target, and RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable':
cargo test -p omnigraph-gqt-core -p omnigraph-gqt-served --locked \
--features omnigraph/failpoints --lib --test gqt_served_conformance \
generated_params -j 3 -- --nocaptureEach temporary GQT variant then used that exact head's built executable, from crates/omnigraph-gqt-served:
/tmp/review-928/target/debug/deps/gqt_served_conformance-fabb28af893213d5 \
generated_params_supply_scalar_and_blob_values --nocaptureAll temporary fixture edits are restored. The isolated checkout is clean at the reviewed head. I did not rerun DST or the full workspace locally, test cloud deployments, or measure memory/performance. The HTTP test uses the existing local conformance server without authentication or policy.
| - target: omnigraph-engine | ||
| storage: local-filesystem | ||
| - target: omnigraph-engine-dst | ||
| storage: in-memory-object-store | ||
| seeds: [0, 42] |
There was a problem hiding this comment.
[P2] Include generated parameters in served conformance
The new wire_params path has no GQT coverage here. This case declares only the direct engine and DST targets. The green served CI job explicitly skips it with no omnigraph-server environment declared.
Add an omnigraph-server / local-filesystem environment to this existing case. Also make its read take a generated $slug parameter and filter on that value. The existing one-entity schema, four-byte Blob, affected count, and returned key are enough to exercise both HTTP callers. Keep the same fixture and expected result.
I ran that temporary extension at 6b655415: one case passed with both served=ok and in-process=ok. Please retain it in the PR. The immediate parent 70d1f6d fails compilation, so it cannot supply a behavioral before-fix result; I report that limitation in the review instead of counting the compile failure as regression proof.
There was a problem hiding this comment.
Done in 2842afb. The case declares omnigraph-server / local-filesystem, and its read is by_slug($slug), with $slug generated by the same recipe as the mutation and filtered in the match. Same fixture, affected count and key. gqt_served_conformance reports served=ok in-process=ok for it. The README's served paragraph now says a recipe's generated parameters travel like spelled ones.
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: approve. The two prior P2 findings are addressed. I found no new blocking defect or optional improvement in this follow-up. This is a COMMENT review because the authenticated account is the PR author.
Reviewed 2842afba303522660dee3410f11d3868f9c9fe30, including all four files changed since 6b655415fa4da8cc4637777b3c2e4ac9be2cf0b9.
This PR lets GQT tests describe large inputs with small recipes. It avoids storing about 44.7 MB of base64 text for a 32 MiB Blob test. The runner retains the recipe, generates each step's JSON parameters, and uses ordinary parameter binding or HTTP requests. The intended Blob boundary regression for #903 remains a separate consumer. This PR does not itself test that 32 MiB engine boundary.
The violated invariant was simple: a generator accepted by the parser must execute through every supported consumer. Zipf parameters passed validation but had no prepared distribution. The fix now uses the same Zipf selector and CDF builder as generated loads. Both recipe parsers use one total-entry limit. Parameter admission checks that limit before building a distribution. This fixes the cause at the generator owner. It covers mutation and query parameters, direct execution, HTTP, and DST workers, which parse the frozen recipe again.
The existing GQT case now uses one Zipf value for the inserted key and the read filter. It keeps one node type, one row, a four-byte Blob, the affected count, and the returned key. The added server environment makes both HTTP callers execute in served conformance. This is a small behavioral regression within the existing fixture.
The liability tradeoff is favorable. Recipes replace bulky fixtures and separate test programs with existing generator primitives. This correction removes divergent Zipf handling and keeps the entry limit in one constant. It adds one private selector and bounded preparation, with no new persisted state, production API, or storage protocol. Five similar consumers can share these rules instead of creating more distribution variants. The remaining obligations are the versioned recipe format, deterministic streams, and resource bounds.
The cost is explicit: each parameter generation builds its Zipf CDF again. The per-column population limit and total-entry limit bound that work. There is no cache to invalidate. This is reasonable for test recipes, but I did not measure CPU or peak memory. The 64 MiB JSON bound is not a peak-memory guarantee. The server still limits ordinary request bodies to 1 MiB, so a 32 MiB Blob recipe needs a direct-engine environment.
I checked the current repository guides, reused the complete relevant upstream reading from the earlier review, and rechecked the unchanged storage path. Generated Blob text passes through compiler binding and append_blob_value into Lance 11.0.0's BlobArrayBuilder::push_bytes, at upstream commit ab6b5bbe46009ed78746b444df8db59a8bc5d842. This follow-up changes test-input preparation, not graph publication or deployment boundaries.
Validation:
| Same GQT fixture | Before: 6b655415fa4da8cc4637777b3c2e4ac9be2cf0b9 |
After: 2842afba303522660dee3410f11d3868f9c9fe30 |
|---|---|---|
| Direct filesystem, DST seeds 0 and 42, one replay per seed | All five executions fail at step 1 with zipf distribution was not prepared. Both replays match. |
All five pass. Both replays match. |
| Served conformance | One case fails. Both served and in-process paths report the same parameter-preparation error. | One case passes with served=ok in-process=ok. |
I copied the head's checked-in fixture unchanged onto the parent. Both reports have the same case digest. These are executed behavioral results, not compile failures or skipped tests. The standalone report marks the server environment unselected. The separate conformance run above executes it.
The commands were identical on both commits, apart from the report directory:
cargo run -p omnigraph-gqt --locked --features omnigraph/failpoints \
--bin omnigraph-gqt -j 3 -- \
crates/omnigraph-gqt/cases/generated_params_supply_scalar_and_blob_values.gqt \
--artifacts /tmp/review-927-zipf/<before-or-head>-artifacts
cargo test -p omnigraph-gqt-core -p omnigraph-gqt-served --locked \
--features omnigraph/failpoints --lib --test gqt_served_conformance \
generated_params -j 3 -- --nocaptureBoth used Rust 1.97.1, CARGO_TARGET_DIR=/tmp/review-928/target, and RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable'.
- Local, exact head: the generated-parameter Rust test and all five existing generator tests passed. These cover admission bounds, Blob encoding, deterministic Zipf output, and batching. The five generator tests used the head's built
omnigraph_gqt_core-7a29a345542feb12test executable withgenerate::tests --nocapture. - CI: GQT and workspace checks are green. GQT logs show this case executing in DST and served conformance. The GQT jobs checked merge commit
b6f091254a3a3c0ae3faf4b6fce40b2bf72fbc8c, whose tree differs from the reviewed head. This is separate from the local exact-head evidence above. - Local static checks: formatting, AGENTS links, all 235 Markdown files, changed-file typo checks, and diff whitespace passed.
All temporary test edits are restored. The isolated checkout is clean at the reviewed head. I did not run the full workspace locally, test cloud deployments, or measure performance. Served conformance uses its existing local server without authentication or policy.
GQT steps can now generate their parameters instead of carrying a JSON body. The first user is the exact-32 MiB Blob boundary regression the review of #903 asked for in GQT: its parameter would otherwise be 44.7 MB of literal
base64:text in a checked-in case.Format
--- params generate: v1 seed: <u64>replaces a JSON--- paramsbody on a query or mutate step. It takesgenerate: v1and a seed, nevermode:orbranch:.params:map names each parameter with one existing column generator, evaluated at ordinal zero. The random stream uses table nameparamsand column namesparam.<name>.blobcolumn:{kind: blob, byte, length}produces thebase64:input text of a managed Blob oflengthcopies ofbyte. It works in generated loads too, where its size bound counts against the existing 16 MiB batch limit.${in a recipe is refused like any stray substitution.omnigraph-servertarget (feat(gqt): add a server target and run cases against omnigraph-server #923) the runner sends the generated values in the request body, so the server's request limits apply.The grammar is documented in the GQT README's generated fixture contract, the RFC 0045 step grammar links it, and a changelog fragment is included.
Tests
generate.rsblob_columns_generate_base64_input_within_their_bound: exact output for empty, padded and unpadded lengths; the bound covers the encoded value; a 16 MiBblobcolumn is refused by the load batch bound.tests.rs::generated_params_supply_a_step_and_are_refused_outside_their_contractgenerate: v2, a missing seed,mode:/branch:, unknown fields,${substitution, an empty map, the 64 MiB bound, a second params section, and params on a generated load.cases/generated_params_supply_scalar_and_blob_values.gqtomnigraph-server: a mutation inserts a Zipf-generated key and a generated Blob, and a read filtered by the same generated$slugreturns it.Next
#903 will add the boundary case to
cases_slow/once this merges.