Skip to content

feat(parser): ClickHouse bare-bracket array literals (#482)#485

Merged
ajitpratap0 merged 3 commits intomainfrom
feat/clickhouse-array-literal-482
Apr 7, 2026
Merged

feat(parser): ClickHouse bare-bracket array literals (#482)#485
ajitpratap0 merged 3 commits intomainfrom
feat/clickhouse-array-literal-482

Conversation

@ajitpratap0
Copy link
Copy Markdown
Owner

Summary

ClickHouse accepts `[1, 2, 3]` as a shorthand for `array(1, 2, 3)`. The parser previously only handled the PostgreSQL `ARRAY[...]` form.

  • New ClickHouse-gated branch in `parsePrimaryExpression` calling `parseBracketArrayLiteral`.
  • Helper mirrors the existing square-bracket path in `parseArrayConstructor`.

Test plan

  • New tests covering int/string/empty/nested literals, function args, and select-list usage
  • `go test -race ./pkg/sql/parser/ ./pkg/gosqlx/` green

Out of scope

  • `IN [1,2,3]` — requires the IN parser to also accept array literals; will be a follow-up.

Part of #482.

🤖 Generated with Claude Code

ClickHouse accepts `[1, 2, 3]` as a shorthand for `array(1, 2, 3)`. Add a
small parsePrimaryExpression branch (gated to the ClickHouse dialect to avoid
colliding with subscript usage in other dialects) that delegates to a new
parseBracketArrayLiteral helper. The helper mirrors the existing
parseArrayConstructor square-bracket path.

Part of #482 (ClickHouse parser gaps from QA sweep).
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 7, 2026

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

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

@github-actions github-actions bot added the parser label Apr 7, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

Claude finished @ajitpratap0's task —— View job


PR Review: ClickHouse Bare-Bracket Array Literals

I've completed my review of this PR. The implementation adds ClickHouse-specific support for bare-bracket array literals ([1, 2, 3]) as a shorthand for array(1, 2, 3).

Review Progress:

  • Read project guidelines and patterns
  • Analyze implementation in modified files
  • Review test coverage and quality
  • Check for potential bugs and issues
  • Verify performance considerations
  • Assess security implications
  • Provide comprehensive feedback

Strengths

Code Quality & Design:

  • Proper dialect gating: The feature is correctly restricted to ClickHouse dialect only (p.dialect == string(keywords.DialectClickHouse)) in expressions_literal.go:77, preventing conflicts with array subscript syntax in other dialects.
  • Memory management: Correctly uses object pooling with ast.GetArrayConstructor() in parseBracketArrayLiteral() following the project's performance patterns.
  • Code reuse: Smart implementation that mirrors the existing PostgreSQL ARRAY[...] parsing logic, minimizing code duplication.
  • Clear separation: New helper function parseBracketArrayLiteral() is well-isolated and focused on a single responsibility.

Test Coverage:

  • Comprehensive scenarios: Tests cover integers, strings, empty arrays, nested arrays, function arguments, and mixed column usage.
  • Proper test structure: Uses the high-level gosqlx.ParseWithDialect() API which tests the full parsing pipeline.
  • Good naming: Test cases are clearly named and cover the main use cases mentioned in the PR description.

🟡 Minor Improvements

Code Quality:

  1. Missing error context: In parseBracketArrayLiteral():346-347, the recursive p.parseExpression() call lacks error wrapping. Consider adding context:

    elem, err := p.parseExpression()
    if err != nil {
        return nil, goerrors.InvalidSyntaxError(
            fmt.Sprintf("failed to parse array element: %v", err),
            p.currentLocation(), "")
    }
  2. Consistent error messages: The error message at line 356 uses , or ] while the similar error in parseArrayConstructor():290 uses , or ]. Both are correct but maintaining exact consistency would be ideal.

