Skip to content

Scan an INSERT ... SELECT source in its own engine - #330

Open
EnRaiha wants to merge 3 commits into
mainfrom
fix/issue311-insert-select-source-engine
Open

EnRaiha wants to merge 3 commits into
mainfrom
fix/issue311-insert-select-source-engine

Conversation

@EnRaiha

@EnRaiha EnRaiha commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Summary

INSERT … SELECT with a kv-engine source copied nothing. The copy pipeline scanned every source with the document materializer, and a kv collection keeps nothing in the document store, so the scan returned no rows and the statement reported INSERT 0 0 — while the filed symptom was expression cells over the copied rows reading NULL because no row arrived at all.

This PR routes the source scan on the catalog engine: a kv source scans through its own materializer and each (key, value) pair is normalized with the one shaping rule every scan path uses. Engines with no materializer, and reads the kv scan cannot honour, are refused by name instead of silently copying nothing.

Behaviour changes

Path Before After
INSERT INTO dst SELECT … FROM kv_src INSERT 0 0 the rows copy; expression cells evaluate
A kv source whose key column is key the key cell copied NULL the row's own key
In-transaction INSERT … SELECT from a kv source committed-only rows copied 42601, naming the unsupported read
A source engine without a materializer (columnar, timeseries, spatial, array) a silent empty copy 42601, naming the engine

Closes #311.

Root cause

  • clone_materializer scanned every source through document::scan_source_page (the name itself says so), regardless of the source's engine. A kv collection's rows live in the kv store, so the document scan returned an empty page; the copy then reported INSERT 0 0 with no error.
  • The kv engine's key slot is not part of the stored row body. Every existing scan path (materializing scan, streaming scan, SQL SELECT, RETURNING, single-key Get) shapes a kv entry with msgpack_scan::kv_row_msgpack, which injects the key into the body at the binary level; a body that already carries its own key keeps it. The new copy arm initially used the raw (key, value) pair, so a kv collection whose key column is the key sentinel copied SQL NULL — silent data loss the audit caught.
  • The kv materialize-scan op (KvOp::MaterializeScan { collection, cursor, count }) carries no snapshot fields, and its dispatch passes txn_id = None. The document arm threads system_as_of_ms and txn_id; for kv there was nothing to thread.

What changed

Commit 1bf454c4e — engine routing:

  • clone_materializer/auto_source.rs (new): one dispatcher that reads the catalog engine and routes document sources to the document materializer and kv sources to their own scan.
  • Engine arms with no INSERT … SELECT materializer (columnar, timeseries, spatial, array) are refused by name: a silent copy with the wrong materializer reads nothing.
  • expand_staged.rs and the insert-select orchestrator call the dispatcher; the document re-export in clone_materializer/mod.rs is retired with its callers; catalog_adapter exposes the engine lookup this needs at the minimum visibility.

Commit 64554ec7a — coverage and one refusal:

  • wire coverage for the kv source route: two rows copy and the expression cell evaluates (upper(v)HELLO/WORLD)
  • the kv arm refuses a point-in-time or transactional read by name: the materialize-scan has no snapshot fields, so those rows cannot be honoured — an in-transaction INSERT … SELECT reaches this path.

Commit 73f1835bd — audit-round coverage and the key column:

  • the transactional refusal and the engine refusals get their wire cases (an in-transaction copy; a columnar source)
  • the kv arm shapes each pair with nodedb_query::msgpack_scan::kv_row_msgpack — the same rule the scan, RETURNING, and Get paths use — so the key column carries the row's own key instead of NULL

Regression proof

On base main, without this change: the kv-source copy reports an empty result, both refusals are absent, and the key column copies NULL. With this change the copy carries both rows with evaluated cells, both refusals answer 42601 naming the cause, and the key column carries the row's key.

Tested

  • cargo nextest run -p nodedb --test wire -E 'test(cases::insert_select_cross_engine) | test(cases::sql_transactions_insert_select_overlay)' — 4 new cases and the neighbouring module tests pass
  • regression set: insert_select_cross_engine, sql_transactions_insert_select_cross_engine, sql_transactions_insert_select_overlay, vector_index_txn_insert_select_stmt_stage17 passed
  • cargo fmt --all -- --check — clean
  • cargo clippy -p nodedb --lib -- -D warnings — clean
  • preflight — pass

Review

A separate, read-only parity audit ran over this branch before submission. Its findings and their resolutions:

Finding Resolution
The snapshot read had no test and is reached in production by an in-transaction INSERT … SELECT the refusal is implemented and covered by a wire case (73f1835bd)
The non-routable-engine refusals had no test a wire case covers a columnar source
A kv source whose key column is key copied NULL shaped with the one rule every scan path uses; covered by a wire case
No pull request existed yet this document

Exclusions

  • Threading a snapshot into the kv materialize-scan would touch the Data Plane op shape (KvOp::MaterializeScan); refusing by name is the honest option in this PR, and the refusal is tested. Threading is a follow-up if the kv read path gains snapshot support.
  • The maintainer-owned paths from the same batch are not touched.

Commits

  • 1bf454c4efix(insert-select): scan a source in its own engine
  • 64554ec7a — kv source coverage, and refuse a snapshot read by name
  • 73f1835bd — refusals and the key column (one shaping rule)

Copilot AI lite review requested due to automatic review settings September 16, 2026 20:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@EnRaiha EnRaiha added the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 16, 2026
@EnRaiha
EnRaiha force-pushed the fix/issue311-insert-select-source-engine branch from 064ba63 to 73f1835 Compare September 17, 2026 02:16
@EnRaiha EnRaiha removed the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 17, 2026
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
The test copies two kv rows into a document target with an expression cell
and asserts both the row count and the evaluated cells. The kv arm also
refuses a point-in-time or transactional read by name: the materialize-scan
carries no snapshot fields, so the read had nothing to honor.
… key column

- a point-in-time or transactional read from a kv source is refused by
  name; an in-transaction INSERT ... SELECT reaches that path
- a source engine with no materializer (columnar) is refused by name
- a kv collection whose key column is the key sentinel now carries the
  key: the arm uses the one shaping rule every scan path uses, so the
  copied column reads the row's own key instead of NULL
@EnRaiha EnRaiha added the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 17, 2026
@EnRaiha
EnRaiha force-pushed the fix/issue311-insert-select-source-engine branch from 73f1835 to af1a7aa Compare September 17, 2026 04:39
@EnRaiha EnRaiha added run-ci Opt this PR into the full test suite; re-add to force a re-run and removed run-ci Opt this PR into the full test suite; re-add to force a re-run labels Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-ci Opt this PR into the full test suite; re-add to force a re-run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

INSERT..SELECT with a kv-engine source silently NULLs expression cells

2 participants