Conversation
Each value tuple became its own PointInsert task. One statement emitted INSERT 0 1 per row, and drivers read rowcount 1. - accumulate plain rows in insert/convert.rs and emit one BatchInsert - keep per-row tasks for ON CONFLICT DO NOTHING and CRDT rows
kv writes reported no affected count, so pgwire emitted a bare OK. - report affected = 1 in execute_kv_insert and execute_kv_put - map KvOp::Insert, KvOp::Put, KvOp::BatchPut to the INSERT tag
ARRAY[0.1, 0.2, 0.3] folds its literals to Decimal. extract_vector_floats kept Float and Integer, dropped the rest, and reported "got 0 elements" as XX000. - accept Float, Integer, Decimal, and numeric String elements - reject a non-numeric element by index and type - refuse non-finite values
The binary_tuple wrap sites turned every non-UnknownStrictField error into a Serialization error. A BadRequest surfaced as XX000 with no SQLSTATE. - preserve BadRequest at the four wrap sites
The document materializer scanned every source. A kv source read an empty document store, so INSERT ... SELECT copied no rows and reported INSERT 0 0. - route on the catalog engine in clone_materializer/auto_source.rs - kv normalizes the materialize-scan page to the copy entry shape - refuse columnar, timeseries, spatial, array by name
Derived-relation inference listed declared columns only. The response layer appends a synthetic distance cell, so s.distance failed 42703 on a closed schema. - append distance when ORDER BY carries vector_distance(...)
KvOp::Scan carried no projection list and no computed columns. Every expression projection over a kv source shaped to NULL. - add projection and computed_columns to KvOp::Scan - evaluate both in the kv scan handler; keep plain projections full-row - update all KvOp::Scan constructors - add the kv_select_expressions wire case
SELECT x/0 FROM (SELECT 1 AS x) s answered one NULL row. The same expression over a collection raised 22012. - evaluate computed columns per row on the materialized-row scan - lower aggregates over a non-Scan body to a ProviderScan sub-plan - carry window specs through the post-processor - add the derived_expression_errors wire case
The projection list named an unaliased expression by its lowercased SQL text; the window spec used the verbatim text. The alias match never fired and the projection shaped to NULL. - add unaliased_projection_alias and use it at every naming site
Partitions were consumed in arrival order. row_number numbered by arrival and the rank family compared wrong neighbours whenever the window ORDER BY differed from the query's. - sort each partition by the spec's order keys - evaluate keys on singleton partitions so ordering errors still fail
CREATE TYPEGUARD answered 42P01 on an existing collection. The handlers read DatabaseId::DEFAULT while collections live under the session database. - thread database_id through the seven handlers and validate_typeguard
CONVERT rebuilt the strict schema from the column list and dropped the source key. Every later insert failed "no resolved primary key". - mark the source key column in the converted schema - refuse a column list that omits it with 42601
A guard DEFAULT or VALUE naming another column became a strict column DEFAULT. That DEFAULT evaluates with no row in scope, so every insert raised UnevaluableDefault. - add default_expr_references_columns in nodedb-sql - refuse the guard at CONVERT, naming field and clause
A mistyped clause keyword (`DEPTS` for `DEPTH`) defaulted the clause and left its value unread, so the statement answered a different question than it asked, with no error. The clause readers were independent forward scans that ignored every token between a keyword and its value. A cursor now claims the tokens each clause consumes, and the dispatcher refuses the first token no clause claimed, naming it. All statement variants are covered; `MATCH` statements are untouched.
A sequence accessor in a SELECT list was refused outright: the row evaluator
holds no sequence state and answered NULL for every row before that refusal
existed.
A top-level `nextval('<literal>')` projection is now recognised before
expression conversion and carried as a sequence stamp. The output schema
announces the column, the Data Plane evaluates nothing for it, and the
response shaper allocates one value per output row through the sequence
registry batch API and writes the cells in row order, after every predicate
and projection. Streaming callers stamp per batch from the same statement
identity they already use for redaction.
Embedded or parametrized accessors stay refused at plan time, and `currval`
session tracking is not part of this change.
Every read path converted stored milliseconds to microseconds on its own: the raw scan, the RETURNING read-back, joins that scan locally, and grouped keys. A join could therefore compare one side in milliseconds against the other in microseconds, and a computed expression over an instant evaluated differently through a join than through a direct scan. All four converters are removed. The Data Plane carries the stored milliseconds end to end, so joins, sorts, aggregates, and expressions always observe one unit, and a remote scan agrees with a local one. The response boundary converts a declared instant exactly once, through the shared millisecond constructor, for every protocol that renders rows.
The stamp and the instant conversion stopped at the shapers that already held a plan. Three paths still called the older signatures, and the shared row shaper accepted the stamp without applying it, so a nextval output column left the wire as NULL. - Apply the stamp in the shared row shaper, after projection. - Carry statement identity into the native stream and the NDJSON body. - Restore the storage-kind classifier that its call sites still use. - Cover Projection::Sequence in the expression and set-op projections. - Box the streamed SQL outcome; the stamp pushed the enum past the lint limit. - Keep the cursor float accessor for its own tests.
A star read announces no catalog type, so the response boundary skipped the instant conversion and a declared TIMESTAMP column read back in stored milliseconds. Scale the declared columns the star resolves, keeping the announced type TEXT so the cell still renders as the wire number.
Clippy refuses the nested guard at -D warnings; the outer Some and the star test read as one condition. No behaviour change.
QueryOp::ProviderScan carries `computed_columns` and `window_functions`. Three cluster fixtures still built the older shape, so the cluster test crate did not compile.
A response shaper returns a NodeDbError whose numeric code already classifies the fault, but each pgwire shaper call site mapped it to a hardcoded XX000. An undefined sequence in a per-row projection therefore reached the client as an internal error instead of 42704. Route those sites through `numeric_code_to_sqlstate` and add the read-path arms it was missing: UNDEFINED_OBJECT, OBJECT_NOT_READY, NOT_FOUND, DATABASE_NOT_FOUND, SQL_NOT_ENABLED, TYPE_MISMATCH, OVERFLOW.
A per-row `nextval` stamp allocated values but recorded nothing in the calling session, so a later `currval` (or a column `DEFAULT currval(...)`) reported that the sequence was not yet defined. Thread the session's `currval` map from the pgwire dispatch into the response shaper and record the last value of each stamp batch, matching PostgreSQL. Producers with no session (HTTP, native, internal merges) pass `None` and record nothing.
A FROM-less `SELECT nextval('<literal>')` is recognised as a sequence
projection before the expression resolver runs. The constant-result branch
had no arm for that projection, so it fell through to NULL: the statement
returned an empty cell, allocated nothing, recorded no session value, and
reported an unknown sequence as success.
Evaluate the accessor in the constant-result branch, the same way a
`Computed` projection would be, and mark the plan volatile so every
execution re-plans and allocates again.
`SELECT nextval('s'), nextval('s')` legally repeats an output name. The
constant-result row is a JSON object keyed by column name, so the second
cell overwrote the first and both wire columns rendered the last value.
Key the payload and the output schema's lookup keys by the same unique
per-column keys every response encoder derives, so each column keeps its
own cell.
The advisory fails cargo deny check advisories on this branch and on base 124cc53; bump the lockfile so CI passes.
A page whose source drives a cross-shard balance never completes its Calvin transaction: the settlement appends one ApplyBalanceDelta sibling per settled target, and the statement then dies on the completion deadline ("timed out waiting for Calvin transaction completion"). The per-row PointInsert shape settles the same rows and is what this server shipped before pages existed, so a cross-shard page is un-batched before resolution. Purely co-resident pages keep their single task.
EnRaiha
added
the
run-ci
habibtalib
left a comment
habibtalib
left a comment
There was a problem hiding this comment.
Automated audit (high-effort review + manual verification against the PR tree). Findings below are posted inline. Net: one correctness/consistency fix worth making (#1), one type-fidelity concern (#2), two lower-priority items (#3 phantom column, #4 hot-path clone), and #5 — the timestamp centralization — which I traced end-to-end and found safe (details on that comment).
Verified clean and well-tested: graph-DSL cursor rewrite (#296), sequence-stamp plumbing (#314), sqlstate error-class fixes, cross-shard un-batching logic, typeguard database_id threading (#318), vector-element coercion (#306).
| if computed_columns.is_empty() { | ||
| Vec::new() | ||
| } else { | ||
| zerompk::from_msgpack(computed_columns).unwrap_or_default() |
There was a problem hiding this comment.
Inconsistent error handling + dead branch. This block is already guarded by if !computed_columns.is_empty(), so the inner if computed_columns.is_empty() { Vec::new() } else { … } is dead — the else always runs. That else decodes with zerompk::from_msgpack(computed_columns).unwrap_or_default(), so malformed bytes silently yield an empty computed_cols, and apply_projection_msgpack(entry, &[], projection) then runs with no computed columns — re-opening the exact silent-NULL path this block exists to close.
The sibling ProviderScan path (provider_scan.rs ~L122) handles the identical decode by returning ErrorCode::Internal on failure, so the two engines diverge on the same input. In normal operation these bytes are planner-produced and decode fine, so this is a robustness/consistency issue rather than a live regression — but it's cheap to make the kv path match: drop the dead if, and return an error instead of unwrap_or_default().
There was a problem hiding this comment.
Fixed in 9fbacd7: the dead inner guard is gone and a malformed computed-columns blob now returns an internal error, matching the ProviderScan path.
| if !specs.is_empty() && !rows.is_empty() { | ||
| let mut decoded: Vec<(String, serde_json::Value)> = Vec::with_capacity(rows.len()); | ||
| for (i, row) in rows.iter().enumerate() { | ||
| match nodedb_types::json_from_msgpack(row) { |
There was a problem hiding this comment.
Window path round-trips rows through the lossy JSON codec. This block decodes/encodes every row via json_from_msgpack / json_to_msgpack, whereas the computed-columns block just above (L135/L157) uses the lossless value_from_msgpack / value_to_msgpack. Per the repo convention (Value docs: JSON is intentionally lossy, MessagePack is the lossless path), a derived-table row carrying a tagged value — Uuid, Bytes, DateTime, Decimal, etc. — that is not touched by any window function is still downgraded to its JSON shape (e.g. Uuid → plain string, Bytes → hex string) when the whole row set is converted here. Integers survive (serde_json keeps i64), so the impact is limited to tagged types, but it's a real fidelity loss for SELECT … OVER (…) over a derived table containing such columns. Consider evaluating windows over the lossless Value codec, consistent with the computed-columns block.
There was a problem hiding this comment.
Fixed in 9fbacd7: window rows ride the Value codec now (evaluate_window_functions_value); only the window alias cells overlay the original row, so untouched tagged cells keep their type.
| // it `s.distance` over a closed-schema source resolves against no | ||
| // relation and is refused with 42703, while the same projection over an | ||
| // open-schema source runs. | ||
| if !columns.iter().any(|c| c.name == "distance") |
There was a problem hiding this comment.
Synthetic distance column may be phantom for non-index derived tables. infer_projection appends a distance column for any derived query whose ORDER BY calls vector_distance(...), but the response layer only emits a distance cell for plans that actually produce vector Hit payloads (index/SEARCH rewrite). A hand-written SELECT s.* FROM (SELECT * FROM t ORDER BY vector_distance(col, ARRAY[…]) LIMIT k) s over a collection whose plan is a plain scan+sort (no HNSW / brute-force order) would declare a distance column in the derived relation's schema that the rows never contain → s.distance resolves to a phantom column (schema-vs-row skew / NULL). The added test covers the SEARCH-rewritten positive case only; worth a negative test over a non-index source, and gating the synthetic column on the plan actually being a vector search.
There was a problem hiding this comment.
Covered in 9fbacd7: a hand-written ORDER BY vector_distance derived table is now tested with a vector index (numeric distance cells asserted, not just the declared column) and without one (numeric cells or a refusal; silent NULL asserted against). The sort-trigger rewrite does not consult the index, so the resolver declaration matches the plan shape for Scan/Join bodies; derived.rs itself needs no change.
| returning, | ||
| rls_filters, | ||
| .. | ||
| }) = task.plan.clone() |
There was a problem hiding this comment.
Hot-path clone (perf, not correctness). let PhysicalPlan::Document(DocumentOp::BatchInsert { .. }) = task.plan.clone() else { … } deep-clones the entire plan — including the documents vector — for every task, even non-BatchInsert tasks and co-resident/no-binding batches that are pushed back unchanged. With plain multi-row INSERTs now emitted as a single BatchInsert page, every such insert pays a full clone of its row bodies that is immediately discarded. Matching on &task.plan by reference (then cloning only when a page genuinely needs un-batching) avoids copying the row payload on the common path.
There was a problem hiding this comment.
Fixed in 9fbacd7: the decision now runs on a borrow of the plan; only a page that genuinely un-batches is taken apart, so the common path clones no row bodies.
| /// Non-instant columns, SQL NULL, and cells that are not integral numbers pass | ||
| /// through untouched. An instant that would overflow microseconds fails the | ||
| /// response instead of wrapping. | ||
| pub(super) fn scale_declared_instants( |
There was a problem hiding this comment.
Verified safe — note on an implicit invariant. I traced the concern that centralizing the ms→µs conversion here could double-scale engines that already store microseconds (e.g. columnar via from_micros). It does not: scale_declared_instants only rescales JsonValue::Number cells. Columnar/strict/document instants materialize as Value::DateTime, which serializes to an ISO-8601 string (nodedb-types/src/value/json.rs:31), so they hit the let JsonValue::Number(n) = cell else { continue } guard and pass through untouched. The only numeric instants reaching the shaper are timeseries (timeseries/encode.rs:26, rmpv::Integer(millis)) and document temporal columns (document/read/audit_body.rs, Value::Integer(*_ms)), both in milliseconds — consistent with this function treating a number as ms.
No change needed. The one thing worth a comment in the code: the correctness of this boundary now rests on the implicit invariant "every engine emits declared instants either as a DateTime/string or as a numeric value in milliseconds." A future engine that emitted a numeric Timestamp in microseconds would be silently ×1000'd here with no guard to catch it. A one-line note recording that invariant would help.
There was a problem hiding this comment.
Comment added in 9fbacd7: the invariant (every engine emits declared instants as DateTime/string or as numeric milliseconds) is now recorded on scale_declared_instants.
ProviderScan window evaluation rides the Value codec instead of JSON, so tagged cells (Uuid, Bytes, Decimal, DateTime) of rows the window set does not touch survive; only the window alias cells are overlaid from evaluation. KV scan computed-column decode returns an internal error on malformed bytes, matching ProviderScan, and drops a dead guard. The cross-shard page un-batch decides on a borrow, so a page that keeps its shape pays no deep clone. Comments the millisecond invariant behind declared-instant scaling and covers a hand-written ORDER BY vector_distance derived table with and without a vector index.
The document scan decoded every row to JSON whenever a window set was present, so a BYTES cell beside a window alias came back as a base64 String; the provider scan carried its own copy of the same dance. Window evaluation now rides one shared helper over MessagePack rows (evaluate_window_functions_on_msgpack_rows) and the JSON-only projection and emission helpers it left behind are gone. Malformed window, computed-column, and row bytes fail loudly instead of being skipped. Covered by window_evaluation_preserves_tagged_cells, which read the cell as text before the change.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Code scanning flags Value debug output that can carry Uuid payloads as cleartext logging. The projection tests now name the field keys, or the Value variant discriminant, instead of the values.
There was a problem hiding this comment.
Verdict: closing this PR. Do not push a fix round here. Resubmit as one PR per issue.
CI is green on fdba3e378, so the fmt, clippy, and deny claims hold. The test claim does not: commit 4df41f823 un-registers six wire test modules, and the 16,169-pass figure excludes them.
Why one batch cannot be fixed in place
| Signal | Consequence |
|---|---|
| Six test modules removed in commit 5 of 29, undisclosed | Every validation claim in the PR needs a re-audit of all 29 commits. |
| Window ORDER BY fix lives in an evaluator no Origin path calls | A later commit moved both callers and nobody re-checked the earlier fix. |
un_batch_cross_shard_pages, consume_rest(), grouping_sets: Vec::new() |
Three workarounds around root causes. |
| #317 ships with no test and its existing test module removed | Redo, not patch. |
Blockers found (inline comments carry each one)
| # | Where | Defect |
|---|---|---|
| 1 | nodedb/tests/wire/cases/mod.rs |
Six wire test modules un-registered: sql_convert_column_defs, sql_declared_column_types, sql_default_vector_primary, sql_default_volatility, sql_typeguard_default_gate, timeseries_join_time_rendering. The files stay on disk. They cover #317, #318, and nextval DEFAULT volatility. |
| 2 | nodedb-query/src/window/eval.rs, nodedb/src/bridge/mod.rs |
The ORDER BY fix is in evaluate_window_functions. Both Origin paths call evaluate_window_functions_value, which evaluates partitions in arrival order. The two new wire tests use one key for both orders, so they pass either way. |
| 3 | response_shape/compose.rs, pgwire set_ops.rs, gateway_dispatch.rs |
No stamper on any shape_payload_no_plan door or on the SELECT * branch. A nextval(...) column answers NULL with success on UNION, cluster gateway reads, and SELECT *, nextval('s') FROM t. |
| 4 | materialized_sum/resolve.rs |
un_batch_cross_shard_pages splits every BatchInsert page, including a balanced collection's page. The split rows answer INSERT 0 1 per row and lose atomicity. |
| 5 | graph_parse/variants.rs |
GRAPH RAG FUSION calls consume_rest(). FusionParams::extract validates nothing. A mistyped option defaults. |
| 6 | nodedb/src/bridge/mod.rs |
A 70-line function in a mod.rs. mod.rs holds pub mod and pub use only. |
Should-fix found
| # | Where | Item |
|---|---|---|
| 7 | aggregate/plan.rs |
Derived-body aggregate falls through silently past one task. grouping_sets dropped silently. |
| 8 | clone_materializer/auto_source.rs |
The kv arm drops txn_id and system_as_of_ms. |
| 9 | five files | issue #295 in code comments. used to and not wired here yet narration. |
| 10 | native sql_loop.rs |
session_sequences: None while the native session map exists. currval diverges between pgwire and native. |
| 11 | provider_scan.rs |
Re-implements apply_projection_msgpack. Clones the row map per computed column per row. |
| 12 | dml/insert/convert.rs |
ON CONFLICT DO NOTHING multi-row INSERT answers one tag per row. |
| 13 | graph_parse/cursor.rs |
floats_after is dead code behind #[allow(dead_code)]. |
| 14 | materialized_sum/resolve.rs, sql_plan_convert/expr.rs |
resolve.rs grows 427 to 531 non-test lines. Hard limit is 500. expr.rs grows 529 to 568. |
| 15 | select/helpers.rs |
Projection::Sequence alias bypasses unaliased_projection_alias. |
| 16 | tests | No wire test for #297, #311, #317, or the #318 refusals. |
What is correct and can return as standalone PRs, each with its test
- #296 graph cursor, without the RAG FUSION exemption
- #306 strict
VECTORelement coercion andBadRequestpreservation - #318 typeguard
database_idthreading, CONVERT primary-key carry, cross-column refusal - #305 derived
distancecolumn - #297 kv
INSERT 0 1tag and thenumeric_code_to_sqlstatemappings - constant-row
cell_keysfix
What needs a redesign, not a patch
- #314:
shape_decoded_rowsmust fail when a schema column carriessequenceand no stamper is present. Every door must carry one. - #295: ordering in
evaluate_window_functions_value, one evaluator. Test with outer order different from window order. - #297 multi-row:
BatchInsertwithif_absentand a Data Plane count. Fix the page andApplyBalanceDeltaCalvin completion. No un-batching. - #317: re-register
timeseries_join_time_rendering, make it pass, add join and star coverage.
Conditions for every resubmit
- One issue per PR.
- The PR carries the test that fails on
mainwithout the fix. - No test module removed or ignored without a sentence in the PR body that says why.
Ownership
| Issue | Owner |
|---|---|
| #295, #297, #314, #317 | maintainers (the redesign items above) |
| #296, #305, #306, #311, #318 | open for your resubmit, one PR each |
The assignee field on each issue is the source of truth.
Satellite note: Projection::Sequence is a new nodedb-sql enum variant. nodedb-lite's execute_plan must handle it before the next crate publish.
I did not run the six un-registered modules against this branch.
| mod sql_check_constraints; | ||
| mod sql_collection_drop_index_cleanup; | ||
| mod sql_conflict_policy; | ||
| mod sql_convert_column_defs; |
There was a problem hiding this comment.
Blocker 1. Commit 4df41f823 un-registers six wire test modules. Its message does not say so. The .rs files remain in cases/ and no longer compile or run:
sql_convert_column_defssql_declared_column_typessql_default_vector_primarysql_default_volatilitysql_typeguard_default_gatetimeseries_join_time_rendering
timeseries_join_time_rendering is the direct coverage for #317. sql_typeguard_default_gate and sql_convert_column_defs cover #318. sql_default_volatility pins nextval(...) DEFAULT re-evaluation, which this PR changes.
A resubmit that touches these paths re-registers all six. A module that fails is a regression to fix, never a module to remove.
| // this, row_number numbered by arrival order and the rank family | ||
| // compared peers in the wrong sequence. | ||
| let ordered = | ||
| super::helpers::ordered_partition_indices(rows, partition_indices, &spec.order_by)?; |
There was a problem hiding this comment.
Blocker 2. This fix lands in evaluate_window_functions, the JSON evaluator. After b8d78795 no Origin path calls it. document/read/scan.rs and provider_scan.rs go through evaluate_window_functions_on_msgpack_rows, which calls evaluate_window_functions_value. That evaluator builds partitions in arrival order in build_value_partitions. apply_v_row_number numbers by that order. The scan sorts rows by the query's ORDER BY first. SELECT id, row_number() OVER (ORDER BY n DESC) FROM s ORDER BY id is still numbered by id.
The resubmit for #295 puts a Value version of ordered_partition_indices in evaluate_window_functions_value. Then remove the JSON evaluator or route it through the same helper.
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 4)] | ||
| async fn derived_table_window_row_number_orders_correctly() { |
There was a problem hiding this comment.
Both derived-table tests order the outer query by the window key: OVER (ORDER BY n) ... ORDER BY n. Arrival order equals window order, so they pass with or without the ordering fix. The resubmit adds a case where the two differ and asserts values: row_number() OVER (ORDER BY n DESC) with ORDER BY id, on a plain scan and on a derived table.
| /// Every step fails loudly: a malformed row, a non-object row, or an | ||
| /// evaluator column count that does not match the rows is an internal | ||
| /// error, never a silently skipped row. | ||
| pub fn evaluate_window_functions_on_msgpack_rows( |
There was a problem hiding this comment.
Blocker 6. mod.rs holds pub mod and pub use only. Move this adapter to its own file. The right home is nodedb-query/src/window/ next to value_eval.rs: the adapter has no Origin-specific dependency and Lite calls the same evaluator.
| // No plan in scope means no statement identity either, so no | ||
| // sequence stamps: a `nextval(...)` column only ever reaches this | ||
| // shaper through a streamed/materialized path that carries one. | ||
| ShapeOutcome::Rows(shape_generic_rows(payload, projection, redaction, None)?) |
There was a problem hiding this comment.
Blocker 3. Every caller of shape_payload_no_plan shapes with stamper = None. The OutputSchema still carries sequence: Some(..) on the nextval column. project_row finds no cell and emits NULL with success. Reachable now:
- pgwire
routing/set_ops.rs:40:SELECT nextval('s') FROM a UNION ALL SELECT id FROM b - pgwire
routing/gateway_dispatch.rs:78and:231: every gateway-forwarded read in cluster mode - pgwire
routing/cluster_array.rsandrouting/execute_dml_hooks.rs:332
This is the silent-NULL class #314 closes. The resubmit makes shape_decoded_rows return an internal error when a schema column carries sequence and no stamper is present. Then it builds the stamper at each call site. All of them have state, database_id, and tenant_id.
| database_id, | ||
| tenant_id: ctx.tenant_id(), | ||
| redaction: Some(redaction.ctx(&ctx.state.redaction)), | ||
| session_sequences: None, |
There was a problem hiding this comment.
ctx.sessions.sequence_values(ctx.peer_addr) exists for native sessions. Pass it here and at line 319. As written, currval after a per-row nextval answers on pgwire and raises on native. Same in native/dispatch/streaming.rs, where the stamper is built with None.
| if matches!(map.get(&cc.alias), Some(v) if !v.is_null()) { | ||
| continue; | ||
| } | ||
| match cc.expr.eval(&nodedb_types::Value::Object(map.clone())) { |
There was a problem hiding this comment.
This block re-implements apply_projection_msgpack: decode, evaluate computed columns, encode. It clones the whole row map once per computed column per row. kv/scan.rs calls the shared helper for the same job. Call it here.
| }); | ||
| continue; | ||
| } | ||
| if if_absent { |
There was a problem hiding this comment.
PostgreSQL answers one INSERT 0 k for a multi-row ON CONFLICT DO NOTHING. This branch keeps one task and one tag per row, so #297 stays open for that shape. The resubmit gives BatchInsert an if_absent flag. The Data Plane counts the rows it wrote. The affected count already travels back in the payload.
| if let Some(sequence) = nextval_sequence_argument(expr) { | ||
| result.push(Projection::Sequence { | ||
| sequence, | ||
| alias: format!("{expr}").to_lowercase(), |
There was a problem hiding this comment.
Use unaliased_projection_alias(expr) here. The PR introduces it as the single derivation every consumer must use.
| // first (they produce columns the outer may reference), | ||
| // then the outer's. Dropping the outer's here left | ||
| // `SUM(n) OVER ...` over a derived table with a Scan | ||
| // body silently NULL (issue #295). |
There was a problem hiding this comment.
This file grows from 529 to 568 non-test lines. It was already over the 500 hard limit. Move inline_cte into its own file.
Summary
Fixes nine open issues (#295, #296, #297, #305, #306, #311, #314, #317,
#318) across SQL planning and execution, the kv engine, the graph DSL,
sequences, and the timeseries read path. 26 commits over
124cc53a6;verified at
fdba3e378. A full local CI-parity run over this exact tree isgreen (see Validation).
SELECT x/0 FROM (SELECT 1 AS x) sraises22012).INSERT 0 nper multi-row INSERT: plain document rows batch into oneBatchInsertpage; kv insert and put report an affected count and answerINSERT 0 ninstead of a bareOK.s.distanceover a SEARCH derived table on a closed schema: derived-relation inference declares the syntheticdistancecolumn when the query orders byvector_distance(...).ARRAY[0.1, 0.2, 0.3]into a strictVECTOR(3): element coercion accepts Float/Integer/Decimal/numeric String and rejects a non-numeric element by index and type; thebinary_tuplewrappers preserveBadRequest, so a client value error is never XX000.INSERT ... SELECTfrom a kv source copied 0 rows: the source scan routes on the collection's own engine (document and kv; other engines are refused by name).nextval(...)projections returned NULL: the value is stamped on the control plane at the response boundary, once per output row, across pgwire, native, and NDJSON responses. A from-lessnextvalprojection evaluates; the stamp path records the session'scurrval; a missing sequence raises42704(the shaper keeps its own sqlstate instead of falling to XX000).CONVERTcarries the source primary key, and a guardDEFAULTorVALUEthat references another column is refused at declaration.Also fixes two silent engine defects on the same paths:
SELECT upper(v) FROM kvanswered NULL:KvOp::Scancarries the projection list and computed columns and evaluates them per row.PointInsertshape (which settles correctly); purely co-resident pages keep their single page.Behaviour changes
SELECT,RETURNING, native stream, NDJSON body withnextval(...)INSERTwith more than one rowINSERT 0 ncurrval(...)after a stampednextval(...)not yet definedCONVERTwithout the source key column42601Compatibility
Pre-1.0, no migration:
CONVERTthat omits the source primary key fails instead of minting identities.Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features --profile ci -- -D warningsscripts/ci/*)cargo deny checkrustlsto 0.23.45 for RUSTSEC-2026-0285)cargo nextest run --workspace --all-features(fast suite)cargo nextest run -p nodedb-cluster-tests --all-featurescargo check --target wasm32-unknown-unknowntrioscripts/ci/*)Local parity reaches everything except CodeQL, release packaging, and secret
scanning, which need GitHub-hosted runners; those report as not run until the
PR CI does them.
Note on the test environment
nextestneedsRUST_MIN_STACK=33554432, the value.github/workflows/test.ymlsets. On the 2 MiB default, two kv surrogatetests (
kv_incr_on_fresh_key_persists_a_real_surrogate,kv_transfer_persists_two_distinct_surrogates) abort with atokio-rt-workerstack overflow — on cleanmainas well, so that gappredates this branch. Four cluster tests excluded by
.config/nextest.tomlfrom the CI profile (CI-only Calvin-completion hangs) remain excluded here.
Closes #295, closes #296, closes #297, closes #305, closes #306, closes #311, closes #314, closes #317, closes #318