github-actions[bot] commented on code in PR #64592: URL: https://github.com/apache/doris/pull/64592#discussion_r3450955923
########## regression-test/suites/ann_index_p0/ivf_rabitq_index_test.groovy: ########## @@ -0,0 +1,79 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("ivf_rabitq_index_test") { + sql "set enable_common_expr_pushdown=true;" + + sql "drop table if exists ivf_rabitq_tbl_ann_l2" + sql """ + CREATE TABLE ivf_rabitq_tbl_ann_l2 ( + id INT NOT NULL, + embedding ARRAY<FLOAT> NOT NULL, + INDEX idx_emb (`embedding`) USING ANN PROPERTIES( + "index_type"="ivf", + "metric_type"="l2_distance", + "quantizer"="rabitq", + "nlist"="3", + "dim"="3" + ) + ) ENGINE=OLAP + DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ("replication_num" = "1"); + """ + + sql """ + INSERT INTO ivf_rabitq_tbl_ann_l2 VALUES + (1, [1.0, 2.0, 3.0]), + (2, [0.5, 2.1, 2.9]), + (3, [10.0, 10.0, 10.0]), + (4, [20.0, 20.0, 20.0]), + (5, [50.0, 20.0, 20.0]), + (6, [60.0, 20.0, 20.0]); + """ + sql "select * from ivf_rabitq_tbl_ann_l2 order by id;" + sql "select id from ivf_rabitq_tbl_ann_l2 order by l2_distance_approximate(embedding, [1.0,2.0,3.0]) limit 2;" Review Comment: This new suite only checks that the RaBitQ queries execute; it never verifies the returned rows. The three result-producing SELECTs in this file use bare `sql`, and the regression framework records expected output only through `qt_*`/`order_qt_*` quick tests. That means a bug in RaBitQ scoring, L2/IP ordering, or returned IDs would still pass CI as long as the query does not throw. Please convert these SELECTs to `qt_sql_*`/`order_qt_*` checks and add the generated `.out` so the new quantizer's results are asserted like the adjacent IVF suites. ########## be/src/storage/index/ann/cmake-protect/CMakeLists.txt: ########## @@ -16,8 +16,9 @@ # under the License. # Make sure compile check in doris will not break compilation of faiss and openblas -set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -w") -set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -w") +# and do not add ub santizer for faiss and openblas. +set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -w -fno-sanitize=undefined") Review Comment: This is masking the RaBitQ bug that the PR just exposed. The reported UBSan site is a real misaligned access in FAISS: `RaBitQuantizer` stores the `FactorsData` floats at `code + (d + 7) / 8`, so for dimensions like the new test's `dim=3` the float fields start at byte offset 1. Disabling UBSan for all of FAISS/OpenBLAS only hides that undefined behavior; it still ships the unaligned loads/stores on every RaBitQ build/search path and also removes sanitizer coverage for unrelated third-party code. Please patch the vendored RaBitQ layout/accessors to align or `memcpy` the factors, or otherwise scope a suppression to a proven-safe operation, instead of compiling the dependency with UBSan disabled. ########## fe/fe-core/src/main/java/org/apache/doris/analysis/AnnIndexPropertiesChecker.java: ########## @@ -144,6 +146,9 @@ public static void checkProperties(Map<String, String> properties) { throw new AnalysisException("nlist of ann index must be specified for ivf/ivf_on_disk type"); } } Review Comment: This makes RaBitQ a supported `ivf_on_disk` quantizer, but the PR only tests `index_type = "ivf"`. The on-disk path has separate save/load behavior: it writes `ann.ivfdata`, serializes metadata-only inverted lists, then reloads through `IO_FLAG_SKIP_IVF_DATA` and `PreadInvertedLists`. A RaBitQ serialization or cached-read regression would not be covered by the new in-memory IVF test. Please add an `ivf_on_disk` RaBitQ regression case with quick-test output, or keep the validation restricted to `ivf` until that path is covered. -- 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]
