suneet-s commented on code in PR #14470:
URL: https://github.com/apache/druid/pull/14470#discussion_r1239032016
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -377,8 +380,20 @@ public void createEntryTable(final String tableName)
+ " PRIMARY KEY (id)\n"
+ ")",
tableName, getPayloadType(), getCollation()
- ),
- StringUtils.format("CREATE INDEX idx_%1$s_active_created_date ON
%1$s(active, created_date)", tableName)
+ )
+ )
+ );
+ createIndex(
+ tableName,
+ "active_created",
Review Comment:
```suggestion
"active_created_date",
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -830,4 +845,73 @@ public Void withHandle(Handle handle)
log.warn(e, "Exception while deleting records from table");
}
}
+
+ /**
+ * create index on the table if not already exist with retry.
+ *
+ * @param handle Connection handle
+ * @param table name of the table to fetch the index map
+ * @return Map of the uppercase index name to List of the columns in index
+ */
+ public Map<String, List<String>> getIndexOnTable(Handle handle, String table)
+ {
+ Map<String, List<String>> res = new HashMap<>();
+ try {
+ DatabaseMetaData databaseMetaData = handle.getConnection().getMetaData();
+ ResultSet resultSet = databaseMetaData.getIndexInfo(null, null,
table.toUpperCase(Locale.ENGLISH), false, false);
+
+ while (resultSet.next()) {
+ String indexName = resultSet.getString("INDEX_NAME");
+ String columnName = resultSet.getString("COLUMN_NAME");
+ if (org.apache.commons.lang.StringUtils.isNotBlank(indexName) &&
org.apache.commons.lang.StringUtils.isNotBlank(
+ columnName)) {
+ columnName = StringUtils.toUpperCase(columnName);
+ List<String> cols = res.getOrDefault(indexName, new ArrayList<>());
+ cols.add(columnName);
+ res.put(indexName, cols);
+ }
+ }
+ }
+ catch (Exception e) {
+ log.error(e, "Exception while listing the index on table %s ", table);
+ }
+ return res;
+ }
+
+ /**
+ * create index on the table if not already exist with retry.
+ *
+ * @param tableName Name of the table to create index on
+ * @param checkString case-insensitive string which substring of index name,
it helps to check the existing index on table
+ * @param indexSQL SQL statement to create index
+ */
+ public void createIndex(final String tableName, final String checkString,
final String indexSQL)
+ {
+ try {
+ retryWithHandle(
+ new HandleCallback<Void>()
+ {
+ @Override
+ public Void withHandle(Handle handle)
+ {
+ if (tableExists(handle, tableName)) {
+ Map<String, List<String>> indexMap = getIndexOnTable(handle,
tableName);
Review Comment:
It looks like you are not taking advantage of the values in this map. Should
`getIndexOnTable` just return a list of index names, and then it can be called
`getIndexNamesForTable`
It would be really cool if we could infer if the index we are trying to
create is already in the table by looking at the column names, ordinal values,
etc. but that seems like overkill and just checking the name seems reasonable
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -377,8 +380,20 @@ public void createEntryTable(final String tableName)
+ " PRIMARY KEY (id)\n"
+ ")",
tableName, getPayloadType(), getCollation()
- ),
- StringUtils.format("CREATE INDEX idx_%1$s_active_created_date ON
%1$s(active, created_date)", tableName)
+ )
+ )
+ );
+ createIndex(
+ tableName,
+ "active_created",
+ StringUtils.format("CREATE INDEX idx_%1$s_active_created_date ON
%1$s(active, created_date)", tableName)
+ );
+ createIndex(
+ tableName,
+ "datasource_active",
+ StringUtils.format(
+ "CREATE INDEX idx_%1$s_datasource_active_date ON %1$s(datasource,
active)",
Review Comment:
```suggestion
"CREATE INDEX idx_%1$s_datasource_active ON %1$s(datasource,
active)",
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -377,8 +380,20 @@ public void createEntryTable(final String tableName)
+ " PRIMARY KEY (id)\n"
+ ")",
tableName, getPayloadType(), getCollation()
- ),
- StringUtils.format("CREATE INDEX idx_%1$s_active_created_date ON
%1$s(active, created_date)", tableName)
+ )
+ )
+ );
+ createIndex(
+ tableName,
+ "active_created",
+ StringUtils.format("CREATE INDEX idx_%1$s_active_created_date ON
%1$s(active, created_date)", tableName)
+ );
+ createIndex(
+ tableName,
+ "datasource_active",
+ StringUtils.format(
+ "CREATE INDEX idx_%1$s_datasource_active_date ON %1$s(datasource,
active)",
+ tableName
Review Comment:
Have you considered moving these create index commands to the function
`alterEntryTable` below? instead of it being part of the function
`createEntryTable`
--
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]
