Skip to content

fix(sql): bind positional INSERT/UPSERT values to declared column names#204

Merged
farhan-syah merged 2 commits into
NodeDB-Lab:mainfrom
emanzx:fix/positional-insert-column-binding
Jul 22, 2026
Merged

fix(sql): bind positional INSERT/UPSERT values to declared column names#204
farhan-syah merged 2 commits into
NodeDB-Lab:mainfrom
emanzx:fix/positional-insert-column-binding

Conversation

@emanzx

@emanzx emanzx commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #202.

The defect

convert_value_rows names each value by position against the caller's column list, falling back to a synthetic name when the list runs out:

// nodedb-sql/src/planner/dml_helpers.rs
let col = columns.get(i).cloned().unwrap_or_else(|| format!("col{i}"));

For a positional INSERT INTO t VALUES (…) that list is empty, so every value is stored under col0, col1, … instead of the declared column names. The row is written correctly and SELECT * returns it (projection-star doesn't care about names), but nothing that addresses a column by name can ever reach it again.

The fix

resolve_insert_columns() (new, in dml_helpers.rs) resolves the effective column list from CollectionInfo.columns when the statement omits one. It is applied at the three VALUES-clause planning sites in dml.rsplan_insert, plan_upsert, and plan_upsert_with_on_conflict — immediately after the collection metadata is already in scope.

Judgment calls

These are the places where I had to choose without knowing your intent. Each is one line to change if you'd rather go the other way:

  1. Arity overflow → hard error. A row with more values than the collection declares now returns a new SqlError::InsertColumnArityMismatch rather than binding the surplus to an invented colN. Inventing one would recreate exactly the unaddressable-column failure this PR closes. This is the only behaviour change for input that previously "succeeded".
  2. Fewer values than columns → bind by position against the leading columns. This matches what a partial named insert already does (INSERT INTO t (id) VALUES (1) on a wider table binds only id), so positional and named forms stay consistent.
  3. Schemaless collections untouched. When info.columns is empty there is no declared order to bind against, so the existing col{i} fallback remains for that case. Not treated as an error.
  4. KV engine untouched. build_kv_insert_plan matches columns by name against pk_col / "key" / "ttl", which is orthogonal to declared column order — binding positionally there looked wrong rather than merely unnecessary, so I left it and noted it in a comment. Happy to extend if you disagree.
  5. The col{i} fallback itself is unchanged. After this fix it should only be reachable for the schemaless case. Removing it entirely is a wider blast radius than this issue warrants.
  6. INSERT … SELECT unaffected — it returns before columns is consumed. Verified, not changed.

Tests

New: nodedb-sql/tests/positional_insert_column_binding.rs, following the local-Catalog-fixture pattern in point_get_operand_order.rs / schema_qualified_rejection.rs.

test asserts
positional_insert_binds_declared_column_names the core regression
positional_upsert_binds_declared_column_names same for UPSERT
positional_insert_arity_overflow_is_rejected overflow errors instead of inventing col2
named_insert_still_binds_by_name no regression on the named path
positional_insert_on_schemaless_collection_keeps_col_i_fallback schemaless behaviour preserved
positional_insert_on_kv_collection_is_unaffected pins the deliberate KV non-change

Verified failing before the fix, not just passing after — with the binding logic neutered (error variant left in place so it still compiles), the three fix-asserting tests fail behaviourally and the three non-regression tests pass unchanged:

left:  [("col0", Int(44)), ("col1", String("upserted"))]
right: [("id",   Int(44)), ("note", String("upserted"))]

test result: FAILED. 3 passed; 3 failed

With the fix: 6 passed. Full cargo test -p nodedb-sql green; cargo clippy -p nodedb-sql --all-targets -- -D warnings clean.

Notes

I'm new to this codebase, so please push back on anything that doesn't match how you'd want it done — particularly the arity-overflow decision, which is the one change in observable behaviour for previously-accepted input.

emanzx and others added 2 commits July 21, 2026 16:48
A `VALUES` clause with no explicit column list left `columns` empty, so
`convert_value_rows` fell back to synthetic `col0`, `col1`, ... names for
every value. The row stored fine and `SELECT *` returned it correctly,
but named projections and WHERE predicates could never address it again:

    CREATE COLLECTION probe (id INT PRIMARY KEY, note TEXT);
    INSERT INTO probe VALUES (42, 'stored?');
    SELECT * FROM probe;                       -- 42|stored?
    SELECT id FROM probe;                      -- empty
    SELECT count(*) FROM probe WHERE id = 42;  -- 0

Resolve the effective column list from the collection's declared column
order when the statement omits one. Named inserts and schemaless
collections (no declared order to bind to) are unchanged. A row carrying
more values than the collection declares is now rejected rather than
bound to an invented `colN`, which would reproduce the same
unaddressable-column failure.

The KV path is deliberately untouched: it matches columns by name
against pk_col/"key"/"ttl", which is orthogonal to declared order.

Fixes NodeDB-Lab#202
KV splits key/value by matching column names against pk_col/"key"/"ttl",
so a positional VALUES row with no column list has no principled key to
bind to and would silently produce an empty-keyed, empty-valued row
(all such rows colliding). Reject with a dedicated error instead.
@farhan-syah
farhan-syah merged commit f0f0517 into NodeDB-Lab:main Jul 22, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants