Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ClickHouse dialect #193

Closed
Shlomixg opened this issue Jun 9, 2022 · 6 comments
Closed

Add support for ClickHouse dialect #193

Shlomixg opened this issue Jun 9, 2022 · 6 comments
Labels
enhancement New feature or request
Milestone

Comments

@Shlomixg
Copy link

Shlomixg commented Jun 9, 2022

Describe the bug
sqlfmt doesn't support the ClickHouse dialect which is case sensitive.

To Reproduce
input code:
SELECT toString(1) AS Test_string, toDateTime64('2022-05-25', 3) AS Test_dateTime64, ifNull(null, 'TestNull') as testIf, JSONExtractString('{"abc": "hello"}', 'abc') as testJSON

Expected behavior
select toString(1) as Test_string, toDateTime64('2022-05-25', 3) as Test_dateTime64, ifNull(null, 'TestNull') as testIf, JSONExtractString('{"abc": "hello"}', 'abc') as testJSON

Actual behavior
select tostring(1) as test_string, todatetime64('2022-05-25', 3) as test_datetime64, ifnull(NULL, 'TestNull') as testif, jsonextractstring('{"abc": "hello"}', 'abc') as testjson
this output is not a valid ClickHouse code

Additional context
What is the output of sqlfmt --version?
sqlfmt, version 0.9.0

@tconbeer
Copy link
Owner

tconbeer commented Jun 9, 2022

Thanks for the report, @Shlomixg !

I had no idea ClickHouse was case sensitive. (Why would they do that to us?!)

Should be easy enough to not lowercase the output, but...

Are SQL keywords (like select) also case sensitive, or is it just some/all function names?

I designed this with multiple dialects in mind, so it shouldn't be too bad to change the parsing logic to be case-sensitive. I'm just a little sad because we've gotten away with only one "polyglot" dialect so far.

@Shlomixg
Copy link
Author

Shlomixg commented Jun 9, 2022

Thanks for your quick response!

I think only the CH unique functions are case-sensitive, meaning that select isn't case-sensitive.
You can find more information in the docs.
Note that aliases are also case-sensitive.

@tconbeer
Copy link
Owner

tconbeer commented Jun 9, 2022

Awesome, thank you for the extra info. That should make this pretty straightforward. We don't have to touch parsing and can still lowercase lots of tokens- we'll just leave identifiers (incl. function names) alone.

CLI flag should be --dialect clickhouse

@tconbeer tconbeer added enhancement New feature or request good first issue Good for newcomers labels Jun 9, 2022
@tconbeer tconbeer added this to the v0.10.0 milestone Jun 9, 2022
@tconbeer tconbeer removed the good first issue Good for newcomers label Jun 16, 2022
@tconbeer
Copy link
Owner

Implementation notes (this is bigger than I thought, to get the Dialect or Mode into the Node.capitalize() method):

  • Add Dialect as a property of Analyzer
  • Create a NodeManager (NodeBuilder?) class with Dialect as a property
  • instantiate a NodeManager where needed in actions.py
  • move Node.from_token() to NodeManager.create_node_from_token(); also move Node.capitalize() and Node.whitespace()
  • update NodeManager.capitalize() to check for the Clickhouse dialect
  • update Line.append_newline() so it doesn't use Node.from_token()

@tconbeer
Copy link
Owner

tconbeer commented Aug 2, 2022

@Shlomixg I'm cutting the release for 0.10.0 right now -- if you upgrade, you can run that version with sqlfmt . --dialect clickhouse and we won't lowercase so many things. Would love to get any more feedback from you!

@Shlomixg
Copy link
Author

Shlomixg commented Aug 3, 2022

Bravo :)
I'll test it soon and leave feedback.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants