fix: Remove 'FORMAT JSON' from CREATE, DROP, ALTER and RENAME statements

[jspeedz]

Removes 'FORMAT JSON' from CREATE, DROP, ALTER and RENAME statements when executing queries 'ON CLUSTER'.

Specifying output format is only supported for SELECT and INSERT statements.
See: https://clickhouse.com/docs/interfaces/formats

Fixes: #241

This PR differs from #214, which is stale / conflicts with the latest version.
This PR only inverts the logic that does not work as intended, it does not add an extra parameter to the write() method like #214.

This change/fix works for my particular use-case. But do not know if I am missing something. Please discuss if this is the correct fix. We might want to make the 'FORMAT JSON' forced format statement optional as in #214. Or maybe we should allow-list SELECT/INSERT statements instead of deny-listing.

[isublimity]

Thanks @jspeedz! Clean fix, correct approach — DDL statements don't support FORMAT. Merging.

[iTearo]

@isublimity @jspeedz In the previous implementation, the condition correctly handled queries starting with CREATE TABLE, DROP TABLE, ALTER TABLE, and RENAME TABLE. We relied on this behavior, and these types of queries worked consistently on our side.

With the changes introduced in this PR, this behavior appears to be broken — the mentioned query types are now handled incorrectly, which affects our existing logic.

@jspeedz Could you please consider adjusting the implementation so that it:

1. Preserves the behavior described in this PR.
2. Continues to correctly support queries starting with CREATE TABLE, DROP TABLE, ALTER TABLE, and RENAME TABLE.

It might make sense to refine or extend the condition to cover both scenarios.

[jspeedz]

@isublimity @jspeedz In the previous implementation, the condition correctly handled queries starting with CREATE TABLE, DROP TABLE, ALTER TABLE, and RENAME TABLE. We relied on this behavior, and these types of queries worked consistently on our side.

With the changes introduced in this PR, this behavior appears to be broken — the mentioned query types are now handled incorrectly, which affects our existing logic.

@jspeedz Could you please consider adjusting the implementation so that it:

1. Preserves the behavior described in this PR.
2. Continues to correctly support queries starting with CREATE TABLE, DROP TABLE, ALTER TABLE, and RENAME TABLE.

It might make sense to refine or extend the condition to cover both scenarios.

@iTearo I just wrote a test to confirm a CREATE TABLE statement works as expected, it does. Can you share a bit more, maybe the code and query that triggers the error?

[iTearo]

@jspeedz To reproduce the issue, you can use a test like this:

$sql = $this->client->write('DROP TABLE IF EXISTS tbl ON CLUSTER clstr')->sql();
$this->assertEquals('DROP TABLE IF EXISTS tbl ON CLUSTER clstr FORMAT JSON', $sql);

This test passes on version 1.6.0, but fails on master with the following error:

Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'DROP TABLE IF EXISTS tbl ON CLUSTER clstr FORMAT JSON'
+'DROP TABLE IF EXISTS tbl ON CLUSTER clstr'

We use a ClickHouse cluster, so our queries always contain ON CLUSTER.

The FORMAT JSON is no longer added to CREATE, DROP, ALTER, and RENAME statements, which causes the assertion to fail.

[jspeedz]

@jspeedz To reproduce the issue, you can use a test like this:

....

@iTearo This is as intended.

The test I did was within this repository, executing the statement on a clickhouse server instance running in docker, instead of comparing the final statement strings (maybe I should commit / PR this test).

Input and output formats are not used/supported for these DDL statements. See https://clickhouse.com/docs/interfaces/formats
There is also no mention of input/output formats here: https://clickhouse.com/docs/sql-reference/statements/drop
So there should be no difference between those two DROP TABLE queries.

The DROP TABLE statement with FORMAT JSON appended does not fail when executed, but the FORMAT has no effect as far as I can tell. For consistency it is nicer to just not (force-) add the FORMAT to any statement where it has no effect in my opinion.
This PR removes forced addition of the FORMAT JSON from statements that do not support it or where they have no effect.

Do the statements actually fail when executing them on a clickhouse server? If so, it might be a different clickhouse version than I am using, maybe?

[iTearo]

Do the statements actually fail when executing them on a clickhouse server?

@jspeedz Statements are executing without errors, but FORMAT JSON is required to ensure the response can be handled correctly. Without it, we hit an exception that originates from the client handling logic:
https://github.com/smi2/phpClickHouse/blob/master/src/Statement.php#L244

[iTearo]

Bacause the response from clustered and standalone Clickhouse is differ

[iTearo]

I think support for both cases - yours and mine - could be implemented

[jspeedz]

I think support for both cases - yours and mine - could be implemented

@iTearo Ah yes, I see what you mean now, thanks. This needs a new PR!

How about something like this? This should fix the original issue with CREATE USER statements and leave the rest as it was before. (ignore composer.json please).

master...jspeedz:phpClickHouse:bugfix/241-v2

[iTearo]

@jspeedz I think we can simply add a TABLE clarification to the conditions like this:

            str_starts_with($sql, 'CREATE TABLE')
            || str_starts_with($sql, 'DROP TABLE')
            || str_starts_with($sql, 'ALTER TABLE')
            || str_starts_with($sql, 'RENAME TABLE')

[jspeedz]

@jspeedz I think we can simply add a TABLE clarification to the conditions like this:

            str_starts_with($sql, 'CREATE TABLE')
            || str_starts_with($sql, 'DROP TABLE')
            || str_starts_with($sql, 'ALTER TABLE')
            || str_starts_with($sql, 'RENAME TABLE')

@iTearo This will change the current behaviour though. I'm not 100% familiar with all available statements. I would have to be sure statements like CREATE DATABASE, CREATE [OR REPLACE] VIEW, CREATE [OR REPLACE] DICTIONARY etc. will also work, as these would no longer be matched.

I'll have to find some time to make the changes and rewrite those tests to cover all scenarios / statements so we can be confident they all work.

[Lanatel]

Hi @jspeedz

Thanks for these changes. I found at least two more statements that currently fail because of the FORMAT JSON added at the end: GRANT and REVOKE.

In my opinion, it would be better to add this only for queries/statements that actually support the format, rather than maintaining a denylist. What do you think?