Skip to content

feat(typed-sql): add the typed SQL compiler core and dialect interface - #206

Merged
trakhimenok merged 7 commits into
mainfrom
pg-sql-02-typed-compiler
Oct 4, 2026
Merged

trakhimenok merged 7 commits into
mainfrom
pg-sql-02-typed-compiler

Conversation

@trakhimenok

@trakhimenok trakhimenok commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

What

Plan task SQL-02 (PostgreSQL launch round): the core of a dialect-parameterised compiler for statically typed SQL engines. It is a pure function with no wiring into the readers (that is SQL-04), proven by golden tests against a fake dialect. No database is needed.

New files only: the ten typed_*.go files (typed_sql.go, typed_dialect.go, typed_placeholders.go and their tests). sqlite_emit.go, reader_*.go and every existing file are untouched.

  • compileTypedSQL(q, dialect, facts) (sql, args, err) renders one SELECT: select list, FROM with INNER and LEFT joins (nested right subtrees parenthesised, same scope rules as compileSQLRelationWithin), WHERE and HAVING (comparisons, AND/OR, IS NULL / IS NOT NULL, IN / NOT IN), GROUP BY, ORDER BY, aggregates, arithmetic, LIMIT and OFFSET.
  • Unexported typedDialect interface: quoteIdent, bind, limitOffset, orderItem, divide, aggregateResult, emptyIn, placeholderStyle, capabilities, catalogFacts, and a reserved window hook the compiler never calls.
  • typedCatalogFacts: per source the ordered columns with type category, NOT NULL and non-deterministic-collation flags. The zero value is valid and gives correct, less optimised SQL, but it cannot check that a field is a column (see "A name is a column or it is refused"); SQL-04 must pass facts for every source it compiles.

