fix(sql): reject unknown and ambiguous columns at plan time - #304
Merged
Conversation
GitHub re-raises a dismissed CodeQL alert whenever an edit shifts its line number, and Rust has no inline suppression comment to pin it. Anchor known false positives to their sink text instead and replay the dismissal via the GitHub API after each CodeQL run on main.
Break the two largest files in the SQL crate into directories, one module per concern. No behavior change. - resolver/expr/convert.rs becomes convert/, split by expression family: entry, identifier, operators, predicates, literals, builtins - planner/subquery.rs becomes subquery/, split by subquery kind: extract, in_list, scalar - Move array table-valued-function resolution to resolver/array_tvf.rs - Move the comma-LATERAL branch to planner/select/comma_lateral.rs - Move CteCatalog to planner/select/cte_catalog.rs - Move LATERAL AST extraction to planner/lateral/subquery.rs
A collection's column set is open or closed depending on its engine. The resolver needs that distinction before it can reject an unknown name. - EngineRules gains accepts_undeclared_columns, implicit_columns, and value_column_name_is_free, answered by all seven engine impls. Only document_schemaless accepts undeclared fields. The kv engine carries an implicit key/value/ttl triple and leaves the value column name free until the DDL declares one. - CollectionInfo carries open_schema, derived from the engine for a stored collection and set independently for a synthesized relation. - TableScope gains check_name, outer-scope chaining for correlated subqueries, output-name widening for ORDER BY and HAVING aliases, and qualified-only relations for a MERGE source and ON CONFLICT excluded. - ColumnScope names the namespace an expression converts against. - resolver/derived.rs infers the output columns a derived, LATERAL, or CTE alias exposes, so those relations close instead of accepting any name. - SqlDataType::Unknown types a computed column with no declared type.
An identifier naming no column of a closed-schema collection resolved to NULL on every engine. Projections returned NULL-filled rows, a WHERE predicate matched nothing, IS NULL matched everything, ORDER BY no-opped, and UPDATE or DELETE touched zero rows while reporting success. Thread ColumnScope through convert_expr and gate both identifier arms, so every clause routing through the converter inherits the check: projection, WHERE, GROUP BY, HAVING, ORDER BY, window clauses, aggregate arguments, joins, and subqueries. Sites that build a column reference without the converter call TableScope::check_name directly: UPDATE and MERGE assignment targets, MERGE ON, INSERT column lists, UPDATE FROM join pairs, CTE join links, join equi-keys, and the IN-subquery outer operand. The document_schemaless engine stays open. It accepts undeclared fields on write, so a read of one resolves to NULL rather than raising. Map the two planner errors to their PostgreSQL SQLSTATEs: UnknownColumn to 42703 and AmbiguousColumn to 42702. Both previously fell through to PlanError and surfaced as 42601.
Two statement shapes the planner refused, both reachable from the column gate's own coverage. EXISTS and NOT EXISTS previously errored 42601 in every form. They now plan as semi and anti joins. The inner scope chains to the outer, so a correlated reference resolves and an unknown column on either side raises. A correlation splits into join keys by relation membership rather than operand position, and SubqueryJoin carries several keys instead of silently keeping the first. Shapes without an equality key are refused by name: correlated inequality, correlation under OR, and set operations. INSERT ... SELECT accepted only SELECT *. It now binds an arbitrary source projection to the target column list, carrying the binding as a column map on the plan, the physical op, and WAL replication, and shaping each copied row before the target surrogate is assigned. An arity mismatch raises.
An equi-correlated LATERAL lowers to a join, and its inner relation's local WHERE was dropped: the residual predicate sat on a Scan that convert_join never lowered, so the subquery returned every row. A LATERAL alias also named its output columns after the inner table's alias, so selecting one by the alias yielded an empty value. HashJoin carries left_scan_filters and right_scan_filters, applied to rows scanned locally before the join, at the same point as the row-level security filters and for the same reason: an excluded row must neither match a partner nor produce a null-extended outer row. The bitmap-prefiltered scan arm dropped the row-level security filters entirely, since its scan plan had no predicate slot. It now applies them alongside the new per-side predicates.
The UPDATE paths logged a warning and skipped any row they could not decode, re-encode, or evaluate, then reported success with a smaller affected count. A row the caller asked to update was silently left untouched. One site was worse: an undecodable row left the match set before the affected count was computed. Each of those now raises a typed error naming the collection and the document. A row genuinely outside the update set, such as a target with no joined source, still skips. An undecodable stored row also files a diagnostics report at the detection site, grouped by collection so a scan over many bad rows produces one report rather than a storm. The strict encoder's unknown-field error is typed and carries the collection, so it reports 42703 through every caller instead of being flattened into a string and re-wrapped as an internal error. The planner gate covers SQL; this path still serves the native client, COPY FROM, CRDT delta merge, and schemaless-to-strict conversion. Deduplicate the scan-filter decode onto one helper beside the ScanFilter type, and share the computed-column encode tail.
farhan-syah
added
the
run-ci
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #292
An identifier naming no column resolved to NULL on every engine, including the closed-schema
document_strictandkv:WHEREequality matches nothing.IS NULLmatches every row.ORDER BYdrops the sort key.UPDATE ... WHERE <unknown> = ...touches zero rows and reports success.Every typo'd identifier returns plausible-but-wrong results with no error.
nodedb-sql/src/resolver/columns.rsalready implemented the check. No planner path called it.convert_exprinnodedb-sql/src/resolver/expr/convert/is the single point every clause funnels an identifier through, and it took no scope. It now takes aColumnScopeand gates both identifier arms, so every clause inherits the check. Sites that build a column reference without the converter callTableScope::check_namedirectly.SQLSTATE mappings: UnknownColumn → 42703 (undefined column), AmbiguousColumn → 42702 (ambiguous column).
Engine scope
Closed-schema engines reject unknown names at plan time. Open-schema engines require runtime NULL:
document_strict,kv,columnar,timeseries,spatial,arraydocument_schemalessdocument_schemalessallows undeclared fields on write, so reads must return NULL. Closing reads would make written data unreadable.Changes
Defects found and fixed while closing the gate
Behavior changes
Verification
62 tests across sql_undefined_column.rs, sql_undefined_column_dml.rs, sql_undefined_column_subquery.rs, and lateral_equi_correlated_keeps_local_inner_predicate in sql_lateral.rs.
Full suite green: 15972 passed in stage 1, 367 in stage 2. Clippy clean.
Run:
Known limits