Add suppport for creating table with MergeTree engine.

[wgzhao]

1. Add support for createing table with MergeTree engine Support for create table with MergeTree engine in ClickHouse connector #7130
2. Add more tests Fix disabled tests in ClickHouse connector #7101

[findepi]

We should validate the order by, primary key, partition by, sample by are not present if engine is not "mergretree".
Otherwise we silently ignore table properties explicitly requested by the user.

Other option would be to pass everything to ClickHouse and let CH do the validation.

[wgzhao]

I means I will add required property (order by) and optional properties (if present) for MergeTree engine.
But for Log engine, all above properties are not required, so i just ignore them.

[hashhar]

Some comments.

A question about testing.
If ClickHouse passes the testAddColumn and testDropColumn defined in superclass then instead of over-riding them add new methods with more descriptive names (e.g. testDropColumn -> testDropMergeTreeColumn, testAddColumn -> testAddMergeTreeColumn.

In case the superclass tests fail I'd suggest to add a SkipException with the reason. This way if the base test class is ever improved in future all connectors will get the improved test coverage for free.

[findepi]

"Required for tables using MergeTree engine property."

But I think it's OK to drop "Required" and "Optional" here and just describe this in docs.

[wgzhao]

These description of table property comes from office documentation.
Ok, I change them.

[findepi]

relating to #7135 (comment),

what happens if i issue

CREATE TABLE clickhouse.default.t(a int) 
WITH (partition_by = ARRAY['x'])

?

I would expect some kind of failure (either from the connector, or from ClickHouse), because there is no column x.

Then, what happens if i issue

CREATE TABLE clickhouse.default.t(a int, x int) 
WITH (engine = 'LOG', partition_by = ARRAY['x'])

?

I would expect some kind of failure (either from the connector, or from ClickHouse), because i asked for the table to be partitioned, but it cannot be.

[wgzhao]

CREATE TABLE clickhouse.default.t(a int) 
WITH (partition_by = ARRAY['x'])

CREATE TABLE clickhouse.default.t(a int, x int) 
WITH (engine = 'LOG', partition_by = ARRAY['x'])

Yes, the above sqls will both create successfully, because the default table engine Log will ignore all other properties.

we can pass all table property regardless table engine, it will be kind of failure from ClickHouse side。
So we take this approach？

[wgzhao]

execute the following sql in ClickHouse directly

create table default.t (a int)  engine = Log order by a;

It print the following errors:

ClickHouse exception, code: 36, host: 10.60.242.112, port: 18123; Code: 36, e.displayText() = DB::Exception: 
Engine Log doesn't support PARTITION_BY, PRIMARY_KEY, ORDER_BY or SAMPLE_BY clauses. 
Currently only the following engines have support for the feature: [MergeTree, ReplicatedVersionedCollapsingMergeTree, 
ReplacingMergeTree, ReplicatedSummingMergeTree, ReplicatedAggregatingMergeTree, ReplicatedCollapsingMergeTree, ReplicatedGraphiteMergeTree, ReplicatedMergeTree, 
ReplicatedReplacingMergeTree, VersionedCollapsingMergeTree, SummingMergeTree,
GraphiteMergeTree, CollapsingMergeTree, AggregatingMergeTree] (version 20.3.5.21 (official build))

[wgzhao]

modified codes:

    @Override
    protected String createTableSql(RemoteTableName remoteTableName, List<String> columns, ConnectorTableMetadata tableMetadata)
    {
        ImmutableList.Builder<String> tableOptions = ImmutableList.builder();
        Map<String, Object> tableProperties = tableMetadata.getProperties();
        ClickHouseEngineType engine = ClickHouseTableProperties.getEngine(tableProperties);
        tableOptions.add("ENGINE = " + engine.getEngineType());
        if (engine == ClickHouseEngineType.MERGETREE && formatProperty(ClickHouseTableProperties.getOrderBy(tableProperties)).isEmpty()) {
            // order_by property is required
            throw new TrinoException(INVALID_TABLE_PROPERTY, format("The property of %s is required for table engine %s", ClickHouseTableProperties.ORDER_BY_PROPERTY, engine.getEngineType()));
        }
        formatProperty(ClickHouseTableProperties.getOrderBy(tableProperties)).ifPresent(value -> tableOptions.add("ORDER BY " + value));
        formatProperty(ClickHouseTableProperties.getPrimaryKey(tableProperties)).ifPresent(value -> tableOptions.add("PRIMARY KEY " + value));
        formatProperty(ClickHouseTableProperties.getPartitionBy(tableProperties)).ifPresent(value -> tableOptions.add("PARTITION BY " + value));
        ClickHouseTableProperties.getSampleBy(tableProperties).ifPresent(value -> tableOptions.add("SAMPLE BY " + value));

        return format("CREATE TABLE %s (%s) %s", quoted(remoteTableName), join(", ", columns), join(" ", tableOptions.build()));
    }

[findepi]

The build status seems related

2021-03-09T03:13:40.7966599Z [ERROR] testDifferentEngine(io.trino.plugin.clickhouse.TestClickHouseConnectorTest)  Time elapsed: 0.124 s  <<< FAILURE!
2021-03-09T03:13:40.7969084Z java.lang.AssertionError: 
2021-03-09T03:13:40.7969522Z 
2021-03-09T03:13:40.7969910Z Expecting message:
2021-03-09T03:13:40.7970589Z   <"The property of order_by is required for table engine MergeTree()">
2021-03-09T03:13:40.7971210Z to match regex:
2021-03-09T03:13:40.7971716Z   <"order_by is required for MergeTree\(\)">
2021-03-09T03:13:40.7972212Z but did not.
2021-03-09T03:13:40.7972475Z 
2021-03-09T03:13:40.7979705Z Throwable that failed the check:

[findepi]

@electrum @kokosing @martint @losipiuk cc @trinodb/maintainers

I have a question how should we model table properties that are arbitrary expessions.

ORDER BY expr
[PARTITION BY expr]
[PRIMARY KEY expr]
[SAMPLE BY expr]

Those expressions can be column names, in which case user could expect the connector to apply proper quoting.
But of course, we don't know whether expression is a column name, since it's a CH's expression.
Actually a n-m could refer be "column n minus m", or could be "column "n-m"".

We could reduce risk for confusion by naming the table properties informatively, eg partition_by_expressions instead of partition_by, although this feels a bit too verbose.

I am therefore inclined to continue with the approach presented here, using shorter table property names, eg partition_by.

Thoughts?

[wgzhao]

@findepi
My understanding is that when a user uses clickhouse connector, we can assume that the user has some understanding of clickhouse, like how to create a table on the clickhouse side, so in trino, if we keep the syntax or keywords similar to clickhouse, isn't it easier for the user to understand?

[findepi]

Merged, thanks!