Skip to content

Added support to change numeric precision#363

Open
marioloko wants to merge 1 commit intopgplex:mainfrom
marioloko:main
Open

Added support to change numeric precision#363
marioloko wants to merge 1 commit intopgplex:mainfrom
marioloko:main

Conversation

@marioloko
Copy link

Fixes #362

@greptile-apps
Copy link

greptile-apps bot commented Mar 22, 2026

Greptile Summary

This PR fixes issue #362 by teaching the diff engine to detect and emit migrations when a numeric/decimal column's precision or scale changes. Previously, precision-only changes were silently ignored because the comparison only looked at the bare DataType string (e.g. "numeric") without considering its modifiers.

Key changes:

  • generateColumnSQL now calls the new columnTypeWithModifiers helper to build the full type string (e.g. numeric(16,6)) before comparing old vs new, so a precision-only change correctly produces ALTER TABLE … ALTER COLUMN … TYPE numeric(16,6);
  • columnsEqual gains explicit Precision and Scale nil/value comparisons so the planner correctly marks these columns as needing migration
  • A new test fixture (alter_numeric_precision) validates the numeric(15,4)numeric(16,6) path
  • The needsUsingClause / IsBuiltInType call chain already handles type strings with parenthesised modifiers (strips them internally), so numeric(15,4)numeric(16,6) correctly avoids generating a spurious USING clause

Minor observations:

  • columnTypeWithModifiers duplicates the modifier-building logic already present in table.go's formatColumnDataType and formatColumnDataTypeForCreate; consolidating would reduce future drift risk
  • columnsEqual still compares bare DataType strings in its first check rather than using columnTypeWithModifiers, which is functionally correct today (precision/scale are checked separately) but creates an asymmetry worth documenting
  • Test coverage is focused on the one scenario from the issue; additional fixtures for adding/removing precision entirely and for decimal aliases would strengthen confidence

Confidence Score: 4/5

  • Safe to merge — the precision-change detection and SQL generation are correct; only minor code-quality improvements remain.
  • The core logic is sound: columnTypeWithModifiers correctly builds modifier-annotated type strings, IsBuiltInType already strips parenthesised modifiers so needsUsingClause works correctly, and columnsEqual now catches precision/scale differences. The two P2 comments (duplicated logic, asymmetry in columnsEqual) are non-blocking style issues. Test coverage is narrowly scoped to the reported issue scenario but is sufficient for merging.
  • Pay close attention to internal/diff/column.go — specifically the new columnTypeWithModifiers function and the asymmetry between how columnsEqual and generateColumnSQL compare types.

Important Files Changed

Filename Overview
internal/diff/column.go Core logic change: generateColumnSQL now builds type strings with precision/scale/length modifiers via the new columnTypeWithModifiers helper, and columnsEqual gains explicit Precision/Scale comparisons. The implementation is correct for the numeric/decimal case, but columnTypeWithModifiers duplicates modifier-building logic already present in table.go's formatColumnDataType.
testdata/diff/create_table/alter_numeric_precision/diff.sql Expected diff output — ALTER TABLE transactions ALTER COLUMN amount TYPE numeric(16,6); — is correct PostgreSQL DDL for a numeric precision change.
testdata/diff/create_table/alter_numeric_precision/old.sql Starting state with amount numeric(15,4). Representative but only covers the precision+scale→precision+scale case; adding/removing precision entirely is not covered.
testdata/diff/create_table/alter_numeric_precision/new.sql Desired state with amount numeric(16,6). Straightforward test fixture.
testdata/diff/create_table/alter_numeric_precision/plan.json Plan JSON output — single step of type table.column/alter with the correct SQL. No issues.
testdata/diff/create_table/alter_numeric_precision/plan.sql Plan SQL output matches diff.sql as expected.
testdata/diff/create_table/alter_numeric_precision/plan.txt Human-readable plan output correctly shows amount (column) as modified under transactions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[columnsEqual called] --> B{DataType equal?}
    B -- No --> C[Return false: type change]
    B -- Yes --> D{Precision nil-ness equal?}
    D -- No --> E[Return false: precision changed]
    D -- Yes --> F{Precision values equal?}
    F -- No --> G[Return false: precision changed]
    F -- Yes --> H{Scale nil-ness equal?}
    H -- No --> I[Return false: scale changed]
    H -- Yes --> J{Scale values equal?}
    J -- No --> K[Return false: scale changed]
    J -- Yes --> L[Continue other checks...]

    M[generateColumnSQL called] --> N[columnTypeWithModifiers old\ne.g. numeric-15-4]
    M --> O[columnTypeWithModifiers new\ne.g. numeric-16-6]
    N --> P{oldType != newType?}
    O --> P
    P -- Yes --> Q[needsUsingClause?\nIsBuiltInType strips parens\nnumeric-15-4 → numeric → true]
    Q -- No USING --> R[ALTER TABLE t ALTER COLUMN c TYPE numeric-16-6]
    Q -- USING needed --> S[ALTER TABLE t ALTER COLUMN c\nTYPE newType USING c::newType]
Loading

Comments Outside Diff (1)

  1. internal/diff/column.go, line 141-152 (link)

    P2 columnsEqual DataType comparison does not include modifiers

    generateColumnSQL now uses columnTypeWithModifiers when computing whether a type change has occurred. columnsEqual, however, still compares only the bare DataType string (e.g. "numeric"), relying on the separate Precision/Scale checks added below.

    This is functionally correct today because Precision and Scale are always checked explicitly afterwards. It is worth noting the asymmetry, though: if a future type gains modifiers that are NOT captured in separate IR fields (e.g. timestamp(p) precision stored only in DataType), columnsEqual and generateColumnSQL could diverge — generateColumnSQL would detect the change and emit an ALTER TYPE, while columnsEqual would still return true (equal) and suppress the planned migration.

    Consider adding a comment documenting this intentional split, or aligning the comparison by also using columnTypeWithModifiers here.

Reviews (1): Last reviewed commit: "Added support to change numeric precisio..." | Re-trigger Greptile

Comment on lines +209 to +221
func columnTypeWithModifiers(col *ir.Column) string {
dt := col.DataType
if col.MaxLength != nil && (dt == "varchar" || dt == "character varying") {
return fmt.Sprintf("varchar(%d)", *col.MaxLength)
} else if col.MaxLength != nil && dt == "character" {
return fmt.Sprintf("character(%d)", *col.MaxLength)
} else if col.Precision != nil && col.Scale != nil && (dt == "numeric" || dt == "decimal") {
return fmt.Sprintf("%s(%d,%d)", dt, *col.Precision, *col.Scale)
} else if col.Precision != nil && (dt == "numeric" || dt == "decimal") {
return fmt.Sprintf("%s(%d)", dt, *col.Precision)
}
return dt
}
Copy link

Choose a reason for hiding this comment

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

P2 Duplicated modifier-formatting logic

columnTypeWithModifiers is essentially the same as formatColumnDataType (in table.go), which already handles the varchar/character/numeric/decimal cases. The only intentional difference is that columnTypeWithModifiers skips the serial → serial/bigserial rewrite, but formatColumnDataType can also return the raw integer type when the column is not a serial.

Reusing formatColumnDataType (or extracting the shared modifier-building block into a small unexported helper) would keep the logic in one place and reduce the risk of future drift — e.g., if bit(n) or timestamp(p) precision support is added later, it would need to be patched in both files.

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.

Bug: pgschema is not detecting changes in numeric precision

1 participant