Repository navigation
feat(typed-sql): add the PostgreSQL dialect with golden tests - #212
Conversation
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. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Review by claude-opus-5-5@claude-code@wf_b045afb3-c58 (head [review r1 #212] independent adversarial review (Opus), head 10616d9 Review r1 of #212 at 10616d9Independent review. I read the diff, What I verified
FindingsNo blocker and no major. Eight minors. Recommended in this pull request (no server needed)
Here or in SQL-08, the lead's choice
Not for this pull requestSQL-04
SQL-08 (server pins)
Lead's disposition of the remaining findings. The dialect is not selectable by any caller yet, so all eight minors are DEFERRED, each as an acceptance line: findings 1 to 6 and 8 to the task that wires the dialect into the reader (SQL-04), the next change to these files; finding 7 to the server pins (SQL-08), with the PostgreSQL 18 case. The lists under "Not for this pull request" are written into the texts of SQL-04 and SQL-08. Each is reviewed independently. VERDICT: blockers=0 majors=0 minors=8 land=yes |
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.What changes for existing callers
Nothing is wired.
DbOptionsdoes not select the dialect, so no caller's statement changes;StructuredQueryDialect: "postgres"is still refused as an unsupported dialect (dispatch is SQL-04). One message changes: a field that is not a column of a known source now readsfield "x" is not a column of source "a"(it used to add a clause about how the server would read the name; that reason is now a comment). The error type is unchanged.The dialect (
dialect_postgres.go)"doubled, NUL / empty / invalid UTF-8 refused. Two modes: exact, and fold to lower case withstrings.ToLower(dalgo2postgres's DDL rule). The 63-byte limit is read on the folded name (U+023A is 2 bytes and folds to 3; U+0130 is 2 and folds to 1), so a name PostgreSQL would truncate is refused, never written.FoldisquoteIdent's rule. One method (fold) is both whatquoteIdentwrites and theFoldthe facts carry.TestPostgresDialectFoldIsTheFoldQuoteIdentAppliespins it over mixed-case and non-ASCII names in both modes (mutating either side fails it).?::bigint; unsigned ints and floats?::numericbound from decimal text (floats as the shortest text for their own width:float32(0.1)is0.1; NaN and the infinities asNaN,Infinity,-Infinity); bool?::boolean;time.Time?::timestamptz; bytes?::bytea; strings untyped, so dates, UUIDs and enums work; nil anddriver.Valueruntyped.$n;LIMIT $n/OFFSET $nbound as int64;ASC NULLS FIRST/DESC NULLS LAST, with the NULLS clause dropped for NOT NULL columns; division((l)::double precision / NULLIF((r)::double precision, 0));((SUM(x))::double precision)and the same for AVG;FALSE/TRUEfor an empty IN. Capabilities: GroupBy, Having, OrderBy, Count, Sum, Avg with DISTINCT forms, Min, Max; First, Last, GroupKeyOrder, StableRowOrder, aggregate ORDER BY off.pg_class,pg_attribute,pg_type,pg_collation, with each source's written text bound as a parameter toto_regclass, so the server resolves it as the statement will, search_path included. It lists every column of each table, view, materialized view, foreign table and partitioned table (not system or dropped columns), in table order, with the type name, a type category, NOT NULL, and a non-deterministic flag (non-deterministic collation or citext). Facts are keyed by the query's own spelling of each source, folded, schema included (an unqualified source has an empty schema key); this is written in the dialect's doc and in thecatalogFactsdoc. Needs PostgreSQL 12 (collisdeterministic).The review minors and follow-ups (all acceptance lines of the task)
Albumonly and forArtistonly, with a pair of different types: with one side unknown the key is accepted.leftKnown && rightKnownto||now fails the suite.AllColumnsExcept("Total")on the colliding facts pins it. Moving the check below the exclusions fails the suite.foldingTypedDialectsintyped_dialect_fake_test.go, and each folding case in the error tables is rerun against it.Golden files (
testdata/postgres)Each holds the exact statement and arguments (with Go types), or the refusal, for one group in one identifier mode:
shapes,constants(every constant type),probes(11 injection probes in every value position; also a test that the statement is byte-identical to the one for a harmless value, so probes appear only in the arguments),datatug-dtql(all six shipped DTQL texts, with and without catalog facts),joins(thetestdata/joinsfixtures plus the join shapes they do not cover, and the two error fixtures checked against their.error.json), andcatalog-query. Rewrite withgo test -run TestPostgresGolden -update-postgres-golden; review the diff like code.dtql_datatug_inventory_test.goonly hoists its case list and query builder to package level so both dialects compile the same inventory.Tests and checks
typed_sql_property_test.gorun against the real dialect in both modes; go-sqlmock for the catalog query (rows to facts, key spelling, dedupe, no source, refused names, query / scan / rows errors, a relation nobody asked for) and an end-to-end path fromtypedQuerySourcesthrough the catalog tocompileTypedSQL.go vet ./...,golangci-lint run ./...,gofmt -l .clean. No database was started; the SQL below was checked by reading, not by a server, and SQL-08 pins it against one.Reviewer notes and open points for later tasks
SELECT $1 AS x), is a server error (42P18). Typing it needs to know where the constant stands, whichbinddoes not; an alias-and-cast in the select list is a SQL-04/08 decision.Countrycomes back ascountry, and an unaliasedCOUNT(*)ascount(*)(DALgo's generic engine names itCOUNT(*)). That is the mode as specified; the reader in SQL-04 may need to map names back.NOT NULL ... NOT VALIDconstraints show inattnotnull. If the flag is set while existing rows may still hold NULL, the NULLS clause would be dropped for such a column. SQL-08's server pins are the place to find out.🤖 Generated with Claude Code