Test Improvements:

  1. Dialect verification: While the current tests verify parsing succeeds, consider adding a test that verifies the feature is properly disabled in non-ClickHouse dialects:

    func TestClickHouseArrayLiteral_NonClickHouseDialect(t *testing.T) {
        // Should fail or be interpreted differently in PostgreSQL
        _, err := gosqlx.ParseWithDialect(`SELECT [1, 2, 3]`, keywords.DialectPostgreSQL)
        // Verify it's not parsed as ClickHouse array literal
    }
  2. AST validation: The current tests only verify parsing success. Consider adding validation of the actual AST structure to ensure ArrayConstructorExpression nodes are created correctly.


Security & Performance

Security:

  • No security concerns identified. The feature doesn't introduce any SQL injection vectors or unsafe operations.
  • Proper bounds checking and error handling prevent potential panics.

Performance:

  • Correctly uses object pooling (ast.GetArrayConstructor()) which is critical for the project's performance goals.
  • Implementation reuses existing parsing logic, maintaining the same performance characteristics as PostgreSQL array constructors.
  • No additional memory allocations beyond what's already optimized in the existing codebase.

Architecture & Integration

Proper Integration:

  • The feature integrates cleanly at the right level in parsePrimaryExpression() without affecting other parsing paths.
  • Dialect gating ensures the feature only activates for ClickHouse, preventing conflicts with array subscripting in other dialects.
  • Follows existing code patterns and maintains consistency with the codebase architecture.

Edge Case Handling:

  • Empty arrays ([]) are properly handled
  • Nested arrays ([[1, 2], [3, 4]]) work correctly through recursive expression parsing
  • Error handling follows the project's structured error patterns

🔍 Additional Verification Suggested

While I cannot execute tests directly, I recommend verifying:

  1. Race condition testing: Run task test:race to ensure the object pooling is thread-safe
  2. Performance regression: Run benchmarks to verify no performance impact on existing array parsing
  3. Cross-dialect testing: Verify that [1, 2, 3] still properly functions as array subscript syntax in PostgreSQL/MySQL dialects

📋 Summary

This is a well-implemented feature that:

  • ✅ Follows project patterns and coding standards
  • ✅ Includes appropriate test coverage
  • ✅ Uses proper memory management with object pooling
  • ✅ Correctly gates the feature to ClickHouse dialect only
  • ✅ Handles edge cases appropriately
  • ✅ Poses no security or performance concerns

The implementation is production-ready and aligns well with the project's architecture and quality standards. The minor suggestions above would enhance code consistency but don't block the feature.

Recommendation: ✅ APPROVE - This PR successfully implements ClickHouse bare-bracket array literals with good design, proper testing, and no significant issues.


Job Run

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

Claude finished @ajitpratap0's task —— View job


PR Review: ClickHouse Bare-Bracket Array Literals

I've completed my review of this PR. The implementation adds ClickHouse-specific support for bare-bracket array literals ([1, 2, 3]) as a shorthand for array(1, 2, 3).

Review Progress:

  • Read project guidelines and patterns
  • Analyze implementation in modified files
  • Review test coverage and quality
  • Check for potential bugs and issues
  • Verify performance considerations
  • Assess security implications
  • Provide comprehensive feedback

Strengths

Code Quality & Design:

  • Proper dialect gating: The feature is correctly restricted to ClickHouse dialect only (p.dialect == string(keywords.DialectClickHouse)) in expressions_literal.go:77, preventing conflicts with array subscript syntax in other dialects.
  • Memory management: Correctly uses object pooling with ast.GetArrayConstructor() in parseBracketArrayLiteral() following the project's performance patterns.
  • Code reuse: Smart implementation that mirrors the existing PostgreSQL ARRAY[...] parsing logic, minimizing code duplication.
  • Clear separation: New helper function parseBracketArrayLiteral() is well-isolated and focused on a single responsibility.

Test Coverage:

  • Comprehensive scenarios: Tests cover integers, strings, empty arrays, nested arrays, function arguments, and mixed column usage.
  • Proper test structure: Uses the high-level gosqlx.ParseWithDialect() API which tests the full parsing pipeline.
  • Good naming: Test cases are clearly named and cover the main use cases mentioned in the PR description.

🟡 Minor Improvements

