Skip to content

Conversation

whscout
Copy link
Contributor

@whscout whscout commented Oct 11, 2025

Adjust the printing order of the EngineExpr.String() method, as placing Settings before Order BY can result in SQL execution errors

eg: CREATE TABLE IF NOT EXISTS test(id Int64, name String, age Int8, timestamp Int64) ENGINE = MergeTree() PARTITION BY toYYYYMMDD(toDateTime(timestamp / 1000)) SETTINGS index_granularity=100 ORDER BY (name, timestamp)
errors info:
Syntax error: failed at position 190 (ORDER):

CREATE table IF NOT EXISTS test(id Int64, name String, age Int8, timestamp Int64) ENGINE = MergeTree() PARTITION BY toYYYYMMDD(toDateTime(timestamp / 1000)) SETTINGS index_granularity=100 ORDER BY (name, timestamp)

Expected one of: EMPTY AS, CLONE AS, AS, COMMENT, INTO OUTFILE, FORMAT, SETTINGS, ParallelWithClause, PARALLEL WITH, end of query

…placing Settings before Order BY can result in SQL execution errors
@git-hulk
Copy link
Member

@whscout Thanks for your great fix. Could you please run make update_test to update the test gold files?

@git-hulk git-hulk self-requested a review October 11, 2025 02:50
@git-hulk git-hulk added the bug Something isn't working label Oct 11, 2025
@whscout
Copy link
Contributor Author

whscout commented Oct 11, 2025

@git-hulk It has been updated.

@git-hulk
Copy link
Member

@whscout seems you have changed other codes in your local? Because your make update_test result is not aligned with CI.

@whscout
Copy link
Contributor Author

whscout commented Oct 11, 2025

@git-hulk I have tried both versions of Go, go 1.25.0 and go 1.18.10, and there is no change in make update_test. I don't know where the problem lies. Can you try it on your local environment
os: ubuntu-latest
goVer:1.18.10

@whscout
Copy link
Contributor Author

whscout commented Oct 11, 2025

@git-hulk All local tests can pass
image

@git-hulk git-hulk changed the title fix: Adjust the printing order of the EngineExpr.String() method Fix the printing order of the EngineExpr.String() method Oct 11, 2025
@git-hulk git-hulk merged commit 61525d5 into AfterShip:master Oct 11, 2025
2 checks passed
@coveralls
Copy link

Pull Request Test Coverage Report for Build 18425097851

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 50.299%

Totals Coverage Status
Change from base Build 18338311265: 0.0%
Covered Lines: 7404
Relevant Lines: 14720

💛 - Coveralls

whscout added a commit to whscout/clickhouse-sql-parser that referenced this pull request Oct 13, 2025
Fix the printing order of the EngineExpr.String() method (AfterShip#201)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants