Copilot commented on code in PR #9776:
URL: https://github.com/apache/gravitino/pull/9776#discussion_r2753325748
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -38,13 +109,190 @@ protected String generateCreateTableSql(
Distribution distribution,
Index[] indexes) {
throw new UnsupportedOperationException(
- "ClickHouseTableOperations.generateCreateTableSql is not implemented
yet.");
+ "generateCreateTableSql with out sortOrders in clickhouse is not
supported");
+ }
+
+ @Override
+ protected String generateCreateTableSql(
+ String tableName,
+ JdbcColumn[] columns,
+ String comment,
+ Map<String, String> properties,
+ Transform[] partitioning,
+ Distribution distribution,
+ Index[] indexes,
+ SortOrder[] sortOrders) {
+
+ // These two are not yet supported in Gravitino now and will be supported
in the future.
+ if (ArrayUtils.isNotEmpty(partitioning)) {
+ throw new UnsupportedOperationException(
+ "Currently we do not support Partitioning in clickhouse");
+ }
+ Preconditions.checkArgument(
+ Distributions.NONE.equals(distribution), "ClickHouse does not support
distribution");
+
+ validateIncrementCol(columns, indexes);
+
+ // First build the CREATE TABLE statement
+ StringBuilder sqlBuilder = new StringBuilder();
+ sqlBuilder
+ .append("CREATE TABLE ")
+ .append(BACK_QUOTE)
+ .append(tableName)
+ .append(BACK_QUOTE)
+ .append(" (\n");
+
+ // Add columns
+ for (int i = 0; i < columns.length; i++) {
+ JdbcColumn column = columns[i];
+ sqlBuilder
+ .append(SPACE)
+ .append(SPACE)
+ .append(BACK_QUOTE)
+ .append(column.name())
+ .append(BACK_QUOTE);
+
+ appendColumnDefinition(column, sqlBuilder);
+ // Add a comma for the next column, unless it's the last one
+ if (i < columns.length - 1) {
+ sqlBuilder.append(",\n");
+ }
+ }
+
+ // Index definition, we only support primary index now, secondary index
will be supported in
+ // future
+ appendIndexesSql(indexes, sqlBuilder);
+
+ sqlBuilder.append("\n)");
+
+ // Extract engine from properties
+ String engine = ENGINE_PROPERTY_ENTRY.getDefaultValue().getValue();
+ if (MapUtils.isNotEmpty(properties)) {
+ String userSetEngine = properties.remove(CLICKHOUSE_ENGINE_KEY);
+ if (StringUtils.isNotEmpty(userSetEngine)) {
+ engine = userSetEngine;
+ }
+ }
+ sqlBuilder.append("\n ENGINE = ").append(engine);
Review Comment:
SQL injection risk: The engine variable is directly appended to SQL without
validation. While it's extracted from properties and has a default value,
there's no validation that it doesn't contain malicious SQL. The engine value
should be validated against a whitelist of allowed engine types before being
used in SQL generation.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -38,13 +109,190 @@ protected String generateCreateTableSql(
Distribution distribution,
Index[] indexes) {
throw new UnsupportedOperationException(
- "ClickHouseTableOperations.generateCreateTableSql is not implemented
yet.");
+ "generateCreateTableSql with out sortOrders in clickhouse is not
supported");
+ }
+
+ @Override
+ protected String generateCreateTableSql(
+ String tableName,
+ JdbcColumn[] columns,
+ String comment,
+ Map<String, String> properties,
+ Transform[] partitioning,
+ Distribution distribution,
+ Index[] indexes,
+ SortOrder[] sortOrders) {
+
+ // These two are not yet supported in Gravitino now and will be supported
in the future.
+ if (ArrayUtils.isNotEmpty(partitioning)) {
+ throw new UnsupportedOperationException(
+ "Currently we do not support Partitioning in clickhouse");
+ }
+ Preconditions.checkArgument(
+ Distributions.NONE.equals(distribution), "ClickHouse does not support
distribution");
+
+ validateIncrementCol(columns, indexes);
+
+ // First build the CREATE TABLE statement
+ StringBuilder sqlBuilder = new StringBuilder();
+ sqlBuilder
+ .append("CREATE TABLE ")
+ .append(BACK_QUOTE)
+ .append(tableName)
+ .append(BACK_QUOTE)
+ .append(" (\n");
+
+ // Add columns
+ for (int i = 0; i < columns.length; i++) {
+ JdbcColumn column = columns[i];
+ sqlBuilder
+ .append(SPACE)
+ .append(SPACE)
+ .append(BACK_QUOTE)
+ .append(column.name())
+ .append(BACK_QUOTE);
+
+ appendColumnDefinition(column, sqlBuilder);
+ // Add a comma for the next column, unless it's the last one
+ if (i < columns.length - 1) {
+ sqlBuilder.append(",\n");
+ }
+ }
+
+ // Index definition, we only support primary index now, secondary index
will be supported in
+ // future
+ appendIndexesSql(indexes, sqlBuilder);
+
+ sqlBuilder.append("\n)");
+
+ // Extract engine from properties
+ String engine = ENGINE_PROPERTY_ENTRY.getDefaultValue().getValue();
+ if (MapUtils.isNotEmpty(properties)) {
+ String userSetEngine = properties.remove(CLICKHOUSE_ENGINE_KEY);
+ if (StringUtils.isNotEmpty(userSetEngine)) {
+ engine = userSetEngine;
+ }
+ }
+ sqlBuilder.append("\n ENGINE = ").append(engine);
+
+ // Omit partition by clause as it will be supported in the next PR
+ // TODO: Add partition by clause support
+
+ // Add order by clause
+ if (ArrayUtils.isNotEmpty(sortOrders)) {
+ if (sortOrders.length > 1) {
+ throw new UnsupportedOperationException(
+ "Currently ClickHouse does not support sortOrders with more than 1
element");
+ } else if (sortOrders[0].nullOrdering() != null ||
sortOrders[0].direction() != null) {
+ // If no value is set earlier, some default values will be set.
+ // It is difficult to determine whether the user has set a value.
+ LOG.warn(
+ "ClickHouse currently does not support nullOrdering: {} and
direction: {} of sortOrders,and will ignore them",
+ sortOrders[0].nullOrdering(),
+ sortOrders[0].direction());
+ }
+ sqlBuilder.append(
+ " \n ORDER BY " + BACK_QUOTE + sortOrders[0].expression() +
BACK_QUOTE + " \n");
+ }
+
+ // Add table comment if specified
+ if (StringUtils.isNotEmpty(comment)) {
+ String escapedComment = comment.replace("'", "''");
+ sqlBuilder.append(" COMMENT '").append(escapedComment).append("'");
+ }
+
+ // Add setting clause if specified, clickhouse only supports predefine
settings
+ if (MapUtils.isNotEmpty(properties)) {
+ String settings =
+ properties.entrySet().stream()
+ .filter(entry -> entry.getKey().startsWith(SETTINGS_PREFIX))
+ .map(
+ entry ->
+ entry.getKey().substring(SETTINGS_PREFIX.length()) + " =
" + entry.getValue())
+ .collect(Collectors.joining(",\n ", " \n SETTINGS ", ""));
+ sqlBuilder.append(settings);
+ }
+
+ // Return the generated SQL statement
+ String result = sqlBuilder.toString();
+
+ LOG.info("Generated create table:{} sql: {}", tableName, result);
+ return result;
+ }
+
+ public static void appendIndexesSql(Index[] indexes, StringBuilder
sqlBuilder) {
+ if (indexes == null) {
+ return;
+ }
+
+ for (Index index : indexes) {
+ String fieldStr = getIndexFieldStr(index.fieldNames());
+ sqlBuilder.append(",\n");
+ switch (index.type()) {
+ case PRIMARY_KEY:
+ if (null != index.name()
+ && !StringUtils.equalsIgnoreCase(
+ index.name(), Indexes.DEFAULT_CLICKHOUSE_PRIMARY_KEY_NAME)) {
+ LOG.warn(
+ "Primary key name must be PRIMARY in ClickHouse, the name {}
will be ignored.",
+ index.name());
+ }
+ sqlBuilder.append(" PRIMARY KEY (").append(fieldStr).append(")");
+ break;
+ case UNIQUE_KEY:
+ throw new IllegalArgumentException(
+ "Gravitino clickHouse doesn't support index : " + index.type());
+ default:
+ throw new IllegalArgumentException(
+ "Gravitino Clickhouse doesn't support index : " + index.type());
Review Comment:
Inconsistent error messaging: The error message says "Gravitino clickHouse"
(with capital H), while elsewhere in the codebase it's referred to as
"ClickHouse" consistently. For consistency, this should be "Gravitino
ClickHouse" (capital C and H) to match the naming convention used throughout
the codebase.
```suggestion
"Gravitino ClickHouse doesn't support index : " +
index.type());
default:
throw new IllegalArgumentException(
"Gravitino ClickHouse doesn't support index : " +
index.type());
```
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -38,13 +109,190 @@ protected String generateCreateTableSql(
Distribution distribution,
Index[] indexes) {
throw new UnsupportedOperationException(
- "ClickHouseTableOperations.generateCreateTableSql is not implemented
yet.");
+ "generateCreateTableSql with out sortOrders in clickhouse is not
supported");
+ }
+
+ @Override
+ protected String generateCreateTableSql(
+ String tableName,
+ JdbcColumn[] columns,
+ String comment,
+ Map<String, String> properties,
+ Transform[] partitioning,
+ Distribution distribution,
+ Index[] indexes,
+ SortOrder[] sortOrders) {
+
+ // These two are not yet supported in Gravitino now and will be supported
in the future.
+ if (ArrayUtils.isNotEmpty(partitioning)) {
+ throw new UnsupportedOperationException(
+ "Currently we do not support Partitioning in clickhouse");
+ }
+ Preconditions.checkArgument(
+ Distributions.NONE.equals(distribution), "ClickHouse does not support
distribution");
+
+ validateIncrementCol(columns, indexes);
+
+ // First build the CREATE TABLE statement
+ StringBuilder sqlBuilder = new StringBuilder();
+ sqlBuilder
+ .append("CREATE TABLE ")
+ .append(BACK_QUOTE)
+ .append(tableName)
+ .append(BACK_QUOTE)
+ .append(" (\n");
+
+ // Add columns
+ for (int i = 0; i < columns.length; i++) {
+ JdbcColumn column = columns[i];
+ sqlBuilder
+ .append(SPACE)
+ .append(SPACE)
+ .append(BACK_QUOTE)
+ .append(column.name())
+ .append(BACK_QUOTE);
+
+ appendColumnDefinition(column, sqlBuilder);
+ // Add a comma for the next column, unless it's the last one
+ if (i < columns.length - 1) {
+ sqlBuilder.append(",\n");
+ }
+ }
+
+ // Index definition, we only support primary index now, secondary index
will be supported in
+ // future
+ appendIndexesSql(indexes, sqlBuilder);
+
+ sqlBuilder.append("\n)");
+
+ // Extract engine from properties
+ String engine = ENGINE_PROPERTY_ENTRY.getDefaultValue().getValue();
+ if (MapUtils.isNotEmpty(properties)) {
+ String userSetEngine = properties.remove(CLICKHOUSE_ENGINE_KEY);
+ if (StringUtils.isNotEmpty(userSetEngine)) {
+ engine = userSetEngine;
+ }
+ }
+ sqlBuilder.append("\n ENGINE = ").append(engine);
+
+ // Omit partition by clause as it will be supported in the next PR
+ // TODO: Add partition by clause support
+
+ // Add order by clause
+ if (ArrayUtils.isNotEmpty(sortOrders)) {
+ if (sortOrders.length > 1) {
+ throw new UnsupportedOperationException(
+ "Currently ClickHouse does not support sortOrders with more than 1
element");
+ } else if (sortOrders[0].nullOrdering() != null ||
sortOrders[0].direction() != null) {
+ // If no value is set earlier, some default values will be set.
+ // It is difficult to determine whether the user has set a value.
+ LOG.warn(
+ "ClickHouse currently does not support nullOrdering: {} and
direction: {} of sortOrders,and will ignore them",
+ sortOrders[0].nullOrdering(),
+ sortOrders[0].direction());
+ }
+ sqlBuilder.append(
+ " \n ORDER BY " + BACK_QUOTE + sortOrders[0].expression() +
BACK_QUOTE + " \n");
+ }
+
+ // Add table comment if specified
+ if (StringUtils.isNotEmpty(comment)) {
+ String escapedComment = comment.replace("'", "''");
+ sqlBuilder.append(" COMMENT '").append(escapedComment).append("'");
+ }
+
+ // Add setting clause if specified, clickhouse only supports predefine
settings
+ if (MapUtils.isNotEmpty(properties)) {
+ String settings =
+ properties.entrySet().stream()
+ .filter(entry -> entry.getKey().startsWith(SETTINGS_PREFIX))
+ .map(
+ entry ->
+ entry.getKey().substring(SETTINGS_PREFIX.length()) + " =
" + entry.getValue())
+ .collect(Collectors.joining(",\n ", " \n SETTINGS ", ""));
+ sqlBuilder.append(settings);
+ }
+
+ // Return the generated SQL statement
+ String result = sqlBuilder.toString();
+
+ LOG.info("Generated create table:{} sql: {}", tableName, result);
+ return result;
+ }
+
+ public static void appendIndexesSql(Index[] indexes, StringBuilder
sqlBuilder) {
+ if (indexes == null) {
Review Comment:
Inconsistent null checking pattern: The null check uses 'indexes == null'
instead of 'ArrayUtils.isEmpty(indexes)' which is used elsewhere in the
codebase (e.g., line 127 and 182 in the same file). For consistency with the
existing codebase patterns, consider using 'ArrayUtils.isEmpty(indexes)' which
handles both null and empty arrays.
```suggestion
if (ArrayUtils.isEmpty(indexes)) {
```
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -38,13 +109,190 @@ protected String generateCreateTableSql(
Distribution distribution,
Index[] indexes) {
throw new UnsupportedOperationException(
- "ClickHouseTableOperations.generateCreateTableSql is not implemented
yet.");
+ "generateCreateTableSql with out sortOrders in clickhouse is not
supported");
+ }
+
+ @Override
+ protected String generateCreateTableSql(
+ String tableName,
+ JdbcColumn[] columns,
+ String comment,
+ Map<String, String> properties,
+ Transform[] partitioning,
+ Distribution distribution,
+ Index[] indexes,
+ SortOrder[] sortOrders) {
+
+ // These two are not yet supported in Gravitino now and will be supported
in the future.
+ if (ArrayUtils.isNotEmpty(partitioning)) {
+ throw new UnsupportedOperationException(
+ "Currently we do not support Partitioning in clickhouse");
+ }
+ Preconditions.checkArgument(
+ Distributions.NONE.equals(distribution), "ClickHouse does not support
distribution");
+
+ validateIncrementCol(columns, indexes);
+
+ // First build the CREATE TABLE statement
+ StringBuilder sqlBuilder = new StringBuilder();
+ sqlBuilder
+ .append("CREATE TABLE ")
+ .append(BACK_QUOTE)
+ .append(tableName)
+ .append(BACK_QUOTE)
+ .append(" (\n");
+
+ // Add columns
+ for (int i = 0; i < columns.length; i++) {
+ JdbcColumn column = columns[i];
+ sqlBuilder
+ .append(SPACE)
+ .append(SPACE)
+ .append(BACK_QUOTE)
+ .append(column.name())
+ .append(BACK_QUOTE);
+
+ appendColumnDefinition(column, sqlBuilder);
+ // Add a comma for the next column, unless it's the last one
+ if (i < columns.length - 1) {
+ sqlBuilder.append(",\n");
+ }
+ }
+
+ // Index definition, we only support primary index now, secondary index
will be supported in
+ // future
+ appendIndexesSql(indexes, sqlBuilder);
+
+ sqlBuilder.append("\n)");
+
+ // Extract engine from properties
+ String engine = ENGINE_PROPERTY_ENTRY.getDefaultValue().getValue();
+ if (MapUtils.isNotEmpty(properties)) {
+ String userSetEngine = properties.remove(CLICKHOUSE_ENGINE_KEY);
+ if (StringUtils.isNotEmpty(userSetEngine)) {
+ engine = userSetEngine;
+ }
+ }
+ sqlBuilder.append("\n ENGINE = ").append(engine);
+
+ // Omit partition by clause as it will be supported in the next PR
+ // TODO: Add partition by clause support
+
+ // Add order by clause
+ if (ArrayUtils.isNotEmpty(sortOrders)) {
+ if (sortOrders.length > 1) {
+ throw new UnsupportedOperationException(
+ "Currently ClickHouse does not support sortOrders with more than 1
element");
+ } else if (sortOrders[0].nullOrdering() != null ||
sortOrders[0].direction() != null) {
+ // If no value is set earlier, some default values will be set.
+ // It is difficult to determine whether the user has set a value.
+ LOG.warn(
+ "ClickHouse currently does not support nullOrdering: {} and
direction: {} of sortOrders,and will ignore them",
+ sortOrders[0].nullOrdering(),
+ sortOrders[0].direction());
+ }
+ sqlBuilder.append(
+ " \n ORDER BY " + BACK_QUOTE + sortOrders[0].expression() +
BACK_QUOTE + " \n");
+ }
+
+ // Add table comment if specified
+ if (StringUtils.isNotEmpty(comment)) {
+ String escapedComment = comment.replace("'", "''");
+ sqlBuilder.append(" COMMENT '").append(escapedComment).append("'");
+ }
+
+ // Add setting clause if specified, clickhouse only supports predefine
settings
+ if (MapUtils.isNotEmpty(properties)) {
+ String settings =
+ properties.entrySet().stream()
+ .filter(entry -> entry.getKey().startsWith(SETTINGS_PREFIX))
+ .map(
+ entry ->
+ entry.getKey().substring(SETTINGS_PREFIX.length()) + " =
" + entry.getValue())
+ .collect(Collectors.joining(",\n ", " \n SETTINGS ", ""));
Review Comment:
SQL injection risk: Properties values are directly concatenated into SQL
without sanitization. An attacker could inject malicious SQL through the
property values. These values should be properly escaped or validated before
inclusion in the SQL statement.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -38,13 +109,190 @@ protected String generateCreateTableSql(
Distribution distribution,
Index[] indexes) {
throw new UnsupportedOperationException(
- "ClickHouseTableOperations.generateCreateTableSql is not implemented
yet.");
+ "generateCreateTableSql with out sortOrders in clickhouse is not
supported");
+ }
+
+ @Override
+ protected String generateCreateTableSql(
+ String tableName,
+ JdbcColumn[] columns,
+ String comment,
+ Map<String, String> properties,
+ Transform[] partitioning,
+ Distribution distribution,
+ Index[] indexes,
+ SortOrder[] sortOrders) {
+
+ // These two are not yet supported in Gravitino now and will be supported
in the future.
+ if (ArrayUtils.isNotEmpty(partitioning)) {
+ throw new UnsupportedOperationException(
+ "Currently we do not support Partitioning in clickhouse");
+ }
+ Preconditions.checkArgument(
+ Distributions.NONE.equals(distribution), "ClickHouse does not support
distribution");
+
+ validateIncrementCol(columns, indexes);
+
+ // First build the CREATE TABLE statement
+ StringBuilder sqlBuilder = new StringBuilder();
+ sqlBuilder
+ .append("CREATE TABLE ")
+ .append(BACK_QUOTE)
+ .append(tableName)
+ .append(BACK_QUOTE)
+ .append(" (\n");
+
+ // Add columns
+ for (int i = 0; i < columns.length; i++) {
+ JdbcColumn column = columns[i];
+ sqlBuilder
+ .append(SPACE)
+ .append(SPACE)
+ .append(BACK_QUOTE)
+ .append(column.name())
+ .append(BACK_QUOTE);
+
+ appendColumnDefinition(column, sqlBuilder);
+ // Add a comma for the next column, unless it's the last one
+ if (i < columns.length - 1) {
+ sqlBuilder.append(",\n");
+ }
+ }
+
+ // Index definition, we only support primary index now, secondary index
will be supported in
+ // future
+ appendIndexesSql(indexes, sqlBuilder);
+
+ sqlBuilder.append("\n)");
+
+ // Extract engine from properties
+ String engine = ENGINE_PROPERTY_ENTRY.getDefaultValue().getValue();
+ if (MapUtils.isNotEmpty(properties)) {
+ String userSetEngine = properties.remove(CLICKHOUSE_ENGINE_KEY);
+ if (StringUtils.isNotEmpty(userSetEngine)) {
+ engine = userSetEngine;
+ }
+ }
+ sqlBuilder.append("\n ENGINE = ").append(engine);
+
+ // Omit partition by clause as it will be supported in the next PR
+ // TODO: Add partition by clause support
+
+ // Add order by clause
+ if (ArrayUtils.isNotEmpty(sortOrders)) {
+ if (sortOrders.length > 1) {
+ throw new UnsupportedOperationException(
+ "Currently ClickHouse does not support sortOrders with more than 1
element");
+ } else if (sortOrders[0].nullOrdering() != null ||
sortOrders[0].direction() != null) {
+ // If no value is set earlier, some default values will be set.
+ // It is difficult to determine whether the user has set a value.
+ LOG.warn(
+ "ClickHouse currently does not support nullOrdering: {} and
direction: {} of sortOrders,and will ignore them",
Review Comment:
Spacing error in log message: Missing space after comma. The message should
read "ClickHouse currently does not support nullOrdering: {} and direction: {}
of sortOrders, and will ignore them".
```suggestion
"ClickHouse currently does not support nullOrdering: {} and
direction: {} of sortOrders, and will ignore them",
```
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/ClickHouseContainer.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.integration.test.container;
+
+import static java.lang.String.format;
+
+import com.google.common.collect.ImmutableSet;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.integration.test.util.TestDatabaseName;
+import org.rnorth.ducttape.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testcontainers.containers.Network;
+
+public class ClickHouseContainer extends BaseContainer {
+ public static final Logger LOG =
LoggerFactory.getLogger(ClickHouseContainer.class);
+
+ public static final String DEFAULT_IMAGE = "clickhouse:24.8.14";
+ public static final String HOST_NAME = "gravitino-ci-clickhouse";
+ public static final int CLICKHOUSE_PORT = 8123;
+ public static final String USER_NAME = "default";
+ public static final String PASSWORD = "default";
+
+ public static Builder builder() {
+ return new Builder();
+ }
+
+ protected ClickHouseContainer(
+ String image,
+ String hostName,
+ Set<Integer> ports,
+ Map<String, String> extraHosts,
+ Map<String, String> filesToMount,
+ Map<String, String> envVars,
+ Optional<Network> network) {
+ super(image, hostName, ports, extraHosts, filesToMount, envVars, network);
+ }
+
+ @Override
+ protected void setupContainer() {
+ super.setupContainer();
+ withLogConsumer(new PrintingContainerLog(format("%-14s| ",
"clickHouseContainer")));
+ }
+
+ @Override
+ public void start() {
+ super.start();
+ Preconditions.check("clickHouse container startup failed!",
checkContainerStatus(5));
+ }
+
+ @Override
+ protected boolean checkContainerStatus(int retryLimit) {
+ for (int attempt = 1; attempt <= retryLimit; attempt++) {
+ try (Connection connection =
+ DriverManager.getConnection(getJdbcUrl(), USER_NAME,
getPassword());
+ Statement statement = connection.createStatement()) {
+ // Simple health check query; ClickHouse should respond if it is ready.
+ statement.execute("SELECT 1");
+ LOG.info("clickHouse container is healthy after {} attempt(s)",
attempt);
+ return true;
+ } catch (SQLException e) {
+ LOG.warn(
+ "Failed to connect to clickHouse container on attempt {}/{}: {}",
+ attempt,
+ retryLimit,
+ e.getMessage(),
+ e);
+ if (attempt < retryLimit) {
+ try {
+ Thread.sleep(1000L);
+ } catch (InterruptedException interruptedException) {
+ Thread.currentThread().interrupt();
+ LOG.error(
+ "Interrupted while waiting to retry clickHouse container
health check",
+ interruptedException);
+ break;
+ }
+ }
+ }
+ }
+ LOG.error("clickHouse container failed health checks after {} attempt(s)",
retryLimit);
+ return false;
+ }
+
+ public void createDatabase(TestDatabaseName testDatabaseName) {
+ String clickHouseJdbcUrl =
+ StringUtils.substring(
+ getJdbcUrl(testDatabaseName), 0,
getJdbcUrl(testDatabaseName).lastIndexOf("/"));
+
+ // Use the default username and password to connect and create the
database;
+ // any non-default password must be configured via Gravitino catalog
properties.
+ try (Connection connection =
+ DriverManager.getConnection(clickHouseJdbcUrl, USER_NAME,
getPassword());
+ Statement statement = connection.createStatement()) {
+
+ String query = String.format("CREATE DATABASE IF NOT EXISTS %s;",
testDatabaseName);
+ // FIXME: String, which is used in SQL, can be unsafe
+ statement.execute(query);
Review Comment:
SQL injection vulnerability: The testDatabaseName parameter is directly
interpolated into SQL without using prepared statement parameters. This is a
known security risk as acknowledged by the FIXME comment. Consider using
parameterized queries or proper escaping for database names.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -18,16 +18,87 @@
*/
package org.apache.gravitino.catalog.clickhouse.operations;
+import static
org.apache.gravitino.catalog.clickhouse.ClickHouseConstants.SETTINGS_PREFIX;
+import static
org.apache.gravitino.catalog.clickhouse.ClickHouseTablePropertiesMetadata.CLICKHOUSE_ENGINE_KEY;
+import static
org.apache.gravitino.catalog.clickhouse.ClickHouseTablePropertiesMetadata.ENGINE_PROPERTY_ENTRY;
+import static org.apache.gravitino.rel.Column.DEFAULT_VALUE_NOT_SET;
+
+import com.google.common.base.Preconditions;
+import java.sql.Connection;
+import java.sql.DatabaseMetaData;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import org.apache.commons.collections4.MapUtils;
+import org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.catalog.jdbc.JdbcColumn;
import org.apache.gravitino.catalog.jdbc.operation.JdbcTableOperations;
+import org.apache.gravitino.exceptions.NoSuchTableException;
import org.apache.gravitino.rel.TableChange;
import org.apache.gravitino.rel.expressions.distributions.Distribution;
+import org.apache.gravitino.rel.expressions.distributions.Distributions;
+import org.apache.gravitino.rel.expressions.sorts.SortOrder;
import org.apache.gravitino.rel.expressions.transforms.Transform;
import org.apache.gravitino.rel.indexes.Index;
+import org.apache.gravitino.rel.indexes.Indexes;
public class ClickHouseTableOperations extends JdbcTableOperations {
+ private static final String BACK_QUOTE = "`";
+
+ @SuppressWarnings("unused")
+ private static final String CLICKHOUSE_AUTO_INCREMENT = "AUTO_INCREMENT";
+
+ @SuppressWarnings("unused")
+ private static final String CLICKHOUSE_NOT_SUPPORT_NESTED_COLUMN_MSG =
+ "Clickhouse does not support nested column names.";
Review Comment:
Inconsistent casing in naming: The constant
CLICKHOUSE_NOT_SUPPORT_NESTED_COLUMN_MSG uses "Clickhouse" while other
constants and class names use "ClickHouse" (capital C and H). This should be
changed to "ClickHouse" for consistency with the rest of the codebase. For
reference, see similar patterns like MYSQL_NOT_SUPPORT_NESTED_COLUMN_MSG in
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java:64
which uses "Mysql" as one word (though this could also be considered
inconsistent).
```suggestion
"ClickHouse does not support nested column names.";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
