yuqi1129 commented on code in PR #13430:
URL: https://github.com/apache/gravitino/pull/13430#discussion_r4139791137
##########
clients/client-python/gravitino/dto/rel/indexes/json_serdes/index_serdes.py:
##########
@@ -51,5 +53,8 @@ def deserialize(cls, data: dict[str, Any]) -> Index:
index_type = Index.IndexType(data[cls.INDEX_TYPE].upper())
return IndexDTO(
- index_type, data.get(cls.INDEX_NAME), data[cls.INDEX_FIELD_NAMES]
+ index_type,
+ data.get(cls.INDEX_NAME),
+ data[cls.INDEX_FIELD_NAMES],
+ data.get("properties"),
Review Comment:
The new deserialization preserves index properties, but `IndexDTO.__eq__`
and `__hash__` still ignore them. Two vector indexes with the same name and
fields but different `distance_function` values therefore compare equal (and
have the same hash), so a comparison of loaded table metadata can miss a real
index configuration change. Could we include properties in both methods and add
a regression test for indexes that differ only in properties? The Java
`IndexDTO` already includes properties in equality and hashing.
##########
docs/jdbc-clickhouse-catalog.md:
##########
@@ -258,11 +260,18 @@ The `engine_parameters` property applies to
`ReplacingMergeTree`, `SummingMergeT
- `DATA_SKIPPING_SET` (default `GRANULARITY 1`, plus configurable `set(N)`
max values)
- `DATA_SKIPPING_NGRAMBFV1` (`GRANULARITY` customizable via
`Index.properties()`, default 1; requires `ngram_size`, `bloom_filter_size`,
`hash_functions`, `random_seed` in `Index.properties()`)
- `DATA_SKIPPING_TOKENBFV1` (`GRANULARITY` customizable via
`Index.properties()`, default 1; requires `bloom_filter_size`,
`hash_functions`, `random_seed` in `Index.properties()`)
+ - `DATA_SKIPPING_VECTOR_SIMILARITY` (requires ClickHouse 25.8+; default
`GRANULARITY 100000000`; requires `type=hnsw`, `distance_function`, and
`dimensions` in `Index.properties()`)
Review Comment:
Documentation suggestion: when a vector index is added to a table that
already contains data, ClickHouse `ALTER TABLE ... ADD INDEX` covers new data
only; existing parts need `ALTER TABLE ... MATERIALIZE INDEX` for the index to
take effect. Could we mention this here or in the ALTER TABLE section? The
current Gravitino ADD INDEX path does not issue MATERIALIZE INDEX, so users may
otherwise expect existing rows to be indexed immediately. See the [ClickHouse
documentation](https://clickhouse.com/docs/reference/engines/table-engines/mergetree-family/annindexes).
This also applies to other data-skipping index types.
--
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]
