yuqi1129 commented on code in PR #11731:
URL: https://github.com/apache/gravitino/pull/11731#discussion_r3449721022
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -387,15 +481,67 @@ protected List<Index> getIndexes(Connection connection,
String databaseName, Str
while (resultSet.next()) {
String indexName = resultSet.getString("Key_name");
String columnName = resultSet.getString("Column_name");
- indexes.add(
- Indexes.of(Index.IndexType.PRIMARY_KEY, indexName, new String[][]
{{columnName}}));
+ // Index_type column may not exist in older Doris versions (e.g.
1.2.x).
+ // Fall back to null so mapDorisIndexType can infer from indexName.
+ String indexType;
+ try {
+ indexType = resultSet.getString("Index_type");
+ } catch (SQLException e) {
+ LOG.warn(
+ "Index_type column not available in SHOW INDEX output, "
+ + "inferring index type from index name '{}'. "
+ + "Upgrade to Doris 3.0+ for accurate index type mapping.",
+ indexName);
+ indexType = null;
+ }
+ Index.IndexType gravitinoIndexType = mapDorisIndexType(indexType,
indexName);
+ indexes.add(Indexes.of(gravitinoIndexType, indexName, new String[][]
{{columnName}}));
}
return indexes;
} catch (SQLException e) {
throw exceptionMapper.toGravitinoException(e);
}
}
+ @VisibleForTesting
+ static Index.IndexType mapDorisIndexType(String indexType, String indexName)
{
+ if (indexType == null) {
+ // Index_type column unavailable (Doris 1.2.x) or returned null.
+ // Infer from index name: "PRIMARY" is the primary key, everything else
defaults to
+ // INVERTED as a safe fallback (Doris 1.2.x indexes are all BTREE-based
key indexes,
+ // but without Index_type we cannot distinguish UNIQUE_KEY from
PRIMARY_KEY).
+ if ("PRIMARY".equals(indexName)) {
+ return Index.IndexType.PRIMARY_KEY;
+ }
+ LOG.warn(
+ "Index_type is null for index '{}', defaulting to INVERTED. "
+ + "Load table metadata from Doris 3.0+ for accurate index type
mapping.",
+ indexName);
+ return Index.IndexType.INVERTED;
+ }
+ switch (indexType.toUpperCase()) {
+ case "BTREE":
+ if ("PRIMARY".equals(indexName)) {
+ return Index.IndexType.PRIMARY_KEY;
+ }
+ return Index.IndexType.UNIQUE_KEY;
+ case "INVERTED":
+ return Index.IndexType.INVERTED;
+ case "BITMAP":
Review Comment:
Read maps `BITMAP`->`IndexType.BITMAP`, but the write path forces
`BITMAP`->`USING INVERTED` (around line 302). A BITMAP index round-tripped
through Gravitino silently becomes INVERTED. Pick one direction consistently,
ideally version-aware.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -233,23 +244,106 @@ private static void appendIndexesSql(Index[] indexes,
StringBuilder sqlBuilder)
return;
}
- // validate indexes
- Arrays.stream(indexes)
- .forEach(
- index -> {
- if (index.fieldNames().length > 1) {
- throw new IllegalArgumentException("Index does not support
multi fields in Doris");
- }
- });
+ // Log index types for debugging
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("appendIndexesSql: processing {} indexes", indexes.length);
+ for (Index index : indexes) {
+ LOG.debug(" index: name={}, type={}", index.name(), index.type());
+ }
+ }
- String indexSql =
+ // Filter out PRIMARY_KEY and UNIQUE_KEY indexes — they are table model
keys in Doris,
+ // defined via UNIQUE KEY(col) / DUPLICATE KEY(col) syntax, not via INDEX
clause.
+ List<Index> nonKeyIndexes =
Arrays.stream(indexes)
- .map(index -> String.format("INDEX %s (%s)", index.name(),
index.fieldNames()[0][0]))
+ .filter(
+ index ->
+ index.type() != Index.IndexType.PRIMARY_KEY
+ && index.type() != Index.IndexType.UNIQUE_KEY)
+ .collect(Collectors.toList());
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("appendIndexesSql: {} non-key indexes after filtering",
nonKeyIndexes.size());
+ }
+
+ if (nonKeyIndexes.isEmpty()) {
+ return;
+ }
+
+ nonKeyIndexes.forEach(
+ index -> {
+ if (index.fieldNames().length > 1) {
+ throw new IllegalArgumentException("Index does not support multi
fields in Doris");
+ }
+ });
+
+ String indexSql =
+ nonKeyIndexes.stream()
+ .map(
+ index -> {
+ String usingClause = mapIndexTypeToUsingClause(index.type());
+ return String.format(
+ "INDEX `%s` (`%s`) %s", index.name(),
index.fieldNames()[0][0], usingClause);
+ })
.collect(Collectors.joining(",\n"));
sqlBuilder.append(",").append(NEW_LINE).append(indexSql);
}
+ private static String mapIndexTypeToUsingClause(Index.IndexType indexType) {
+ switch (indexType) {
+ case PRIMARY_KEY:
+ case UNIQUE_KEY:
+ throw new IllegalStateException(
+ "PRIMARY_KEY and UNIQUE_KEY should be filtered out before index
SQL generation, got: "
+ + indexType);
+ case INVERTED:
+ return "USING INVERTED";
+ case BITMAP:
Review Comment:
This unconditionally emits `USING INVERTED` for every non-key index
(including BITMAP). There is no Doris server-version detection anywhere in the
catalog, so the "cross-version compatibility" here is really one hardcoded
choice applied to all versions. Two concerns:
1. Inverted index is not GA until Doris 2.0. On 1.2.x (still in the test
matrix as `VERSION_1_2`) `USING INVERTED` likely fails, whereas the previous
`INDEX name (col)` form defaulted to a bitmap index and worked — this looks
like a regression for the oldest supported version.
2. BITMAP is silently rewritten to INVERTED even on versions that still
support BITMAP (2.x/3.x), changing user intent. Note the read path
(`mapDorisIndexType`) maps `BITMAP`->`BITMAP`, so write/read are asymmetric.
Consider detecting the server version once and branching, or explicitly
narrowing the supported range. Please also verify against Doris docs: when
inverted GA'd, and whether 3.0+ truly converts BITMAP->INVERTED internally /
4.0.6 removed BITMAP.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -233,23 +244,106 @@ private static void appendIndexesSql(Index[] indexes,
StringBuilder sqlBuilder)
return;
}
- // validate indexes
- Arrays.stream(indexes)
- .forEach(
- index -> {
- if (index.fieldNames().length > 1) {
- throw new IllegalArgumentException("Index does not support
multi fields in Doris");
- }
- });
+ // Log index types for debugging
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("appendIndexesSql: processing {} indexes", indexes.length);
+ for (Index index : indexes) {
+ LOG.debug(" index: name={}, type={}", index.name(), index.type());
+ }
+ }
- String indexSql =
+ // Filter out PRIMARY_KEY and UNIQUE_KEY indexes — they are table model
keys in Doris,
+ // defined via UNIQUE KEY(col) / DUPLICATE KEY(col) syntax, not via INDEX
clause.
+ List<Index> nonKeyIndexes =
Arrays.stream(indexes)
- .map(index -> String.format("INDEX %s (%s)", index.name(),
index.fieldNames()[0][0]))
+ .filter(
+ index ->
+ index.type() != Index.IndexType.PRIMARY_KEY
+ && index.type() != Index.IndexType.UNIQUE_KEY)
+ .collect(Collectors.toList());
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("appendIndexesSql: {} non-key indexes after filtering",
nonKeyIndexes.size());
+ }
+
+ if (nonKeyIndexes.isEmpty()) {
+ return;
+ }
+
+ nonKeyIndexes.forEach(
+ index -> {
+ if (index.fieldNames().length > 1) {
+ throw new IllegalArgumentException("Index does not support multi
fields in Doris");
+ }
+ });
+
+ String indexSql =
+ nonKeyIndexes.stream()
+ .map(
+ index -> {
+ String usingClause = mapIndexTypeToUsingClause(index.type());
+ return String.format(
+ "INDEX `%s` (`%s`) %s", index.name(),
index.fieldNames()[0][0], usingClause);
+ })
.collect(Collectors.joining(",\n"));
sqlBuilder.append(",").append(NEW_LINE).append(indexSql);
}
+ private static String mapIndexTypeToUsingClause(Index.IndexType indexType) {
+ switch (indexType) {
+ case PRIMARY_KEY:
+ case UNIQUE_KEY:
+ throw new IllegalStateException(
+ "PRIMARY_KEY and UNIQUE_KEY should be filtered out before index
SQL generation, got: "
+ + indexType);
+ case INVERTED:
+ return "USING INVERTED";
+ case BITMAP:
+ // Doris 4.0.6 removed BITMAP index support; 3.0+ internally converts
BITMAP to INVERTED.
+ // Always generate USING INVERTED for cross-version compatibility.
+ return "USING INVERTED";
+ case VECTOR:
+ return "USING ANN";
+ default:
+ // Doris does not support BTREE as an explicit USING clause for
non-key indexes.
+ // Known types that reach here (e.g. BLOOMFILTER) are table-level
properties, not indexes.
+ throw new UnsupportedOperationException(
+ "Doris does not support index type " + indexType + " via ADD INDEX
syntax");
+ }
+ }
+
+ private static void appendTableModelKeySql(Index[] indexes, StringBuilder
sqlBuilder) {
Review Comment:
Validation/generation mismatch. The base `validateIncrementCol(columns,
indexes)` accepts an auto-increment column keyed by **PRIMARY_KEY or
UNIQUE_KEY**, but this method only emits `UNIQUE KEY(...)` for UNIQUE_KEY and
silently skips PRIMARY_KEY. So a table with an auto-inc column + PRIMARY_KEY
passes validation yet generates DDL with no key (DUPLICATE model): the key
intent is lost and Doris may reject the auto-inc column. Either map
PRIMARY_KEY->`UNIQUE KEY` on supported versions, or reject it clearly — don't
accept in validation and drop in generation. (Also a behavior change vs. the
old code, which emitted PRIMARY_KEY as an INDEX; uniqueness is now silently
dropped on create.)
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -233,23 +244,106 @@ private static void appendIndexesSql(Index[] indexes,
StringBuilder sqlBuilder)
return;
}
- // validate indexes
- Arrays.stream(indexes)
- .forEach(
- index -> {
- if (index.fieldNames().length > 1) {
- throw new IllegalArgumentException("Index does not support
multi fields in Doris");
- }
- });
+ // Log index types for debugging
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("appendIndexesSql: processing {} indexes", indexes.length);
+ for (Index index : indexes) {
+ LOG.debug(" index: name={}, type={}", index.name(), index.type());
+ }
+ }
- String indexSql =
+ // Filter out PRIMARY_KEY and UNIQUE_KEY indexes — they are table model
keys in Doris,
+ // defined via UNIQUE KEY(col) / DUPLICATE KEY(col) syntax, not via INDEX
clause.
+ List<Index> nonKeyIndexes =
Arrays.stream(indexes)
- .map(index -> String.format("INDEX %s (%s)", index.name(),
index.fieldNames()[0][0]))
+ .filter(
+ index ->
+ index.type() != Index.IndexType.PRIMARY_KEY
+ && index.type() != Index.IndexType.UNIQUE_KEY)
+ .collect(Collectors.toList());
+
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("appendIndexesSql: {} non-key indexes after filtering",
nonKeyIndexes.size());
+ }
+
+ if (nonKeyIndexes.isEmpty()) {
+ return;
+ }
+
+ nonKeyIndexes.forEach(
+ index -> {
+ if (index.fieldNames().length > 1) {
+ throw new IllegalArgumentException("Index does not support multi
fields in Doris");
+ }
+ });
+
+ String indexSql =
+ nonKeyIndexes.stream()
+ .map(
+ index -> {
+ String usingClause = mapIndexTypeToUsingClause(index.type());
+ return String.format(
+ "INDEX `%s` (`%s`) %s", index.name(),
index.fieldNames()[0][0], usingClause);
+ })
.collect(Collectors.joining(",\n"));
sqlBuilder.append(",").append(NEW_LINE).append(indexSql);
}
+ private static String mapIndexTypeToUsingClause(Index.IndexType indexType) {
+ switch (indexType) {
+ case PRIMARY_KEY:
+ case UNIQUE_KEY:
+ throw new IllegalStateException(
+ "PRIMARY_KEY and UNIQUE_KEY should be filtered out before index
SQL generation, got: "
+ + indexType);
+ case INVERTED:
+ return "USING INVERTED";
+ case BITMAP:
+ // Doris 4.0.6 removed BITMAP index support; 3.0+ internally converts
BITMAP to INVERTED.
+ // Always generate USING INVERTED for cross-version compatibility.
+ return "USING INVERTED";
+ case VECTOR:
+ return "USING ANN";
+ default:
+ // Doris does not support BTREE as an explicit USING clause for
non-key indexes.
+ // Known types that reach here (e.g. BLOOMFILTER) are table-level
properties, not indexes.
+ throw new UnsupportedOperationException(
+ "Doris does not support index type " + indexType + " via ADD INDEX
syntax");
+ }
+ }
+
+ private static void appendTableModelKeySql(Index[] indexes, StringBuilder
sqlBuilder) {
+ // Only emit UNIQUE KEY declaration when an explicit UNIQUE_KEY index is
present.
+ // PRIMARY_KEY indexes are intentionally skipped: Doris 1.2.x does not
support UNIQUE KEY
+ // syntax (only DUPLICATE KEY / AGGREGATE KEY), and the default DUPLICATE
KEY model already
+ // satisfies PRIMARY_KEY's uniqueness semantics. Users targeting Doris
2.0+ who need the
+ // UNIQUE KEY model should pass Index.IndexType.UNIQUE_KEY instead.
+ long keyIndexCount =
+ Arrays.stream(indexes)
+ .filter(
+ index ->
+ index.type() == Index.IndexType.PRIMARY_KEY
+ || index.type() == Index.IndexType.UNIQUE_KEY)
+ .count();
+ Preconditions.checkArgument(
+ keyIndexCount <= 1,
+ "Doris table model key can have at most one PRIMARY_KEY or UNIQUE_KEY
index, got: %s",
+ keyIndexCount);
+
+ Arrays.stream(indexes)
+ .filter(index -> index.type() == Index.IndexType.UNIQUE_KEY)
+ .findFirst()
+ .ifPresent(
+ keyIndex -> {
+ String cols =
+ Arrays.stream(keyIndex.fieldNames())
+ .map(field -> BACK_QUOTE + field[0] + BACK_QUOTE)
+ .collect(Collectors.joining(", "));
+ sqlBuilder.append(NEW_LINE).append("UNIQUE
KEY(").append(cols).append(")");
Review Comment:
Doris requires key columns to be an ordered prefix of the schema. This emits
`UNIQUE KEY(col)` without validating/reordering column position, so a key
column that isn't first produces invalid DDL (Doris: "Key columns should be a
ordered prefix of the schema"). At minimum document the constraint, or validate
it.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -387,15 +481,67 @@ protected List<Index> getIndexes(Connection connection,
String databaseName, Str
while (resultSet.next()) {
String indexName = resultSet.getString("Key_name");
String columnName = resultSet.getString("Column_name");
- indexes.add(
- Indexes.of(Index.IndexType.PRIMARY_KEY, indexName, new String[][]
{{columnName}}));
+ // Index_type column may not exist in older Doris versions (e.g.
1.2.x).
+ // Fall back to null so mapDorisIndexType can infer from indexName.
+ String indexType;
+ try {
+ indexType = resultSet.getString("Index_type");
+ } catch (SQLException e) {
+ LOG.warn(
+ "Index_type column not available in SHOW INDEX output, "
+ + "inferring index type from index name '{}'. "
+ + "Upgrade to Doris 3.0+ for accurate index type mapping.",
+ indexName);
+ indexType = null;
+ }
+ Index.IndexType gravitinoIndexType = mapDorisIndexType(indexType,
indexName);
+ indexes.add(Indexes.of(gravitinoIndexType, indexName, new String[][]
{{columnName}}));
}
return indexes;
} catch (SQLException e) {
throw exceptionMapper.toGravitinoException(e);
}
}
+ @VisibleForTesting
+ static Index.IndexType mapDorisIndexType(String indexType, String indexName)
{
+ if (indexType == null) {
+ // Index_type column unavailable (Doris 1.2.x) or returned null.
+ // Infer from index name: "PRIMARY" is the primary key, everything else
defaults to
+ // INVERTED as a safe fallback (Doris 1.2.x indexes are all BTREE-based
key indexes,
+ // but without Index_type we cannot distinguish UNIQUE_KEY from
PRIMARY_KEY).
+ if ("PRIMARY".equals(indexName)) {
+ return Index.IndexType.PRIMARY_KEY;
+ }
+ LOG.warn(
+ "Index_type is null for index '{}', defaulting to INVERTED. "
+ + "Load table metadata from Doris 3.0+ for accurate index type
mapping.",
+ indexName);
+ return Index.IndexType.INVERTED;
Review Comment:
Inconsistent mapping for the same physical index: when `Index_type` is
present and `BTREE` (non-PRIMARY) you return `UNIQUE_KEY` (line 527), but when
the column is absent (1.2.x, where everything is BTREE) the same index defaults
to `INVERTED` here. For consistency the null branch should default to
`UNIQUE_KEY`, matching the BTREE case the comment describes.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -87,7 +87,21 @@ protected String generateCreateTableSql(
Distribution distribution,
Index[] indexes) {
- validateIncrementCol(columns);
+ // Log index information for debugging
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "generateCreateTableSql: tableName={}, indexes.length={}",
tableName, indexes.length);
+ for (int i = 0; i < indexes.length; i++) {
+ LOG.debug(
+ " indexes[{}]: name={}, type={}, type.name()={}",
+ i,
+ indexes[i].name(),
+ indexes[i].type(),
+ indexes[i].type().name());
+ }
+ }
+
+ validateIncrementCol(columns, indexes);
Review Comment:
Delegating to the base validator is fine, but it also removes the only
version guard: Doris auto-increment requires 2.1.0+. On older servers the
column now reaches Doris and fails with a less clear server-side error (the old
code gave a clear message). The title says 3.0+, but this affects 1.2.x/2.0.x
too — consider a clear lower-bound check or documenting the minimum.
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/DorisImageName.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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;
+
+/**
+ * Doris Docker image versions for multi-version integration testing.
+ *
+ * <p>{@link #VERSION_1_2} uses the Gravitino CI all-in-one image (FE + BE in
a single image) for
+ * backward-compatible testing against Doris 1.2.x.
+ *
+ * <p>{@link #VERSION_3_0} uses the official Apache Doris split images ({@code
apache/doris:fe-3.x}
+ * and {@code apache/doris:be-3.x}). {@link DorisContainer} detects the {@code
:fe-} prefix and
+ * automatically derives the corresponding BE image, passing both via compose
environment variables.
+ *
+ * @see DorisContainer
+ * @see ContainerSuite#startDorisContainer(DorisImageName)
+ */
+public enum DorisImageName {
+ VERSION_1_2("apache/gravitino-ci:doris-0.1.5"),
+ VERSION_3_0("apache/doris:fe-3.0.6.2");
Review Comment:
The PR description says integration tests ran on Doris 4.0.6 and 3.0.6.2,
but the image matrix only has `VERSION_1_2` and `VERSION_3_0` (3.0.6.2) — no
4.0.6. The BITMAP->INVERTED logic is justified by "4.0.6 removed BITMAP", yet
4.0.6 isn't in the automated matrix, so that version-specific behavior isn't
actually covered. Either add a 4.0.6 image or adjust the description.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -87,7 +87,21 @@ protected String generateCreateTableSql(
Distribution distribution,
Index[] indexes) {
- validateIncrementCol(columns);
+ // Log index information for debugging
Review Comment:
Minor: this debug block logs both `index.type()` and `index.type().name()`
(redundant), and there is a second similar loop in `appendIndexesSql`. Consider
trimming to one concise line.
--
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]
