You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "jtuglu1 (via GitHub)" <gi...@apache.org> on 2025/09/12 07:28:53 UTC
[PR] Support truncation of SQL index names in metadata store (druid)
jtuglu1 opened a new pull request, #18515:
URL: https://github.com/apache/druid/pull/18515
For metadata stores with upper limits on the base table name, creation of indices can sometimes fail. Because these failures are not critical to the operation of the cluster, they are logged as warnings and can result in performance degradatation. This patch allows for index names to be truncated according to an engine-specific limit, so that they are always created.
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
Fixes #XXXX.
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
#### Fixed the bug ...
#### Renamed the class ...
#### Added a forbidden-apis entry ...
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
##### Key changed/added classes in this PR
* `MyFoo`
* `OurBar`
* `TheirBaz`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377842026
##########
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 using formatters etc.
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()` method changes
(in fact, it is desirable to have to update the test in that case).
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377897537
##########
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.
We aren't prefixing table name. SHA is the only thing being cut to 10 chars (down from 20 chars). Again, this is still best effort after a certain length of `idx_<table>`. In the worst case, for `datasource_upgraded_from_segment_id`, it's saving 25 characters of space.
If this is still too long for the engine, we throw an exception, log it, and silently continue. I feel like 10 chars is even generous (we could probably do 5 bytes of entropy and be fine).
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377842026
##########
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 using formatters etc.
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).
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377878833
##########
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:
Normally this will capitalize the string – it doesn't matter in this case as all supported engines' quotations don't contain any alphabetical chars, so I left it as-is to reduce diff.
##########
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:
Normally this will capitalize the provided string – it doesn't matter in this case as all supported engines' quotations don't contain any alphabetical chars, so I left it as-is to reduce diff.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
Hmm. That doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap much (I think the likelihood of this is extremely low as it is currently, even lower than chance of colliding on table name). I'd suggest leaving for now.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support unique SQL index names in metadata store (druid)
Posted by "maytasm (via GitHub)" <gi...@apache.org>.
maytasm commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2383437801
##########
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}}.
Review Comment:
nit:
- Add to javadoc on the expected format if legacyIndexNameFormat is specified (since this method will only substitute the tableName and that should be the only placeholder)
- the name legacyIndexNameFormat might be misleading. the current full index name is not legacy / replaced. It's just two alternative/options ... full vs short name , which we still support / keep both. So maybe just call it fullIndexNameFormat
##########
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}}.
Review Comment:
`@param fullIndexNameFormat Template to create index ID. If specified, the only placeholder should be for the tableName. If null, uses the format: {@code IDX_{table name}_{columns}}.`
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
> We should include the table name in generating the digest
Sure.
> rather than truncating the digest, we should truncate the table name itself.
Hmm, that doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap (I think the likelihood of this is extremely low as it is currently, even lower than chance of colliding on table name if we were to truncate the table name). I'd suggest leaving for now.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
Hmm. That doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap (I think the likelihood of this is extremely low as it is currently, even lower than chance of colliding on table name). I'd suggest leaving for now.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support unique SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 merged PR #18515:
URL: https://github.com/apache/druid/pull/18515
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support unique SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2383474546
##########
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}}.
Review Comment:
Will wait for @kfaraz to give final review and address comments in one pass.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support unique SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2383792271
##########
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:
This one the connector choose random UUID for base table name. I'll override it but not necessary IMO.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377842026
##########
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 using formatters etc.
Instead, we should verify the actual index names. Something like:
```java
connector.createSegmentsTable(...);
verifyIndicesPresentOnTable(
"IDX_ABC",
"IDX_DEF",
"IDX_XYZ
);
```
This would make the tests much more readable and exposes the end result while hiding the impl details.
It is okay even if we have to change the test when the logic in the `SQLMetadataConnector.generateIndexName()` method changes
(in fact, it is desirable to have to update the test in such cases).
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support unique SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2383792453
##########
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:
I'd like to keep as-is.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377897537
##########
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.
We aren't prefixing table name. But, I still think it's fine to omit. SHA is the only thing being cut to 10 chars (down from 20 chars). Again, this is still best effort after a certain length of `idx_<table>`. In the worst case, for `datasource_upgraded_from_segment_id`, it's saving 25 characters of space.
If this is still too long for the engine, we throw an exception, log it, and silently continue. I feel like 10 chars is even generous (we could probably do 5 bytes of entropy and be fine).
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377904204
##########
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:
> What do you think?
Sure
##########
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:
> What do you think?
Sure
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2354249703
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -396,13 +409,16 @@ private void createUpgradeSegmentsTable(final String tableName)
+ " PRIMARY KEY (id)\n"
+ ")",
tableName, getSerialType()
- ),
- StringUtils.format(
- "CREATE INDEX idx_%1$s_task ON %1$s(task_id)",
- tableName
)
)
);
+ final Set<String> createdIndexSet = getIndexOnTable(tableName);
+ createIndex(
+ tableName,
+ "idx_%1$s_task",
Review Comment:
Yeah, I agree, let's just update these cases.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377842026
##########
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 using formatters etc.
Instead, we should verify the actual index names. 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()` method changes
(in fact, it is desirable to have to update the test in that case).
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377842026
##########
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).
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2366682138
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,47 @@ 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 indexTemplate} 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 indexTemplate Template to create index ID (nullable for forwards compatibility with SHA-based indices)
* @param indexCols List of columns to be indexed on
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String indexTemplate,
final List<String> indexCols,
final Set<String> createdIndexSet
)
{
+ final String shaIndexName = StringUtils.toUpperCase(generateSHABasedIndexIdentifier(tableName, indexCols));
+ final String legacyIndexName = indexTemplate != null ? StringUtils.toUpperCase(StringUtils.format(
+ indexTemplate,
+ tableName
+ )) : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
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 -> {
Review Comment:
Thanks for checking.
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
Yeah, I was concerned about the uniqueness of the hash and thus wanted to always include the full length.
But you make a fair point and even a truncated hash should be good enough.
So, I would suggest _always_ using a fixed size substring of the hash (say 10 or 12 characters), similar to what we do for task IDs.
This ensures that the size of the hash is fixed across all the index names and they all
adhere to the same format.
The current logic might reduce the size of the hash string to zero (or even throw an exception)
if the table name is too long.
To handle that, if the `idx_<table-name>_<12-char-hash>` is still longer than the limit, we just raise an alert.(Failing service startup might be too drastic since we currently don't fail on this scenario.)
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2361771892
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,47 @@ 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 indexTemplate} 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 indexTemplate Template to create index ID (nullable for forwards compatibility with SHA-based indices)
* @param indexCols List of columns to be indexed on
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String indexTemplate,
Review Comment:
```suggestion
final String legacyIndexNameFormat,
```
##########
processing/src/main/java/org/apache/druid/metadata/MetadataStorageConnector.java:
##########
@@ -95,4 +95,12 @@ default void exportTable(
* SegmentSchema table is created only when CentralizedDatasourceSchema feature is enabled.
*/
void createSegmentSchemasTable();
+
+ /**
+ * Returns the maximum identifier length for table indices in this MetadataStorageConnector implementation.
Review Comment:
Nit:
```suggestion
* Returns the maximum allowed length of table index names in this MetadataStorageConnector implementation.
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,47 @@ 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 indexTemplate} 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 indexTemplate Template to create index ID (nullable for forwards compatibility with SHA-based indices)
* @param indexCols List of columns to be indexed on
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String indexTemplate,
final List<String> indexCols,
final Set<String> createdIndexSet
)
{
+ final String shaIndexName = StringUtils.toUpperCase(generateSHABasedIndexIdentifier(tableName, indexCols));
+ final String legacyIndexName = indexTemplate != null ? StringUtils.toUpperCase(StringUtils.format(
+ indexTemplate,
+ tableName
+ )) : null;
Review Comment:
```suggestion
final String legacyIndexName = indexTemplate != null
? StringUtils.toUpperCase(StringUtils.format(indexTemplate, tableName))
: null;
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -572,10 +603,15 @@ public void createSupervisorsTable(final String tableName)
+ " PRIMARY KEY (id)\n"
+ ")",
tableName, getSerialType(), getPayloadType()
- ),
- StringUtils.format("CREATE INDEX idx_%1$s_spec_id ON %1$s(spec_id)", tableName)
+ )
)
);
+ createIndex(
+ tableName,
+ "idx_%1$s_spec_id ",
Review Comment:
Nit: Since the index name has only one occurrence of the table name, can we just use `%s` instead of `%1$s` across all the invocations of the `createIndex` method?
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
We should include the table name in generating the digest,
and rather than truncating the digest, we should truncate the table name itself.
##########
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(
Review Comment:
Nit: reads better
```suggestion
List.of(
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
Review Comment:
We can skip this annotation.
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,47 @@ 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 indexTemplate} 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 indexTemplate Template to create index ID (nullable for forwards compatibility with SHA-based indices)
* @param indexCols List of columns to be indexed on
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String indexTemplate,
final List<String> indexCols,
final Set<String> createdIndexSet
)
{
+ final String shaIndexName = StringUtils.toUpperCase(generateSHABasedIndexIdentifier(tableName, indexCols));
Review Comment:
Should using the new index name be an opt-in feature based on some config?
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
Review Comment:
```suggestion
* Creates a unique index name of the format {@code idx_<tableName>_<SHA of column list>}
* while honoring the {@link #getMaxLengthOfIndexName}.
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,47 @@ 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 indexTemplate} 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 indexTemplate Template to create index ID (nullable for forwards compatibility with SHA-based indices)
* @param indexCols List of columns to be indexed on
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String indexTemplate,
final List<String> indexCols,
final Set<String> createdIndexSet
)
{
+ final String shaIndexName = StringUtils.toUpperCase(generateSHABasedIndexIdentifier(tableName, indexCols));
+ final String legacyIndexName = indexTemplate != null ? StringUtils.toUpperCase(StringUtils.format(
+ indexTemplate,
+ tableName
+ )) : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
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)",
+ shaIndexName,
+ tableName,
+ Joiner.on(",").join(indexCols)
+ );
+ log.info("Creating Index on Table [%s], sql: [%s] ", tableName, indexSQL);
Review Comment:
Let's log the index name as it might have been truncated.
```suggestion
log.info("Creating index[%s] on table[%s] using SQL[%s].", indexName, tableName, indexSQL);
```
##########
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:
can we delegate the quoting to the `createIndex` method itself? It could just quote all the methods by default.
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
Review Comment:
```suggestion
protected String generateUniqueIndexName(String tableName, List<String> columns)
```
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,47 @@ 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 indexTemplate} 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 indexTemplate Template to create index ID (nullable for forwards compatibility with SHA-based indices)
* @param indexCols List of columns to be indexed on
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String indexTemplate,
final List<String> indexCols,
final Set<String> createdIndexSet
)
{
+ final String shaIndexName = StringUtils.toUpperCase(generateSHABasedIndexIdentifier(tableName, indexCols));
+ final String legacyIndexName = indexTemplate != null ? StringUtils.toUpperCase(StringUtils.format(
+ indexTemplate,
+ tableName
+ )) : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
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 -> {
Review Comment:
Nit: not sure, but I think the casting can be avoided since there are no other methods which expect a similar lambda signature.
```suggestion
handle -> {
```
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
> We should include the table name in generating the digest
Sure.
> rather than truncating the digest, we should truncate the table name itself.
Hmm, that doesn't sound great. I'd prefer to always have the table name. I'm optimizing for avoiding collisions with names within same cluster/table as well as tables between clusters. The SHA was supposed to introduce enough entropy where suffixes didn't overlap (I think the likelihood of this is extremely low as it is currently, even lower than chance of colliding on table name if we were to truncate the table name). I'd suggest leaving for now.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377901063
##########
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?
I think that's an outdated comment. This was before the config flag was re-added.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2378446715
##########
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:
Previous code does this. This should normally capitalize the string but there's no capitalized equivalent for any engines' quote string. I had left to reduce diff area. Updated.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2366760334
##########
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:
> It could just quote all the methods by default.
This actually causes some trouble with how engines parse the quotes vs not when creating indices on the tables. With Derby, if the column isn't created in the table with the same format the index creation fails. I've left this as-is for now as it's only a handful of columns.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377842026
##########
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 using formatters etc.
Instead, we should verify the actual index names. Something like:
```java
connector.createSegmentsTable(...);
verifyIndicesPresentOnTable(
"IDX_ABC",
"IDX_DEF",
"IDX_XYZ
);
```
This would make the tests much more readable and exposes the end result while hiding the impl details.
It is okay even if we have to change the test when the logic in the `SQLMetadataConnector.generateIndexName()` method changes
(in fact, it is desirable to have to update the test in cases where say the index name format changes
or SHA generation technique changes).
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
Hmm. That doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap (I think the likelihood of this is extremely low as it is currently, even lower than chance of colliding on table name). I'd suggest leaving for now. I can also always add the table name into the digest as well.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
Hmm. That doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap much (I think the likelihood of this is extremely low as it is currently). But, I suppose I can take the digest of the table name + column list, then truncate that?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
> We should include the table name in generating the digest
Sure.
> rather than truncating the digest, we should truncate the table name itself.
Hmm, that doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap (I think the likelihood of this is extremely low as it is currently, even lower than chance of colliding on table name). I'd suggest leaving for now.
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
> We should include the table name in generating the digest
Sure.
> rather than truncating the digest, we should truncate the table name itself.
Hmm, that doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap (I think the likelihood of this is extremely low as it is currently, even lower than chance of colliding on table name). I'd suggest leaving for now.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2369417252
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,48 @@ 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
+ ? StringUtils.toUpperCase(StringUtils.format(legacyIndexNameFormat, tableName))
+ : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
+
+ final String indexName = tablesConfigSupplier.get().getUseUniqueIndexNames() ? shaIndexName : 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;
}
);
Review Comment:
Yeah, that seems reasonable. 👍🏻
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support unique SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support unique SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2383792271
##########
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:
This one the connector choose random UUID for base table name. I'll override it but unclean IMO.
##########
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:
This one the connector choose random UUID for base table name. I'll override it but less clean IMO.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support unique SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2383792271
##########
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:
This one the connector choose random UUID. I'll change it but not necessary IMO.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2377842026
##########
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 using formatters etc.
Instead, we should verify the actual index names. Something like:
```java
connector.createSegmentsTable(...);
verifyIndicesPresentOnTable(
"IDX_ABC",
"IDX_DEF",
"IDX_XYZ
);
```
This would make the tests much more readable and exposes the end result while hiding the impl details.
It is okay even if we have to change the test when the logic in the `SQLMetadataConnector.generateIndexName()` method changes
(in fact, it is desirable to have to update the test in that case).
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2369219765
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,48 @@ 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
+ ? StringUtils.toUpperCase(StringUtils.format(legacyIndexNameFormat, tableName))
+ : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
+
+ final String indexName = tablesConfigSupplier.get().getUseUniqueIndexNames() ? shaIndexName : 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;
}
);
Review Comment:
Considering its across multiple createIndex calls, for completeness, I could insert `indexName` into `createdIndexSet`, as I see this was never actually done before. Thoughts @kfaraz?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2366699190
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
> The current logic might reduce the size of the hash string to zero (or even throw an exception) if the table name is too long.
Sure. I don't think tracking the length per engine is even necessary at that point.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363518049
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,47 @@ 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 indexTemplate} 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 indexTemplate Template to create index ID (nullable for forwards compatibility with SHA-based indices)
* @param indexCols List of columns to be indexed on
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String indexTemplate,
final List<String> indexCols,
final Set<String> createdIndexSet
)
{
+ final String shaIndexName = StringUtils.toUpperCase(generateSHABasedIndexIdentifier(tableName, indexCols));
Review Comment:
I think it should be the default, but can certainly put behind a flag.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
Hmm. That doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap much (I think the likelihood of this is extremely low as it is currently).
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2364162580
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,47 @@ 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 indexTemplate} 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 indexTemplate Template to create index ID (nullable for forwards compatibility with SHA-based indices)
* @param indexCols List of columns to be indexed on
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String indexTemplate,
final List<String> indexCols,
final Set<String> createdIndexSet
)
{
+ final String shaIndexName = StringUtils.toUpperCase(generateSHABasedIndexIdentifier(tableName, indexCols));
+ final String legacyIndexName = indexTemplate != null ? StringUtils.toUpperCase(StringUtils.format(
+ indexTemplate,
+ tableName
+ )) : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
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 -> {
Review Comment:
Yeah. I thought this as well, but seems like editor got confused once removed:
<img width="356" height="298" alt="Screenshot 2025-09-19 at 11 59 12 AM" src="https://github.com/user-attachments/assets/12efb3f7-a776-4a85-9973-53e98bfe9896" />
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363518049
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,47 @@ 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 indexTemplate} 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 indexTemplate Template to create index ID (nullable for forwards compatibility with SHA-based indices)
* @param indexCols List of columns to be indexed on
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String indexTemplate,
final List<String> indexCols,
final Set<String> createdIndexSet
)
{
+ final String shaIndexName = StringUtils.toUpperCase(generateSHABasedIndexIdentifier(tableName, indexCols));
Review Comment:
I think it should be the default.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363540179
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -572,10 +603,15 @@ public void createSupervisorsTable(final String tableName)
+ " PRIMARY KEY (id)\n"
+ ")",
tableName, getSerialType(), getPayloadType()
- ),
- StringUtils.format("CREATE INDEX idx_%1$s_spec_id ON %1$s(spec_id)", tableName)
+ )
)
);
+ createIndex(
+ tableName,
+ "idx_%1$s_spec_id ",
Review Comment:
Sure. For the sake of review, I left these format strings the same to make it easier to view the diff between old/new methods.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
Hmm. That doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap much (I think the likelihood of this is extremely low as it is currently). But, to make this practically impossible, I suppose I can take the digest of the table name + column list, then truncate that?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
Hmm. That doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap much. I suppose I can take the digest of the table name + column list, then truncate that?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2364162580
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,47 @@ 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 indexTemplate} 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 indexTemplate Template to create index ID (nullable for forwards compatibility with SHA-based indices)
* @param indexCols List of columns to be indexed on
* @param createdIndexSet
*/
public void createIndex(
final String tableName,
- final String indexName,
+ final String indexTemplate,
final List<String> indexCols,
final Set<String> createdIndexSet
)
{
+ final String shaIndexName = StringUtils.toUpperCase(generateSHABasedIndexIdentifier(tableName, indexCols));
+ final String legacyIndexName = indexTemplate != null ? StringUtils.toUpperCase(StringUtils.format(
+ indexTemplate,
+ tableName
+ )) : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
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 -> {
Review Comment:
Yeah. I thought this as well, but seems like editor got confused once removed and gave a warning:
<img width="356" height="298" alt="Screenshot 2025-09-19 at 11 59 12 AM" src="https://github.com/user-attachments/assets/12efb3f7-a776-4a85-9973-53e98bfe9896" />
I left as-is to remove the warning.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2366760334
##########
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:
> It could just quote all the methods by default.
This actually causes some trouble with how engines parse the quotes vs not when creating indices on the tables. With Derby, if the column isn't created in the table with the same quoting format the index creation fails(e.g. "used" vs used in the CT). I've left this as-is for now as it's only a handful of columns.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2369424526
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,48 @@ 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
+ ? StringUtils.toUpperCase(StringUtils.format(legacyIndexNameFormat, tableName))
+ : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
+
+ final String indexName = tablesConfigSupplier.get().getUseUniqueIndexNames() ? shaIndexName : 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;
}
);
Review Comment:
Or you could even return the created index name from the method.
Either way is fine.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support unique SQL index names in metadata store (druid)
Posted by "glasser (via GitHub)" <gi...@apache.org>.
glasser commented on PR #18515:
URL: https://github.com/apache/druid/pull/18515#issuecomment-4710198518
> These changes also switch to using the createIndex helper tool for every index creation, to make future changes easier. Although this separates the table/index creation into 2 separate transactions, from what I can tell, this shouldn't have any correctness side-effects since the indices are always followed the createTable() statements which would just mean the index may not be created even if the table was created.
I think this is a correctness improvement — we have been using Druid since v0.12 and found out today that the pending segments indexes (introduced in v0.13) never got created in our DB!
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2343561196
##########
server/src/test/java/org/apache/druid/metadata/SQLMetadataConnectorTest.java:
##########
@@ -305,6 +307,43 @@ public void testIsTransientException()
);
}
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testIndexIdentifierTruncation(boolean truncate)
+ {
+ String longTableName = "this_is_an_extremely_long_table_name_for_testing";
+ String identifierTemplate = "idx_%1$s_datasource_active";
+
+ tablesConfig = MetadataStorageTablesConfig.fromBase(longTableName, truncate);
+ MetadataStorageConnectorConfig config =
+ MetadataStorageConnectorConfig.create("connectURI", "user", "password", Collections.emptyMap());
+
+ TestSQLMetadataConnector testSQLMetadataConnector = new TestSQLMetadataConnector(
+ Suppliers.ofInstance(config),
+ Suppliers.ofInstance(tablesConfig),
+ CentralizedDatasourceSchemaConfig.create()
+ )
+ {
+ @Override
+ public int indexIdentifierLengthLimit()
+ {
+ return 30;
+ }
+ };
+
+ if (truncate) {
+ Assert.assertEquals(
+ "idx_this_is__datasource_active",
+ testSQLMetadataConnector.getIndexIdentifier(identifierTemplate, longTableName)
Review Comment:
I'll switch the test to test `createIndex` behavior directly if we're ok with the `createIndex` changes. Otherwise, I'll leave this as-is for now.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
> We should include the table name in generating the digest
Sure.
> rather than truncating the digest, we should truncate the table name itself.
Hmm, that doesn't sound great. I'd prefer to always have the table name. I'm optimizing for avoiding collisions with names within same cluster/table as well as tables between clusters. The SHA was supposed to introduce enough entropy where suffixes didn't overlap (I think the likelihood of this is extremely low as it is currently, given a sufficiently long digest (even 8 bytes should do), even lower than chance of colliding on table name if we were to truncate the table name). I'd suggest leaving for now.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
Hmm. That doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap (I think the likelihood of this is extremely low as it is currently, even lower than chance of colliding on table name). I'd suggest leaving for now.
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2369219765
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,48 @@ 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
+ ? StringUtils.toUpperCase(StringUtils.format(legacyIndexNameFormat, tableName))
+ : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
+
+ final String indexName = tablesConfigSupplier.get().getUseUniqueIndexNames() ? shaIndexName : 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;
}
);
Review Comment:
Considering its used multiple times, for completeness, I could insert `indexName` into `createdIndexSet`, as I see this was never actually done before. Thoughts @kfaraz?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2363536890
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1206,4 +1274,27 @@ public static boolean isStatementException(Throwable e)
return e instanceof StatementException ||
(e instanceof CallbackFailedException && e.getCause() instanceof StatementException);
}
+
+ /**
+ * Used to ensure full index coverage in the presence of large index names.
+ * Returned indentifiers are of the format: `idx_{tableName}_{SHA of column list}`.
+ *
+ * @param tableName the table name
+ * @param columns the set of columns to create the index on (case-insensitive)
+ * @return unique index identifier
+ */
+ @VisibleForTesting
+ protected String generateSHABasedIndexIdentifier(String tableName, List<String> columns)
+ {
+ final String prefix = "idx_" + tableName + "_";
+ final String columnDigest = DigestUtils.sha1Hex(
+ columns.stream()
+ .map(StringUtils::toLowerCase)
+ .collect(Collectors.joining("_"))
+ );
+ return prefix + columnDigest.substring(
Review Comment:
Hmm. That doesn't sound great. I'd prefer to always have the table name. The SHA was supposed to introduce enough entropy where suffixes didn't overlap much (I think the likelihood of this is extremely low as it is currently). But, to make this nearly impossible, I suppose I can take the digest of the table name + column list, then truncate that?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2369219765
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,48 @@ 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
+ ? StringUtils.toUpperCase(StringUtils.format(legacyIndexNameFormat, tableName))
+ : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
+
+ final String indexName = tablesConfigSupplier.get().getUseUniqueIndexNames() ? shaIndexName : 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;
}
);
Review Comment:
Considering it's use across multiple createIndex calls, for completeness, I could insert `indexName` into `createdIndexSet`, as I see this was never actually done before. Thoughts @kfaraz?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support truncation of SQL index names in metadata store (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2369219765
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -1106,41 +1168,48 @@ 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
+ ? StringUtils.toUpperCase(StringUtils.format(legacyIndexNameFormat, tableName))
+ : null;
+
+ // Avoid creating duplicate indices if an index with either naming convention already exists
+ if (legacyIndexName != null && createdIndexSet.contains(legacyIndexName)) {
+ log.info("Legacy index[%s] on Table[%s] already exists", legacyIndexName, tableName);
+ return;
+ } else if (createdIndexSet.contains(shaIndexName)) {
+ log.info("Index[%s] on Table[%s] already exists", shaIndexName, tableName);
+ return;
+ }
+
+ final String indexName = tablesConfigSupplier.get().getUseUniqueIndexNames() ? shaIndexName : 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;
}
);
Review Comment:
For completeness, I could insert `indexName` into `createdIndexSet`, as I see this was never actually done before. Thoughts @kfaraz?
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Support unique SQL index names in metadata store (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18515:
URL: https://github.com/apache/druid/pull/18515#discussion_r2383790104
##########
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 make the verifications simpler:
```suggestion
"druidTest",
```
--
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-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org