Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the expression parsing and formatting to preserve type casts in PostgreSQL default values and expressions instead of stripping them. The key changes ensure PostgreSQL's canonical representation is maintained by using pg_query.Deparse for expression handling.
- Replaced manual expression parsing with
pg_query.Deparseto preserve type casts and handle edge cases - Updated formatting to preserve canonical PostgreSQL representation with type casts
- Added normalization to handle case differences between inspector output and parser output
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ir/parser.go | Replaced manual expression parsing with pg_query.Deparse-based approach for better accuracy |
| ir/normalize.go | Added keyword normalization to handle case differences between PostgreSQL functions |
| ir/formatter.go | Removed type cast stripping logic to preserve canonical representation |
| internal/diff/table.go | Removed stripTypeQualifiers function and type cast removal from column defaults |
| testdata/diff/migrate/v5/* | Updated test data to reflect lowercase PostgreSQL keywords |
| testdata/diff/migrate/v3/* | Updated test data to reflect lowercase PostgreSQL keywords |
| testdata/diff/dependency/* | Updated test data to reflect lowercase PostgreSQL keywords |
| testdata/diff/create_table/* | Updated test data to reflect lowercase PostgreSQL keywords |
| cmd/dump/issue_82_view_logic_expr_* | Test files showing preserved vs stripped type casts in view expressions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
89b52ff to
66d4787
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ir/parser.go
Outdated
| // Use simple case-insensitive replacement | ||
| // This works because we're only processing deparsed SQL from pg_query, | ||
| // which formats keywords consistently | ||
| lowercase := strings.ToLower(keyword) | ||
| result = strings.ReplaceAll(result, lowercase, keyword) |
There was a problem hiding this comment.
The simple string replacement approach can cause incorrect replacements when the keyword appears as part of another word. For example, 'current_user_id' would be incorrectly transformed to 'CURRENT_USER_id'. Use word boundary matching with regex or more precise string matching to avoid partial word replacements.
| // Use simple case-insensitive replacement | |
| // This works because we're only processing deparsed SQL from pg_query, | |
| // which formats keywords consistently | |
| lowercase := strings.ToLower(keyword) | |
| result = strings.ReplaceAll(result, lowercase, keyword) | |
| // Use regex word boundary replacement, case-insensitive | |
| // This avoids replacing keywords inside longer identifiers | |
| pattern := `(?i)\b` + regexp.QuoteMeta(keyword) + `\b` | |
| re := regexp.MustCompile(pattern) | |
| result = re.ReplaceAllString(result, keyword) |
ir/normalize.go
Outdated
| // Remove unnecessary whitespace | ||
| value = strings.TrimSpace(value) | ||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] Unnecessary blank line added. This doesn't add any value and should be removed to maintain consistent code formatting.
7c7b819 to
b970e9f
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| lowercase := strings.ToLower(keyword) | ||
| pattern := regexp.MustCompile(`\b` + regexp.QuoteMeta(lowercase) + `\b`) | ||
| result = pattern.ReplaceAllString(result, keyword) |
There was a problem hiding this comment.
The regex compilation inside the loop is inefficient. Consider pre-compiling the regex patterns or using a more efficient string replacement approach for better performance when processing multiple keywords.
ir/normalize.go
Outdated
| // Pattern explanation: | ||
| // '([^']*)' - matches a quoted string literal (capturing the content) | ||
| // ::[\w.]+(\[\])? - matches ::typename or ::typename[] (including schema.typename and array types) | ||
| re := regexp.MustCompile(`'([^']*)'::[\w.]+(\[\])?`) |
There was a problem hiding this comment.
The regex is compiled on every function call. Consider moving this to a package-level variable or using sync.Once to compile it only once for better performance.
b970e9f to
331008c
Compare
331008c to
ff7796d
Compare
Fix #91