Safety properties (each has a test)

  • Every value is a bound argument; no byte of a value reaches the SQL text. The text depends on the shape of the query and on exactly these facts about its values, each of which chooses between fixed fragments and writes no content: a nil constant on the right of == renders IS NULL instead of a marker; the length of an IN / NOT IN list sets how many markers it writes, and an empty list renders the dialect's emptyIn; a zero limit or offset leaves its clause out; whether a GROUP BY or ORDER BY expression that carries a constant equals a selected one (same shape, constants of the same kind and content) picks the select-list position or the inline bound expression; the Go type of a constant (never its content) picks the cast the dialect writes beside its marker. A property test compiles 300 random draws (hostile, empty, digit-only and date-looking strings; zero, negative and 64-bit integers; whole, zero, negative and fractional floats; times, bytes, IN lists, limit and offset) and asserts the text is byte-identical every time and the arguments match in order; a second test proves the equal-or-different-constants choice yields exactly one of two fixed texts. The proof takes a dialect, so SQL-03 runs it on the PostgreSQL dialect; seven dialects that type a marker by content (bigint for a whole float, numeric for a fraction, and so on) are each shown to fail it.

  • Every identifier goes through quoteIdent, then the compiler verifies the result is one well-formed quoted identifier (a faulty dialect cannot let a name escape its quotes). Identifier validation (non-empty, no NUL, valid UTF-8, length limit) never echoes the identifier.

  • Placeholders are numbered once, over the final text, skipping quoted identifiers, and the count must equal the argument count. A statement may bind at most 65535 values in total (PostgreSQL's wire limit), counted over the whole statement, not per IN list.

  • The compiler enforces the dialect contract on every fragment a dialect writes. SQL-03 must obey it: a fragment holds no string literal and no comment (the numbering pass sees quoted identifiers only); an operand that carries a marker is written verbatim, exactly once, in the order it was passed (so divide may not swap or repeat a marker-carrying operand; the compiler also probes divide with two different marker-carrying operands, so divide must be a function of its operand texts alone: that catches a fragment such as (1.0 / right * left), which for two operands written the same is byte-identical to the same form with the operands in the order they were passed, so the text check cannot object, and would compute the inverse quotient); bind and limitOffset write exactly one marker per argument and every other method writes none; an error a dialect returns must not quote a value.

  • No error message the compiler builds quotes a constant. dal.ValidateAggregation words its message with the query's own expressions, so its text is replaced by a fixed message. That validation rejects for more reasons than grouping (SUM(*), COUNT(DISTINCT *), DISTINCT on MIN or MAX, a wildcard in an aggregate select, an unsupported aggregate or condition), so the message does not claim which rule failed: it says the query breaks DALgo's aggregation rules and points at dal.ValidateAggregation(q), which names it.

  • Unsupported nodes return errors matching errors.Is(err, dal.ErrNotSupported) and no SQL: cursors, subqueries, FIRST / LAST, RIGHT / FULL / CROSS joins, non-equality ON, nested ON naming an outer alias, parented or group sources, unknown comparison and group operators, unsupported value types, Param, dal.DocumentID() (it has no column; mapping it to the primary key is not implemented; dal.ID(name, value) names a real column and compiles), aggregates the dialect cannot run natively, the DTQL money option (exact decimal results are computed by DALgo's federated engine; the statement would compute double precision), an ORDER BY name the server would read as a different select-list output, compared as written, and a bare name the server would read as the whole row (both below). Malformed queries (invalid aggregation, unknown source, bad identifier, negative limit) deliberately do not match ErrNotSupported.

  • Source options the statement cannot honour are refused, never dropped. In a statement with joins, a scan-bounded source (CollectionRef.WithScan) or a source in a named database is refused, because rendering it as the whole local table would join rows DALgo's generic join never sees. A lone scan-bounded source compiles only when the statement restates its scan: ORDER BY equal to the scan's orders, a LIMIT within the scan's limit, and no WHERE, GROUP BY, aggregate or OFFSET (the leaf query DALgo's generic join and federated executor send); anything else is refused and runs in the generic engine. An ORDER BY alias that resolves to a column whose name another select alias reuses is refused even when it equals the scan order, because the server would sort by the other output (the next section); that holds when the dialect folds case, where the two names differ only in case. The scan comparison itself stays on the query's spelling, so under a folding dialect the scan order Total and an ORDER BY TOTAL are different and the statement is refused. A lone source in a named database compiles by its table name because the federated executor already routed it; SQL-04 must give the compiler the connection's own database name to check that.

  • A bare ORDER BY name is never captured by a select-list output. PostgreSQL reads a name that stands alone in ORDER BY as an output column first, so an alias rewritten to a bare column is refused when any select-list output of that name is another expression: SELECT Total AS a, Name AS Total ORDER BY a would sort by Name, and SELECT a AS x, COUNT(b) AS a GROUP BY a ORDER BY x would sort by the count where DALgo sorts by the group key. An unaliased expression counts too, because the compiler names it by its text. A qualified name and a name nested in an expression are input columns and compile as before. The check compares the quoted names the dialect wrote, not the query's spelling, because the server compares what is written and a dialect's quoteIdent may fold case (the PostgreSQL dialect's FoldLower mode writes Total and total as "total"): with a lower-casing quoteIdent, SELECT Total AS a, Name AS total ORDER BY a is written ORDER BY "total" and would sort by Name, so it is refused, while SELECT Total AS a, TOTAL ORDER BY a, whose second output is the column itself, compiles. Alias lookup, source qualifiers and the scan comparison stay on the query's spelling, which is what DALgo reads; under a folding dialect they refuse or leave a name as the column and never pick another expression, and the written-name check catches the names that then meet. Wildcard exclusions match catalog names exactly, as dal.WildcardProjection.Excludes defines. GROUP BY and HAVING need no refusal under any case rule: PostgreSQL reads a GROUP BY name as an input column before an output, and HAVING cannot see outputs; both are pinned by tests, for an exact and for a folding dialect, and typedDialect records the assumption for the next dialect. Writing the column qualified would keep these queries native; refusing is the narrower change.

  • A name is a column or it is refused. PostgreSQL does not fail on a name that matches no column, it reads it as something else: a bare name equal to a source's identity (its alias, else its table name) as the whole row, one composite value with every column in it (SELECT x FROM Invoice AS x), and a qualified name x.f that is no column of x as the function f applied to the row (x.to_json is to_json(x); row_to_json, to_jsonb, concat, count and quote_literal read alike). The caller chooses both the alias and the field, and a field policy that checks field names only would pass both and leak every column inside the value. The compiler closes both with the catalog facts: when the facts know a field's source and list no column of that name, the statement fails with a plain error naming the column (not ErrNotSupported: the table has no such column), for a bare and a qualified field, in every clause and in a join's ON; when the facts do not know the source, an unqualified field written as the base source's identity is refused with ErrNotSupported, compared on the quoted names written, so a folding quoteIdent is covered. An aliased source hides its table's own name, so only the alias counts. A qualified name without facts cannot be checked: the typedDialect contract says SQL-04 must pass facts for every source it compiles, each listing every column a query may name (a system column such as ctid is refused unless the dialect lists it).

  • A facts lookup matches a column only when the catalog's own name is the name the dialect writes. The lookup folded both the query's name and the catalog's names and took the first match, so with a folding dialect and a catalog that holds Total (NOT NULL) and total (nullable), ORDER BY total read Total's facts, dropped the NULLS clause and sorted the nullable column with NULL on the wrong side. It now compares the catalog's own name with the folded query name, and a select-all is refused when a catalog column's name is not its own folded form, because the statement could not name that column. Fold must be the case rule quoteIdent applies; the contract says so.

Decisions a reviewer should check

  • A grouped expression that carries constants is referenced by select-list ordinal in GROUP BY and ORDER BY, because PostgreSQL compares expressions structurally and two bound parameters never match. HAVING has no ordinal form, so HAVING through an alias of such an expression is refused; so is ORDER BY through such an alias nested in a larger expression.
  • Constant-only GROUP BY / ORDER BY expressions, a constant on the left of a comparison, and IS NULL of a constant are refused (a bound parameter standing alone has no inferable type in PostgreSQL).
  • Unaliased select expressions are named by their text, matching DALgo's generic engine; an unaliased expression carrying a constant is refused because that name would contain the value.
  • LIMIT and OFFSET are bound arguments (dialect.limitOffset).
  • dal.PlanAggregation(q, dialect.capabilities()) gates aggregation, so a dialect that does not declare a capability can never be handed that query.
  • With catalog facts the compiler also expands AllColumnsExcept, drops the NULLS clause for NOT NULL columns not on the nullable side of an outer join, refuses a join whose key types differ (typedJoinKeysComparable), and refuses a field that is not one of its source's columns.
  • Join algorithm hints (JoinedSource.Algorithms) are not read: they are preferences and the server picks its own plan.
  • Two constants of different kinds that DALgo treats as equal by text (1 and 1.0) compile as different expressions, which can only produce a server error, never wrong rows.

Tests

Golden SQL plus arguments per clause, error table, property tests, direct unit tests of the helpers, a test that pins every place a bare name is written into ORDER BY, GROUP BY or HAVING, tests for the whole-row and function-of-the-row readings (refusals with and without facts, in SELECT, WHERE, GROUP BY, HAVING, ORDER BY and a join's ON, and the goldens that must still compile: a column named like its table or alias when the facts list it, a qualified column with facts), and the same tests against a fake dialect whose quoteIdent lower-cases (refusals and the statements that still compile). SQL-03 should run them against its real FoldLower dialect; SQL-08 should pin the two readings against a real server, since they are stated here from PostgreSQL's documentation and parser, not from a run. 100% statement coverage of the module (CI gate), go vet, golangci-lint and staticcheck clean.

Changed in review round 5

  • A field that is not a column can no longer be read by the server as the whole row or as a function of the row: with facts for its source it must be a listed column (plain error otherwise); without facts a bare name written as the base source's identity is refused (ErrNotSupported). Tests were red first: SELECT x FROM Invoice AS x, SELECT Invoice FROM Invoice, the same names in WHERE, ORDER BY, GROUP BY, an aggregate and HAVING, SELECT X FROM Invoice AS x under the folding fake, x.to_json and its relatives with facts that do not list them, and a join key the facts do not list.
  • Two existing tests relied on the gap and changed: TestCompileTypedSQLCheckedJoinKeyTypes accepted "a column missing from the facts is left to the server" and now rejects it, and TestCompileTypedSQLOrderBy's facts for Album did not list ArtistId, which its own join key reads, so they now do.
  • The facts lookup matches the catalog's own name against the folded query name, and a select-all over a catalog column that is not in its folded form is refused (the review's minor). Red first: the review's exact trace, NULLS clause dropped and "total" written twice.
  • The typedDialect name-resolution contract lists the facts lookup as the third kind of lookup that does not compare written names, states the whole-row and function-of-the-row readings and that SQL-04 must pass facts for every source it compiles, and says Fold must be the case rule quoteIdent applies.

🤖 Generated with Claude Code

Pure compileTypedSQL(q, dialect, facts) for statically typed SQL engines,
with an unexported typedDialect interface, catalog facts and a final
placeholder numbering pass that skips quoted identifiers. Every value is a
bound argument and every identifier is validated and quoted, so the SQL text
never depends on a value. Unsupported nodes return errors matching
dal.ErrNotSupported. Not wired into the readers yet (SQL-04).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@trakhimenok

Copy link
Copy Markdown
Contributor Author

[review r1 #206] independent adversarial review (Opus), head 30cccbf

Round 1, independent review of head 30cccbf561ad1fae8a83b3fe3899439bdfe72329. The worktree HEAD matches the pull request head and is clean; the branch is level with main (3604df1). I read all nine files, the SQLite compiler, and the dal model in the dalgo module version go.mod pins (v0.89.1). Two real defects, both refusals that are missing; the security properties held against everything I tried.

What I verified

  • Scope. Nine new files and nothing else (git diff --stat 3604df1..HEAD). sqlite_emit.go, reader_base.go, reader_records.go, reader_recordset.go untouched. No exported identifier added or changed. The repository has no AGENTS.md or CONTRIBUTING; .junie/guidelines.md asks for vet, lint, build, tests and 100% coverage, all met in CI.
  • Checks. Lint and Build & test are the two required checks and both passed on this head. The CI log reports total: (statements) 100.0%, so the 100% claim is true for the three new files. No test opens a database. I ran wb run -- go test -run Typed -count=1 ./...: ok. I did not re-run the author's mutation checks (read-only review).
  • No value in the text. Tried to break it; could not. I traced every write into the SQL string. The only String() result that reaches the text is the label of an unaliased expression, emitted only when that expression bound zero arguments and only through quote. strconv.Itoa writes a select-list position. Constants, IN elements, LIMIT and OFFSET, values in arithmetic, HAVING, ORDER BY and aggregate arguments are all bound. Unsupported value types (struct, map, pointer, nested slice) are refused. Argument order equals text order across clauses, and a value used twice is bound twice (golden {1, 1, 1}), for both $n and ? styles.
  • No unvalidated name in the text. Tried to break it; could not. Collection, schema, alias, field, column label and catalog column all go through quote, which re-checks the dialect's output as one well-formed quoted identifier with the quote byte doubled. Aggregate names, join keywords, sort direction, AND/OR and comparison and arithmetic operators are compiler constants chosen by allow-list. I tried the quote character, ? and $1 inside names, a dialect whose quote byte differs from the declared one, and Unicode case folding of function names (ſum upper-cases to SUM, but the emitted token is the constant).
  • Refusal. Every default branch returns an error; none renders with String(), %v or the legacy emitter. Cursors, subqueries, Param, Array outside IN, RIGHT/FULL/CROSS joins, non-equality ON, parented and group sources, FIRST/LAST and unknown operators are refused. Two things are dropped instead of refused: finding 2 and finding 4.
  • Text identical when only values change. True for content, and the property test (300 seeded draws, two placeholder styles) is not vacuous. It is not literally true for structure: finding 3.
  • Semantics table. == null, empty IN, three-valued IN/NOT IN, parenthesised AND/OR, aggregates with and without GROUP BY, HAVING alias rewrite, DISTINCT, NULLS FIRST/LAST with the NOT NULL shortcut (including the nullable side of nested LEFT joins), LIMIT without ORDER BY, and nested join parentheses match the plan. Unaliased column naming matches the generic engine (dal/aggregation_execute.go:908-916). One hole: finding 1.
  • Dialect interface. Unexported and sufficient for PostgreSQL in SQL-03 as far as I can tell without a server. Two notes for SQL-03, not findings: bind has no position context, so confirm against PostgreSQL 17 that an untyped text or NULL parameter is accepted wherever the compiler lets a constant stand without a typed neighbour (select list, aggregate argument); and if the DTQL decoder yields float64 for whole numbers, typing by Go type gives ::numeric against integer columns.
  • Declared deviations. I checked each against the code. IN in HAVING really is rejected by core (dal/aggregation_plan.go:215-240). The PlanAggregation gate, catalog-fact extras, typedQuerySources, the constant-only refusals, bound LIMIT/OFFSET and the error classification are as described and acceptable. Not declared: findings 1, 2 and 3.

Findings

Major

  1. ORDER BY over an alias of a grouped constant-carrying expression, nested in a larger expression, compiles to SQL the server rejects (typed_sql.go:328-345, 355-369, 905-913). HAVING refuses this case (316-320); ORDER BY ignores the rewriter's groupedConstant. Core accepts the query.

    • Input: bucket := Binary(Field("Total"), Add, Const(1)); From("Invoice").GroupBy(bucket).OrderBy(Ascending(Binary(Field("bucket"), Multiply, Const(2)))).SelectColumns(Column(bucket, "bucket"), Column(COUNT(*), "n")).
    • Output, by reading the code: SELECT ("Total" + $1::bigint) AS "bucket", COUNT(*) AS "n" FROM "Invoice" GROUP BY 1 ORDER BY (("Total" + $2::bigint) * $3::bigint) ASC NULLS FIRST, args [1, 1, 2]. By the pull request's own premise PostgreSQL answers that "Total" must appear in GROUP BY. Not run against a server.
    • Fix: a fresh rewriter per ORDER BY item; when the query has GROUP BY, the rewriter recorded groupedConstant and no select item matches the rewritten expression, return typedUnsupported as having does. Leave queries without GROUP BY alone. Add the case to the ErrNotSupported table.
  2. A scan-bounded source is rendered as the whole table (typed_sql.go:522-568). CollectionRef.WithScan bounds a relation before it joins (dal/q_collection_ref.go:81-98) and DTQL exposes it as from.scan (dtql/deserialize.go:171-183). The compiler never reads ScanLimit or ScanOrders, so a native join returns rows the generic join would not (dal/join_execute.go:485-493).

    • Input: From(NewRootCollectionRef("Invoice","i").WithScan(10, Descending(Field("Total")))).Join(Customer AS c ON i.CustomerId == c.CustomerId).SelectColumns(c.Name).
    • Output, by reading: SELECT "c"."Name" FROM "Invoice" AS "i" INNER JOIN "Customer" AS "c" ON ("i"."CustomerId" = "c"."CustomerId"), no bound.
    • Fix: when the FROM tree has joins, refuse a source with a scan using typedUnsupported("...join_plan: a scan-bounded source needs the generic engine"). Keep accepting it for a single source, with a comment: core's generic join sends each bounded relation as a leaf query that restates the scan as ORDER BY and LIMIT while the base still carries it. One refusal test, one golden. The SQLite compiler has the same gap (sqlite_emit.go:255-282), already on main and out of scope here; it needs its own issue. Join algorithm hints are ignored too, which core allows; add one comment line.

Minor

  1. "Never depends on values" is not literally true (typed_sql.go:31-33, 373-381; typed_sql_property_test.go:61-79). Ordinal versus inline depends on whether two constants are equal: ORDER BY 2 becomes ORDER BY ("AlbumId" + $2::bigint) when only the ORDER BY constant changes. The property test reuses one value for both positions, so it cannot see this. No value content reaches the text. Also typedOrdinal matches by String(), which is empty for NaN and infinities and identical for Valuer structs without exported fields. Fix: list the structural exceptions in the comment, add a generated test with independent constants asserting one of exactly two texts, and compare constants with reflect.DeepEqual.
  2. dal.DocumentID() is rendered as a column named __name__ (typed_sql.go:610-626), against dal/q_document_id.go:9-14. Declared as left for SQL-04. Fix: refuse it until SQL-04 maps it; keep dal.ID(name, value) working.
  3. typedMaxArguments is checked per IN list, not per statement (typed_sql.go:15-17, 789-791). Fix: check len(args) once in compileTypedSQL.
  4. The dialect contract omits two rules the numbering pass relies on (typed_dialect.go:19-28). Operands must be rendered exactly once and in argument order, because a reordering dialect passes the count check and binds values to the wrong positions; and fragments must contain no string literal or comment, because the scanner only understands quoted identifiers. Fix: state both.
  5. Three goldens are missing (typed_sql_test.go): join plus WHERE plus GROUP BY plus HAVING plus ORDER BY by alias plus LIMIT in one statement (the journey's step 4); a constant inside an aggregate argument; a source alias or schema containing the quote character used as a qualifier.
  6. The value-independence proof is hard-wired to the fake dialect (typed_dialect.go:41-45, typed_sql_property_test.go:90-133). bind sees the value, so the rule is a convention. Fix: make the property test a helper taking a typedDialect so SQL-03 runs it against the PostgreSQL dialect.

Neither major changes the dialect interface, so SQL-03 can start against this branch while they are fixed. Both fixes are a few lines plus a test each.

VERDICT: blockers=0 majors=2 minors=6 land=no

trakhimenok and others added 5 commits October 4, 2026 21:33
…ect contract

Review round 1 of the typed SQL compiler.

- A scan-bounded or named-database source inside a join is refused with
  ErrNotSupported instead of being rendered as the whole local table; a lone
  source keeps working because the generic join restates its scan as ORDER BY
  and LIMIT.
- ORDER BY over a grouped, constant-carrying alias nested in a larger
  expression is refused as HAVING already refuses it (one alias rewriter per
  ORDER BY item; only for statements with a GROUP BY).
- dal.DocumentID() is refused until it is mapped to the primary key; dal.ID
  keeps naming a real column.
- The argument limit is checked per statement, not per IN list.
- Ordinal references compare expressions structurally with constants of the
  same kind and content, not by String().
- The dialect contract states the operand count/order rule and the
  no-literal/no-comment rule, and the compiler enforces both on every
  fragment; the numbering pass refuses string literals and comments.
- compileTypedSQL documents exactly what the text depends on.
- The value-independence proof is a helper that takes a typedDialect; a
  generated test covers two independently drawn constants.
- Goldens for the journey statement, an aggregate argument with a constant and
  quote characters in qualifiers.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…t restates its scan

A lone source with WithScan compiled as the whole table for every statement
shape. It now compiles only for the leaf query DALgo builds (ORDER BY equal to
the scan orders after alias resolution, LIMIT within the scan limit, no WHERE,
aggregation or OFFSET) and otherwise returns ErrNotSupported.

Also: probe divide with two different marker-carrying operands so operands with
identical text cannot hide a swap; drop DALgo's grouping-validation text, which
quotes the query's constants, from the compile error; broaden the property
draws (whole, zero and negative floats, integers beyond 32 bits, digit-only,
date-looking and empty strings) and prove seven content-typing dialects fail
the proof; state each limit in the dialect contract.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…pture, and the money option

An ORDER BY alias rewritten to a bare column name is read by PostgreSQL as an
output column first, so when another select alias reuses that name the server
sorts by the wrong column (and a scan restated through such an alias passes the
scan check). typedCheckOrderByName refuses an item whose written bare name any
other select-list output shares, unaliased expressions included. GROUP BY and
HAVING are pinned safe by tests: GROUP BY prefers an input column and HAVING
cannot see outputs.

Also: refuse a query carrying the DTQL money option instead of dropping it;
reword the fixed aggregation message so it does not claim which rule failed and
points at dal.ValidateAggregation; correct the divide probe test and comment to
the (1.0 / right * left) case; record the name-resolution assumption on
typedDialect.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ure guard

The guard compared the query's spelling, so a quoteIdent that folds case (the
PostgreSQL FoldLower mode) wrote Total and total as one identifier and let a
statement through that sorts by the wrong output. It now compares the quoted
names the dialect wrote: typedSelectItem keeps the quoted output name and the
quoted field name. Tests with a lower-casing fake dialect were red first.

Also document what stays on the query's spelling and fail closed (alias lookup,
source qualifiers, the scan comparison), and fix the divide comment.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…e row or a function of it

A name that matches no column is not an error to PostgreSQL: a bare name equal
to a source's identity is the whole row, and a qualified x.f is f(x). With the
catalog facts for a source, a field must now be one of its columns (plain error
otherwise); without them, a bare name written as the base source's identity is
refused with ErrNotSupported, compared on the quoted names written. The dialect
contract states both readings and that SQL-04 must pass facts for every source.

A facts lookup now matches a column only when the catalog's own name is the
folded query name, and a select-all is refused over a catalog column that is
not in its folded form.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@trakhimenok

Copy link
Copy Markdown
Contributor Author

Review by claude-opus-5-5@claude-code@wf_adde141d-f1b (head 30a8f1ac4272)
Reviewed-Head: 30a8f1a

[review r6 #206] independent adversarial review (Opus), head 30a8f1ac427282a241745916074d3dcb2d4c051f

Round 6. Land it: both round 5 findings are closed, I could not break the fix, and the required checks are green on this head. Three minors, none blocking. The remaining open reading (a qualified name on a source the facts do not know) belongs to SQL-04 and is listed under "For later tasks".

What I verified

  • Head and checks. gh pr view and the author's worktree both give 30a8f1ac; the worktree is clean. wb ci wait refused once: the head does not contain main at d20098fe (main is 6 commits ahead; none touches a typed_* file or projection.go, and executeQueryFunc is unchanged on main). Read instead: run 37239665879 is for this head and succeeded; strongo_workflow / Lint and strongo_workflow / Build & test, the two required contexts, pass with the 100% coverage gate on. Locally wb run -- go test -count=1 -cover . gives 100.0%, go vet and gofmt -l are clean, and the six new or changed tests pass (57 subtests).
  • Scope of the fix. Five typed_* files changed since 9357733. The test files lose exactly four lines: the two "missing from the facts is left to the server" cases and the two Album/Artist facts lines, as the author says. The pull request is still ten new files. The description matches the code.
  • Every writer of a name. The only places a caller's field name reaches the text are field() (both paths now call checkNamesAColumn) and expandWildcard (catalog names). Join ON goes through field() with the join's visible sources and never reads c.base, which is not set yet at that point.
  • Tried and could not break. Alias or table name as a bare field in SELECT, WHERE, GROUP BY, HAVING, ORDER BY and an aggregate, with and without facts; a schema-qualified source without an alias; an alias containing a double quote; an ORDER BY alias equal to the source identity (rewritten to its expression before the check); a column named like its alias (the server reads the column first, so the golden is right); an aliased source with a bare name equal to its table name (the alias hides it); two aliases that swap the table names in a join; folding with a catalog column that exists only in mixed case. The round 4 ORDER BY capture guard still fires after the new check when the facts list the column.
  • Does the plain error break a valid DALgo query? No. dal.ValidateAggregation does not accept a select alias as a group key, and HAVING and ORDER BY aliases are rewritten before field(), so with facts every field that reaches the check is meant as an input column.
  • Not done. I ran no mutants and wrote no test (read-only), so the author's eleven mutants are unverified. PostgreSQL's rules are from its parser and documentation, from knowledge; no server was run.

Earlier findings

  1. A name that is not a column read as the whole row or a function of the row (major): CLOSED. Parts (a), (b) and (c) are as ordered, and every requested test is present.
  2. Facts lookups fold separately and take the first match (minor): CLOSED. The round 5 trace is pinned.

Findings

Minor

  1. A deleted test leaves one branch unpinned (typed_sql_errors_test.go:703-709, typed_sql.go:847-851). The two deleted accepted cases were the only tests where one side of a join key was unknown to the facts. No join test now has facts for only one source, so leftKnown && rightKnown can become ||, or lose an operand, with the suite green (by reading). Fix: two accepted subtests with facts for Album only and for Artist only.
  2. Contract sentence and code differ on the select-all refusal (typed_sql.go:422-431, typed_dialect.go:84-86). The contract says a select-all "that would have to write" such a column is refused; the code refuses even when the column is excluded. Both are safe; no test pins which. Fix: reword the sentence and add AllColumnsExcept("Total") on the colliding facts, or move the check below Excludes and pin that.
  3. Error wording (typed_sql.go:1017). For a misspelt column the message says the server "would read the name as the row of the source or as a function of it"; the server would say the column does not exist. Fix: field %q is not a column of source %s, reason in the comment.

For later tasks (not findings)

  • SQL-04: refuse a source the facts do not know. Without facts a qualified name is still unchecked (x.to_json compiles; pinned by the golden "a qualified name written like its alias, without facts"). This is the author's residual. It is stated in the contract and was the fix round 5 ordered, so it is not a defect here. SQL-04 must check every name from typedQuerySources against facts.source before compiling and fail with table-not-found, with a test using x.to_json. Facts must be keyed by the query's own spelling of the source, folded, schema included.
  • SQL-03: Fold must be the function quoteIdent applies. The closure with facts rests on it. Trace: Fold nil with a folding quoteIdent, catalog column X, alias x, field X passes the facts check and is written "x", which is the row. Pin it with a test over mixed-case and non-ASCII names, or match the catalog name against the unquoted text the dialect wrote and drop the assumption.
  • SQL-03: apply the 63-byte limit to the folded name. Both written-name comparisons (ORDER BY guard, identity check) assume the server does not truncate.
  • SQL-03: catalogFacts must list every column of each source, views included, for the table the server will resolve through search_path.
  • SQL-04: wildcard exclusions under FoldLower are fail-open (AllColumnsExcept("Secret") keeps catalog column secret). Fold exclusions with the dialect's fold on both paths, before OVDB uses FoldLower with exclusions.
  • SQL-04: duplicate output names under folding, and fold names before any field-mask check if ACL is ever allowed on a FoldLower mount.
  • SQL-08: pin against a real server: SELECT "x" FROM t AS "x" returns the row; "x"."to_json" calls the function; a column named like the alias wins over the row; an aliased source hides its table name; two aliases that fold together fail with 42712.

Answers to the author's notes

  • Residual (qualified name without facts): SQL-04, as above. Do not add a compile mode here.
  • Refusing before Excludes: keep it, and fix the sentence (minor 2).
  • System columns and misspelt columns now a plain error with facts: accepted; the contract says so.

Lead's disposition of the remaining findings. Minors 1 to 3 are DEFERRED to the PostgreSQL dialect task (SQL-03), the next change to these files, reviewed independently. The items under "For later tasks" are written into the task texts of SQL-03, SQL-04 and SQL-08 as acceptance lines.

VERDICT: blockers=0 majors=0 minors=3 land=yes

@trakhimenok
trakhimenok enabled auto-merge (squash) October 4, 2026 23:04
@trakhimenok
trakhimenok merged commit c3f67fc into main Oct 4, 2026
5 checks passed
@trakhimenok
trakhimenok deleted the pg-sql-02-typed-compiler branch October 4, 2026 23:05
trakhimenok added a commit that referenced this pull request Oct 4, 2026
SQL-03 (task `pg-sql-03-postgres-dialect`): the PostgreSQL dialect for the typed SQL compiler core (#206, v0.23.0), with golden tests, plus the three minors and the follow-ups the core's last review left for this task.

Source commits:

- 10616d9 feat(typed-sql): add the PostgreSQL dialect with golden tests
  Add the PostgreSQL typedDialect (identifier modes exact and fold to lower
  case, 63-byte limit on the folded name, constants typed by Go type, one
  catalog query for column facts) with golden files, go-sqlmock tests for the
  catalog query and the 100% coverage the package gates on. Close the three
  minors and the follow-ups the compiler core review left for this task, run the
  core fold tests against the real dialect, and rewrite the README section on
  the legacy text path.

Pull request: #212
Review: #212 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant