server: fix non default schema are not supported in postgres connector

[shreeharsha-factly]

Summary by CodeRabbit

• Bug Fixes

  • Improved table qualification handling to correctly resolve catalog, schema, and table references with explicit catalog prefixes.

• Tests

  • Updated test coverage to verify accurate table qualification behavior with catalog-prefixed references.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes rework table qualification logic in DuckDB SQL processing by replacing string-based dot checking with explicit catalog and schema name handling. Unqualified and partially-qualified tables now receive appropriate catalog prefixes based on provided aliases, with the logic determining qualification based on the presence of catalog_name and schema_name fields.

Changes

Cohort / File(s)	Summary
Table Qualification Logic server/infrastructure/duckdb/duckdbsql/qualifier.go	Reworked qualification decision logic: removed isQualified helper and dot-based checks; now uses catalog_name and schema_name presence to determine qualification strategy. Unqualified tables get schema_name set to alias, and partially-qualified tables (with schema but no catalog) get catalog_name set to alias. Skips requalification when catalog_name already present.
Qualification Test Updates server/infrastructure/duckdb/duckdbsql/qualifier_test.go	Updated test expectations to include explicit catalog prefixes in qualified references (e.g., public.roles becomes "app".public.roles). Added new test case validating catalog prefix injection for schema-qualified tables (e.g., analysis.sales becomes "pg_ext_abc".analysis.sales).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Poem

🐰 A rabbit hops through catalog seas,
Where tables dance with new decrees,
No dots to parse, just names so clear—
Aliases whisper, qualifications premiere!
Tables leap from schema to catalog's embrace,
A hop, a skip—qualification's new grace! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title describes a fix for non-default schema support in postgres connector, but the actual changes involve reworking table qualification logic for DuckDB SQL to handle catalog/schema prefixes.	Update the title to reflect the actual change: something like 'fix: rework table qualification logic to support catalog/schema prefixes in DuckDB' or similar to accurately represent the DuckDB-focused modifications.
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/sql-qualifier

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @shreeharsha-factly's task —— View job

---

Code Review for PostgreSQL Schema Qualifier Fix

Reviewing the changes to fix non-default schema support in the PostgreSQL connector.

Todo List:

• Read and analyze qualifier.go changes
• Read and analyze qualifier_test.go changes
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Look for security concerns
• Evaluate test coverage
• Provide comprehensive feedback

---

Code Review Summary

This PR makes important improvements to handle PostgreSQL schema qualification in DuckDB SQL parsing, particularly for non-default schemas. The changes are well-thought-out and address a real limitation.

✅ Strengths

1. Logical Fix: The core issue was that the previous logic using strings.Contains(tableName, ".") to check qualification was flawed. The new approach properly examines AST fields (schema_name, catalog_name) which is more robust.

2. Comprehensive Test Coverage: The test cases cover critical scenarios:

   • Simple unqualified tables
   • Mixed qualification scenarios
   • CTE handling (correctly excludes CTEs from qualification)
   • Nested subqueries
   • Schema-qualified tables that need catalog prefixes

3. Clean Code Structure: The logic at qualifier.go:53-62 is well-structured with clear conditionals for different qualification scenarios.

🔍 Technical Analysis

Key Changes Made:

• Removed flawed isQualified() function that used string parsing
• Added proper AST field checking for catalog_name
• Implemented two-tier qualification logic:
  • Unqualified tables → set schema_name
  • Schema-qualified tables → set catalog_name (for alias.schema.table resolution)

The logic correctly handles:

• users → "public".users (unqualified case)
• analysis.sales → "pg_ext_abc".analysis.sales (schema-qualified case)

⚠️ Potential Issues

1. Edge Case Handling: The code should validate that schemaName parameter is not empty. Consider adding:

   if schemaName == "" {
       return nil // or return an error
   }

2. Error Propagation: The qualifyTablesInNode function doesn't return errors, but some AST operations might fail. Consider if error handling should be added for robustness.

3. Performance: The recursive traversal of AST nodes could be expensive for very large queries. Current implementation is fine for typical use cases, but consider if optimization is needed for your workload.

🔒 Security Assessment

• No security concerns identified
• The code properly handles user input through AST parsing rather than string manipulation
• No SQL injection risks introduced

🧪 Test Coverage Assessment

• Excellent test coverage with 8 comprehensive test cases
• Tests verify both the transformation logic and output formatting
• Good coverage of edge cases including CTEs, subqueries, and mixed scenarios
• Test case at line 74-78 specifically validates the main fix (schema-qualified tables getting catalog prefix)

💡 Minor Suggestions

1. Documentation: Consider adding a comment explaining the two-tier qualification strategy:

   // Two-tier qualification strategy:
   // 1. Unqualified tables (no schema): set schema_name to alias
   // 2. Schema-qualified tables (has schema, no catalog): set catalog_name to alias

2. Naming: The variable name schemaName could be more descriptive like aliasName since it acts as both schema and catalog depending on context.

Overall Assessment: ✅ APPROVED

This is a solid fix that properly addresses PostgreSQL non-default schema support. The implementation is clean, well-tested, and follows good practices. The changes are focused and don't introduce unnecessary complexity.

Recommendation: Ready to merge after considering the minor suggestions above.

---

[coderabbitai]

🧹 Nitpick comments (1)

server/infrastructure/duckdb/duckdbsql/qualifier_test.go (1)

37-79: Consider adding a test for already fully-qualified tables.

The current test suite thoroughly covers unqualified and schema-qualified tables. For completeness, consider adding a test case verifying that fully-qualified tables (e.g., catalog.schema.table) remain unchanged:

{
    title:       "Fully_Qualified_Table_Unchanged",
    sql:         "SELECT * FROM existing_catalog.analysis.sales",
    schemaName:  "pg_ext_abc",
    expectedSql: "SELECT * FROM existing_catalog.analysis.sales",
},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/infrastructure/duckdb/duckdbsql/qualifier_test.go` around lines 37 -
79, Add a new test case to the testCases slice in qualifier_test.go to ensure
fully-qualified table names remain unchanged: create a case with title
"Fully_Qualified_Table_Unchanged", sql "SELECT * FROM
existing_catalog.analysis.sales", schemaName "pg_ext_abc", and expectedSql
"SELECT * FROM existing_catalog.analysis.sales"; append it alongside the
existing cases so the test harness (the testCases variable) will validate that
catalog.schema.table inputs are preserved by the qualifier logic.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@server/infrastructure/duckdb/duckdbsql/qualifier_test.go`:
- Around line 37-79: Add a new test case to the testCases slice in
qualifier_test.go to ensure fully-qualified table names remain unchanged: create
a case with title "Fully_Qualified_Table_Unchanged", sql "SELECT * FROM
existing_catalog.analysis.sales", schemaName "pg_ext_abc", and expectedSql
"SELECT * FROM existing_catalog.analysis.sales"; append it alongside the
existing cases so the test harness (the testCases variable) will validate that
catalog.schema.table inputs are preserved by the qualifier logic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a0211719-433f-4598-a317-63943093dbdb

📥 Commits

Reviewing files that changed from the base of the PR and between 1906b3b and f7a3d32.

📒 Files selected for processing (2)

• server/infrastructure/duckdb/duckdbsql/qualifier.go
• server/infrastructure/duckdb/duckdbsql/qualifier_test.go