Skip to content

feat(parser): add DDL formatter, CONNECT BY, and SAMPLE clause (#450 #454 #455)#472

Merged
ajitpratap0 merged 3 commits intomainfrom
feat/sql-parser-additions
Mar 29, 2026
Merged

feat(parser): add DDL formatter, CONNECT BY, and SAMPLE clause (#450 #454 #455)#472
ajitpratap0 merged 3 commits intomainfrom
feat/sql-parser-additions

Conversation

@ajitpratap0
Copy link
Copy Markdown
Owner

Summary

Note

PIVOT/UNPIVOT (#456) is deferred to a follow-up PR due to complexity — it requires significant FROM-clause parser restructuring.

Test plan

  • All existing parser tests pass (verified locally)
  • All existing formatter tests pass (verified locally)
  • Pre-commit hooks pass (fmt, vet, test -short)
  • go test -race ./pkg/... -timeout 120s

🤖 Generated with Claude Code

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 29, 2026

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

Project Deployment Actions Updated (UTC)
gosqlx-website Ready Ready Preview, Comment Mar 29, 2026 11:38am

@github-actions github-actions bot added parser ast AST node changes labels Mar 29, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 29, 2026

Claude finished @ajitpratap0's task —— View job


PR Review Complete ✅

This pull request implements three significant SQL parsing features with high quality code. Here's my comprehensive review:

Review Summary:

  • Read repository guidelines and setup
  • Review DDL formatter changes (pkg/formatter/render.go and pkg/formatter/render_ddl.go)
  • Review Oracle CONNECT BY implementation (pkg/sql/parser/select.go and related files)
  • Review ClickHouse SAMPLE clause implementation
  • Review AST changes (pkg/sql/ast/ast.go)
  • Review test coverage and quality
  • Check for potential bugs and security issues
  • Assess performance implications
  • Provide comprehensive feedback

APPROVAL - High Quality Implementation

Strengths:

1. Code Quality & Architecture (Excellent)

  • Proper separation of concerns: New DDL formatters in dedicated file render_ddl.go
  • Consistent patterns: All changes follow existing codebase conventions and patterns
  • Clean interfaces: New AST nodes properly implement Node, Statement, and Expression interfaces
  • Good error handling: Proper error messages with context and location information

2. AST Design (Very Good)

  • Well-documented: Excellent documentation with examples in ast.go:2027-2055 for SampleClause
  • Proper Children() implementations: All nodes correctly implement tree traversal
  • Memory efficient: Follows existing pointer patterns for optional fields
  • Backward compatible: No breaking changes to existing AST structure

3. Parser Implementation (Strong)

  • Multi-dialect support: CONNECT BY correctly supports both Oracle and MariaDB (mariadb.go:34-48)
  • Robust validation: ClickHouse SAMPLE clause properly validates numeric literals
  • Proper clause ordering: Correctly integrates into SELECT statement parsing flow
  • Good helper functions: isMariaDBClauseStart() prevents alias parsing conflicts

4. Test Coverage (Good)

  • Comprehensive test scenarios: Tests cover basic, NOCYCLE, Oracle vs MariaDB variants
  • Safe type assertions: Uses two-value form stmt, ok := tree.Statements[0].(*ast.SelectStatement) as per repo guidelines
  • Multiple test files: Dedicated parser_connectby_test.go and formatter tests
  • Edge case coverage: Tests CONNECT BY without START WITH, different PRIOR positions

5. Formatter Integration (Very Good)

  • Keyword casing consistency: Uses nodeFormatter properly for configurable keyword casing
  • Proper clause separators: Respects formatting options like NewlinePerClause
  • Complete coverage: All new DDL statements have dedicated formatter handlers
  • Fallback handling: Maintains existing stmtSQL() fallback for unknown types

Minor Observations:

Performance Considerations ⚠️

  • Memory allocation: New string building in formatters is appropriate for the use case
  • Parser complexity: Added dialect checks have minimal performance impact
  • Object pooling: Changes don't affect existing pooling strategies

Security Assessment

  • No injection vectors: All user input properly tokenized before parsing
  • Safe string operations: No dynamic SQL construction in new code
  • Proper validation: Input validation prevents malformed AST construction

Technical Excellence:

  1. Adherence to CLAUDE.md guidelines: ✅

    • Follows Go 1.26+ requirements
    • Implements proper error handling with position tracking
    • Uses safe type assertions in tests
    • Maintains thread-safety patterns
  2. SQL Standard Compliance: ✅

    • Oracle CONNECT BY follows Oracle syntax specifications
    • ClickHouse SAMPLE implements correct ClickHouse syntax
    • DDL formatters maintain SQL-99 compliance where applicable
  3. Code Maintainability: ✅

    • Clear function names and documentation
    • Proper separation of dialect-specific logic
    • Consistent error message format
    • Good code organization

Recommendation: ✅ APPROVE

This PR demonstrates excellent software engineering practices:

  • High code quality with proper architecture and patterns
  • Comprehensive test coverage with good edge case handling
  • Backward compatibility with no breaking changes
  • Performance conscious implementation
  • Security aware with proper input validation

The implementation correctly handles the complexity of multi-dialect SQL parsing while maintaining the codebase's high standards for performance, thread-safety, and maintainability.


Ajit Pratap Singh and others added 3 commits March 29, 2026 17:07
…RIBE statements (#455)

Add dedicated FormatStatement case arms and render functions for
CreateSequenceStatement, AlterSequenceStatement, DropSequenceStatement,
ShowStatement, and DescribeStatement so they produce proper SQL output
instead of falling back to stmtSQL/TokenLiteral.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…upport (#450)

Extend the MariaDB CONNECT BY / START WITH hierarchical query parser to also
activate for Oracle dialect. Update isMariaDBClauseStart() to guard table-alias
parsing for both dialects, preventing START WITH / CONNECT BY from being
misidentified as aliases.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Parse SAMPLE <ratio|count|N/D> [OFFSET N/D] after FROM in ClickHouse
dialect. Adds SampleClause AST node and sampleSQL formatter function.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ajitpratap0 ajitpratap0 force-pushed the feat/sql-parser-additions branch from b65dfa9 to ceb3eca Compare March 29, 2026 11:38
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 29, 2026

Claude finished @ajitpratap0's task —— View job


Pull Request Review ✅

I've conducted a comprehensive review of this PR that adds DDL formatter, Oracle CONNECT BY, and ClickHouse SAMPLE clause support.

Review Progress:

  • Read repository guidelines and setup
  • Review DDL formatter changes
  • Review Oracle CONNECT BY implementation
  • Review ClickHouse SAMPLE clause implementation
  • Review AST changes
  • Review test coverage and quality
  • Check for potential bugs and security issues
  • Assess performance implications
  • Provide comprehensive feedback

APPROVAL - High Quality Implementation

This pull request implements three significant SQL parsing features with excellent code quality and follows all repository guidelines. Here's my detailed analysis:

Strengths:

1. Code Quality & Architecture (Excellent)

  • Proper separation of concerns: New DDL formatters cleanly separated in render_ddl.go:15-177
  • Consistent patterns: All changes follow existing codebase conventions perfectly
  • Clean interfaces: New AST nodes properly implement Node, Statement, and Expression interfaces
  • Error handling: Proper error messages with context and position information (select_clauses.go:318-323)

2. AST Design (Very Good)

  • Well-documented: Excellent documentation with examples in ast.go:2027-2055 for SampleClause
  • Proper Children() implementations: All nodes correctly implement tree traversal (ast.go:2020-2025, ast.go:2054)
  • Memory efficient: Follows existing pointer patterns for optional fields
  • Backward compatible: No breaking changes to existing AST structure

3. Parser Implementation (Strong)

  • Multi-dialect support: CONNECT BY correctly supports both Oracle and MariaDB (select.go:120-156, mariadb.go:34-48)
  • Robust validation: ClickHouse SAMPLE clause properly validates numeric literals (select_clauses.go:329-336)
  • Proper clause ordering: Correctly integrates into SELECT statement parsing flow
  • Smart helper functions: isMariaDBClauseStart() prevents alias parsing conflicts (mariadb.go:34-48)

4. Test Coverage (Good)

  • Comprehensive scenarios: Tests cover basic, NOCYCLE, Oracle vs MariaDB variants (parser_connectby_test.go:25-107)
  • Safe type assertions: Uses two-value form as per repo guidelines (parser_connectby_test.go:35-37)
  • Multiple test files: Dedicated test files for each feature
  • Edge case coverage: Tests CONNECT BY without START WITH, different PRIOR positions

5. Formatter Integration (Very Good)

  • Keyword casing consistency: Uses nodeFormatter properly for configurable keyword casing (render.go:42-52)
  • Proper clause separators: Respects formatting options like NewlinePerClause (render.go:66-72)
  • Complete coverage: All new DDL statements have dedicated formatter handlers (render.go:157-166)
  • Fallback handling: Maintains existing stmtSQL() fallback for unknown types (render.go:168-170)

Technical Assessment:

Security

  • No injection vectors: All user input properly tokenized before parsing
  • Safe string operations: No dynamic SQL construction in new code
  • Proper validation: Input validation prevents malformed AST construction

Performance

  • Minimal overhead: New dialect checks have negligible performance impact
  • Memory conscious: String building in formatters is appropriate for use case
  • Object pooling: Changes don't interfere with existing pooling strategies

Thread Safety

  • Stateless operations: All new code maintains thread-safety patterns
  • No shared mutable state: Parser state remains per-instance
  • Race-free: Follows existing patterns that pass race detection

Error Handling

  • Position tracking: All errors include proper location information
  • Clear messages: Error messages are descriptive and actionable
  • Graceful degradation: Proper fallback mechanisms in place

Minor Observations:

  1. Code organization: The separation of DDL formatters into render_ddl.go is excellent architectural decision
  2. Documentation quality: Inline comments are thorough and helpful
  3. Test naming: Test function names clearly describe what they're testing
  4. Dialect handling: Multi-dialect logic is well-structured and maintainable

Compliance with Repository Guidelines:

  • ✅ Follows Go 1.26+ requirements
  • ✅ Implements proper error handling with position tracking
  • ✅ Uses safe type assertions in tests (two-value form)
  • ✅ Maintains thread-safety patterns
  • ✅ Follows SQL standard compliance patterns
  • ✅ Proper separation of concerns across modules

Recommendation: ✅ APPROVE

This PR demonstrates exceptional software engineering practices:

  • High-quality implementation with proper architecture
  • Comprehensive feature coverage for all three SQL features
  • Excellent test coverage with edge case handling
  • Performance-conscious design with minimal overhead
  • Security-aware implementation with proper validation
  • Maintainable code following established patterns

The implementation correctly handles the complexity of multi-dialect SQL parsing while maintaining the codebase's high standards for performance, thread-safety, and maintainability. All three features (DDL formatting, CONNECT BY, SAMPLE clause) are production-ready.


@ajitpratap0 ajitpratap0 merged commit 4d96d5c into main Mar 29, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ast AST node changes parser

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant