kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377761570
##########
processing/src/main/java/org/apache/druid/metadata/MetadataStorageTablesConfig.java:
##########
@@ -101,6 +106,7 @@ public MetadataStorageTablesConfig(
this.auditTable = makeTableName(auditTable, "audit");
this.supervisorTable = makeTableName(supervisorTable, "supervisors");
this.segmentSchemasTable = makeTableName(segmentSchemasTable,
"segmentSchemas");
+ this.useUniqueIndexNames = Configs.valueOrDefault(useUniqueIndexNames,
Boolean.FALSE);
Review Comment:
```suggestion
this.useUniqueIndexNames = Configs.valueOrDefault(useUniqueIndexNames,
false);
```
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java:
##########
@@ -305,6 +307,170 @@ public void testIsTransientException()
);
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void test_tableIndices_areNotReplaced_ifExist(boolean
useUniqueIndexName)
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ useUniqueIndexName
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+ final List<String> legacyIndexTemplates = Arrays.asList(
+ "idx_%1$s_used",
+ "idx_%1$s_datasource_used_end_start",
+ "idx_%1$s_datasource_upgraded_from_segment_id"
+ );
+ final List<List<String>> indexColumns = List.of(
+ List.of("used"),
+ List.of(
+ "dataSource",
+ "used",
+ StringUtils.format("%send%S", connector.getQuoteString(),
connector.getQuoteString()),
Review Comment:
Nit: Why capitalize the second `%S`?
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,43 +1168,51 @@ public ResultSet getIndexInfo(DatabaseMetaData
databaseMetaData, String tableNam
}
/**
- * create index on the table with retry if not already exist, to be called
after createTable
+ * Create index on the table with retry if not already exist, to be called
after createTable
+ * Format of index name is either specified via legacy {@param
legacyIndexNameFormat} or {@code generateSHABasedIndexIdentifier}.
*
* @param tableName Name of the table to create index on
- * @param indexName case-insensitive string index name, it helps to
check the existing index on table
- * @param indexCols List of columns to be indexed on
+ * @param legacyIndexNameFormat Template to create index ID (nullable for
forwards compatibility with SHA-based indices)
+ * @param indexCols List of un-escaped columns to be indexed on
(case-sensitive).
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String legacyIndexNameFormat,
final List<String> indexCols,
final Set<String> createdIndexSet
)
{
+ final String shaIndexName =
StringUtils.toUpperCase(generateUniqueIndexName(tableName, indexCols));
+ final String legacyIndexName = legacyIndexNameFormat != null
Review Comment:
Since we use the `legacyIndexName` to ensure that the legacy index doesn't
already exist,
I think the `legacyIndexNameFormat` should never be null.
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1276,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof
StatementException);
}
+
+ /**
+ * Creates a unique index name of the format {@code idx_<tableName>_<SHA of
table name + column list>}
+ * with an SHA length of {@value MAX_INDEX_HASH_LENGTH}.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on
(case-insensitive)
+ * @return unique index identifier
+ */
+ protected String generateUniqueIndexName(String tableName, List<String>
columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String tableAndColumnDigest = DigestUtils.sha1Hex(
+ StringUtils.format(
+ "%s_%s",
+ tableName,
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_")
Review Comment:
This may cause duplicates since `_` can occur in a column name.
Better use a character that cannot occur in a column name like comma or
something.
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java:
##########
@@ -305,6 +307,170 @@ public void testIsTransientException()
);
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void test_tableIndices_areNotReplaced_ifExist(boolean
useUniqueIndexName)
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ useUniqueIndexName
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+ final List<String> legacyIndexTemplates = Arrays.asList(
+ "idx_%1$s_used",
+ "idx_%1$s_datasource_used_end_start",
+ "idx_%1$s_datasource_upgraded_from_segment_id"
+ );
+ final List<List<String>> indexColumns = List.of(
+ List.of("used"),
+ List.of(
+ "dataSource",
+ "used",
+ StringUtils.format("%send%S", connector.getQuoteString(),
connector.getQuoteString()),
+ "start"
+ ),
+ List.of("dataSource", "upgraded_from_segment_id")
+ );
+ final List<String> legacyIndexNames = legacyIndexTemplates.stream()
+ .map(template ->
StringUtils.toUpperCase(StringUtils.format(
+ template,
+ segmentsTable
+ )))
+
.collect(Collectors.toList());
+ final List<String> uniqueIndexNames = indexColumns.stream()
+ .map(cols ->
connector.generateUniqueIndexName(
+ segmentsTable,
+ cols
+ ))
+
.collect(Collectors.toList());
+
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+
+ // drop legacy/unique indices and create the other type
+ connector.getDBI().withHandle(handle -> {
+ for (int i = 0; i < legacyIndexNames.size(); i++) {
+ handle.execute(StringUtils.toUpperCase(StringUtils.format(
+ "DROP INDEX %s",
+ useUniqueIndexName ? uniqueIndexNames.get(i) :
legacyIndexNames.get(i)
+ )));
+ handle.execute(StringUtils.format(
+ "CREATE INDEX %s ON %s(%s)",
+ useUniqueIndexName ? legacyIndexNames.get(i) :
uniqueIndexNames.get(i),
+ segmentsTable,
+ String.join(",", indexColumns.get(i))
+ ));
+ }
+ return null;
+ });
+
+ // run again to index name checks are run and duplicate indices are not
created
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+ assertIndicesPresentOnTable(segmentsTable, useUniqueIndexName ?
legacyIndexNames : uniqueIndexNames);
+
+ dropTable(segmentsTable);
+ connector.tearDown();
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void test_tableIndices_areReplaced_IfNotExist(boolean
useUniqueIndexName)
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ useUniqueIndexName
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+ final List<String> legacyIndexTemplates = Arrays.asList(
+ "idx_%1$s_used",
+ "idx_%1$s_datasource_used_end_start",
+ "idx_%1$s_datasource_upgraded_from_segment_id"
+ );
+ final List<List<String>> indexColumns = List.of(
+ List.of("used"),
+ List.of(
+ "dataSource",
+ "used",
+ StringUtils.format("%send%S", connector.getQuoteString(),
connector.getQuoteString()),
+ "start"
+ ),
+ List.of("dataSource", "upgraded_from_segment_id")
+ );
Review Comment:
Since this is a test setup, we shouldn't have to go through the whole flow
of generating index names.\
Instead, we should just do something like:
```java
connector.createSegmentsTable(...);
verifyIndicesPresentOnTable(
"IDX_ABC",
"IDX_DEF",
"IDX_XYZ
)
```
This would make the tests much more readable.
It is okay even if we have to change the test when the logic in the
`SQLMetadataConnector.generateIndexName()` (in fact, it is desirable).
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java:
##########
@@ -305,6 +307,170 @@ public void testIsTransientException()
);
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void test_tableIndices_areNotReplaced_ifExist(boolean
useUniqueIndexName)
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ useUniqueIndexName
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+ final List<String> legacyIndexTemplates = Arrays.asList(
+ "idx_%1$s_used",
+ "idx_%1$s_datasource_used_end_start",
+ "idx_%1$s_datasource_upgraded_from_segment_id"
+ );
+ final List<List<String>> indexColumns = List.of(
+ List.of("used"),
+ List.of(
+ "dataSource",
+ "used",
+ StringUtils.format("%send%S", connector.getQuoteString(),
connector.getQuoteString()),
+ "start"
+ ),
+ List.of("dataSource", "upgraded_from_segment_id")
+ );
+ final List<String> legacyIndexNames = legacyIndexTemplates.stream()
+ .map(template ->
StringUtils.toUpperCase(StringUtils.format(
+ template,
+ segmentsTable
+ )))
+
.collect(Collectors.toList());
+ final List<String> uniqueIndexNames = indexColumns.stream()
+ .map(cols ->
connector.generateUniqueIndexName(
+ segmentsTable,
+ cols
+ ))
+
.collect(Collectors.toList());
+
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+
+ // drop legacy/unique indices and create the other type
+ connector.getDBI().withHandle(handle -> {
+ for (int i = 0; i < legacyIndexNames.size(); i++) {
+ handle.execute(StringUtils.toUpperCase(StringUtils.format(
+ "DROP INDEX %s",
Review Comment:
We should avoid these formatters and instead just drop some of the indexes
(preferably not all so that we can verify the difference in behaviour)
e.g. `DROP INDEX IDX_ABC`.
##########
processing/src/main/java/org/apache/druid/metadata/MetadataStorageTablesConfig.java:
##########
@@ -72,6 +73,9 @@ public static MetadataStorageTablesConfig fromBase(String
base)
@JsonProperty("segmentSchemas")
private final String segmentSchemasTable;
+ @JsonProperty("useUniqueIndexNames")
+ private final Boolean useUniqueIndexNames;
Review Comment:
Since this will always be non-null
```suggestion
private final boolean useUniqueIndexNames;
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,43 +1168,51 @@ public ResultSet getIndexInfo(DatabaseMetaData
databaseMetaData, String tableNam
}
/**
- * create index on the table with retry if not already exist, to be called
after createTable
+ * Create index on the table with retry if not already exist, to be called
after createTable
+ * Format of index name is either specified via legacy {@param
legacyIndexNameFormat} or {@code generateSHABasedIndexIdentifier}.
*
* @param tableName Name of the table to create index on
- * @param indexName case-insensitive string index name, it helps to
check the existing index on table
- * @param indexCols List of columns to be indexed on
+ * @param legacyIndexNameFormat Template to create index ID (nullable for
forwards compatibility with SHA-based indices)
Review Comment:
Side note (non blocking):
there can be another use for this nullability (assuming we don't need it for
forward compat).
```java
String legacyIndexName = legacyIndexNameFormat == null
? upperCase(StringUtils.format("IDX_%s_%s", tableName, columns.join(`_`)))
: upperCase(StringUtils.format("IDX_%s", tableName);
```
This will help simplify arguments to `createIndex` method. What do you think?
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1276,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof
StatementException);
}
+
+ /**
+ * Creates a unique index name of the format {@code idx_<tableName>_<SHA of
table name + column list>}
+ * with an SHA length of {@value MAX_INDEX_HASH_LENGTH}.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on
(case-insensitive)
+ * @return unique index identifier
+ */
+ protected String generateUniqueIndexName(String tableName, List<String>
columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String tableAndColumnDigest = DigestUtils.sha1Hex(
+ StringUtils.format(
+ "%s_%s",
+ tableName,
Review Comment:
I recall I had advised adding the table name in the generation of the digest.
But now since we are already prefixing the table name, we may omit this.
##########
processing/src/main/java/org/apache/druid/metadata/MetadataStorageTablesConfig.java:
##########
@@ -174,4 +180,9 @@ public String getSegmentSchemasTable()
{
return segmentSchemasTable;
}
+
+ public boolean getUseUniqueIndexNames()
Review Comment:
```suggestion
public boolean isUseUniqueIndexNames()
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,43 +1168,51 @@ public ResultSet getIndexInfo(DatabaseMetaData
databaseMetaData, String tableNam
}
/**
- * create index on the table with retry if not already exist, to be called
after createTable
+ * Create index on the table with retry if not already exist, to be called
after createTable
+ * Format of index name is either specified via legacy {@param
legacyIndexNameFormat} or {@code generateSHABasedIndexIdentifier}.
*
* @param tableName Name of the table to create index on
- * @param indexName case-insensitive string index name, it helps to
check the existing index on table
- * @param indexCols List of columns to be indexed on
+ * @param legacyIndexNameFormat Template to create index ID (nullable for
forwards compatibility with SHA-based indices)
Review Comment:
How does making this nullable help with forwards compat?
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1276,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof
StatementException);
}
+
+ /**
+ * Creates a unique index name of the format {@code idx_<tableName>_<SHA of
table name + column list>}
+ * with an SHA length of {@value MAX_INDEX_HASH_LENGTH}.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on
(case-insensitive)
+ * @return unique index identifier
+ */
+ protected String generateUniqueIndexName(String tableName, List<String>
columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
Review Comment:
Nit: doesn't really affect functionality but probably helps with visualizing
the final index names.
```suggestion
final String prefix = "IDX_" + tableName + "_";
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,43 +1168,51 @@ public ResultSet getIndexInfo(DatabaseMetaData
databaseMetaData, String tableNam
}
/**
- * create index on the table with retry if not already exist, to be called
after createTable
+ * Create index on the table with retry if not already exist, to be called
after createTable
+ * Format of index name is either specified via legacy {@param
legacyIndexNameFormat} or {@code generateSHABasedIndexIdentifier}.
*
* @param tableName Name of the table to create index on
- * @param indexName case-insensitive string index name, it helps to
check the existing index on table
- * @param indexCols List of columns to be indexed on
+ * @param legacyIndexNameFormat Template to create index ID (nullable for
forwards compatibility with SHA-based indices)
+ * @param indexCols List of un-escaped columns to be indexed on
(case-sensitive).
Review Comment:
```suggestion
* @param indexCols List of un-escaped column names to be indexed on
(case-sensitive).
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1038,18 +1089,29 @@ public void createSegmentSchemaTable(final String
tableName)
+ " UNIQUE (fingerprint) \n"
+ ")",
tableName, getSerialType(), getPayloadType()
- ),
- StringUtils.format("CREATE INDEX idx_%1$s_fingerprint ON
%1$s(fingerprint)", tableName),
- StringUtils.format("CREATE INDEX idx_%1$s_used ON %1$s(used,
used_status_last_updated)", tableName)
+ )
)
);
+ final Set<String> createdIndexSet = getIndexOnTable(tableName);
+ createIndex(
+ tableName,
+ "idx_%s_fingerprint",
Review Comment:
Since we are touching this anyway, should we capitalize the index name
formats?
I feel like it helps with visualizing the actual index name.
```suggestion
"IDX_%s_FINGERPRINT",
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -371,15 +378,28 @@ public void createSegmentTable(final String tableName)
StringUtils.format(
createStatementBuilder.toString(),
tableName, getPayloadType(), getQuoteString(), getCollation()
- ),
- StringUtils.format("CREATE INDEX idx_%1$s_used ON %1$s(used)",
tableName),
- StringUtils.format(
- "CREATE INDEX idx_%1$s_datasource_used_end_start ON
%1$s(dataSource, used, %2$send%2$s, start)",
- tableName,
- getQuoteString()
)
)
);
+
+ final Set<String> createdIndexSet = getIndexOnTable(tableName);
+ createIndex(
+ tableName,
+ "idx_%1$s_used",
+ ImmutableList.of("used"),
+ createdIndexSet
+ );
+ createIndex(
+ tableName,
+ "idx_%1$s_datasource_used_end_start",
+ ImmutableList.of(
+ "dataSource",
+ "used",
+ StringUtils.format("%send%S", getQuoteString(), getQuoteString()),
Review Comment:
I see. Thanks for checking.
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -371,15 +378,28 @@ public void createSegmentTable(final String tableName)
StringUtils.format(
createStatementBuilder.toString(),
tableName, getPayloadType(), getQuoteString(), getCollation()
- ),
- StringUtils.format("CREATE INDEX idx_%1$s_used ON %1$s(used)",
tableName),
- StringUtils.format(
- "CREATE INDEX idx_%1$s_datasource_used_end_start ON
%1$s(dataSource, used, %2$send%2$s, start)",
- tableName,
- getQuoteString()
)
)
);
+
+ final Set<String> createdIndexSet = getIndexOnTable(tableName);
+ createIndex(
+ tableName,
+ "idx_%s_used",
+ List.of("used"),
+ createdIndexSet
+ );
+ createIndex(
+ tableName,
+ "idx_%s_datasource_used_end_start",
+ List.of(
+ "dataSource",
+ "used",
+ StringUtils.format("%send%S", getQuoteString(), getQuoteString()),
Review Comment:
```suggestion
StringUtils.format("%1$send%1$s", getQuoteString()),
```
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java:
##########
@@ -305,6 +307,170 @@ public void testIsTransientException()
);
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void test_tableIndices_areNotReplaced_ifExist(boolean
useUniqueIndexName)
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ useUniqueIndexName
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+ final List<String> legacyIndexTemplates = Arrays.asList(
+ "idx_%1$s_used",
+ "idx_%1$s_datasource_used_end_start",
+ "idx_%1$s_datasource_upgraded_from_segment_id"
+ );
+ final List<List<String>> indexColumns = List.of(
+ List.of("used"),
+ List.of(
+ "dataSource",
+ "used",
+ StringUtils.format("%send%S", connector.getQuoteString(),
connector.getQuoteString()),
Review Comment:
Suggestion (doesn't need to be in this PR):
To simplify this, we should probably add a method to `SQLMetadataConnector`,
```java
public String quote(String column)
{
return StringUtils.format("%1$s%2$s%1$s", getQuoteString(), column);
}
```
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java:
##########
@@ -305,6 +307,170 @@ public void testIsTransientException()
);
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void test_tableIndices_areNotReplaced_ifExist(boolean
useUniqueIndexName)
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ useUniqueIndexName
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+ final List<String> legacyIndexTemplates = Arrays.asList(
+ "idx_%1$s_used",
+ "idx_%1$s_datasource_used_end_start",
+ "idx_%1$s_datasource_upgraded_from_segment_id"
+ );
+ final List<List<String>> indexColumns = List.of(
+ List.of("used"),
+ List.of(
+ "dataSource",
+ "used",
+ StringUtils.format("%send%S", connector.getQuoteString(),
connector.getQuoteString()),
+ "start"
+ ),
+ List.of("dataSource", "upgraded_from_segment_id")
+ );
+ final List<String> legacyIndexNames = legacyIndexTemplates.stream()
+ .map(template ->
StringUtils.toUpperCase(StringUtils.format(
+ template,
+ segmentsTable
+ )))
+
.collect(Collectors.toList());
+ final List<String> uniqueIndexNames = indexColumns.stream()
+ .map(cols ->
connector.generateUniqueIndexName(
+ segmentsTable,
+ cols
+ ))
+
.collect(Collectors.toList());
+
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+
+ // drop legacy/unique indices and create the other type
+ connector.getDBI().withHandle(handle -> {
+ for (int i = 0; i < legacyIndexNames.size(); i++) {
+ handle.execute(StringUtils.toUpperCase(StringUtils.format(
+ "DROP INDEX %s",
+ useUniqueIndexName ? uniqueIndexNames.get(i) :
legacyIndexNames.get(i)
+ )));
+ handle.execute(StringUtils.format(
+ "CREATE INDEX %s ON %s(%s)",
+ useUniqueIndexName ? legacyIndexNames.get(i) :
uniqueIndexNames.get(i),
+ segmentsTable,
+ String.join(",", indexColumns.get(i))
+ ));
+ }
+ return null;
+ });
+
+ // run again to index name checks are run and duplicate indices are not
created
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+ assertIndicesPresentOnTable(segmentsTable, useUniqueIndexName ?
legacyIndexNames : uniqueIndexNames);
+
+ dropTable(segmentsTable);
+ connector.tearDown();
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void test_tableIndices_areReplaced_IfNotExist(boolean
useUniqueIndexName)
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ null,
+ useUniqueIndexName
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+ final List<String> legacyIndexTemplates = Arrays.asList(
+ "idx_%1$s_used",
+ "idx_%1$s_datasource_used_end_start",
+ "idx_%1$s_datasource_upgraded_from_segment_id"
+ );
+ final List<List<String>> indexColumns = List.of(
+ List.of("used"),
+ List.of(
+ "dataSource",
+ "used",
+ StringUtils.format("%send%S", connector.getQuoteString(),
connector.getQuoteString()),
+ "start"
+ ),
+ List.of("dataSource", "upgraded_from_segment_id")
+ );
Review Comment:
Please simplify the other test too.
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java:
##########
@@ -305,6 +307,170 @@ public void testIsTransientException()
);
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
Review Comment:
These should be separate test cases.
##########
processing/src/main/java/org/apache/druid/metadata/MetadataStorageTablesConfig.java:
##########
@@ -72,6 +73,9 @@ public static MetadataStorageTablesConfig fromBase(String
base)
@JsonProperty("segmentSchemas")
private final String segmentSchemasTable;
+ @JsonProperty("useUniqueIndexNames")
+ private final Boolean useUniqueIndexNames;
Review Comment:
Setting this flag to false would imply that index names may not be unique
anymore which is not exactly right.
Maybe use some other name like `useShortIndexNames` or `useShaIndexNames` or
something?
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -371,15 +378,28 @@ public void createSegmentTable(final String tableName)
StringUtils.format(
createStatementBuilder.toString(),
tableName, getPayloadType(), getQuoteString(), getCollation()
- ),
- StringUtils.format("CREATE INDEX idx_%1$s_used ON %1$s(used)",
tableName),
- StringUtils.format(
- "CREATE INDEX idx_%1$s_datasource_used_end_start ON
%1$s(dataSource, used, %2$send%2$s, start)",
- tableName,
- getQuoteString()
)
)
);
+
+ final Set<String> createdIndexSet = getIndexOnTable(tableName);
+ createIndex(
+ tableName,
+ "idx_%s_used",
+ List.of("used"),
+ createdIndexSet
+ );
+ createIndex(
+ tableName,
+ "idx_%s_datasource_used_end_start",
+ List.of(
+ "dataSource",
+ "used",
+ StringUtils.format("%send%S", getQuoteString(), getQuoteString()),
Review Comment:
Why capitalize the second `%S`?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
