fix(parser): allow TABLE/PARTITION/TABLES as identifiers in ClickHouse (#480)

[ajitpratap0]

Summary

• Fixes [BUG] multiple errors in ClickHouse dialect #480: ClickHouse queries using table, partition, or system.tables as identifiers now parse correctly.
• Extends the non-reserved-keyword-as-identifier path (previously SQL Server only) to also cover ClickHouse, where these tokens are valid column/relation names in system.replicas, system.parts, system.tables.
• Adds TokenTypePartition and the "TABLES" keyword value to isNonReservedKeyword.

Test plan

• New regression test pkg/sql/parser/clickhouse_table_identifier_test.go covering all 5 queries from the issue
• go test -race ./pkg/sql/parser/ ./pkg/sql/keywords/ ./pkg/gosqlx/ — all green
• task check (full suite) — all green
• No regressions in DDL parsing (CREATE TABLE, ALTER TABLE) — TABLE in DDL position is consumed before reaching expression context

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
gosqlx-website	Ready	Preview, Comment	Apr 7, 2026 7:36am

[github-actions]

Claude finished @ajitpratap0's task —— View job

---

Code Review Complete ✅

Reviewing PR #481: ClickHouse table identifier fix

Todo List:

• Read repository guidelines and setup
• Review core parser changes in parser.go
• Review tokenizer changes in tokenizer.go
• Review new test file clickhouse_table_identifier_test.go
• Review formatter changes in render.go
• Review AST changes in ast.go
• Review new PIVOT functionality in pivot.go
• Review expression parsing changes
• Check for security concerns
• Analyze performance implications
• Assess test coverage completeness
• Provide final recommendations

---

Code Quality Assessment

✅ Strengths:

1. Targeted Fix Implementation: The core changes correctly address issue [BUG] multiple errors in ClickHouse dialect #480:

   • Added TokenTypePartition to isNonReservedKeyword() function
   • Added "TABLES" keyword to the value-based check
   • Extended dialect support from SQL Server-only to include ClickHouse in expressions_literal.go:106

2. Comprehensive Test Coverage:

   • New test file covers all 5 real-world ClickHouse queries from the original issue
   • Tests use proper ParseWithDialect(query, keywords.DialectClickHouse)
   • Follows safe type assertion patterns as recommended in CLAUDE.md:135-142

3. Code Quality:

   • Proper error handling throughout
   • Clear, descriptive variable names
   • Good documentation in comments

⚠️ Major Concerns:

1. Scope Creep - CRITICAL

This PR mixes two unrelated features:

• Primary: ClickHouse identifier fix (6 lines changed)
• Secondary: Complete PIVOT/UNPIVOT implementation (252+ lines)

The PIVOT functionality should be in a separate PR as it's:

• Unrelated to the ClickHouse identifier issue
• Adds significant complexity (new AST nodes, parsing logic, formatter changes)
• Increases risk of introducing unrelated bugs

2. Dialect Logic Issue - HIGH

// pkg/sql/parser/parser.go:827-838
case models.TokenTypeTable, models.TokenTypeIndex, models.TokenTypeView,
    models.TokenTypeKey, models.TokenTypeColumn, models.TokenTypeDatabase,
    models.TokenTypePartition:
    // DDL keywords that are commonly used as quoted identifiers in MySQL (backtick)
    // and SQL Server (bracket) dialects, and as plain column names in ClickHouse
    // system tables (system.parts.partition, system.replicas.table, etc).
    return true

The "TABLES" keyword is now allowed as an identifier in ALL dialects, not just ClickHouse. This could cause parsing inconsistencies in other SQL dialects where TABLES might be reserved.

Recommendation: Add dialect-specific logic:

case "TABLES":
    return p.dialect == string(keywords.DialectClickHouse)

🔒 Security Assessment: ✅ CLEAN

• No injection vulnerabilities introduced
• No exposed secrets or sensitive data
• Input validation remains intact via existing tokenizer limits

⚡ Performance Assessment: ✅ MINIMAL IMPACT

• Changes to isNonReservedKeyword() add minimal overhead (single case match)
• No impact on object pooling or zero-copy optimizations
• New test file doesn't affect runtime performance

📊 Test Coverage Assessment: ✅ GOOD

• Covers all 5 problematic queries from issue [BUG] multiple errors in ClickHouse dialect #480
• Uses proper dialect specification
• Missing: negative test cases (ensuring other dialects still reject these as identifiers)

---

Recommendations

Required Changes:

1. Split this PR:

   • Keep only ClickHouse identifier changes (parser.go, tokenizer.go, expressions_literal.go, test file)
   • Move PIVOT functionality to separate PR

2. Add dialect-specific logic for "TABLES" keyword:

   case "TABLES":
       return p.dialect == string(keywords.DialectClickHouse)

3. Add negative test cases to ensure other dialects properly reject these keywords as identifiers

Optional Improvements:

1. Performance: Consider caching dialect comparisons if this becomes a hot path
2. Documentation: Add examples of ClickHouse system table usage to parser comments

---

Overall Assessment: ⚠️ NEEDS WORK

While the core ClickHouse fix is well-implemented and solves the stated problem, the scope creep and dialect logic issues need to be addressed before merge. The PIVOT functionality should definitely be in a separate PR for easier review and testing.

Recommendation: Request author to split the PR and add dialect-specific logic for the "TABLES" keyword.

---

Job Run

[github-actions]

Claude finished @ajitpratap0's task —— View job

---

Code Review Complete ✅

Reviewing PR #481: ClickHouse table identifier fix

Todo List:

• Read repository guidelines and setup
• Review core parser changes in parser.go
• Review expression parsing changes in expressions_literal.go
• Review new test file clickhouse_table_identifier_test.go
• Check for security concerns
• Analyze performance implications
• Assess test coverage completeness
• Provide final recommendations

---

Code Quality Assessment

✅ Strengths:

1. Targeted and Focused Fix: This PR successfully addresses issue [BUG] multiple errors in ClickHouse dialect #480 with minimal, surgical changes:

   • Added TokenTypePartition to isNonReservedKeyword() function at parser.go:962
   • Added "TABLES" keyword to the value-based check at parser.go:971
   • Extended dialect support from SQL Server-only to include ClickHouse at expressions_literal.go:106

2. Excellent Test Coverage:

   • New test file clickhouse_table_identifier_test.go covers all 5 real-world ClickHouse queries from issue [BUG] multiple errors in ClickHouse dialect #480
   • Tests use proper ParseWithDialect(query, keywords.DialectClickHouse) with correct dialect specification
   • Follows project's safe type assertion patterns as recommended in CLAUDE.md:135-142
   • Test cases represent realistic ClickHouse system table queries (system.replicas, system.tables, system.parts)

3. Clean Implementation:

   • Proper error handling throughout the test cases
   • Clear, descriptive test names and comments explaining the purpose
   • Follows existing code patterns and conventions

📊 Technical Analysis:

Changes Made:

1. parser.go:962: Added TokenTypePartition to the token type switch case for non-reserved keywords
2. parser.go:971: Added "TABLES" to the string value switch case for keywords with generic TokenTypeKeyword
3. expressions_literal.go:106: Extended the dialect condition to include ClickHouse alongside SQL Server for non-reserved keyword usage
4. New test file: Comprehensive test coverage with 5 realistic ClickHouse queries

Scope Assessment:

• ✅ Focused: Unlike the previous PR review, this one is properly scoped to only the ClickHouse identifier issue
• ✅ No Scope Creep: No unrelated PIVOT/UNPIVOT functionality mixed in
• ✅ Clean: Only the minimal necessary changes to solve the stated problem

🔒 Security Assessment: ✅ CLEAN

• No SQL injection vulnerabilities introduced
• No exposure of sensitive data or secrets
• Input validation remains intact via existing tokenizer bounds
• Changes only affect parsing logic, not execution or data handling

⚡ Performance Assessment: ✅ MINIMAL IMPACT

• Added one case to isNonReservedKeyword() switch - O(1) lookup with minimal overhead
• One additional string case for "TABLES" keyword - negligible impact
• No impact on the core zero-copy tokenization or object pooling systems
• Changes are in the parsing path, not the hot tokenization path

📝 Code Quality: ✅ EXCELLENT

• Readability: Clear comments explaining the purpose (parser.go:963-965)
• Maintainability: Changes are localized and easy to understand
• Consistency: Follows existing patterns in the codebase
• Documentation: Good inline comments explaining ClickHouse system table usage

🎯 Test Coverage: ✅ COMPREHENSIVE

• Positive Cases: All 5 problematic queries from issue [BUG] multiple errors in ClickHouse dialect #480 are covered
• Realistic Scenarios: Tests use actual ClickHouse system table patterns
• Proper Setup: Uses correct dialect specification
• Edge Cases: Covers different contexts where identifiers appear (SELECT, GROUP BY, function args, etc.)

⚠️ Minor Considerations:

1. Cross-Dialect Impact: The "TABLES" keyword is now treated as non-reserved across ALL dialects when it has TokenTypeKeyword. This is likely acceptable since it's only checked after dialect-specific type checks, but worth noting.

2. Missing Negative Tests: Could benefit from tests ensuring other SQL dialects still handle these keywords appropriately, but not critical for this fix.

---

Overall Assessment: ✅ EXCELLENT

This PR is a significant improvement over the previous version. It:

• Solves the exact problem stated in issue [BUG] multiple errors in ClickHouse dialect #480
• Maintains clean scope without unrelated features
• Provides comprehensive test coverage with realistic scenarios
• Follows project conventions and coding standards
• Has minimal performance impact
• Introduces no security risks

Recommendations:

1. ✅ Ready to Merge: This PR successfully addresses the ClickHouse identifier parsing issue with a clean, focused implementation.

2. Optional Future Enhancement: Consider adding negative test cases to verify other dialects still behave as expected, but this is not blocking.

3. Performance: No concerns - the changes are minimal and efficient.

Final Verdict:

APPROVE - This is a well-implemented fix that solves the stated problem cleanly and efficiently.

---

Job Run

[github-actions]

Claude finished @ajitpratap0's task —— View job

---

Code Review Complete ✅

Reviewing PR #481: ClickHouse table identifier fix

Todo List:

• Read repository guidelines and setup
• Review core parser changes in parser.go
• Review expression parsing changes in expressions_literal.go
• Review new test file clickhouse_table_identifier_test.go
• Check for security concerns
• Analyze performance implications
• Assess test coverage completeness
• Provide final recommendations

---

Code Quality Assessment

✅ Strengths:

1. Targeted and Focused Fix: This PR successfully addresses issue [BUG] multiple errors in ClickHouse dialect #480 with minimal, surgical changes:

   • Added TokenTypePartition to isNonReservedKeyword() function at parser.go:962
   • Added "TABLES" keyword to the value-based check at parser.go:972
   • Extended dialect support from SQL Server-only to include ClickHouse at expressions_literal.go:106

2. Excellent Test Coverage:

   • New test file clickhouse_table_identifier_test.go covers all 5 real-world ClickHouse queries from issue [BUG] multiple errors in ClickHouse dialect #480
   • Tests use proper ParseWithDialect(query, keywords.DialectClickHouse) with correct dialect specification
   • Follows project's safe type assertion patterns as recommended in CLAUDE.md:135-142
   • Test cases represent realistic ClickHouse system table queries (system.replicas, system.tables, system.parts)

3. Clean Implementation:

   • Proper error handling throughout the test cases
   • Clear, descriptive test names and comments explaining the purpose
   • Follows existing code patterns and conventions

📊 Technical Analysis:

Changes Made:

1. parser.go:962: Added TokenTypePartition to the token type switch case for non-reserved keywords
2. parser.go:972: Added "TABLES" to the string value switch case for keywords with generic TokenTypeKeyword
3. expressions_literal.go:106: Extended the dialect condition to include ClickHouse alongside SQL Server for non-reserved keyword usage
4. New test file: Comprehensive test coverage with 5 realistic ClickHouse queries

Scope Assessment:

• ✅ Focused: Unlike previous iterations, this PR is properly scoped to only the ClickHouse identifier issue
• ✅ No Scope Creep: No unrelated functionality mixed in
• ✅ Clean: Only the minimal necessary changes to solve the stated problem

🔒 Security Assessment: ✅ CLEAN

• No SQL injection vulnerabilities introduced
• No exposure of sensitive data or secrets
• Input validation remains intact via existing tokenizer bounds
• Changes only affect parsing logic, not execution or data handling

⚡ Performance Assessment: ✅ MINIMAL IMPACT

• Added one case to isNonReservedKeyword() switch - O(1) lookup with minimal overhead
• One additional string case for "TABLES" keyword - negligible impact
• No impact on the core zero-copy tokenization or object pooling systems
• Changes are in the parsing path, not the hot tokenization path

📝 Code Quality: ✅ EXCELLENT

• Readability: Clear comments explaining the purpose (parser.go:963-965)
• Maintainability: Changes are localized and easy to understand
• Consistency: Follows existing patterns in the codebase
• Documentation: Good inline comments explaining ClickHouse system table usage

🎯 Test Coverage: ✅ COMPREHENSIVE

• Positive Cases: All 5 problematic queries from issue [BUG] multiple errors in ClickHouse dialect #480 are covered
• Realistic Scenarios: Tests use actual ClickHouse system table patterns
• Proper Setup: Uses correct dialect specification
• Edge Cases: Covers different contexts where identifiers appear (SELECT, GROUP BY, function args, etc.)

⚠️ Minor Considerations:

1. Cross-Dialect Impact: The "TABLES" keyword is now treated as non-reserved across ALL dialects when it has TokenTypeKeyword. This is likely acceptable since it's only checked after dialect-specific type checks, but worth noting.

2. Missing Negative Tests: Could benefit from tests ensuring other SQL dialects still handle these keywords appropriately, but not critical for this fix.

---

Overall Assessment: ✅ EXCELLENT

This PR is a significant improvement over previous versions. It:

• Solves the exact problem stated in issue [BUG] multiple errors in ClickHouse dialect #480
• Maintains clean scope without unrelated features
• Provides comprehensive test coverage with realistic scenarios
• Follows project conventions and coding standards
• Has minimal performance impact
• Introduces no security risks

Recommendations:

1. ✅ Ready to Merge: This PR successfully addresses the ClickHouse identifier parsing issue with a clean, focused implementation.

2. Optional Future Enhancement: Consider adding negative test cases to verify other dialects still behave as expected, but this is not blocking.

3. Performance: No concerns - the changes are minimal and efficient.

Final Verdict:

APPROVE - This is a well-implemented fix that solves the stated problem cleanly and efficiently.

---

Job Run