Code Quality:

  1. Missing error context: In parseBracketArrayLiteral():346-347, the recursive p.parseExpression() call lacks error wrapping. Consider adding context:

    elem, err := p.parseExpression()
    if err != nil {
        return nil, goerrors.InvalidSyntaxError(
            fmt.Sprintf("failed to parse array element: %v", err),
            p.currentLocation(), "")
    }
  2. Consistent error messages: The error message at line 356 uses , or ] while the similar error in parseArrayConstructor():290 uses , or ]. Both are correct but maintaining exact consistency would be ideal.

Test Improvements:

  1. Dialect verification: While the current tests verify parsing succeeds, consider adding a test that verifies the feature is properly disabled in non-ClickHouse dialects:

    func TestClickHouseArrayLiteral_NonClickHouseDialect(t *testing.T) {
        // Should fail or be interpreted differently in PostgreSQL
        _, err := gosqlx.ParseWithDialect(`SELECT [1, 2, 3]`, keywords.DialectPostgreSQL)
        // Verify it's not parsed as ClickHouse array literal
    }
  2. AST validation: The current tests only verify parsing success. Consider adding validation of the actual AST structure to ensure ArrayConstructorExpression nodes are created correctly.


Security & Performance

Security:

  • No security concerns identified. The feature doesn't introduce any SQL injection vectors or unsafe operations.
  • Proper bounds checking and error handling prevent potential panics.

Performance:

  • Correctly uses object pooling (ast.GetArrayConstructor()) which is critical for the project's performance goals.
  • Implementation reuses existing parsing logic, maintaining the same performance characteristics as PostgreSQL array constructors.
  • No additional memory allocations beyond what's already optimized in the existing codebase.

Architecture & Integration

Proper Integration:

  • The feature integrates cleanly at the right level in parsePrimaryExpression() without affecting other parsing paths.
  • Dialect gating ensures the feature only activates for ClickHouse, preventing conflicts with array subscripting in other dialects.
  • Follows existing code patterns and maintains consistency with the codebase architecture.

Edge Case Handling:

  • Empty arrays ([]) are properly handled
  • Nested arrays ([[1, 2], [3, 4]]) work correctly through recursive expression parsing
  • Error handling follows the project's structured error patterns

🔍 Additional Verification Suggested

While I cannot execute tests directly, I recommend verifying:

  1. Race condition testing: Run task test:race to ensure the object pooling is thread-safe
  2. Performance regression: Run benchmarks to verify no performance impact on existing array parsing
  3. Cross-dialect testing: Verify that [1, 2, 3] still properly functions as array subscript syntax in PostgreSQL/MySQL dialects

📋 Summary

This is a well-implemented feature that:

  • ✅ Follows project patterns and coding standards
  • ✅ Includes appropriate test coverage
  • ✅ Uses proper memory management with object pooling
  • ✅ Correctly gates the feature to ClickHouse dialect only
  • ✅ Handles edge cases appropriately
  • ✅ Poses no security or performance concerns

The implementation is production-ready and aligns well with the project's architecture and quality standards. The minor suggestions above would enhance code consistency but don't block the feature.

Recommendation: ✅ APPROVE - This PR successfully implements ClickHouse bare-bracket array literals with good design, proper testing, and no significant issues.


Job Run

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

Claude finished @ajitpratap0's task —— View job


PR Review: ClickHouse Bare-Bracket Array Literals

I've completed my review of this PR. The implementation adds ClickHouse-specific support for bare-bracket array literals ([1, 2, 3]) as a shorthand for array(1, 2, 3).

Review Progress:

  • Read project guidelines and patterns
  • Analyze implementation in modified files
  • Review test coverage and quality
  • Check for potential bugs and issues
  • Verify performance considerations
  • Assess security implications
  • Provide comprehensive feedback

Strengths

