Added support to change numeric precision#363
Conversation
Greptile SummaryThis PR fixes issue #362 by teaching the diff engine to detect and emit migrations when a Key changes:
Minor observations:
Confidence Score: 4/5
Important Files Changed
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]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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 | ||
| } |
There was a problem hiding this comment.
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.
tianzhou
left a comment
tianzhou
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Please take a look at review comments
| @@ -0,0 +1 @@ | |||
| ALTER TABLE transactions ALTER COLUMN amount TYPE numeric(16,6); | |||
There was a problem hiding this comment.
for file name, pls prefix with issue_xxx
There was a problem hiding this comment.
Pull request overview
This PR addresses #362 by ensuring column type diffs account for NUMERIC precision/scale changes (and similar typmod-style modifiers), so schema plans correctly emit ALTER TABLE ... ALTER COLUMN ... TYPE ... when only modifiers change.
Changes:
- Include precision/scale (and length) modifiers when determining whether a column’s type has changed during SQL generation.
- Extend column structural equality to consider numeric
PrecisionandScale. - Add a diff fixture covering numeric precision/scale modification planning.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/diff/column.go |
Updates column equality and type-change SQL generation to consider precision/scale/length modifiers. |
testdata/diff/create_table/alter_numeric_precision/plan.txt |
Adds expected human-readable plan output for numeric precision/scale alteration. |
testdata/diff/create_table/alter_numeric_precision/plan.sql |
Adds expected SQL plan output for numeric precision/scale alteration. |
testdata/diff/create_table/alter_numeric_precision/plan.json |
Adds expected JSON plan output for numeric precision/scale alteration. |
testdata/diff/create_table/alter_numeric_precision/old.sql |
Fixture: prior schema state with numeric typmod. |
testdata/diff/create_table/alter_numeric_precision/new.sql |
Fixture: updated schema state with modified numeric typmod. |
testdata/diff/create_table/alter_numeric_precision/diff.sql |
Fixture: expected diff SQL for the typmod change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) | ||
| } |
There was a problem hiding this comment.
columnTypeWithModifiers only applies MaxLength modifiers for varchar/character types. But columnsEqual compares MaxLength for all columns, and the inspector populates MaxLength from character_maximum_length, which also applies to types like bit / bit varying (varbit). If a bit-string length changes, it will be detected as a column change but generateColumnSQL won’t emit an ALTER ... TYPE bit(n) because oldType/newType will both remain the bare DataType. Consider extending columnTypeWithModifiers (or using a shared formatter) to include length modifiers for other length-typed columns where MaxLength is set.
| 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) | |
| } | |
| // Length-based modifiers (character and bit string types) | |
| if col.MaxLength != nil { | |
| switch dt { | |
| case "varchar", "character varying": | |
| // Normalize to varchar(n) for varying-length character types | |
| return fmt.Sprintf("varchar(%d)", *col.MaxLength) | |
| case "character": | |
| return fmt.Sprintf("character(%d)", *col.MaxLength) | |
| case "bit": | |
| return fmt.Sprintf("bit(%d)", *col.MaxLength) | |
| case "bit varying", "varbit": | |
| // Use canonical "bit varying(n)" representation | |
| return fmt.Sprintf("bit varying(%d)", *col.MaxLength) | |
| } | |
| } | |
| // Numeric precision/scale modifiers | |
| 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) | |
| } |
| 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 |
There was a problem hiding this comment.
columnTypeWithModifiers duplicates the type/modifier formatting logic already implemented in internal/diff/table.go (e.g., formatColumnDataType / formatColumnDataTypeForCreate). To avoid these drifting out of sync (especially as more modifier-carrying types are added), consider refactoring to reuse a single formatter (possibly with an option to skip SERIAL transformation, since this helper mentions that requirement).
| 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 | |
| return formatColumnDataType(col) |
Fixes #362