Introduce syntax tokenizer to correctly handle parentheses in INSERT INTO statement in batch prepare

[timmyb32r]

Summary

Checklist

Delete items not relevant to your PR:

• Unit and integration tests covering the common scenarios were added
• A human-readable description of the changes was provided to include in CHANGELOG
• For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Tim Brunko seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[timmyb32r]

actually it's #1247, don't know how to merge these my two issues

[jkaflik]

@timmyb32r there is no need to merge them. One is issue report, second is PR. It's fine.

Could you please update PR title and description so it reflects the changes? Also, please take a look on ClickHouse CLA agreement above.

[timmyb32r]

I confirmed CLA yesterday, and yesterday button for confirming disappeared. But bar was "not signed yet"
Today button for confirming appeared, I confirmed again, button disappeared, and bar is still "not signed yet" :-(

how to fix that?

[timmyb32r]

updated title.

what can I also to do, to help my PR to became merged?

[jkaflik]

First of all, thanks for your work. Your PR indeed solves the issue you raised in #1247. However, it introduces an additional level of complexity, which we would like to avoid as a part of the simple ClickHouse driver.

The maintenance cost for this additional 2,5k LoC (I know most of it is auto-generated) is non-zero. Regex with simple string operations is smelly but does not require us to bring this logic into a language driver. The purpose of parsing columns is to expose more accessible API, not to bring entire SQL lexer logic.

I'm sorry, but I need to refuse your PR. Here is a PR that fixes your issue: #1252

[timmyb32r]

It's not smelly - it's a wrong mechanism for parsing queries (or maybe it's possible in this particular case, but by very complicated regex, not by the current one).

This is correct clickhouse queries:

    CREATE TABLE `TableNameWithParenthesesAndCommas( INSERT INTO  VALUES ),(),)(` (
        `ColumnNameWithParenthesesAndCommas(),(), INSERT INTO  VALUES ,)(` String
    ) Engine MergeTree() ORDER BY tuple()

    INSERT INTO `TableNameWithParenthesesAndCommas( INSERT INTO  VALUES ),(),)(` (`ColumnNameWithParenthesesAndCommas(),(), INSERT INTO  VALUES ,)(`) VALUES ('abc');

I've just checked:

93599382e58d :)     CREATE TABLE `TableNameWithParenthesesAndCommas( INSERT INTO  VALUES ),(),)(` (
        `ColumnNameWithParenthesesAndCommas(),(), INSERT INTO  VALUES ,)(` String
    ) Engine MergeTree() ORDER BY tuple()

CREATE TABLE `TableNameWithParenthesesAndCommas( INSERT INTO  VALUES ),(),)(`
(
    `ColumnNameWithParenthesesAndCommas(),(), INSERT INTO  VALUES ,)(` String
)
ENGINE = MergeTree
ORDER BY tuple()

Query id: 32780daf-329c-4427-997c-5c04e1cf50cd

Ok.

0 rows in set. Elapsed: 0.009 sec.

93599382e58d :) INSERT INTO `TableNameWithParenthesesAndCommas( INSERT INTO  VALUES ),(),)(` (`ColumnNameWithParenthesesAndCommas(),(), INSERT INTO  VALUES ,)(`) VALUES ('abc');

INSERT INTO `TableNameWithParenthesesAndCommas( INSERT INTO  VALUES ),(),)(` (`ColumnNameWithParenthesesAndCommas(),(), INSERT INTO  VALUES ,)(`) FORMAT Values

Query id: a73f7efc-0255-4cce-870d-7b6ed6c03c85

Ok.

1 row in set. Elapsed: 0.006 sec.

Can you confirm, that this query will never be supported by this clickhouse-driver?

[timmyb32r]

@jkaflik

Okay, if you don't want to include such big dependency - I understand that. But I still have to give users of my product good functionality.

Then don't you mind, if I move in current code parsing query function in special function, and into driver it will be used via global variable on pkg-level, and I will override it in my code?

It will looks like that:

// conn_batch.go

type queryAndColumnsExtractor = func(query string) (string, []string, error) 

var QueryAndColumnsExtractor queryAndColumnsExtractor = extractQueryAndColumns

func extractQueryAndColumns(query string) (string, []string, error) {
    // current implementation
}

func prepareBatch() {
    batchQuery, columns, err := QueryAndColumnsExtractor(query)
}

and I will be on init() of my component assign to QueryAndColumnsExtractor field of clickhouse-go package my own parsing method, which will be based on code from my pr?

Then users of driver clickhouse-go can got proper functionality with zero maintainance from driver maintainers?