T8: documentation sweep and remaining ADR amendments (epic #2904) #2915

Activity

1. pmoust commented on Oct 14, 2026

   

   pmoust

   on Oct 14, 2026

   MemberAuthor

   More actions

   Docs sweep item: ADR-2904 decision 8 says a join over the tenant's memory budget "fails with the existing typed memory error". Per ADR-0013 "Amendment: memory ceilings are best-effort for joins" (issue #1370), that holds only where the join's allocations reach the pool through the fallible path. The user docs this ticket writes for multi-signal joins should state the limit as best-effort for joins and link #1370, not repeat decision 8's wording. Raised in review of #2984 (ADR-2983).
2. pmoust commented on Oct 17, 2026

   

   pmoust

   on Oct 17, 2026

   MemberAuthor

   More actions

   Carried from the #3056 checkpoint review, for this docs sweep: (1) ADR-2904 decision 8 says an over-budget join fails with the existing typed memory error; join memory is best-effort per the ADR-0013 amendment, and the user docs already say so. Amend decision 8. (2) Decision 9 promises the MCP ravel_query_sql refusal of a multi-table statement; the adapter (services/ravel-server/src/mcp/service_impl.rs:437,487) calls the HTTP-path sql_execute and sql_explain, which now admit joins. It is latent while both tools are NotShipped; record in the ADR that the refusal is owed by whoever ships them.
3. pmoust commented on Oct 18, 2026

   

   pmoust

   on Oct 18, 2026

   MemberAuthor

   More actions

   Carried from the #3061 review (nit): the MCP envelope field min_commit_tokens_applied (services/ravel-server/src/mcp/service_impl.rs:601) now echoes the qualified wire form <table>:<token>, but the envelope docs still describe it as commit tokens. Say so in the envelope docs during the T8 sweep.
4. pmoust commented on Oct 18, 2026

   

   pmoust

   on Oct 18, 2026

   MemberAuthor

   More actions

   Carried from the PR #3070 (T5) fleet review, nit, for this ticket because it owns the EXPLAIN surfaces:

   explain_accounted merges its per-lane cost into the caller's QueryAccounting through MergeOnDrop on every exit (crates/ravel-sql/src/executor.rs, around lines 1941 to 1955), and docs/query-engine.md says so. Only the success and error exits are tested (explain_reports_each_signal_table_and_the_caller_keeps_the_total, a_failed_explain_still_merges_its_cost_into_the_caller). Add a test that trips the deadline, or drops the explain_accounted future at its first store call, and asserts the caller's accounting is nonzero. The server's cancelled-explain cost also rests on drop order (the guard drops before UsageGuard in services/ravel-server/src/service/mod.rs sql_explain); a comment next to MergeOnDrop saying so would keep a refactor from moving it.
5. pmoust commented on Oct 18, 2026

   

   pmoust

   on Oct 18, 2026

   MemberAuthor

   More actions

   Carried from the #2913 (PR #3092) checkpoint review. The review called these nits; none blocked:

   • checked_lanes (crates/ravel-sql/src/executor.rs, about 2511-2524) takes any single lane on a one-table statement and relabels it without checking its signal. An (Alerts, s) lane with SELECT .. FROM logs would plan alert records as log rows rather than return PinnedSignalsMismatch, which the variant's doc implies. No production caller can reach this, only the pub API. Check the signal there.
   • No test pins "refused before an admission permit". The v10 mismatch test still passes with the DoGet check removed, because a later plan-time check raises the same error with zero store ops. Add an assertion on the permit, for example a semaphore with no permits left.
   • The Flight erasure test (each_signal_group_carries_its_own_pending_erasure) has a samples-side .dreq only. Add a logs-side one, so that a dropped logs predicate fails a test, and so does one applied to samples.
   • redemption_lanes is used only under flight-sql, so a build with pin-codec alone probably warns dead_code. This is inferred; no lane builds that combination.
   • Doc sweep: ADR-0033's ADR-2904 amendment (docs/adrs/0033-sql-query-over-logs.md, about line 391) still says a min_commit_token on a statement naming two or more signal tables is refused over HTTP as not supported yet. T4: table-qualified min_commit_token (epic #2904) #2911 made that untrue.
6. pmoust commented on Oct 18, 2026

   

   pmoust

   on Oct 18, 2026

   MemberAuthor

   More actions

   Carried from the PR #3092 (#2913) fleet review. Both are nits; the review was otherwise clean. Both are stale sentences in docs/query-engine.md:

   • About line 3945, the lead-in to "Statements over several signal tables": it says such a statement "runs on the HTTP endpoint". It runs on Flight SQL too since T6: Flight ticket version 10 for multi-signal statements (epic #2904) #2913, and the subsection heading below already names both. Make it "runs on the HTTP endpoint and on Flight SQL".
   • About line 3966: "A retry resolves every signal again" holds for HTTP only. A Flight DoGet retry redeems the same ticket and replans against its pinned snapshots, with no second catalog resolve; docs/consistency-model.md already says so. Scope the sentence to HTTP and say what a Flight retry does.

   The review also asked whether lanes_covering_window should use a window per lane on multi-signal statements. It takes the min and max ingest_hour_bucket across every lane, so each lane's load_column_stats gets asked for a wider window than its own segments cover. That is a cost question only, not correctness; act on it only if a bench shows the extra part reads.
7. pmoust commented on Oct 19, 2026

   

   pmoust

   on Oct 19, 2026

   MemberAuthor

   More actions

   Note: the ADR-0033 sentence carried here from the #2913 checkpoint (a min_commit_token on a multi-signal statement, and a signal table beside a Parquet table, "refused over HTTP as not supported yet") is now owned by #2914 (T7, task 75039c70), which rewrites it. T8 does not need to redo it.
8. pmoust commented on Oct 19, 2026

   

   pmoust

   on Oct 19, 2026

   MemberAuthor

   More actions

   Carried from the #2914 (T7) fix round: ADR-2904 decision 5 describes samples name postings pruning, but says nothing of its loss beside a Parquet table. In a samples JOIN parquet statement the samples lane resolves without name pruning (pushed_down_name_filter builds its planning session from the signal lanes only). Rows are unchanged; every segment in the window is read. T7 states this in docs/query-engine.md and the ADR-2040 amendment. The ADR-2904 text belongs to the doc sweep.
9. pmoust commented on Oct 19, 2026

   

   pmoust

   on Oct 19, 2026

   MemberAuthor

   More actions

   Carried from the PR #3107 (#2914) fleet review, both nits:

   • docs/adrs/2040-parquet-tables-queried-in-place.md, about line 1492: the earlier ADR-2904 amendment was edited in place. It still says, in the present tense, that "the executor refuses such a statement with SqlError::SignalBesideParquet", a variant T7: a Parquet table beside a signal table (epic #2904) #2914 deletes. The inserted parenthetical satisfies the amendment-integrity pointer but leaves the sentence contradicting itself. Suggested fix: "Until ticket T7 of epic Epic: cross-signal SQL statements #2904 (T7: a Parquet table beside a signal table (epic #2904) #2914) landed, the executor refused such a statement with SqlError::SignalBesideParquet ...; see the interim refusal amendment below."
   • crates/ravel-sql/src/executor.rs, about line 3340: let several = lanes.len() > 1; counts signal lanes only. So a RequestBudgetExceeded from check_lane_request_budget on a statement with one signal table and a Parquet table carries table: None. The plan path computes several_tables = lanes.len() > 1 || (parquet.is_some() && reads_lanes) for the same statement. Make the two agree, or document why the lane name is withheld.
   • Cosmetic: the README paragraph edited by T7: a Parquet table beside a signal table (epic #2904) #2914 has a sentence break in the middle of a line ("...bounds only the five. On").