fix: emit USING clause for ALTER COLUMN TYPE when base types differ (#537)

[tianzhou]

Summary

• Rewrites needsUsingClause to compare normalized base type names instead of relying on built-in type classification
• Emits USING col::newtype whenever base types differ (e.g. text → integer), preventing SQLSTATE 42804 errors
• Omits USING when only modifiers change (e.g. numeric(18,6) → numeric(20,6))
• Adds regression test for the exact repro from issue ALTER COLUMN TYPE omits USING clause for non-implicitly-castable built-in type changes (SQLSTATE 42804) #537 (text → integer)

Closes #537

Test plan

• Existing alter_column_types and alter_column_quoted_identifier golden files updated and passing
• New issue_537_alter_column_type_using fixture (text→integer) passes diff and integration tests
• Full integration test suite passes

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

The PR broadens explicit USING-clause generation for ALTER COLUMN TYPE operations by comparing normalized base type names and adds a regression fixture for text-to-integer conversion.

• Emits explicit casts when normalized base type names differ.
• Avoids USING for modifier-only changes such as numeric precision updates.
• Updates existing golden plans and adds issue ALTER COLUMN TYPE omits USING clause for non-implicitly-castable built-in type changes (SQLSTATE 42804) #537 coverage.

Confidence Score: 4/5

The quoted custom-type case collision should be fixed before merging because it can still generate an ALTER COLUMN TYPE statement that PostgreSQL rejects.

The new normalization lowercases case-sensitive quoted type names, allowing distinct custom types to compare equal and suppressing the explicit cast required for a reachable migration.

Files Needing Attention: internal/diff/column.go

Important Files Changed

Filename	Overview
internal/diff/column.go	Replaces built-in-type classification with base-name comparison, but lowercasing quoted custom type identifiers can incorrectly suppress a required USING clause.
testdata/diff/create_table/issue_537_alter_column_type_using/old.sql	Adds the current-schema side of the text-to-integer regression fixture.
testdata/diff/create_table/issue_537_alter_column_type_using/new.sql	Adds the desired-schema side of the text-to-integer regression fixture.
testdata/diff/create_table/issue_537_alter_column_type_using/diff.sql	Verifies generation of an explicit integer cast for the reported text-to-integer migration.

_{Reviews (1): Last reviewed commit: "fix: emit USING clause for ALTER COLUMN ..." | Re-trigger Greptile}

[Copilot]

Pull request overview

Fixes ALTER TABLE ... ALTER COLUMN ... TYPE planning by ensuring a USING col::newtype clause is emitted when the base type changes, preventing PostgreSQL SQLSTATE 42804 for non-implicitly-castable built-in type conversions (issue #537).

Changes:

• Rewrites needsUsingClause to compare normalized base type names and emit USING col::newtype when the base types differ.
• Updates existing golden fixtures to reflect USING being added for built-in→built-in type changes.
• Adds a new regression fixture covering text → integer (issue_537_alter_column_type_using).

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/diff/column.go	Updates type-change logic to decide when to emit a USING cast expression.
testdata/diff/create_table/issue_537_alter_column_type_using/plan.txt	New regression plan output for text → integer with USING.
testdata/diff/create_table/issue_537_alter_column_type_using/plan.sql	New regression SQL output with USING.
testdata/diff/create_table/issue_537_alter_column_type_using/plan.json	New regression JSON plan output with USING.
testdata/diff/create_table/issue_537_alter_column_type_using/old.sql	New fixture: old schema uses text.
testdata/diff/create_table/issue_537_alter_column_type_using/new.sql	New fixture: new schema uses integer.
testdata/diff/create_table/issue_537_alter_column_type_using/diff.sql	New expected diff SQL for the regression case.
testdata/diff/create_table/alter_column_types/plan.txt	Updates golden plan text to include USING for built-in type changes.
testdata/diff/create_table/alter_column_types/plan.sql	Updates golden plan SQL to include USING for built-in type changes.
testdata/diff/create_table/alter_column_types/plan.json	Updates golden plan JSON steps to include USING for built-in type changes.
testdata/diff/create_table/alter_column_types/diff.sql	Updates expected diff SQL to include USING for built-in type changes.
testdata/diff/create_table/alter_column_quoted_identifier/plan.txt	Updates golden plan text to include USING while preserving quoted column identifier.
testdata/diff/create_table/alter_column_quoted_identifier/plan.sql	Updates golden plan SQL to include USING while preserving quoted column identifier.
testdata/diff/create_table/alter_column_quoted_identifier/plan.json	Updates golden plan JSON step to include USING while preserving quoted column identifier.
testdata/diff/create_table/alter_column_quoted_identifier/diff.sql	Updates expected diff SQL to include USING while preserving quoted column identifier.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

internal/diff/column.go:117

• The PR description says a new regression fixture "issue_537_alter_column_type_using" was added, but there are no files in the repo matching "issue_537" (grep/glob). The regression coverage appears to be implemented by extending the existing create_table/alter_column_types fixture instead; please either add the promised dedicated fixture or update the PR description to reflect the actual test change.

// needsUsingClause determines if a type conversion requires a USING clause.
//
// PostgreSQL rejects bare ALTER COLUMN TYPE when no implicit cast exists between
// the old and new types (SQLSTATE 42804). When the base type names differ, we
// always emit USING col::newtype — a redundant USING is harmless when an implicit

[Copilot]

Pull request overview

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

Suppressed comments (2)

internal/diff/column.go:133

• normalizeBaseTypeName currently truncates the type string at the first "(", which drops important suffixes like array brackets and time zone qualifiers. For example, "character varying(128)[]" (used in the repo; see issue_420_varchar_array_length_modifier) normalizes to "character varying", making scalar↔array changes appear identical and potentially causing needsUsingClause to skip a required USING clause (leading to SQLSTATE 42804 at apply). Strip typmod while preserving any suffix after the closing ")" (e.g. "varchar(128)[]" → "varchar[]"; "timestamp(6) with time zone" → "timestamp with time zone").

func normalizeBaseTypeName(typeName string) string {
	t := typeName
	if idx := strings.Index(t, "("); idx != -1 {
		t = t[:idx]
	}

testdata/diff/create_table/alter_column_types/old.sql:16

• PR description says a new regression fixture issue_537_alter_column_type_using was added, but there are no matching files/strings in the repo. The text→integer regression appears to have been added by expanding the existing alter_column_types fixture (arfcn_dl). Please either add the named fixture or update the PR description so it matches what actually changed.

    arfcn_dl text DEFAULT 'unknown',
    priority text,
    flag public."MyStatus" DEFAULT 'active'::"MyStatus",

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

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

Suppressed comments (1)

internal/diff/column.go:120

• PR description says a new regression fixture issue_537_alter_column_type_using was added, but there are no new testdata/**/issue_537* files in this change set. Instead, the repro seems to be covered by extending the existing testdata/diff/create_table/alter_column_types fixture. Please either update the PR description/test plan to match what was actually added, or include the promised fixture so future readers can locate the regression easily.

// needsUsingClause determines if a type conversion requires a USING clause.
//
// PostgreSQL rejects bare ALTER COLUMN TYPE when no implicit cast exists between
// the old and new types (SQLSTATE 42804). When the base type names differ, we
// always emit USING col::newtype — a redundant USING is harmless when an implicit
// cast exists, but a missing one breaks the migration. When only modifiers change
// (e.g. numeric(18,6) → numeric(20,6)), no USING is needed.
func needsUsingClause(oldType, newType string) bool {