github-actions[bot] commented on code in PR #61994:
URL: https://github.com/apache/doris/pull/61994#discussion_r3045167452
##########
regression-test/suites/ann_index_p0/create_ann_index_test.groovy:
##########
@@ -300,25 +300,22 @@ suite("create_ann_index_test") {
"""
sql "drop table if exists tbl_ann_unique_key"
- test {
- sql """
- CREATE TABLE tbl_ann_unique_key (
- id INT NOT NULL COMMENT "",
- embedding ARRAY<FLOAT> NOT NULL COMMENT "",
- INDEX idx_test_ann (`embedding`) USING ANN PROPERTIES(
- "index_type"="hnsw",
- "metric_type"="inner_product",
- "dim"="128"
- )
- ) ENGINE=OLAP
- UNIQUE KEY(id) COMMENT "OLAP"
- DISTRIBUTED BY HASH(id) BUCKETS 2
- PROPERTIES (
- "replication_num" = "1"
- );
- """
- exception "ANN index can only be used in DUP_KEYS table"
- }
+ sql """
+ CREATE TABLE tbl_ann_unique_key (
+ id INT NOT NULL COMMENT "",
+ embedding ARRAY<FLOAT> NOT NULL COMMENT "",
+ INDEX idx_test_ann (`embedding`) USING ANN PROPERTIES(
+ "index_type"="hnsw",
+ "metric_type"="inner_product",
+ "dim"="128"
+ )
Review Comment:
This test now turns a plain `UNIQUE KEY` table into an expected-success
case, but the PR goal is specifically MoW support. Because
`enable_unique_key_merge_on_write` is omitted here,
`TableProperty#getEnableUniqueKeyMergeOnWrite()` will treat this as
merge-on-read by default. That no longer matches either the PR description or
the BE-side MoW-specific ANN handling.
Please either keep this as a negative case, or make it a positive MoW case
by adding `"enable_unique_key_merge_on_write" = "true"` so the coverage matches
the actual supported behavior.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/IndexDefinition.java:
##########
@@ -158,8 +158,8 @@ public void checkColumn(ColumnDefinition column, KeysType
keysType,
if (!itemType.isFloatType()) {
throw new AnalysisException("ANN index column item type must
be float type, invalid index: " + name);
}
- if (keysType != KeysType.DUP_KEYS) {
- throw new AnalysisException("ANN index can only be used in
DUP_KEYS table");
+ if (keysType != KeysType.DUP_KEYS && keysType !=
KeysType.UNIQUE_KEYS) {
+ throw new AnalysisException("ANN index can only be used in
DUP_KEYS or UNIQUE_KEYS table");
}
return;
}
Review Comment:
Allowing every `UNIQUE_KEYS` table here looks too broad for the feature this
PR describes. `TableProperty#getEnableUniqueKeyMergeOnWrite()` still defaults
to `false`, so this change now accepts ANN indexes on merge-on-read unique
tables as well. But the BE-side logic you rely on for correctness is explicitly
MoW-specific: the scan fast path is gated to `DUP_KEYS` or `UNIQUE_KEYS` with
MoW in `be/src/storage/segment/segment_iterator.cpp:1326-1329`, and the PR
description itself says deleted rows are filtered by delete bitmap, which only
exists for MoW.
Please keep rejecting non-MoW unique tables here, e.g. by checking
`enableUniqueKeyMergeOnWrite` before allowing `KeysType.UNIQUE_KEYS`, otherwise
this PR exposes a DDL path that the implementation has not proven correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