Code Quality & Design:

  • Proper dialect gating: The feature is correctly restricted to ClickHouse dialect only (p.dialect == string(keywords.DialectClickHouse)) in expressions_literal.go:77, preventing conflicts with array subscript syntax in other dialects.
  • Memory management: Correctly uses object pooling with ast.GetArrayConstructor() in parseBracketArrayLiteral() following the project's performance patterns.
  • Code reuse: Smart implementation that mirrors the existing PostgreSQL ARRAY[...] parsing logic, minimizing code duplication.
  • Clear separation: New helper function parseBracketArrayLiteral() is well-isolated and focused on a single responsibility.

Test Coverage:

  • Comprehensive scenarios: Tests cover integers, strings, empty arrays, nested arrays, function arguments, and mixed column usage.
  • Proper test structure: Uses the high-level gosqlx.ParseWithDialect() API which tests the full parsing pipeline.
  • Good naming: Test cases are clearly named and cover the main use cases mentioned in the PR description.

🟡 Minor Improvements

Code Quality:

  1. Missing error context: In parseBracketArrayLiteral():346-347, the recursive p.parseExpression() call lacks error wrapping. Consider adding context:

    elem, err := p.parseExpression()
    if err != nil {
        return nil, goerrors.InvalidSyntaxError(
            fmt.Sprintf("failed to parse array element: %v", err),
            p.currentLocation(), "")
    }
  2. Consistent error messages: The error message at line 356 uses , or ] while the similar error in parseArrayConstructor():290 uses , or ]. Both are correct but maintaining exact consistency would be ideal.

Test Improvements:

  1. Dialect verification: While the current tests verify parsing succeeds, consider adding a test that verifies the feature is properly disabled in non-ClickHouse dialects:

    func TestClickHouseArrayLiteral_NonClickHouseDialect(t *testing.T) {
        // Should fail or be interpreted differently in PostgreSQL
        _, err := gosqlx.ParseWithDialect(`SELECT [1, 2, 3]`, keywords.DialectPostgreSQL)
        // Verify it's not parsed as ClickHouse array literal
    }
  2. AST validation: The current tests only verify parsing success. Consider adding validation of the actual AST structure to ensure ArrayConstructorExpression nodes are created correctly.


Security & Performance

Security:

  • No security concerns identified. The feature doesn't introduce any SQL injection vectors or unsafe operations.
  • Proper bounds checking and error handling prevent potential panics.

Performance:

  • Correctly uses object pooling (ast.GetArrayConstructor()) which is critical for the project's performance goals.
  • Implementation reuses existing parsing logic, maintaining the same performance characteristics as PostgreSQL array constructors.
  • No additional memory allocations beyond what's already optimized in the existing codebase.

Architecture & Integration

Proper Integration:

  • The feature integrates cleanly at the right level in parsePrimaryExpression() without affecting other parsing paths.
  • Dialect gating ensures the feature only activates for ClickHouse, preventing conflicts with array subscripting in other dialects.
  • Follows existing code patterns and maintains consistency with the codebase architecture.

Edge Case Handling:

  • Empty arrays ([]) are properly handled
  • Nested arrays ([[1, 2], [3, 4]]) work correctly through recursive expression parsing
  • Error handling follows the project's structured error patterns

🔍 Additional Verification Suggested

While I cannot execute tests directly, I recommend verifying:

  1. Race condition testing: Run task test:race to ensure the object pooling is thread-safe
  2. Performance regression: Run benchmarks to verify no performance impact on existing array parsing
  3. Cross-dialect testing: Verify that [1, 2, 3] still properly functions as array subscript syntax in PostgreSQL/MySQL dialects

📋 Summary

This is a well-implemented feature that:

  • ✅ Follows project patterns and coding standards
  • ✅ Includes appropriate test coverage
  • ✅ Uses proper memory management with object pooling
  • ✅ Correctly gates the feature to ClickHouse dialect only
  • ✅ Handles edge cases appropriately
  • ✅ Poses no security or performance concerns

The implementation is production-ready and aligns well with the project's architecture and quality standards. The minor suggestions above would enhance code consistency but don't block the feature.

Recommendation: ✅ APPROVE - This PR successfully implements ClickHouse bare-bracket array literals with good design, proper testing, and no significant issues.


Job Run

@ajitpratap0 ajitpratap0 merged commit 57ee598 into main Apr 7, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant