Skip to content

Conversation

git-hulk
Copy link
Member

This closes #187.

Before ClickHouse supported JSON type, the select result should have at most 3 fields(db.table.column), so it's good to represent it with ColumnIdentifier.

After introducing JSON type, the JSON path might have more 3 levels, and it's weird
to represent the JSON path with db.table.column. So this PR introduces Path for this
purpose. And yes, it will break user behaviors if using ColumnIdentifier.

@coveralls
Copy link

coveralls commented Sep 11, 2025

Pull Request Test Coverage Report for Build 17633065282

Details

  • 20 of 34 (58.82%) changed or added relevant lines in 4 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.04%) to 50.425%

Changes Missing Coverage Covered Lines Changed/Added Lines %
parser/ast_visitor.go 0 1 0.0%
parser/walk.go 3 5 60.0%
parser/ast.go 10 21 47.62%
Files with Coverage Reduction New Missed Lines %
parser/ast.go 1 39.5%
parser/parser_table.go 1 66.23%
Totals Coverage Status
Change from base Build 17367436417: 0.04%
Covered Lines: 7416
Relevant Lines: 14707

💛 - Coveralls

@git-hulk git-hulk requested review from Lance726 and Copilot and removed request for Copilot September 11, 2025 03:24
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Replaces the ColumnIdentifier type with a new Path type to support JSON paths with more than 3 levels in ClickHouse select results. This change enables handling of nested JSON fields like a.b.c.d.e while maintaining compatibility with existing database.table.column paths.

  • Introduces new Path type with a Fields array to handle arbitrary-length dot-separated paths
  • Removes the ColumnIdentifier type that was limited to database.table.column structure
  • Updates the AST walker, visitor pattern, and parser to support the new Path type

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
parser/walk.go Adds Path case to AST walker and removes ColumnIdentifier case
parser/ast_visitor.go Updates visitor interface to replace VisitColumnIdentifier with VisitPath
parser/ast.go Replaces ColumnIdentifier struct with new Path struct implementation
parser/parser_table.go Updates parser logic to create Path objects instead of ColumnIdentifier
parser/testdata/ Adds test cases and updates golden files to reflect the new Path structure
Comments suppressed due to low confidence (1)

parser/walk.go:1

  • The condition logic is inverted - this should return false when Walk() returns false, not true. This will cause the walker to incorrectly stop traversal when it should continue.
package parser

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Collaborator

@Lance726 Lance726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@git-hulk git-hulk merged commit 294aafb into master Sep 11, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CAST with dotted identifiers of more than 3 parts fails

3 participants