Enhance SQL query structure and formatting#425
Conversation
Refactor SQL queries for improved clarity and consistency.
Greptile SummaryThis PR modifies Beyond the missing regeneration, several of the SQL changes themselves are semantically incorrect:
Confidence Score: 1/5Not safe to merge — changes are currently inert but will cause compilation failures and semantic regressions when the generated code is updated. A P0 (generated code not regenerated, guaranteed compilation break on sqlc generate) combined with multiple P1s (array detection regression, INCLUDE column corruption, removed fields breaking active inspector features) places this well below the P0 ceiling of 2/5. ir/queries/queries.sql — every changed section has a blocking issue; ir/queries/queries.sql.go and ir/inspector.go must also be updated in tandem. Important Files Changed
Reviews (1): Last reviewed commit: "Enhance SQL query structure and formatti..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| SELECT | ||
| t.table_schema, | ||
| t.table_name, | ||
| t.table_type, | ||
| COALESCE(d.description, '') AS table_comment, | ||
| c.relpersistence::text AS relpersistence | ||
| COALESCE(d.description, '') AS table_comment |
There was a problem hiding this comment.
queries.sql.go was not re-generated with sqlc generate after this PR's changes. This has two consequences: (1) the SQL changes here are entirely inert at runtime — the app still executes the old queries from the embedded string constants in the generated file, and (2) running sqlc generate after merging will produce a Go file that fails to compile because inspector.go still references fields that are being removed:
inspector.go:264—table.Relpersistence(removed fromGetTables/GetTablesForSchema)inspector.go:495—constraint.NoInherit(removed from both constraints queries)inspector.go:497—constraint.NullsNotDistinct.Bool(removed fromGetConstraintsForSchema)inspector.go:773—indexRow.IncludeColumns(removed fromGetIndexesForSchema)
queries.sql.go must be regenerated and inspector.go must be updated to stop referencing the removed fields before this PR is mergeable.
| ELSE dn.nspname || '.' || format_type(dt.oid, NULL) | ||
| END | ||
| WHEN dt.typtype = 'b' AND dt.typcategory = 'A' THEN | ||
| WHEN dt.typtype = 'b' AND dt.typelem <> 0 THEN |
There was a problem hiding this comment.
The condition was changed from dt.typcategory = 'A' to dt.typelem <> 0, but the original code had an explicit comment explaining exactly why typelem <> 0 is wrong:
Use
typcategory = 'A'rather thantypelem <> 0; the latter is true for non-array fixed-length types likename(typelempoints tochar).
The name type (pg_catalog.name) has typelem = 18 (pointing to char), but typcategory = 'S' (string), not 'A'. With this change, columns of type name would hit the array branch and be misidentified as char[]. The same incorrect condition appears at line 198 in GetColumnsForSchema.
| WHEN dt.typtype = 'b' AND dt.typelem <> 0 THEN | |
| WHEN dt.typtype = 'b' AND dt.typcategory = 'A' THEN |
| FROM generate_series(1, idx.indnatts) k | ||
| ) as column_directions, | ||
| ARRAY( | ||
| SELECT CASE | ||
| WHEN opc.opcdefault THEN '' -- Omit default operator classes | ||
| ELSE COALESCE(opc.opcname, '') | ||
| END | ||
| FROM generate_series(1, idx.indnkeyatts) k | ||
| FROM generate_series(1, idx.indnatts) k | ||
| LEFT JOIN pg_opclass opc ON opc.oid = idx.indclass[k-1] |
There was a problem hiding this comment.
column_directions and column_opclasses now iterate over indnatts (all columns including INCLUDE columns) instead of indnkeyatts (key columns only). INCLUDE columns have no sort order and no operator class — they are not indexing key columns, just stored values. For an index like CREATE INDEX ON t (a) INCLUDE (b), this will generate ['ASC', 'ASC'] for directions and look up an indclass entry that is 0/undefined for the INCLUDE column slot.
The indoption and indclass arrays are only defined for the first indnkeyatts positions; entries beyond that point are meaningless. The include_columns array (now removed) was the correct place to handle the INCLUDE columns separately.
| c.convalidated AS is_valid, | ||
| COALESCE((to_jsonb(c) ->> 'conperiod')::boolean, false) AS is_period, | ||
| c.connoinherit AS no_inherit | ||
| COALESCE((to_jsonb(c) ->> 'conperiod')::boolean, false) AS is_period |
There was a problem hiding this comment.
no_inherit and nulls_not_distinct drops active features
Both GetConstraints (line 341) and GetConstraintsForSchema (line 920) have had c.connoinherit AS no_inherit removed. Additionally GetConstraintsForSchema lost nulls_not_distinct and its LEFT JOIN pg_index i. These fields drive active behaviour:
inspector.go:495populatesConstraint.NoInherit— removing this silently makes all CHECK constraints appear inheritable, producing incorrect DDL for any schema that usesNO INHERITchecks.inspector.go:497populatesConstraint.NullsNotDistinct(PG15+ UNIQUE NULLS NOT DISTINCT) — removing this will generate incorrect migration diffs for unique constraints that useNULLS NOT DISTINCT.
Both fields need to be retained, or their consumers in inspector.go must be updated accordingly.
| t.table_type, | ||
| COALESCE(d.description, '') AS table_comment, | ||
| c.relpersistence::text AS relpersistence | ||
| COALESCE(d.description, '') AS table_comment |
There was a problem hiding this comment.
relpersistence removal breaks UNLOGGED table detection
Removing c.relpersistence::text AS relpersistence from both GetTables and GetTablesForSchema will drop the ability to detect UNLOGGED tables. inspector.go:264 reads:
Unlogged: table.Relpersistence.Valid && table.Relpersistence.String == "u",Without this column, Relpersistence would always be the zero value and every table would appear as a regular (logged) table, causing UNLOGGED tables to lose their UNLOGGED modifier in dumps and diffs.
|
Do no accept mere formatting changes |
2 participants
Refactor SQL queries for improved clarity and consistency.
Closes #422