Skip to content

fix: always schema-qualify table in COMMENT ON COLUMN (#502)#504

Merged
tianzhou merged 2 commits into
mainfrom
fix/issue-502-comment-column-schema-stripping
Jul 6, 2026
Merged

fix: always schema-qualify table in COMMENT ON COLUMN (#502)#504
tianzhou merged 2 commits into
mainfrom
fix/issue-502-comment-column-schema-stripping

Conversation

@tianzhou

@tianzhou tianzhou commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Always schema-qualify the table name in COMMENT ON COLUMN emissions so that stripSchemaQualifications can safely strip the schema prefix without mangling the table.column reference
  • Fixes the case where a table name matches the target schema name (e.g., table public in schema public), which caused stripSchemaQualifications to strip the table qualifier producing invalid SQL
  • Updated 3 COMMENT ON COLUMN emission sites in internal/diff/table.go and all golden test files

Test plan

  • Added comment/issue_502_column_comment_table_name_matches_schema test fixture with table named public in schema public
  • All existing comment diff tests pass (golden files updated with public. prefix)
  • All plan-and-apply comment tests pass
  • Full test suite passes

Closes #502

🤖 Generated with Claude Code

When a table name matches the target schema (e.g. table "public" in
schema "public"), stripSchemaQualifications strips the table qualifier
thinking it is a schema prefix, producing invalid SQL like
"COMMENT ON COLUMN column IS ...". Always emitting the schema-qualified
form (schema.table.column) ensures the schema prefix can be safely
stripped while preserving the table.column reference.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 6, 2026 08:02
@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a bug where stripSchemaQualifications would incorrectly strip the table qualifier from COMMENT ON COLUMN SQL when the table name matches the target schema name (e.g., a table named public in schema public). The fix is to always schema-qualify the table name in all three COMMENT ON COLUMN emission sites, so the output is always schema.table.column, giving stripSchemaQualifications an unambiguous schema prefix to strip.

  • All three COMMENT ON COLUMN emission sites in internal/diff/table.go now unconditionally use ir.QuoteIdentifier(schema).ir.QuoteIdentifier(table) instead of the conditional qualifyEntityNameMode/getTableNameWithSchema helpers that omit the schema prefix when schema equals targetSchema.
  • A new golden test fixture (issue_502_column_comment_table_name_matches_schema) validates the specific failure case, and all existing golden files are updated to reflect the new schema.table.column format.

Confidence Score: 5/5

The change is safe to merge — it correctly fixes a narrow SQL-generation bug without any collateral risk to the broader diff or plan pipeline.

All three COMMENT ON COLUMN emission sites are updated consistently. The fix is logically sound: always emitting schema.table lets stripSchemaQualifications strip exactly one prefix level, regardless of whether the table name coincidentally matches the schema name. The golden test files are fully updated and a dedicated regression fixture is added. No other code paths are affected.

No files require special attention.

Important Files Changed

Filename Overview
internal/diff/table.go Three COMMENT ON COLUMN emission sites updated to always schema-qualify the table name; COMMENT ON TABLE is intentionally unchanged (2-part name has no ambiguity).
testdata/diff/comment/issue_502_column_comment_table_name_matches_schema/new.sql New test fixture reproducing the bug: table named public in schema public, correctly verifies public.public.column output.
testdata/diff/comment/add_column_comments/diff.sql Golden file updated to reflect always-schema-qualified COMMENT ON COLUMN output (public.products.id, etc.).
testdata/diff/comment/mixed_comments/diff.sql Golden file updated consistently with the new schema-qualified column comment format.
testdata/diff/create_table/add_table_like/diff.sql Golden file updated to include schema prefix on COMMENT ON COLUMN for the table-like test case.

Reviews (1): Last reviewed commit: "fix: always schema-qualify table in COMM..." | Re-trigger Greptile

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a dump→plan round-trip failure for COMMENT ON COLUMN when a table name matches the target schema name (Issue #502) by ensuring COMMENT ON COLUMN statements are emitted with a schema-qualified table reference (i.e., schema.table.column), allowing stripSchemaQualifications to remove only the schema prefix without misinterpreting table.column as schema.object.

Changes:

  • Always emit COMMENT ON COLUMN using schema.table.column in internal/diff/table.go (create tables, added columns, modified column comments).
  • Add a regression fixture covering the “table name equals schema name” case.
  • Update existing golden outputs to reflect the new public.-qualified COMMENT ON COLUMN SQL.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
internal/diff/table.go Emits schema-qualified table names for COMMENT ON COLUMN to prevent stripSchemaQualifications from stripping the wrong qualifier.
testdata/diff/create_table/add_table_like/plan.txt Updates golden plan output to include public. qualification for column comments.
testdata/diff/create_table/add_table_like/plan.sql Updates golden SQL output to include public. qualification for column comments.
testdata/diff/create_table/add_table_like/plan.json Updates golden JSON output to include public. qualification for column comments.
testdata/diff/create_table/add_table_like/diff.sql Updates golden diff output to include public. qualification for column comments.
testdata/diff/comment/mixed_comments/plan.txt Updates golden plan output to include public. qualification for column comments.
testdata/diff/comment/mixed_comments/plan.sql Updates golden SQL output to include public. qualification for column comments.
testdata/diff/comment/mixed_comments/plan.json Updates golden JSON output to include public. qualification for column comments.
testdata/diff/comment/mixed_comments/diff.sql Updates golden diff output to include public. qualification for column comments.
testdata/diff/comment/issue_502_column_comment_table_name_matches_schema/plan.txt Adds regression fixture demonstrating correct handling when table name matches schema name.
testdata/diff/comment/issue_502_column_comment_table_name_matches_schema/plan.sql Adds regression fixture SQL output for the new edge case.
testdata/diff/comment/issue_502_column_comment_table_name_matches_schema/plan.json Adds regression fixture JSON output for the new edge case.
testdata/diff/comment/issue_502_column_comment_table_name_matches_schema/old.sql Adds old-state SQL for the regression fixture.
testdata/diff/comment/issue_502_column_comment_table_name_matches_schema/new.sql Adds new-state SQL (with column comments) for the regression fixture.
testdata/diff/comment/issue_502_column_comment_table_name_matches_schema/diff.sql Adds expected diff SQL for the regression fixture.
testdata/diff/comment/column_comment_quoted_identifier/plan.txt Updates golden output to include public. qualification for quoted-column comment case.
testdata/diff/comment/column_comment_quoted_identifier/plan.sql Updates golden SQL output to include public. qualification for quoted-column comment case.
testdata/diff/comment/column_comment_quoted_identifier/plan.json Updates golden JSON output to include public. qualification for quoted-column comment case.
testdata/diff/comment/column_comment_quoted_identifier/diff.sql Updates golden diff output to include public. qualification for quoted-column comment case.
testdata/diff/comment/add_column_comments/plan.txt Updates golden plan output to include public. qualification for column comments.
testdata/diff/comment/add_column_comments/plan.sql Updates golden SQL output to include public. qualification for column comments.
testdata/diff/comment/add_column_comments/plan.json Updates golden JSON output to include public. qualification for column comments.
testdata/diff/comment/add_column_comments/diff.sql Updates golden diff output to include public. qualification for column comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/diff/table.go Outdated
Comment thread internal/diff/table.go Outdated
Comment thread internal/diff/table.go Outdated
Only schema-qualify table names in COMMENT ON COLUMN when the table
name matches the target schema (the case where
stripSchemaQualifications would mangle the table.column reference).
This avoids changing dump output format for all other cases while
still fixing the issue #502 bug.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

@tianzhou tianzhou merged commit 552ca4a into main Jul 6, 2026
2 of 3 checks 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

Development

Successfully merging this pull request may close these issues.

COMMENT ON COLUMN misresolved when a table shares the target schema name — breaks dump→plan round trip

2 participants