[WIP] Add preliminary Clickhouse formatter

[mattbasta]

Addresses #614

I've started putting together a Clickhouse formatter. Before I start getting into the weeds with tests and updating all of the docs/tests/playground, I wanted to check that things are directionally correct. Please let me know if there's anything you'd like to see done differently before I proceed.

[nene]

Looks mostly in the right direction, but I spotted some issues.

Most glaringly the fact that formatting of SELECT clauses has been completely neglected.

[nene]

I really would encourage you to add tests as early on as possible. You can start with just:

import { format as originalFormat, FormatFn } from '../src/sqlFormatter.js';
import behavesLikeSqlFormatter from './behavesLikeSqlFormatter.js';

describe('ClickhouseFormatter', () => {
  const language = 'clickhouse';
  const format: FormatFn = (query, cfg = {}) => originalFormat(query, { ...cfg, language });

  behavesLikeSqlFormatter(format);
  // or maybe (I'm unsure how similar exactly Clickhouse is to PostgreSQL)
  // behavesLikePostgresqlFormatter(format);
});

That will make sure your configuration will work for the basic stuff that should be the same in all SQL dialects.

If any of these bahavesLikeSqlFormatter() tests fail, then I would ask you to fix the problem anyway. It's fairly unlikely that there's something in Clickhouse dialect that would necessitate a change to these core tests.

[nene]

Also, it would be of great help if you went through the wiki and added information there about the Clickhouse dialect.

Especially these first few pages about Identifiers, Parameters, ... Comments.

Doing that will also help you to get these details right about Clickhouse dialect.

[mattbasta]

@nene I appreciate you taking the time to look! I'm very aware that it's not nearly ready to land and that it needs quite a bit of love, I just didn't want to invest a ton more time if it's completely off the mark. I have a commit in progress with the tests; I'm trying to aim for nearly all of the example queries from the docs to get handled correctly. I'll ping you when I have something that's ready for review.

[mattbasta]

Hey @nene, I've been making a lot of progress getting things in order. The code is in a much better place, but I wanted to pause before continuing to ask for your opinion, because I don't know that there's an obvious right answer to what I'm running into. There's two components to this, which are intertwined.

---

The first part is that Clickhouse lets you drop/alter multiple resources at once. For instance:

DROP TABLE foo, bar;

would be valid according to https://clickhouse.com/docs/sql-reference/statements/drop#drop-table

However, with DROP TABLE appearing as a tabular one-line clause, this would be formatted as

DROP TABLE foo,
bar;

which doesn't seem correct. It would seem to me that the more appropriate choice would be to make DROP TABLE a reserved clause, which would format it like this instead:

DROP TABLE
  foo,
  bar

This makes it consistent with similar syntax, like with ORDER BY. In the simplest case where these are just table identifiers, this doesn't feel great, especially when a simple DROP TABLE statement ends up looking like

DROP TABLE
  foo

To my knowledge, there's no way to make a clause conditionally tabular, unless I'm missing something. On one hand, this could be made consistent with the other dialects and the feature tests would pass, but the multi-resource case would be pretty unfortunate. On the other hand, I could break convention with the other formatters and use a reserved clause here, and have a slightly less pleasant single-resource case that looks solid with multiple resources.

As a note, SELECT without a WHERE/FROM/etc. looks like the second case:

-- SELECT foo
SELECT
  foo

So this isn't completely unprecedented cosmetically.

My personal preference is the second option, but I can also appreciate that you'd want the different formatters to behave consistently.

---

The second piece is essentially the same issue. Consider this statement:

ALTER ROW POLICY IF EXISTS policy1 ON CLUSTER cluster_name1 ON database1.table1 RENAME TO new_name1, policy2 ON CLUSTER cluster_name2 ON database2.table2 RENAME TO new_name2;

RENAME TO behaves sort of like an infix operator here, and I think that this statement looks great when it's considered a keyword phrase, with ALTER ROW POLICY treated as a reserved clause:

