Skip to content

Conversation

@pyramation
Copy link
Collaborator

@pyramation pyramation commented Nov 23, 2025

Refactor TypeCast deparser to use AST-driven logic

Summary

Refactored the TypeCast deparser method to eliminate string-based heuristics (like arg.includes('(') and arg.startsWith('-')) and replace them with AST-driven logic that inspects node types and structure directly.

Key Changes

  • Added 4 helper methods for AST inspection:

    • isQualifiedName() - Check if names array matches expected path (e.g., ['pg_catalog', 'bpchar'])
    • isBuiltinPgCatalogType() - Check if type is built-in pg_catalog type
    • normalizeTypeName() - Extract normalized type name from TypeName node
    • argumentNeedsCastSyntax() - Determine if argument needs CAST() syntax based on AST node type
  • Refactored TypeCast method to:

    • Use AST node types (A_Const, ColumnRef, FuncCall) instead of rendered string inspection
    • Detect negative numbers by checking ival/fval values in A_Const nodes
    • Use CAST() syntax for negative numbers to avoid SQL precedence issues
    • Preserve round-trip fidelity for pg_catalog.bpchar and negative number casts
    • Allow FuncCall nodes to use :: syntax with parentheses (matching original behavior)
  • Test results: All 657 tests passing (reduced from 43 failures initially)

Technical Details

The original code used string inspection on the rendered argument:

if (arg.includes('(') || arg.startsWith('-')) {
  return `CAST(${arg} AS ${typeName})`;
}

The new code inspects the AST structure directly:

// FuncCall nodes can use :: syntax (TypeCast will add parentheses)
if (argType === 'FuncCall') {
  return false;
}

const needsCastSyntax = this.argumentNeedsCastSyntax(node.arg);
if (!needsCastSyntax) {
  return `${arg}::${cleanTypeName}`;
}
return `CAST(${arg} AS ${typeName})`;

Updates Since Initial Commit

Fixed CI failures by correcting argumentNeedsCastSyntax() to allow FuncCall nodes to use :: syntax. The initial implementation incorrectly returned true for all FuncCall nodes, preventing the existing FuncCall handling code (which wraps them in parentheses) from ever executing. This caused 3 snapshot test failures where function calls were being formatted with CAST() instead of (...)::type.

Review & Testing Checklist for Human

  • Review the large diff carefully - The diff includes extensive auto-formatting changes to switch statements (indentation changes). Verify these are cosmetic and don't hide unintended logic changes.
  • Test negative number edge cases - The negative number detection checks ival and fval fields. Test with very large negative numbers (int8 range: -9223372036854775808), negative floats, and edge cases like -0.
  • Verify FuncCall handling - Test complex nested function calls with type casts to ensure parentheses are added correctly and precedence is preserved (e.g., (func1(func2()))::text).
  • Spot-check other expression types - Verify that A_Expr, SubLink, TypeCast, and other complex expression types still use CAST() syntax correctly and don't regress.

Test Plan

  1. Run the full test suite locally: cd packages/deparser && yarn test
  2. Test a few manual cases with negative numbers: SELECT (-2147483648)::int4, SELECT (-1.5)::numeric
  3. Test nested function calls: SELECT (gen_random_uuid())::text, SELECT (CAST(now() AS text))::date
  4. Verify round-trip parsing: Parse → Deparse → Parse should produce identical ASTs

Notes

  • The diff includes extensive formatting changes (switch statement indentation) that appear to be auto-formatting. These are cosmetic but make the diff larger (~220KB).
  • The negative number detection uses String(fval).startsWith('-') which is technically string inspection on the AST literal value (not the rendered SQL). This is a pragmatic compromise for handling float representations.
  • Link to Devin run: https://app.devin.ai/sessions/992c2003f5d948ebabe32124f5209211
  • Requested by: Dan Lynch ([email protected]) / @pyramation

…inspection

- Add helper functions for AST predicates:
  - isQualifiedName: Check if names array matches expected path
  - isBuiltinPgCatalogType: Check if type is built-in pg_catalog type
  - normalizeTypeName: Extract normalized type name from TypeName node
  - argumentNeedsCastSyntax: Determine if argument needs CAST() syntax based on AST structure

- Replace string-based heuristics (arg.includes('('), arg.startsWith('-')) with AST node type checks
- Detect negative numbers in A_Const nodes by checking ival/fval values directly
- Preserve round-trip fidelity for bpchar and negative number casts
- Use CAST() syntax for negative numbers to avoid precedence issues
- Maintain same output behavior as before while using pure AST logic

Test results: Reduced failures from 43 to 3 (all snapshot updates, no AST mismatches)

Co-Authored-By: Dan Lynch <[email protected]>
@devin-ai-integration
Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

The original implementation allowed FuncCall nodes to use :: syntax (with
parentheses for precedence), but the initial refactoring incorrectly marked
them as needing CAST() syntax.

This fix updates argumentNeedsCastSyntax() to return false for FuncCall nodes,
allowing the existing FuncCall handling code to run and wrap them in parentheses.

Fixes snapshot test failures:
- pg-catalog.test.ts: (public.gen_random_uuid())::text
- misc-pretty.test.ts: (t.date AT TIME ZONE 'America/New_York')::text

All 657 tests now pass.

Co-Authored-By: Dan Lynch <[email protected]>
@devin-ai-integration
Copy link
Contributor

This PR has been superseded by #235, which contains the same functional changes but without the formatting/linting modifications that made this diff hard to review.

PR #235 has:

  • Only 169 insertions and 23 deletions (vs 6,213 lines changed here)
  • The same 4 helper methods and TypeCast refactoring
  • All 657 tests passing
  • CI passing

Please review #235 instead.

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.

2 participants