fix: view logic expr #87
Conversation
There was a problem hiding this comment.
Pull Request Overview
Fixes incorrect dumping of view definitions containing CASE with IN clauses (issue #82), ensuring logical expressions are preserved and ORDER BY uses aliases.
- Add AST-based normalization to convert "= ANY (ARRAY[...]::type[])" into "IN (...)" and to normalize ORDER BY to column aliases.
- Enhance formatter to strip unnecessary casts in CASE ELSE and support ScalarArrayOpExpr/A_ArrayExpr formatting.
- Add integration test and testdata for the repro.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/dump/issue_82_view_logic_expr/raw.sql | Source schema to reproduce the issue (view with CASE + IN + ORDER BY alias). |
| testdata/dump/issue_82_view_logic_expr/pgschema.sql | Expected dump output validating correct expression and ORDER BY aliasing. |
| testdata/dump/issue_82_view_logic_expr/pgdump.sql | pg_dump output used for comparison (shows "= ANY (ARRAY[...]::type[])"). |
| testdata/dump/issue_82_view_logic_expr/manifest.json | Test metadata describing the scenario. |
| ir/normalize.go | Adds normalizeViewWithAST to perform AST-based conversions and formatting in one pass. |
| ir/formatter.go | Teaches formatter to handle ScalarArrayOpExpr/A_ArrayExpr and strips casts in CASE ELSE and IN lists. |
| cmd/dump/dump_integration_test.go | Adds integration test for issue #82. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // UseOr means ANY (disjunction), !UseOr means ALL (conjunction) | ||
| isEqualAny := arrayOp.UseOr && len(arrayOp.Args) == 2 | ||
|
|
||
| if isEqualAny { | ||
| // Args[0] is the left side (column), Args[1] is the right side (array) | ||
| // Format as "column IN (values)" |
There was a problem hiding this comment.
formatScalarArrayOpExpr does not verify the operator is '=' and will convert other ANY/ALL operators (e.g., '<> ANY', '> ANY', or '= ALL') to IN, which is incorrect. Add an explicit operator check (e.g., via Opname if available, or a safe deparse check for ' = ANY') and only rewrite when the operator is equality with ANY; otherwise, fall back to deparse.
| if len(arrayOp.Args) >= 2 { | ||
| // Format left side (the column) | ||
| f.formatExpression(arrayOp.Args[0]) | ||
|
|
||
| f.buffer.WriteString(" IN (") | ||
|
|
||
| // Extract values from the array | ||
| if arrayExpr := arrayOp.Args[1].GetArrayExpr(); arrayExpr != nil { | ||
| // Format array elements as comma-separated list | ||
| for i, elem := range arrayExpr.Elements { | ||
| if i > 0 { | ||
| f.buffer.WriteString(", ") | ||
| } | ||
| f.formatExpression(elem) | ||
| } | ||
| } else { | ||
| // Fallback: format the right expression as-is | ||
| f.formatExpression(arrayOp.Args[1]) | ||
| } | ||
|
|
||
| f.buffer.WriteString(")") | ||
| return | ||
| } |
There was a problem hiding this comment.
The RHS extraction uses GetArrayExpr and falls back to emitting the entire array as-is, which can produce invalid SQL like 'col IN (ARRAY[...]::type[])'. Additionally, the LHS retains type casts (e.g., 'status::text'). Unwrap TypeCast nodes on the RHS to reach A_ArrayExpr/ArrayExpr and iterate elements, formatting each via formatExpressionStripCast; also use formatExpressionStripCast on the LHS to drop superfluous casts. If the RHS cannot be unwrapped to an array literal, fall back to deparsing the whole ScalarArrayOpExpr rather than emitting an invalid IN.
| // Special case: Detect "column = ANY (ARRAY[...])" pattern and convert to "column IN (...)" | ||
| // This pattern appears when parsing view definitions from pg_get_viewdef() | ||
| if len(expr.Name) == 1 && expr.Rexpr != nil { | ||
| if str := expr.Name[0].GetString_(); str != nil && str.Sval == "=" { | ||
| if aArrayExpr := expr.Rexpr.GetAArrayExpr(); aArrayExpr != nil { | ||
| // Direct array comparison: column = ARRAY[...] | ||
| // Convert to IN syntax, stripping unnecessary type casts from constants | ||
| f.formatExpressionStripCast(expr.Lexpr) | ||
| f.buffer.WriteString(" IN (") | ||
| for i, elem := range aArrayExpr.Elements { | ||
| if i > 0 { | ||
| f.buffer.WriteString(", ") | ||
| } | ||
| // Strip type casts from constants in IN list | ||
| f.formatExpressionStripCast(elem) | ||
| } | ||
| f.buffer.WriteString(")") | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This rewrites a plain '=' comparison against an array literal (e.g., 'col = ARRAY[...]') to 'IN (...)', which changes semantics. Unless this node represents the '= ANY' form (which should be handled by ScalarArrayOpExpr or A_Expr ANY/ALL kinds), this transformation is unsafe. Remove this branch or tighten it to only trigger for an explicit '= ANY (ARRAY[...])' construct.
| // Format ELSE clause, stripping unnecessary type casts from constants/NULL | ||
| if caseExpr.Defresult != nil { | ||
| f.buffer.WriteString(" ELSE ") | ||
| f.formatExpression(caseExpr.Defresult) | ||
| f.formatExpressionStripCast(caseExpr.Defresult) | ||
| } |
There was a problem hiding this comment.
[nitpick] Good improvement on ELSE formatting. Consider applying the same cast-stripping to WHEN result expressions as well (e.g., formatExpressionStripCast on CaseWhenExpr.Result) to ensure consistent formatting of constants in all CASE branches.
1 participant
Fix #82