ALTER ROW POLICY IF EXISTS
  policy1 ON CLUSTER cluster_name1 ON database1.table1 RENAME TO new_name1,
  policy2 ON CLUSTER cluster_name2 ON database2.table2 RENAME TO new_name2;

However, this disagrees with the existing built-in feature tests, which would format

ALTER TABLE supplier RENAME TO the_one_who_supplies

as

ALTER TABLE supplier
RENAME TO the_one_who_supplies

where RENAME TO is considered a tabular one-line clause. With my choice to make this a keyword phrase, this feature test would be formatted as a single line (essentially unchanged from the input).

Similar to the first item I noted above, I could break convention with the existing tests and follow the rules that I believe make Clickhouse format in a way that I believe looks the best and with internal consistency (making multi-resource statements into reserved clauses and having RENAME TO as a keyword phrase). I could also choose to make it consistent with other formatters at the expense of having certain statements produce confusing/ugly results (using tabular one-line clauses for all statements and RENAME TO).

---

One other option (which addresses the tabular one-line clause vs reserved clause discrepancy, but not the RENAME TO discrepancy) is for me to implement the ability to have a conditionally tabular one-line clause. For a conditionally tabular clause, it would format as a tabular one-line clause if an EOF/semicolon or another clause was encountered first, or as a reserved clause if another clause was encountered first. E.g., making DROP TABLE a conditionally tabular clause would format

-- DROP TABLE foo
DROP TABLE foo -- EOF triggers tabular behavior

-- DROP TABLE foo, bar
DROP TABLE
  foo, -- comma triggers reserved clause behavior
  bar

I haven't done a lot of research to understand how big of an undertaking this would be, and as far as I can tell, this isn't something that exists already (but if it does and I've missed it, please let me know!)

---

I'd love your thoughts on this.

[nene]

I think with both of these it's best to consider which is the common way one would use a statement. Like the DROP TABLE table1, table2, ... syntax is supported by several SQL dialects. But in practice one rarely drops multiple tables at once. And even when one does, you need to be intimately familiar with the syntax supported by your SQL dialect. Much simpler to just write multiple DROP TABLE statements in row to achieve the same.

Similarly with these policies. I would guess it's quite rare that one needs to alter multiple policies at once. So I wouldn't optimize the formatter for these rare cases, but rather for the most common case.

In general...

The thing is, that this formatter works using heuristics and it doesn't really understand SQL. It just looks for some patterns and then tries to make some best guesses. There's no shortage of cases where it does a poor job. But there's only so much it can do with this sort of limited architecture.

To address these fundamental issues I built a new tool: prettier-plugin-sql-cst, which actually parses the SQL and is able to handle this sort of cases. Heh... actually now that I tested it, I discovered it messes up the DROP TABLE foo, bar formatting, but similar things like DROP VIEW foo, bar do work as expected. Which again highlights that it's a syntax one tends to not use.

[nene]

A few quick comments.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[mattbasta]

Hey @nene, apologies for the delay in following up. It's been a busy month here.

I think I've addressed all of your comments. Please let me know if there's anything that you'd like changed or that you aren't happy with in the current implementation.

[nene]

I spotted a few issues, mostly minor. The main thing being about map literals.

[nene]

Would be more consistent to avoid using \n in all the tests and use the dedent utility instead.

[nene]

The IN-function/operator logic was removed, but the comments still talk about it.

[nene]

Minor inconsistency: the \s* is not necessary in the first regex.

[nene]

You are missing some operators: ==, <=> and ||.

[nene]

This was added, but is unused.

[nene]

ClickHouse also supports map literals, so we should include '{}' in here.

But unfortunately that syntax conflicts with parameter syntax. It seems to me that in map literals the keys have to be quoted. I think we could distinguish these two based on that.

An example test which currently fails:

  it('supports map literals', () => {
    expect(format(`SELECT {'foo':1,'bar':10,'baz':2,'zap':8};`)).toBe(dedent`
      SELECT
        {'foo': 1, 'bar': 10, 'baz': 2, 'zap': 8};
    `);
  });

[nene]

Also, please run yarn pretty. The CI currently doesn't do that - should fix it.