Repository navigation
feat(sql): run a Parquet table beside a signal table - #3107
Conversation
A SQL statement may now name Parquet tables beside signal tables on both POST /api/v1/sql and Flight SQL, as ADR-2904 decision 11 allows. Such a statement was refused with SqlError::SignalBesideParquet, which is removed. The Parquet tables resolve first and the signal lanes then resolve on top of those reads, so each lane's resolve-time max_s3_requests check counts the Parquet resolve reads, and the cost estimate is the Parquet estimate plus each lane's. Tests pin both: a budget one below the Parquet and logs resolve reads together is refused by the logs lane's resolve check with that sum, and a mixed estimate equals the Parquet-only estimate plus the logs-only one. The session registers the named signal tables beside the Parquet tables and takes the Parquet store registry, read limits and parquet_session_config. A signal-only session still gets no registry and a Parquet-only session registers no signal table. A test pins that those settings leave a signal scan's physical plan as it is without the Parquet tables. The samples lane does not plan the same, though: beside a Parquet table it resolves without the __name__ postings pruning, because the throwaway planning session cannot plan a Parquet table before its footers are read, so a samples filter on __name__ reads every segment in its window where alone it reads only those whose postings name the metric. The rows are the same. The Parquet reader now checks each read against the statement's total requests and bytes through a budget view on ReadLimits when the statement also reads a signal table, and still records the read on the Parquet handle. Three checks see the statement total: the lanes' resolve-time request check, the Parquet reader's request and byte check before each Parquet read, and the samples scan's checkpoint. The logs, spans, alerts and audit scans make no scan-time request check and no byte check. Tests refuse the last Parquet GET of a logs aggregate joined with a Parquet table under a request budget and under a byte budget each one below the statement total. A commit token bounds the signal tables only, and Flight SQL now refuses a token on a Parquet-only statement at GetFlightInfo as HTTP does. A row window filters the signal scans only. page_plan refuses a signal table beside any other table, since one signal row can join many rows of another table. Flight tickets pin the Parquet manifests beside the signal snapshots in their existing layouts; slice tickets still carry no Parquet table. A test joins two signal tables with a Parquet table and pins its exact rows. The docs, the ADR-2040 amendment that recorded the interim refusal, the ADR-0033, ADR-0045 and ADR-1101 status sentences and the claims registry are updated to match. The test a_logs_key_equality_beside_a_parquet_table_returns_exact_rows pins that a logs KEY_IDX key equality beside a Parquet table returns the rows of the same statement over a VALUES list, for a join and for COUNT(*) and MIN over it, and that the KEY_IDX probe runs. Fixes: #2914 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
|
@claude-fleet review |
|
Review task queued. Task: f7908461-e821-438c-b52e-c72ff38b48ea |
There was a problem hiding this comment.
Review of #3107 at 0b70fe4 (merge base 1b4d298).
Verdict: comment
The core change is sound. I read every modified file against its diff, and the three things most likely to break here all hold up:
Verified:
- The budget view is the statement total, and nothing is double-counted.
ReadLimits::with_budget_view(crates/ravel-parquet/src/limits.rs) replaces only the source of the pooled snapshot inrefusal; the in-flight bytes and the+1request for the read being considered are still added on top, and admission still records on the Parquet handle.StatementAccounting::pooled_snapshotsums lanes + Parquet + statement, and the closure captures a clone that shares the underlying counters, so the view is live rather than a snapshot taken at plan time. The executor installs it only whenreads_lanesis true, so a Parquet-only statement keeps reading the Parquet handle, which equals the total there. reads_lanesdistinguishes Parquet-only from mixed on every branch it gates (executor.rs:2663): target signal,several_tables, the budget view, the stats gate, lane registration, and the row-window fallback. A Parquet-only statement still registers no signal provider (a_parquet_only_statement_registers_no_signal_table), andlogs_column_stats_eligibletakes the conservative per-scan path for mixed.- Estimate arithmetic. The Parquet estimate is computed from
resolve_snapshot().total_s3_requests()(actuals, Parquet-only at that point) and each lane addsestimated_catalog_requests, a static figure, so there is no overlap between the two terms.a_mixed_estimate_is_the_parquet_estimate_plus_the_lanespins it. - Resolve ordering. Parquet resolves first and lanes resolve after, so
check_lane_request_budget, which reads the statement-wide resolve snapshot, counts the Parquet manifest and grant reads. Pinned bya_lanes_resolve_budget_check_counts_the_parquet_resolve_reads. - Flight tickets.
parquet_tablesis encoded and decoded outside the version branch, so the flat (v8/9/11) shape a mixed single-lane statement actually mints carries the pins as well as the new grouped case the unit test covers;redemption_lanestakes the flat branch totables.first_lane(), which is the one signal table, andTicketPinsOneSignalstill guards the >1 case. The end-to-enda_flight_mixed_statement_pins_the_parquet_table_and_the_signal_laneexercises the flat shape with both sides changed between the RPCs, and is non-vacuous (it re-plans and sees both changes). - The Flight token refusal is equivalent to HTTP's.
unqualified_tokensalready rejects every qualified token on Flight, so the newCommitTokenOnParquetStatementcheck inresolve_pinned_tablescovers exactly the remaining case, and the test asserts the sameclient_messageas HTTP plus zero store reads. - Paging.
page_target's newtables.len() == 1guard uses the CTE-strippedreferenced_base_tablesset, so a signal table reached through a subquery or anINlist refuses too; the test covers all three shapes and both call levels. - Row window. Filtering stays per-
TableScanon signal tables, and the parquet-onlyrow_windowrefusal is unchanged (row_window && tables.signals.is_empty()). SqlError::SignalBesideParquetis gone from code, docs and claims (the only remaining mentions are the ADR-2040 history).- Every test named in the new
claims.yamlentries and in the doc prose exists incrates/ravel-sql/tests/parquet_tables.rs, and each new test carries aFLIP:line naming the wrong implementation it catches.
Findings (details in the JSON below):
crates/ravel-sql/src/session.rs— theLIKErewrite silently narrows toLogsColumnsOnlyfor alogsstatement that gains a Parquet table. should_fix, documentation and test.docs/adrs/2040-parquet-tables-queried-in-place.md— the earlier amendment is edited in place and still reads, in the present tense, that the executor refuses with a variant this PR deletes. nit.crates/ravel-sql/src/executor.rs—severalfor the refusal's table name counts signal lanes only, so a mixed single-lane statement'sRequestBudgetExceededcarries no table while the plan path treats the same statement as several tables. nit.
Checks run: git diff --stat then each modified file's diff separately; read the unchanged call sites that the changed branches depend on (validate.rs referenced_base_tables, like_udf.rs LikeScope, scan.rs samples checkpoint, provider.rs with_statement_accounting, flight/service.rs get_flight_info_statement, flight/stream.rs, flight_ticket.rs encode/decode/redemption_lanes, phase_accounting.rs); grepped for other copies of the lifted refusal and for stale user-facing text; checked the description's claims against the diff.
Smaller things: the README edit leaves a sentence break mid-line ("...bounds only the five. On\nPOST /api/v1/sql...") that reads oddly in the rendered paragraph; purely cosmetic. The estimate-based pre-check still covers only the Parquet term (the lanes are checked on actual resolve requests, not on the summed estimate), which is not a regression but means the early refusal for a mixed statement is weaker than the sum of its parts.
Not checked: nothing was compiled or executed here — no cargo test, no clippy, no feature lanes (sql, flight-sql), no scripts/gates.sh, and no guard scripts (check-doc-claims.py, the amendment-integrity guard over the new ADR markers); I did not run the revert check on any new test, so the FLIP: claims are read, not demonstrated. I did not verify against DataFusion's own source that the built-in LIKE refuses a custom escape — finding 1 rests on like_udf.rs's own doc comment saying so. I did not review the ES|QL or MCP surfaces beyond confirming they carry no Parquet table concept, did not examine the alert-rule evaluation path, and did not check services/ravel-server's rendering of stats.signals for a mixed statement.
Not checked:
- Nothing was compiled or executed: no cargo build, test, clippy, doctest, or scripts/gates.sh run, and no sql/flight-sql feature lane.
- No revert check on any new test, so each test's FLIP: claim was read rather than demonstrated failing without the change.
- Guard scripts were not run: check-doc-claims.py over the rewritten docs/review/claims.yaml entries, and the amendment-integrity guard over the new amendment-applies / amendment-supersedes markers in ADR-2040.
- Whether DataFusion's built-in LIKE really refuses a custom ESCAPE in the pinned version; finding 1 rests on crates/ravel-sql/src/like_udf.rs's own doc comment stating it.
- The ES|QL and MCP surfaces beyond confirming neither carries a Parquet table concept, and the SQL alert-rule evaluation path for a mixed statement.
- services/ravel-server's rendering of stats.signals and stats.phases for a mixed statement; only the in-crate accounting arithmetic was read.
- The ravel-parquet reader internals beyond ReadLimits::refusal: concurrent read interleaving between signal scans and Parquet scans under a shared budget view was reasoned about from the saturating sums, not measured.
- Performance of the samples lane without name postings pruning beside a Parquet table; the correctness claim (same rows) was read in the docs and not measured.
Task f7908461-e821-438c-b52e-c72ff38b48ea on rp2, model claude-opus-5, effort medium.
| /// exactly one Parquet entry gets a store registry that answers anything. | ||
| /// two entries for one table name, and two Parquet entries. A Parquet entry | ||
| /// may sit beside signal tables (ADR-2904 decision 11). Only a set with a | ||
| /// Parquet entry gets a store registry that answers anything, and its session |
There was a problem hiding this comment.
should_fix: A logs statement that gains a Parquet table also changes its LIKE semantics, and nothing in the user-facing documentation says so.
like_scope is LikeScope::Every only when the session has exactly one table entry. Before this change, a logs-plus-Parquet set was refused, so a logs-only statement always got Every. Now the same statement with a Parquet table joined in builds a two-entry session and takes LogsColumnsOnly. By like_udf.rs's own doc, a LIKE on a logs column with a custom escape or a non-string operand then goes to the built-in operator, "which refuses it, while the same predicate runs in a logs-only session".
So SELECT ... FROM logs WHERE body LIKE 'a!_b' ESCAPE '!' answers today and becomes an error once the statement also names a Parquet table, and a plain logs LIKE loses the dictionary fast path. That is the same class of cost as the __name__ postings regression, which this PR documents carefully in changelog.d/2914.added.md, the ADR-2040 amendment and docs/query-engine.md, with a named test. This one appears in none of them, and no test pins it.
Suggest adding a sentence beside the __name__ note in the changelog entry and the amendment, and a test that asserts which evaluator a logs LIKE takes beside a Parquet table (or that the escape form is refused there), so the narrowing is a pinned decision rather than a side effect of the session's table count.
If the intent is instead that a mixed session keep Every for logs columns, that is a behavior change rather than a doc change, and the current tables.len() == 1 test is the thing to revisit.
There was a problem hiding this comment.
Answered, with no change in this PR.
- No statement that runs today changes. Before this PR, a
logsstatement that also named a Parquet table was refused outright withSignalBesideParquet. So nothing "answers today and becomes an error". A statement that used to be refused now runs, under the multi-table LIKE rule. - The rule is already documented, and covers any table beside
logs. ADR-2904's LIKE-scope amendment (T2: session registers a set of tables, with a column-scoped LIKE rewrite (epic #2904) #2909, "a session that registerslogsbeside another table") lists the four rewrite conditions. It also states this exact cost: a genuinelogsLIKE with a custom escape, or a non-string operand, goes to the built-in operator and is refused, while the same predicate runs in a logs-only session.session.rssays the same at thelike_scopedoc: "beside another table isLogsColumnsOnly". A Parquet table is "another table" there, just assamplesis. - The fix is sql: refuse names that shadow a registered table, and drop the LIKE type and escape conditions #3026. It makes the qualifier test exact and drops the type and escape conditions, so a genuine
logsLIKE behaves the same with or without another table. I've added a note to sql: refuse names that shadow a registered table, and drop the LIKE type and escape conditions #3026 that a Parquet table besidelogsnow reaches this rule too, so its test should cover that shape.
| unchanged. Until ticket T7 of epic #2904 (#2914) lands, the executor refuses | ||
| such a statement with `SqlError::SignalBesideParquet`, a 400 that says it is | ||
| not supported yet, before any signal catalog listing. | ||
| unchanged. Until ticket T7 of epic #2904 (#2914) lands (it has; see the |
There was a problem hiding this comment.
nit: The earlier amendment is edited in place, and the edited sentence still asserts in the present tense that "the executor refuses such a statement with SqlError::SignalBesideParquet" for a variant this PR deletes. The inserted parenthetical "(it has; see the interim refusal amendment below)" satisfies the amendment-integrity pointer requirement but leaves the sentence self-contradicting for anyone who reads that paragraph alone.
Two things follow. First, the PR description says the earlier amendment is not edited in place; the diff does edit it. Second, the pointer requirement can be met with a sentence that is also true now, for example: "Until ticket T7 of epic #2904 (#2914) landed, the executor refused such a statement with SqlError::SignalBesideParquet, a 400 ...; see the interim refusal amendment below." Past tense keeps the superseded phrase traceable while removing the false present-tense claim.
There was a problem hiding this comment.
Nit, carried to #2915 (the epic doc sweep), so the reviewed head stays as it is.
| } | ||
| } | ||
| let lanes = tables.lanes(); | ||
| let several = lanes.len() > 1; |
There was a problem hiding this comment.
nit: let several = lanes.len() > 1; counts signal lanes only, so for a statement naming one signal table beside a Parquet table every RequestBudgetExceeded raised by check_lane_request_budget carries table: None, even though the statement does name more than one table and the refusal's cost includes the Parquet resolve reads. The plan path takes the opposite reading three hunks later: several_tables = lanes.len() > 1 || (parquet.is_some() && reads_lanes).
The Parquet-estimate refusal just above (table: None at the early return) is correct, since that one is attributable to the Parquet side alone. This is only the message, not the verdict, but the two flags being spelled differently for the same statement shape is the kind of thing that reads as a bug later. Either make this one match several_tables, or say in the check_lane_request_budget doc why the lane name is withheld when a Parquet table is the second table.
There was a problem hiding this comment.
Nit, carried to #2915 (the epic doc sweep), so the reviewed head stays as it is.
T7 of epic #2904 (ADR-2904 decision 11, plus the paging rule from decision 9). A SQL statement that named a Parquet table beside one of
samples,logs,spans,alertsandauditwas refused withSqlError::SignalBesideParquet. It now runs onPOST /api/v1/sqland Flight SQL alike. ADR-2904 rejected alternative G calls for one rule per statement text.What changed
max_s3_requestscheck counts the Parquet resolve reads.SqlError::SignalBesideParquetis removed.parquet_session_config.ReadLimits(ravel-parquet) gains a budget view. When the statement also reads a signal table, the Parquet reader checks each read against the statement's total requests and bytes, and still records the read on the Parquet handle.samplesscan's checkpoint.logs,spans,alertsandauditscans check neither budget while they read; the docs say so.min_commit_tokenbounds only the signal tables. A token qualified with a Parquet table's name is a 400.GetFlightInfo. It used to accept the token and ignore it; HTTP already refused it (decision 3).page_targetsees non-signal names, so a signal table beside any other table returnsPagePlanError::CrossSignal. One signal row can join many rows of another table, so its row identity is not a total order over the join. Parquet-only statements keep theNoTableoutcome.sampleslane resolves without__name__postings pruning: the planning session that derives it cannot plan a Parquet table before its footers are read. Asamplesfilter on__name__reads every segment in its window, and the rows are the same. The docs state this.Docs
docs/query-engine.md,docs/guides/query.mdandREADME.mdare updated.docs/review/claims.yamlentries now bind the new tests.changelog.d/2914.added.mdis new. The unreleased2910.added.mdfragment drops its "returns a 400" sentence.Tests
All tests are in
crates/ravel-sql/tests/parquet_tables.rsunless noted.a_parquet_table_joined_with_logs_returns_exact_rows, with read counts for the Parquet resolve plus the logs lane;samples_joined_with_a_parquet_table_returns_exact_rows;two_signal_tables_joined_with_a_parquet_table_return_exact_rows.a_signal_only_statement_gets_no_parquet_store_registry, and its mirror for Parquet-only statements, on HTTP and Flight.a_mixed_statement_is_held_to_one_request_budgetanda_mixed_statement_is_held_to_one_byte_budget;a_lanes_resolve_budget_check_counts_the_parquet_resolve_reads;a_mixed_estimate_is_the_parquet_estimate_plus_the_lanes.CommitTokenOnParquetStatementtext.stats.signalsplus the Parquet share equalsstats.phases); and Flight mixed statements, including a Parquet replace and a signal commit between the two calls thatDoGetdoes not see.page_plan::tests::a_signal_table_beside_a_parquet_table_is_not_pagedanda_parquet_only_join_still_reports_no_table.a_logs_key_equality_beside_a_parquet_table_returns_exact_rowsruns on the combined tree. It returns the same rows as the statement over a VALUES list, for a join and forCOUNT(*)andMIN, and asserts that the KEY_IDX range GETs fired.Each of these wrong implementations was shown to fail a test:
page_targetcounting signal names onlysamplesproviderRequestBudgetExceeded { requests: 10, max: 6 }, expected 7)Verification
sql, 1814;flight-sql, 3544;esql, 1835;sql-latency,profiling,flight-lane, 692; benchsaturation, 399;stage-timing;ravel-logsegandravel-ingeststage-timing, 1115;sql-explain, 743;ravel-sql,flight-sqland serversql;ravel-sql(1499),flight-sql(3571) and serversql(1830);check-doc-claims.py.Known gaps, tracked elsewhere
__name__postings pruning should mention the loss beside a Parquet table.logs_column_stats_eligibledoes not mirror thekey_equalscondition feat(sql): probe KEY_IDX for logs exact-key equalities #3091 added tostats_are_exact. It fails open, so this costs reads and never correctness.Fixes: #2914