docs(rfc): add RFC 0043, GQ logic tests - #584
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
0b5249b to
1c3573c
Compare
1c3573c to
2ce553b
Compare
aaltshuler
left a comment
There was a problem hiding this comment.
Three RFC-level comments: the regression-gate guarantee, ranked ordering semantics, and bounded runner concurrency.
| does). A comment, string, or fixture line mentioning the issue never | ||
| satisfies the gate. Each accepted shape executes: the corpus walker runs | ||
| every case and refuses a malformed one, and the workspace clippy gate | ||
| refuses a dead test function; whether the test asserts the right thing |
There was a problem hiding this comment.
RFC-level concern: this paragraph promises that every accepted Rust regression executes, but the proposed required PR job only runs gq_logic_tests, and Clippy only compiles test targets. A function line can satisfy the diff check without being a registered or executed test. The crates/*/tests/** restriction also conflicts with docs/dev/testing.md, which assigns several packages' regressions to in-source tests. Please define the gate in terms of a registered and executed regression at the declared owner boundary, or narrow the guarantee accordingly; the implementation details can remain open.
| `ordered` without one. With an explicit `order` the engine's output order is | ||
| a total, deterministic, shipped contract (`apply_ordering` appends the bound | ||
| entities' key columns as an ascending tie-break), so positional comparison | ||
| is stable. |
There was a problem hiding this comment.
RFC-level semantic issue: expect ordered is allowed for any order clause because this claims apply_ordering supplies a total key tie-break. That is not true for ranked search: nearest and BM25 skip apply_ordering, and RRF sorts by score alone. Equal distances or scores therefore have no stable order. Please choose the contract here—either ranked queries gain a deterministic tie-break, or the format disallows ordered ranked expectations—without needing to specify the implementation.
| point (tokio is already every engine integration test's runtime) that | ||
| lists `tests/gq_logic_tests/*.gqt` rooted at `CARGO_MANIFEST_DIR` | ||
| (the `forbidden_apis.rs` walk precedent) and spawns each case as a task | ||
| into a `tokio::task::JoinSet`. Case concurrency comes from that task set, |
There was a problem hiding this comment.
RFC-level invariant issue: this chooses to spawn the entire corpus into one JoinSet. Because each case creates a store and may build indexes, concurrency grows with corpus size while libtest sees only one test. The RFC only needs to state that case execution is bounded; the actual worker count, limits, and timeout values can remain implementation details.
What & why
RFC 0043 makes query-behavior tests one self-contained text file per case (
.gqt): a header (issue anchor,red_on:provenance), a.pgschema, JSONL seed rows, then steps (--- query/--- mutate/--- restart, loopable), each with params and an expected outcome (rows, affected counts, or an error substring). One test target,crates/omnigraph/tests/gq_logic_tests.rs, runs each case against a fresh temporary store through public engine surfaces only; no second toolchain. An enforcement ladder makes the medium stick: AGENTS.md contract sentences, a CI gate holding issue-closing fix PRs to a matching regression, and ano-reprowaiver label.Why now: review of the #563 fix surfaced a pure input-to-output defect (aggregates computed over the capped BM25 scan window) a twenty-row logic test case would have caught before review. The format adopts the sqllogictest lineage (SQLite, DuckDB, DataFusion) at its mature state and omits its documented mistakes (result hashing, type strings, bare any-error expectations).
Backing issue / RFC
docs/rfcs/0043-gq-logic-tests.mdChecklist
Local verification
python3 scripts/check-docs.py— green (front matter, registry row, links)cargo test --workspace— not run: docs-only diffNotes for reviewers
docs/rfcs/README.mdconflicts on the index table; keep both new rows in number order, 0044 stands.proptest_equivalence.rskeeps owning equivalence.Greptile Summary
The PR proposes RFC 0045, defining a self-contained
.gqtformat and harness for query-behavior tests, plus CI enforcement and regression-test conventions.Confidence Score: 5/5
The documentation-only PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Case[".gqt case"] --> Parse["Parse header, schema, seed, and steps"] Parse --> Store["Create temporary store"] Store --> Seed["Load JSONL seed rows"] Seed --> Index["Ensure required indexes"] Index --> Steps{"Execute steps in order"} Steps --> Query["Query"] Steps --> Mutate["Mutate"] Steps --> Restart["Restart store"] Query --> Compare["Compare rows or error"] Mutate --> CompareCounts["Compare affected counts or error"] Restart --> Steps Compare --> Result["Case result"] CompareCounts --> ResultReviews (4): Last reviewed commit: "docs(rfc): tighten the fix-PR gate parag..." | Re-trigger Greptile