feat(parser): ClickHouse ORDER BY … WITH FILL (#482)

[ajitpratap0]

Summary

Fixes a ClickHouse parser gap where `ORDER BY expr WITH FILL` was mis-routed to the WITH-CTE parser.

What now parses

• `ORDER BY id WITH FILL`
• `ORDER BY id WITH FILL STEP 1`
• `ORDER BY day WITH FILL FROM '2024-01-01' TO '2024-12-31' STEP INTERVAL 1 DAY`
• `ORDER BY id DESC WITH FILL STEP 1`
• Multiple columns with per-item WITH FILL

Implementation

• Per-entry WITH FILL detection inside `parseOrderByClause` after the NULLS clause. Gated to ClickHouse.
• New `skipClickHouseWithFillTail` helper consumes the optional `FROM expr`, `TO expr`, `STEP expr` arguments in any order.
• Permissive: the FILL modifier is consumed but not yet modeled on the AST (follow-up).

The bug was that after parsing the ORDER BY items, the parser returned to the statement dispatcher which saw `WITH` and dispatched to the WITH-CTE parser. Consuming the tail inside the ORDER BY clause prevents that.

Test plan

• `TestClickHouseOrderByWithFill`: 5 shapes
• `go test -race ./pkg/sql/parser/ ./pkg/gosqlx/ ./pkg/sql/ast/` 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 8, 2026 1:42pm

[github-actions]

Claude finished @ajitpratap0's task —— View job

---

Code Review: ClickHouse ORDER BY ... WITH FILL Implementation

Review Tasks

• Analyze implementation in select_clauses.go
• Review test coverage in clickhouse_with_fill_test.go
• Check code quality and best practices
• Evaluate performance and security considerations
• Provide comprehensive feedback

Summary

This PR successfully implements ClickHouse ORDER BY ... WITH FILL syntax parsing by fixing a routing issue where WITH FILL was incorrectly handled by the WITH-CTE parser. The implementation is clean, well-documented, and follows project conventions.

✅ Strengths

Code Quality & Best Practices

• Excellent documentation: Clear comments explaining ClickHouse-specific behavior and permissive parsing approach
• Proper dialect gating: Implementation correctly restricts WITH FILL to ClickHouse dialect only (select_clauses.go:554)
• Follows project patterns: Consistent with existing ClickHouse-specific features like FINAL and GLOBAL JOIN
• Clean separation: New functionality isolated in dedicated helper method skipClickHouseWithFillTail()

Implementation Approach

• Correct fix: Addresses the core issue by consuming WITH FILL tokens within ORDER BY parsing instead of letting them fall through to CTE parser
• Permissive parsing: Smart design choice to consume syntax without full AST modeling, enabling forward compatibility
• Flexible argument parsing: Handles FROM/TO/STEP in any order (select_clauses.go:576-590)
• Proper error handling: Uses graceful error recovery in skipClickHouseWithFillTail() to handle complex expressions

Test Coverage

• Comprehensive scenarios: Tests cover all major use cases including basic FILL, STEP modifier, FROM/TO ranges, multiple columns, and DESC ordering
• Proper test structure: Uses table-driven test pattern consistent with codebase standards
• Dialect-specific testing: Correctly uses keywords.DialectClickHouse for testing

⚠️ Areas for Consideration

AST Modeling (Future Enhancement)

The PR correctly notes that WITH FILL modifiers are not yet modeled in the AST. While this is acceptable for the current fix, consider:

• Adding a WithFill *FillClause field to OrderByExpression in a follow-up
• Ensuring the current implementation can be extended without breaking changes

Edge Case Testing

Consider adding tests for:

// Mixed FILL and non-FILL columns
"SELECT * FROM t ORDER BY a WITH FILL, b DESC, c WITH FILL STEP 1"

// Complex expressions with FILL
"SELECT * FROM t ORDER BY date_trunc('day', ts) WITH FILL STEP INTERVAL 1 DAY"

// Error cases (if desired)
"SELECT * FROM t ORDER BY id WITH FILL STEP" // Missing step value

Performance Considerations

• ✅ Minimal overhead: Implementation only activates for ClickHouse dialect
• ✅ Efficient parsing: Uses simple string comparisons and single-pass token consumption
• ✅ No memory allocations: Uses existing parser infrastructure without additional allocations

🔒 Security Analysis

• ✅ No injection risks: Implementation only parses syntax tokens, doesn't execute or evaluate expressions
• ✅ Input validation: Proper bounds checking in token advancement
• ✅ Error containment: Parse errors are gracefully handled without exposing internal state

🧪 Test Quality Assessment

The test implementation is solid:

• Uses project's testing patterns (gosqlx.ParseWithDialect)
• Covers realistic ClickHouse scenarios
• Includes complex expressions like INTERVAL 1 DAY
• Tests interaction with other ORDER BY features (DESC, multiple columns)

📋 Recommendations

1. Consider adding negative tests: Test that WITH FILL is rejected in non-ClickHouse dialects
2. Document future AST work: Add a TODO comment referencing the follow-up issue for AST modeling
3. Benchmark impact: Consider adding a simple benchmark to ensure no performance regression

🎯 Overall Assessment

✅ Recommended for merge

This is a well-crafted fix that:

• Solves the stated problem correctly
• Follows all project conventions
• Includes appropriate test coverage
• Uses a forward-compatible design
• Has minimal performance impact
• Introduces no security concerns

The permissive parsing approach is particularly smart, allowing the parser to handle ClickHouse's WITH FILL syntax today while leaving room for full AST modeling in the future.

---

Job Run: View CI Results