kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2383788944
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1038,18 +1074,26 @@ 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)
+ )
)
);
+ createIndex(
+ tableName,
+ "IDX_%s_FINGERPRINT",
Review Comment:
Should we update this one and other cases to pass null here?
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java:
##########
@@ -305,6 +303,140 @@ public void testIsTransientException()
);
}
+ @Test
+ public void test_useShortIndexNames_true_tableIndices_areNotAdded_ifExist()
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ "druidTest_base_table_name_true",
+ null, null, null, null, null, null, null, null, null, null, null,
+ true
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+ connector.getDBI().withHandle(handle -> {
+ handle.execute("DROP INDEX
IDX_B859F28F30657A58518416E445E0AA1A78A68177");
+ handle.execute(StringUtils.format("CREATE INDEX IDX_%1$s_USED ON
%1$s(used)", segmentsTable));
+ return null;
+ });
+
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+
+ final Set<String> expectedIndices = Sets.newHashSet(
+ StringUtils.format("IDX_%S_USED", segmentsTable),
+ "IDX_3A789D647A2A70597C2AA5CB1DC4E0F231529B75",
+ "IDX_00F9CD53AC9353C0E98B554ECD8CF5A84D28370F"
+ );
+ assertIndicesPresentOnTable(segmentsTable, expectedIndices);
+
+ dropTable(segmentsTable);
+ connector.tearDown();
+ }
+
+ @Test
+ public void test_useShortIndexNames_false_tableIndices_areNotAdded_ifExist()
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ "druidTest_base_table_name",
+ null, null, null, null, null, null, null, null, null, null, null,
+ false
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+ connector.getDBI().withHandle(handle -> {
+ handle.execute(StringUtils.format("DROP INDEX IDX_%s_USED",
segmentsTable));
+ handle.execute(StringUtils.format(
+ "CREATE INDEX IDX_3E94AF560719CBA0E08C0B35FB5DF8079DC6D595 ON
%1$s(used)",
+ segmentsTable
+ ));
+ return null;
+ });
+
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+
+ final Set<String> expectedIndices = Sets.newHashSet(
+ "IDX_3E94AF560719CBA0E08C0B35FB5DF8079DC6D595",
+ StringUtils.format("IDX_%S_DATASOURCE_USED_END_START", segmentsTable),
+ StringUtils.format("IDX_%S_DATASOURCE_UPGRADED_FROM_SEGMENT_ID",
segmentsTable)
+ );
+ assertIndicesPresentOnTable(segmentsTable, expectedIndices);
+
+ dropTable(segmentsTable);
+ connector.tearDown();
+ }
+
+ @Test
+ public void test_useShortIndexNames_true_tableIndices_areAdded_IfNotExist()
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ "druidTest_base_table_name_true",
+ null, null, null, null, null, null, null, null, null, null, null,
+ true
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+
+ final Set<String> expectedIndices = Sets.newHashSet(
+ "IDX_B859F28F30657A58518416E445E0AA1A78A68177",
+ "IDX_3A789D647A2A70597C2AA5CB1DC4E0F231529B75",
+ "IDX_00F9CD53AC9353C0E98B554ECD8CF5A84D28370F"
+ );
+
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+
+ assertIndicesPresentOnTable(segmentsTable, expectedIndices);
+ dropTable(segmentsTable);
+ connector.tearDown();
+ }
+
+ @Test
+ public void test_useShortIndexNames_false_tableIndices_areAdded_IfNotExist()
+ {
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+
+ final Set<String> expectedIndices = Sets.newHashSet(
+ StringUtils.format("IDX_%S_USED", segmentsTable),
Review Comment:
Please try to use the final index names here as well.
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java:
##########
@@ -305,6 +303,140 @@ public void testIsTransientException()
);
}
+ @Test
+ public void test_useShortIndexNames_true_tableIndices_areNotAdded_ifExist()
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ "druidTest_base_table_name_true",
Review Comment:
Let's use a shorter base name here to aid with the next comments:
```suggestion
"druidTest",
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,46 +1150,56 @@ 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 {@code tableName} with retry if not already
exist, to be called after {@link #createTable}.
+ * Format of index name is either specified via legacy {@code
legacyIndexNameFormat} or {@link #generateShortIndexName}.
*
- * @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 createdIndexSet
+ * @param tableName Name of the table to create index on
+ * @param legacyIndexNameFormat Template to create index ID. If null, uses
the format: {@code IDX_{table name}_{columns}}.
+ * @param indexCols List of un-escaped column names to be
indexed on (case-sensitive).
*/
public void createIndex(
final String tableName,
- final String indexName,
- final List<String> indexCols,
- final Set<String> createdIndexSet
+ final String legacyIndexNameFormat,
+ final List<String> indexCols
)
{
+ final Set<String> createdIndexSet = getIndexOnTable(tableName);
+ final String shortIndexName = generateShortIndexName(tableName, indexCols);
+ String legacyIndexName = StringUtils.toUpperCase(legacyIndexNameFormat !=
null
Review Comment:
```suggestion
final String legacyIndexName =
StringUtils.toUpperCase(legacyIndexNameFormat != null
```
##########
processing/src/main/java/org/apache/druid/metadata/MetadataStorageTablesConfig.java:
##########
@@ -174,4 +180,9 @@ public String getSegmentSchemasTable()
{
return segmentSchemasTable;
}
+
+ public boolean isUseShortIndexNames()
Review Comment:
If you are making further commits, please add a short javadoc here
mentioning that if enabled, this causes the tables to use SHA based index names
etc.
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,46 +1150,56 @@ 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 {@code tableName} with retry if not already
exist, to be called after {@link #createTable}.
+ * Format of index name is either specified via legacy {@code
legacyIndexNameFormat} or {@link #generateShortIndexName}.
*
- * @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 createdIndexSet
+ * @param tableName Name of the table to create index on
+ * @param legacyIndexNameFormat Template to create index ID. If null, uses
the format: {@code IDX_{table name}_{columns}}.
+ * @param indexCols List of un-escaped column names to be
indexed on (case-sensitive).
*/
public void createIndex(
final String tableName,
- final String indexName,
- final List<String> indexCols,
- final Set<String> createdIndexSet
+ final String legacyIndexNameFormat,
+ final List<String> indexCols
)
{
+ final Set<String> createdIndexSet = getIndexOnTable(tableName);
+ final String shortIndexName = generateShortIndexName(tableName, indexCols);
+ String legacyIndexName = StringUtils.toUpperCase(legacyIndexNameFormat !=
null
+ ?
StringUtils.format(legacyIndexNameFormat, tableName)
+ : StringUtils.format(
+ "IDX_%S_%S",
+ tableName,
+
Joiner.on("_").join(indexCols)
+ ));
+
+ // Avoid creating duplicate indices if an index with either naming
convention already exists
+ if (createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists",
legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shortIndexName)) {
+ log.info("Short index[%s] on Table[%s] already exists", shortIndexName,
tableName);
+ return;
+ }
+
+ final String indexName = tablesConfigSupplier.get().isUseShortIndexNames()
? shortIndexName : legacyIndexName;
try {
retryWithHandle(
- new HandleCallback<Void>()
- {
- @Override
- public Void withHandle(Handle handle)
- {
- if
(!createdIndexSet.contains(StringUtils.toUpperCase(indexName))) {
- String indexSQL = StringUtils.format(
- "CREATE INDEX %1$s ON %2$s(%3$s)",
- indexName,
- tableName,
- Joiner.on(",").join(indexCols)
- );
- log.info("Creating Index on Table [%s], sql: [%s] ",
tableName, indexSQL);
- handle.execute(indexSQL);
- } else {
- log.info("Index [%s] on Table [%s] already exists", indexName,
tableName);
- }
- return null;
- }
+ (HandleCallback<Void>) handle -> {
+ String indexSQL = StringUtils.format(
+ "CREATE INDEX %1$s ON %2$s(%3$s)",
+ indexName,
+ tableName,
+ Joiner.on(",").join(indexCols)
+ );
+ log.info("Creating index[%s] on table[%s] using SQL[%s].",
indexName, tableName, indexSQL);
+ handle.execute(indexSQL);
+ return null;
}
);
}
catch (Exception e) {
- log.error(e, StringUtils.format("Exception while creating index on table
[%s]", tableName));
+ log.error(e, StringUtils.format("Exception while creating index[%s] on
table[%s]", indexName, tableName));
Review Comment:
```suggestion
log.error(e, "Exception while creating index[%s] on table[%s]",
indexName, tableName);
```
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java:
##########
@@ -305,6 +303,140 @@ public void testIsTransientException()
);
}
+ @Test
+ public void test_useShortIndexNames_true_tableIndices_areNotAdded_ifExist()
+ {
+ tablesConfig = new MetadataStorageTablesConfig(
+ "druidTest_base_table_name_true",
+ null, null, null, null, null, null, null, null, null, null, null,
+ true
+ );
+ connector = new TestDerbyConnector(new MetadataStorageConnectorConfig(),
tablesConfig);
+
+ final String segmentsTable = tablesConfig.getSegmentsTable();
+
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+ connector.getDBI().withHandle(handle -> {
+ handle.execute("DROP INDEX
IDX_B859F28F30657A58518416E445E0AA1A78A68177");
+ handle.execute(StringUtils.format("CREATE INDEX IDX_%1$s_USED ON
%1$s(used)", segmentsTable));
+ return null;
+ });
+
+ connector.createSegmentTable(segmentsTable);
+ connector.alterSegmentTable();
+
+ final Set<String> expectedIndices = Sets.newHashSet(
+ StringUtils.format("IDX_%S_USED", segmentsTable),
Review Comment:
Let's use the final index name where possible since we know what they are
going to be.
```suggestion
"IDX_DRUIDTEST_SEGMENTS_USED",
```
--
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: commits-unsubscr...@druid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
