feat(parser): ClickHouse parametric aggregates (#482)

[ajitpratap0]

Summary

ClickHouse aggregate functions can take parameters before their arguments — `quantileTDigest(0.95)(value)`, `topK(10)(name)`, `quantiles(0.5, 0.9, 0.99)(latency_ms)`. The first paren group is parameters; the second is arguments.

• Add `Parameters []Expression` to `ast.FunctionCall`.
• After `parseFunctionCall` consumes the first arg list, if the dialect is ClickHouse and the next token is `(`, swap what we parsed into Parameters and consume a second arg list as the actual Arguments.

Test plan

• `TestClickHouseParametricAggregates`: 4 shapes including GROUP BY usage
• `go test -race ./pkg/sql/parser/ ./pkg/sql/ast/ ./pkg/gosqlx/` green

Part of #482.

🤖 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 11:53am

[github-actions]

Claude finished @ajitpratap0's task —— View job

---

PR Review: ClickHouse Parametric Aggregates

Reviewed this PR for code quality, potential issues, performance, security, and test coverage.

Review Findings

✅ Strengths

1. Clean AST Design: The Parameters []Expression field addition to FunctionCall is well-positioned and documented with a clear comment explaining its purpose.

2. Dialect-Gated Implementation: The ClickHouse parametric aggregate parsing is properly gated to DialectClickHouse, avoiding false positives in other dialects.

3. Good Test Coverage: Tests cover 4 key scenarios including basic parametric aggregates, multiple parameters, and GROUP BY usage.

4. Follows Project Patterns: Implementation follows established parser patterns and error handling conventions from the codebase.

❌ Critical Issue: Missing Parameters in AST Traversal

File: pkg/sql/ast/ast.go:706-723

The FunctionCall.Children() method does not include Parameters, which will break AST traversal and visitor pattern functionality:

func (f FunctionCall) Children() []Node {
    children := nodifyExpressions(f.Arguments) // Only Arguments, missing Parameters!
    if f.Over != nil {
        children = append(children, f.Over)
    }
    // ... rest of method
}

Required Fix: Add Parameters to the Children method:

func (f FunctionCall) Children() []Node {
    children := nodifyExpressions(f.Arguments)
    children = append(children, nodifyExpressions(f.Parameters)...) // Add this line
    if f.Over != nil {
        children = append(children, f.Over)
    }
    // ... rest stays the same
}

⚠️ Minor Issues

1. Test Coverage: While good, tests only verify parsing success. Consider adding tests that inspect the AST structure to ensure Parameters are correctly populated.

2. Documentation: The feature could benefit from a code comment in the parser explaining the ClickHouse parametric aggregate syntax for maintainability.

✅ Security & Performance

• Security: No security concerns identified. Dialect gating prevents unintended behavior.
• Performance: Minimal impact. Parser logic only activates for ClickHouse dialect and adds negligible overhead.
• Memory: Uses existing expression slices, no additional memory concerns.

📋 Recommendations

1. MUST FIX: Add Parameters to FunctionCall.Children() method
2. SHOULD ADD: Test cases that verify AST structure and Parameters field population
3. NICE TO HAVE: Add parser comment documenting the ClickHouse syntax

---

[github-actions]

Claude finished @ajitpratap0's task —— View job

---

PR Review: ClickHouse Parametric Aggregates

Reviewed this PR for code quality, potential issues, performance, security, and test coverage.

Todo List

• Read and analyze changed files
• Check code quality and implementation patterns
• Identify potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide comprehensive feedback

Review Findings

✅ Strengths

1. Clean AST Design: The Parameters []Expression field addition to FunctionCall is well-positioned and documented with a clear comment explaining its ClickHouse-specific purpose (pkg/sql/ast/ast.go:695).

2. Proper AST Traversal: Unlike what the previous automated review claimed, the FunctionCall.Children() method correctly includes Parameters in the traversal at pkg/sql/ast/ast.go:708-710:

   if len(f.Parameters) > 0 {
       children = append(children, nodifyExpressions(f.Parameters)...)
   }

3. Dialect-Gated Implementation: The ClickHouse parametric aggregate parsing is properly gated to DialectClickHouse (pkg/sql/parser/window.go:161), preventing false positives in other SQL dialects.

4. Excellent Test Coverage: The tests cover both parsing success and AST structure validation:

   • 4 realistic test cases including multi-parameter scenarios and GROUP BY usage
   • TestClickHouseParametricAggregates_ASTShape specifically validates that Parameters are accessible via the visitor pattern
   • Tests verify both Parameters and Arguments are populated correctly

5. Follows Project Patterns: Implementation follows established parser patterns, error handling conventions, and memory management from the codebase.

✅ Code Quality & Implementation

• Parser Logic: The two-phase parsing approach is elegant - first parse as normal function arguments, then if ClickHouse dialect and next token is (, swap to Parameters and parse real Arguments (pkg/sql/parser/window.go:157-177)
• Error Handling: Proper error propagation and expected token validation
• Documentation: Clear comments explaining the ClickHouse syntax and implementation approach

✅ Performance Considerations

• Minimal Impact: Parser logic only activates for ClickHouse dialect when a second ( is encountered
• Memory Efficiency: Reuses existing expression slice infrastructure, no additional memory overhead
• Zero Copy Principle: Maintains the library's zero-copy optimization principles

✅ Security Assessment

• Input Validation: Proper bounds checking and token validation
• Dialect Isolation: ClickHouse-specific behavior is properly isolated, preventing unintended parsing in other dialects
• No Injection Risks: Standard expression parsing used for both parameters and arguments

✅ Test Coverage Analysis

The test suite is comprehensive:

• Functional Tests: 4 different parametric aggregate patterns
• Structural Tests: AST visitor pattern validation ensures Parameters are reachable
• Integration: Tests work with the high-level gosqlx.ParseWithDialect() API

🔧 Minor Recommendations

1. Consider adding edge case tests: Empty parameters func()(args) or nested function calls as parameters
2. Documentation: The implementation is well-commented, but consider adding usage examples to the FunctionCall struct docs

Summary

This is a high-quality implementation that correctly adds ClickHouse parametric aggregate support. The previous automated review incorrectly flagged the AST traversal issue - the code properly includes Parameters in the Children() method. The implementation is clean, well-tested, secure, and follows the project's established patterns.

Recommendation: ✅ Approve - This PR is ready to merge.

---

[github-actions]

Claude finished @ajitpratap0's task —— View job

---

PR Review: ClickHouse Parametric Aggregates

Reviewing this PR for code quality, potential issues, performance, security, and test coverage.

Todo List

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Check code quality and best practices
• Identify potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide comprehensive feedback

---

Review Summary

This is a well-implemented feature that adds ClickHouse parametric aggregate support to the SQL parser. The implementation correctly handles the funcName(parameters)(arguments) syntax specific to ClickHouse.

✅ Strengths

1. Clean AST Design:

   • Added Parameters []Expression field to FunctionCall (pkg/sql/ast/ast.go:695)
   • Clear documentation comment explaining ClickHouse-specific usage
   • Properly integrated into the Children() method for AST traversal (pkg/sql/ast/ast.go:708-710)

2. Robust Parser Implementation (pkg/sql/parser/window.go:157-185):

   • Correctly gated to DialectClickHouse only, preventing false positives
   • Two-phase parsing: first parses normal function, then detects second ( and swaps to parametric mode
   • Proper error handling with expected token validation
   • Follows established project patterns for dialect-specific features

3. Comprehensive Test Coverage (pkg/sql/parser/clickhouse_parametric_test.go):

   • Tests 4 realistic scenarios including complex queries with GROUP BY
   • TestClickHouseParametricAggregates_ASTShape specifically validates AST structure
   • Tests verify both Parameters and Arguments are populated correctly
   • Uses visitor pattern to ensure proper AST traversal integration

4. Code Quality:

   • Follows project conventions for safe type assertions and error handling
   • Consistent with other ClickHouse dialect implementations in the codebase
   • No new dependencies or external interfaces introduced

✅ Security & Performance Assessment

• Security: ✅ No security concerns. Standard expression parsing used for both parameters and arguments
• Performance: ✅ Minimal overhead - only activates for ClickHouse dialect when second ( encountered
• Memory: ✅ Reuses existing expression slice infrastructure, no additional allocations

✅ Architecture Compliance

• Follows established patterns seen in other ClickHouse features (SAMPLE, FINAL, GLOBAL clauses)
• Maintains zero-copy principles and object pooling compatibility
• Integrates properly with visitor pattern for AST traversal
• No breaking changes to existing APIs

🔧 Minor Suggestions (Optional improvements)

1. Edge Case Testing: Consider adding tests for:

   • Empty parameters: func()(args)
   • Nested function calls as parameters: quantile(func())(value)

2. Error Message Enhancement: The parser uses generic "expected ')'" messages - could be more specific for parametric aggregates

📋 Technical Details Verified

• ✅ AST Children() method correctly includes Parameters for traversal
• ✅ Parser properly handles comma-separated parameter lists
• ✅ Dialect gating prevents conflicts with other SQL dialects
• ✅ Memory management patterns maintained (no pooling needed for slices)
• ✅ Error propagation follows established conventions

Final Recommendation

✅ APPROVE - This PR is well-designed, thoroughly tested, and ready for merge. The implementation correctly adds ClickHouse parametric aggregate support while maintaining code quality standards and following established project patterns.

---

[github-actions]

Claude finished @ajitpratap0's task —— View job

---

PR Review: ClickHouse Parametric Aggregates

Reviewing this PR for code quality, potential issues, performance, security, and test coverage.

Todo List

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Check code quality and best practices
• Identify potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide comprehensive feedback

---

Review Summary

This is a well-implemented feature that adds ClickHouse parametric aggregate support to the SQL parser. The implementation correctly handles the funcName(parameters)(arguments) syntax specific to ClickHouse.

✅ Strengths

1. Clean AST Design:

   • Added Parameters []Expression field to FunctionCall (pkg/sql/ast/ast.go:695)
   • Clear documentation comment explaining ClickHouse-specific usage
   • Properly integrated into the Children() method for AST traversal (pkg/sql/ast/ast.go:708-710)

2. Robust Parser Implementation (pkg/sql/parser/window.go:157-185):

   • Correctly gated to DialectClickHouse only, preventing false positives
   • Two-phase parsing: first parses normal function, then detects second ( and swaps to parametric mode
   • Proper error handling with expected token validation
   • Follows established project patterns for dialect-specific features

3. Comprehensive Test Coverage (pkg/sql/parser/clickhouse_parametric_test.go):

   • Tests 4 realistic scenarios including complex queries with GROUP BY
   • TestClickHouseParametricAggregates_ASTShape specifically validates AST structure
   • Tests verify both Parameters and Arguments are populated correctly
   • Uses visitor pattern to ensure proper AST traversal integration

4. Code Quality:

   • Follows project conventions for safe type assertions and error handling
   • Consistent with other ClickHouse dialect implementations in the codebase
   • No new dependencies or external interfaces introduced

✅ Security & Performance Assessment

• Security: ✅ No security concerns. Standard expression parsing used for both parameters and arguments
• Performance: ✅ Minimal overhead - only activates for ClickHouse dialect when second ( encountered
• Memory: ✅ Reuses existing expression slice infrastructure, no additional allocations

✅ Architecture Compliance

• Follows established patterns seen in other ClickHouse features (SAMPLE, FINAL, GLOBAL clauses)
• Maintains zero-copy principles and object pooling compatibility
• Integrates properly with visitor pattern for AST traversal
• No breaking changes to existing APIs

🔧 Minor Suggestions (Optional improvements)

1. Edge Case Testing: Consider adding tests for:

   • Empty parameters: func()(args)
   • Nested function calls as parameters: quantile(func())(value)

2. Error Message Enhancement: The parser uses generic "expected ')'" messages - could be more specific for parametric aggregates

📋 Technical Details Verified

• ✅ AST Children() method correctly includes Parameters for traversal
• ✅ Parser properly handles comma-separated parameter lists
• ✅ Dialect gating prevents conflicts with other SQL dialects
• ✅ Memory management patterns maintained (no pooling needed for slices)
• ✅ Error propagation follows established conventions

Final Recommendation

✅ APPROVE - This PR is well-designed, thoroughly tested, and ready for merge. The implementation correctly adds ClickHouse parametric aggregate support while maintaining code quality standards and following established project patterns.

---

[github-actions]

Claude finished @ajitpratap0's task —— View job

---

PR Review: ClickHouse Parametric Aggregates

Conducting a comprehensive code review of this PR that adds support for ClickHouse parametric aggregates.

Todo List

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Check code quality and best practices
• Identify potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide comprehensive feedback

---

Review Summary

This is a well-implemented feature that adds ClickHouse parametric aggregate support to the SQL parser. The implementation correctly handles the funcName(parameters)(arguments) syntax specific to ClickHouse.

✅ Strengths

1. Clean AST Design:

   • Added Parameters []Expression field to FunctionCall (pkg/sql/ast/ast.go:695)
   • Clear documentation comment explaining ClickHouse-specific usage
   • Properly integrated into the Children() method for AST traversal (pkg/sql/ast/ast.go:708-710)

2. Robust Parser Implementation (pkg/sql/parser/window.go:157-185):

   • Correctly gated to DialectClickHouse only, preventing false positives
   • Two-phase parsing: first parses normal function, then detects second ( and swaps to parametric mode
   • Proper error handling with expected token validation
   • Follows established project patterns for dialect-specific features

3. Comprehensive Test Coverage (pkg/sql/parser/clickhouse_parametric_test.go):

   • Tests 4 realistic scenarios including complex queries with GROUP BY
   • TestClickHouseParametricAggregates_ASTShape specifically validates AST structure
   • Tests verify both Parameters and Arguments are populated correctly
   • Uses visitor pattern to ensure proper AST traversal integration

4. Code Quality:

   • Follows project conventions for safe type assertions and error handling
   • Consistent with other ClickHouse dialect implementations in the codebase
   • No new dependencies or external interfaces introduced

✅ Security & Performance Assessment

• Security: ✅ No security concerns. Standard expression parsing used for both parameters and arguments
• Performance: ✅ Minimal overhead - only activates for ClickHouse dialect when second ( encountered
• Memory: ✅ Reuses existing expression slice infrastructure, no additional allocations

✅ Architecture Compliance

• Follows established patterns seen in other ClickHouse features (SAMPLE, FINAL, GLOBAL clauses)
• Maintains zero-copy principles and object pooling compatibility
• Integrates properly with visitor pattern for AST traversal
• No breaking changes to existing APIs

🔧 Minor Suggestions (Optional improvements)

1. Edge Case Testing: Consider adding tests for:

   • Empty parameters: func()(args)
   • Nested function calls as parameters: quantile(func())(value)

2. Error Message Enhancement: The parser uses generic "expected ')'" messages - could be more specific for parametric aggregates

📋 Technical Details Verified

• ✅ AST Children() method correctly includes Parameters for traversal
• ✅ Parser properly handles comma-separated parameter lists
• ✅ Dialect gating prevents conflicts with other SQL dialects
• ✅ Memory management patterns maintained (no pooling needed for slices)
• ✅ Error propagation follows established conventions

Final Recommendation

✅ APPROVE - This PR is well-designed, thoroughly tested, and ready for merge. The implementation correctly adds ClickHouse parametric aggregate support while maintaining code quality standards and following established project patterns.

---

[github-actions]

Claude finished @ajitpratap0's task —— View job

---

PR Review: ClickHouse Parametric Aggregates

Conducting a comprehensive code review of this PR that adds support for ClickHouse parametric aggregates.

Todo List

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Check code quality and best practices
• Identify potential bugs and issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide comprehensive feedback

---

Review Summary

This is a well-implemented feature that adds ClickHouse parametric aggregate support to the SQL parser. The implementation correctly handles the funcName(parameters)(arguments) syntax specific to ClickHouse.

✅ Strengths

1. Clean AST Design:

   • Added Parameters []Expression field to FunctionCall (pkg/sql/ast/ast.go:695)
   • Clear documentation comment explaining ClickHouse-specific usage
   • Properly integrated into the Children() method for AST traversal (pkg/sql/ast/ast.go:708-710)

2. Robust Parser Implementation (pkg/sql/parser/window.go:157-185):

   • Correctly gated to DialectClickHouse only, preventing false positives
   • Two-phase parsing: first parses normal function, then detects second ( and swaps to parametric mode
   • Proper error handling with expected token validation
   • Follows established project patterns for dialect-specific features

3. Comprehensive Test Coverage (pkg/sql/parser/clickhouse_parametric_test.go):

   • Tests 4 realistic scenarios including complex queries with GROUP BY
   • TestClickHouseParametricAggregates_ASTShape specifically validates AST structure
   • Tests verify both Parameters and Arguments are populated correctly
   • Uses visitor pattern to ensure proper AST traversal integration

4. Code Quality:

   • Follows project conventions for safe type assertions and error handling
   • Consistent with other ClickHouse dialect implementations in the codebase
   • No new dependencies or external interfaces introduced

✅ Security & Performance Assessment

• Security: ✅ No security concerns. Standard expression parsing used for both parameters and arguments
• Performance: ✅ Minimal overhead - only activates for ClickHouse dialect when second ( encountered
• Memory: ✅ Reuses existing expression slice infrastructure, no additional allocations

✅ Architecture Compliance

• Follows established patterns seen in other ClickHouse features (SAMPLE, FINAL, GLOBAL clauses)
• Maintains zero-copy principles and object pooling compatibility
• Integrates properly with visitor pattern for AST traversal
• No breaking changes to existing APIs

🔧 Minor Suggestions (Optional improvements)

1. Edge Case Testing: Consider adding tests for:

   • Empty parameters: func()(args)
   • Nested function calls as parameters: quantile(func())(value)

2. Error Message Enhancement: The parser uses generic "expected ')'" messages - could be more specific for parametric aggregates

📋 Technical Details Verified

• ✅ AST Children() method correctly includes Parameters for traversal
• ✅ Parser properly handles comma-separated parameter lists
• ✅ Dialect gating prevents conflicts with other SQL dialects
• ✅ Memory management patterns maintained (no pooling needed for slices)
• ✅ Error propagation follows established conventions

Final Recommendation

✅ APPROVE - This PR is well-designed, thoroughly tested, and ready for merge. The implementation correctly adds ClickHouse parametric aggregate support while maintaining code quality standards and following established project patterns.

---