jerryshao commented on code in PR #14067:
URL: https://github.com/apache/gravitino/pull/14067#discussion_r4344323541
##########
catalogs/catalog-jdbc-starrocks/src/main/java/org/apache/gravitino/catalog/starrocks/operations/StarRocksTableOperations.java:
##########
@@ -260,12 +262,24 @@ protected List<Index> getIndexes(Connection connection,
String databaseName, Str
try (PreparedStatement preparedStatement =
connection.prepareStatement(sql);
ResultSet resultSet = preparedStatement.executeQuery()) {
+ // SHOW INDEX lists only secondary indexes, one row per single-column
index; table keys are
+ // not included.
List<Index> indexes = new ArrayList<>();
while (resultSet.next()) {
String indexName = resultSet.getString("Key_name");
String columnName = resultSet.getString("Column_name");
- indexes.add(
- Indexes.of(Index.IndexType.PRIMARY_KEY, indexName, new String[][]
{{columnName}}));
+ String starRocksIndexType = resultSet.getString("Index_type");
+ Index.IndexType indexType = mapStarRocksIndexType(starRocksIndexType);
+ if (indexType == null) {
+ LOG.debug(
+ "Skip StarRocks index '{}' on table {}.{} with unsupported type
'{}'",
+ indexName,
+ databaseName,
+ tableName,
+ starRocksIndexType);
+ continue;
+ }
+ indexes.add(Indexes.of(indexType, indexName, new String[][]
{{columnName}}));
Review Comment:
[Question] This drops PRIMARY_KEY reporting for StarRocks entirely: every
row whose `Index_type` is not BITMAP/GIN/VECTOR is skipped, which would include
a `PRIMARY`/BTREE row if `SHOW INDEX` ever returns one. The sibling Doris
catalog keeps an explicit fallback for exactly that case
(`"PRIMARY".equals(indexName)` -> PRIMARY_KEY,
`DorisTableOperations.java:694-700` and `731-739`).
The description says StarRocks does not return table keys from `SHOW INDEX`,
and the manual check covered BITMAP, GIN and NGRAMBF - did it also cover a
PRIMARY KEY table and a UNIQUE KEY table? If such a row does appear on any
supported version, a real key is now silently dropped rather than mis-typed,
and consumers filtering on PRIMARY_KEY
(`flink-connector/flink-common/.../BaseCatalog.java:870`) will see no key at
all. A name-based fallback like Doris's, or one sentence in the new docs table
confirming keys never appear, would settle it.
Verified by: reading `getIndexes` in both catalogs on this head; `grep -i
index` over `CatalogStarRocksIT.java` returns nothing, so no test pins this
behavior either way.
##########
clients/client-python/gravitino/api/rel/indexes/index.py:
##########
@@ -52,6 +52,15 @@ class IndexType(str, Enum):
columns that are not designated as the primary key. The uniqueness
constraint imposed by
UNIQUE KEY helps in avoiding redundancy and ensuring data accuracy in
the database."""
+ BITMAP = "BITMAP"
+ """Bitmap index, which uses bit arrays to represent the presence of
values in a column."""
+
+ INVERTED = "INVERTED"
+ """Inverted index, which maps terms to their locations to speed up
full-text searches."""
+
+ VECTOR = "VECTOR"
Review Comment:
[Nit] These three were the blocking gap, but the python enum still lacks
other types the Java side already emits, and `index_serdes.py:56` does
`Index.IndexType(data[...].upper())`, which raises ValueError for anything
missing - the same break this PR is fixing. Still absent:
`DATA_SKIPPING_BLOOM_FILTER` (the Doris catalog returns it for BLOOMFILTER,
`DorisTableOperations.java:747-748`), plus `SCALAR`, `BTREE`, `LABEL_LIST`,
`IVF_FLAT`, `IVF_SQ`, `IVF_PQ`, `IVF_HNSW_SQ`, `IVF_HNSW_PQ`,
`DATA_SKIPPING_MINMAX`, `DATA_SKIPPING_SET`, `DATA_SKIPPING_NGRAMBFV1`,
`DATA_SKIPPING_TOKENBFV1` (Lance and ClickHouse types in `Index.java`).
Pre-existing, so a follow-up is fine, but cheap to finish while you are here.
Verified by: diffing `Index.IndexType` in
`api/src/main/java/org/apache/gravitino/rel/indexes/Index.java` against the
python enum, and reading `index_serdes.py`.
##########
catalogs/catalog-jdbc-starrocks/src/main/java/org/apache/gravitino/catalog/starrocks/operations/StarRocksTableOperations.java:
##########
@@ -260,12 +262,24 @@ protected List<Index> getIndexes(Connection connection,
String databaseName, Str
try (PreparedStatement preparedStatement =
connection.prepareStatement(sql);
ResultSet resultSet = preparedStatement.executeQuery()) {
+ // SHOW INDEX lists only secondary indexes, one row per single-column
index; table keys are
+ // not included.
List<Index> indexes = new ArrayList<>();
while (resultSet.next()) {
String indexName = resultSet.getString("Key_name");
String columnName = resultSet.getString("Column_name");
- indexes.add(
- Indexes.of(Index.IndexType.PRIMARY_KEY, indexName, new String[][]
{{columnName}}));
+ String starRocksIndexType = resultSet.getString("Index_type");
Review Comment:
[Question] `resultSet.getString("Index_type")` throws SQLException when the
column is absent, so a StarRocks build whose `SHOW INDEX` lacks it would fail
the whole `loadTable`, not just index mapping - and the `@Nullable` parameter
on `mapStarRocksIndexType` suggests a null was expected there instead. The
Doris catalog guards this with `ResultSetMetaData` before reading its optional
columns, with a comment about older versions
(`DorisTableOperations.java:676-691`).
This PR's own doc file pins support to StarRocks 3.3.x, so it may well be a
non-issue; worth confirming that every supported version exposes `Index_type`,
otherwise the same metadata check is a two-line safeguard.
Verified by: reading `DorisTableOperations.getIndexes` on this head and
`docs/jdbc-starrocks-catalog.md:28` ("Supports metadata management of StarRocks
(3.3.x)").
##########
catalogs/catalog-jdbc-starrocks/src/main/java/org/apache/gravitino/catalog/starrocks/operations/StarRocksTableOperations.java:
##########
@@ -260,12 +262,24 @@ protected List<Index> getIndexes(Connection connection,
String databaseName, Str
try (PreparedStatement preparedStatement =
connection.prepareStatement(sql);
ResultSet resultSet = preparedStatement.executeQuery()) {
+ // SHOW INDEX lists only secondary indexes, one row per single-column
index; table keys are
+ // not included.
List<Index> indexes = new ArrayList<>();
while (resultSet.next()) {
String indexName = resultSet.getString("Key_name");
String columnName = resultSet.getString("Column_name");
- indexes.add(
- Indexes.of(Index.IndexType.PRIMARY_KEY, indexName, new String[][]
{{columnName}}));
+ String starRocksIndexType = resultSet.getString("Index_type");
+ Index.IndexType indexType = mapStarRocksIndexType(starRocksIndexType);
+ if (indexType == null) {
+ LOG.debug(
Review Comment:
[Nit] Dropping an index the catalog cannot represent is metadata loss, and
at DEBUG it is invisible in a default deployment, so "why is my NGRAMBF index
missing" has no trace to point at. Doris logs the comparable unknown-type case
at WARN (`DorisTableOperations.java:752-756`). `LOG.warn` (or at least INFO)
would be more useful here.
Verified by: comparing with `DorisTableOperations.mapDorisIndexType` on this
head.
--
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]
