yuqi1129 commented on code in PR #13432:
URL: https://github.com/apache/gravitino/pull/13432#discussion_r4151226303
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java:
##########
@@ -920,6 +926,223 @@ void testSetIndexParameterReadbackLifecycle() {
assertSetIndexMetadata(nativeLoaded, "idx_native_set", setProperties);
}
+ @Test
+ void testLoadLegacyUsearchIndexMetadata() throws Exception {
+ String legacyCatalogName =
GravitinoITUtils.genRandomName("clickhouse_legacy_catalog");
+ String legacySchemaName =
GravitinoITUtils.genRandomName("clickhouse_legacy_schema");
+ String legacyTableName =
GravitinoITUtils.genRandomName("clickhouse_legacy_usearch");
+ ClickHouseContainer legacyContainer =
+ ClickHouseContainer.builder()
+ .withImage("clickhouse/clickhouse-server:23.8.16.16")
+ .withHostName("gravitino-ci-clickhouse-legacy")
+ .withEnvVars(Map.of("CLICKHOUSE_PASSWORD",
ClickHouseContainer.PASSWORD))
+ .withExposePorts(Set.of(ClickHouseContainer.CLICKHOUSE_PORT))
+ .withNetwork(containerSuite.getNetwork())
+ .build();
+ Catalog legacyCatalog = null;
+ try {
+ legacyContainer.start();
+ legacyContainer.createDatabase(TEST_DB_NAME);
+
+ Map<String, String> catalogProperties = Maps.newHashMap();
+ String jdbcUrl = legacyContainer.getJdbcUrl(TEST_DB_NAME);
+ String baseJdbcUrl = jdbcUrl.substring(0, jdbcUrl.lastIndexOf("/"));
+ catalogProperties.put(JdbcConfig.JDBC_URL.getKey(), baseJdbcUrl);
+ catalogProperties.put(
+ JdbcConfig.JDBC_DRIVER.getKey(),
legacyContainer.getDriverClassName(TEST_DB_NAME));
+ catalogProperties.put(JdbcConfig.USERNAME.getKey(),
legacyContainer.getUsername());
+ catalogProperties.put(JdbcConfig.PASSWORD.getKey(),
legacyContainer.getPassword());
+ legacyCatalog =
+ metalake.createCatalog(
+ legacyCatalogName,
+ Catalog.Type.RELATIONAL,
+ provider,
+ "legacy ClickHouse index metadata fixture",
+ catalogProperties);
+ legacyCatalog = metalake.loadCatalog(legacyCatalogName);
+ legacyCatalog.asSchemas().createSchema(legacySchemaName, "legacy index
metadata", Map.of());
+
+ // This official 23.8 image exposes USearch but not Annoy; Annoy
metadata parsing is covered
+ // by the catalog unit tests.
+ String usearchTypeFull;
+ String legacyJdbcUrl = baseJdbcUrl +
"?custom_settings=allow_experimental_usearch_index%3D1";
+ try (Connection legacyConnection =
+ DriverManager.getConnection(
+ legacyJdbcUrl, legacyContainer.getUsername(),
legacyContainer.getPassword());
+ Statement legacyStatement = legacyConnection.createStatement()) {
+ legacyStatement.execute(
+ String.format(
+ "CREATE TABLE `%s`.`%s` ("
+ + "`id` UInt64, `usearch_embedding` Array(Float32), "
+ + "INDEX `idx_legacy_usearch` `usearch_embedding` TYPE
usearch('cosineDistance') GRANULARITY 1"
+ + ") ENGINE = MergeTree ORDER BY `id`",
+ legacySchemaName, legacyTableName));
+
+ try (ResultSet resultSet =
+ legacyStatement.executeQuery(
+ String.format(
+ "SELECT type_full FROM system.data_skipping_indices WHERE
database = '%s' "
+ + "AND table = '%s' AND name = 'idx_legacy_usearch'",
+ legacySchemaName, legacyTableName))) {
+ Assertions.assertTrue(resultSet.next(), "Expected to find the legacy
USearch index");
+ usearchTypeFull = resultSet.getString("type_full");
+ }
+ }
+ Assertions.assertEquals("usearch('cosineDistance')", usearchTypeFull);
+
+ Table loaded =
+ legacyCatalog
+ .asTableCatalog()
+ .loadTable(NameIdentifier.of(legacySchemaName, legacyTableName));
+ assertLegacyIndexMetadata(
+ loaded,
+ "idx_legacy_usearch",
+ Index.IndexType.DATA_SKIPPING_USEARCH,
+ "usearch_embedding",
+ Map.of(
+ USEARCH_DISTANCE_FUNCTION,
+ "cosineDistance",
+ CLICKHOUSE_TYPE_FULL,
+ usearchTypeFull,
+ GRANULARITY,
+ "1"));
+ } finally {
+ if (legacyCatalog != null) {
+ try {
+ legacyCatalog.asSchemas().dropSchema(legacySchemaName, true);
+ } catch (Exception ignored) {
+ // The historical container is discarded below; cleanup failures
must not hide test
+ // errors.
+ }
+ metalake.disableCatalog(legacyCatalogName);
+ metalake.dropCatalog(legacyCatalogName, true);
+ }
+ legacyContainer.close();
Review Comment:
Nit: if `disableCatalog` or `dropCatalog` throws during cleanup, this
`close()` is skipped and the test container remains running. The Annoy test
below already protects container cleanup with a nested `finally`; could this
test use the same pattern?
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -2310,11 +2353,113 @@ private static boolean
isParameterizedBloomFilterIndex(Index.IndexType indexType
|| indexType == Index.IndexType.DATA_SKIPPING_TOKENBFV1;
}
+ private static Map<String, String> parseLegacyAnnUsearchProperties(
+ Index.IndexType indexType, String typeFull, String indexName) {
+ String expectedType = legacyAnnUsearchTypeName(indexType);
+ String rawTypeFull = StringUtils.defaultString(typeFull);
+ String normalizedTypeFull = rawTypeFull.trim();
+ Map<String, String> properties = new HashMap<>();
+ if (StringUtils.isNotEmpty(rawTypeFull)) {
+ properties.put(CLICKHOUSE_TYPE_FULL, rawTypeFull);
+ }
+
+ int paramsStart = normalizedTypeFull.indexOf('(');
+ int paramsEnd = normalizedTypeFull.lastIndexOf(')');
+ if (paramsStart < 0 && StringUtils.equalsIgnoreCase(expectedType,
normalizedTypeFull)) {
+ LOG.warn(
+ "ClickHouse metadata does not expose legacy {} parameters for index
'{}'; "
+ + "preserving the reported type expression only",
+ expectedType,
+ indexName);
+ return Map.copyOf(properties);
+ }
+ if (paramsStart <= 0
+ || paramsEnd != normalizedTypeFull.length() - 1
+ || !StringUtils.equalsIgnoreCase(
+ expectedType, normalizedTypeFull.substring(0,
paramsStart).trim())) {
+ LOG.warn(
+ "Could not parse legacy ClickHouse type expression '{}' for index
'{}' of type {}; "
+ + "preserving it in '{}'",
+ rawTypeFull,
+ indexName,
+ expectedType,
+ CLICKHOUSE_TYPE_FULL);
+ return Map.copyOf(properties);
+ }
+
+ String[] parameters = normalizedTypeFull.substring(paramsStart + 1,
paramsEnd).split(",", -1);
+ if (parameters.length != 1 || StringUtils.isBlank(parameters[0])) {
Review Comment:
Nit: `annoy()` is a valid legacy definition. I created one with ClickHouse
24.8 and `system.data_skipping_indices` returned `type_full = annoy()` and
granularity `1`. The blank-parameter branch logs a WARN on every table load
even though the index loads correctly. Could we treat the empty Annoy parameter
list as a normal case, preserving `clickhouse_type_full` without a parsing
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
