From c85b990b81e6b54899888ea004e05eb501fad70c Mon Sep 17 00:00:00 2001 From: lihangyu Date: Wed, 16 Sep 2026 05:56:58 +0800 Subject: [PATCH 1/6] [fix](inverted index) Skip BM25 norms for variant subcolumn indexes ### What problem does this PR solve? Issue Number: None Related PR: #53980, #60722 Problem Summary: A VARIANT column with many sparse paths indexed by analyzed field_pattern inverted indexes can grow its index size by about 10x after compaction. Root cause: since #53980, InvertedIndexColumnWriter::create_field enables CLucene norms. CLucene's DocumentsWriter::writeNorms stores one byte per document and fills documents without a value with defaultNorm, so every .nrm file is exactly as long as the segment has rows, however sparse the column is. Each extracted variant subcolumn gets its own index, so a segment pays rows * indexed paths * analyzed indexes bytes of norms. #60722 removed norms only for non-tokenized indexes. Reproduction: 1M rows with 1000 typed sparse paths and 11 field_pattern indexes (3 analyzed), loaded on 3.1 and then fully compacted. - Compacted on 3.1: .idx 99.6 MiB, no .nrm. - Compacted after an in-place upgrade to 4.1: .idx 934.1 MiB (9.4x). 875 .nrm files of 1,000,012 bytes each account for 834.5 MiB; the rest is 99.3 MiB. Fix: analyzed indexes on variant subcolumns (non-empty index suffix) no longer write norms. The mutable BE config inverted_index_write_norms_for_variant_subcolumn (default false) restores them. Indexes on ordinary columns are unchanged. Without norms, BM25 scores used to become NaN: CLucene keeps a field's token count in the .nrm header, so sumTotalTermFreq() is empty, avgdl is 0, and BM25Similarity::compute_tf_cache divides 0 by 0. This already hit any norm-less segment. compute_tf_cache now skips length normalization when avgdl is 0, so scores stay finite and depend on term frequency and idf only. With 1M rows compacted on this build, the norms switch off shrinks .idx from 457.3 MiB (357.6 MiB of norms) to 99.6 MiB, and MATCH queries return the same rows. ### Release note Analyzed inverted indexes on variant subcolumns no longer write BM25 norms, which removes index bloat for variant columns with many sparse indexed paths. BM25 score() on variant subcolumns no longer applies document-length normalization; set BE config inverted_index_write_norms_for_variant_subcolumn=true to keep norms. ### Check List (For Author) - Test: - Unit Test: BM25SimilarityTest.* and InvertedIndexWriterTest.* (34 tests, including the new ZeroAvgDlScoresWithoutLengthNorm and NormsFileSkippedForVariantSubcolumn) - Regression test: inverted_index_p0 test_variant_subcolumn_index_norms (new), test_bm25_score_variant, test_omit_norms - Manual test: 3.1 -> 4.1 upgrade reproduction, and a norms on/off comparison with BM25 score checks on this build - Behavior changed: Yes. BM25 scores on variant subcolumn indexes no longer use length normalization by default. - Does this need documentation: No Co-Authored-By: Claude Opus 5 --- be/src/common/config.cpp | 4 + be/src/common/config.h | 2 + .../index/inverted/inverted_index_writer.cpp | 6 +- .../inverted/similarity/bm25_similarity.cpp | 8 ++ .../similarity/bm25_similarity_test.cpp | 19 +++ .../segment/inverted_index_writer_test.cpp | 40 ++++++- .../test_variant_subcolumn_index_norms.out | 9 ++ .../test_variant_subcolumn_index_norms.groovy | 109 ++++++++++++++++++ 8 files changed, 195 insertions(+), 2 deletions(-) create mode 100644 regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out create mode 100644 regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy diff --git a/be/src/common/config.cpp b/be/src/common/config.cpp index 96eb4362142884..ba6ea7b19b667e 100644 --- a/be/src/common/config.cpp +++ b/be/src/common/config.cpp @@ -1337,6 +1337,10 @@ DEFINE_mDouble(inverted_index_ram_buffer_size, "512"); // -1 indicates not working. // Normally we should not change this, it's useful for testing. DEFINE_mInt32(inverted_index_max_buffered_docs, "-1"); +// Norms of a variant subcolumn index are dense even when the path is sparse, so a segment with +// thousands of indexed paths pays rows * paths bytes. Off by default; enable it only when BM25 on +// variant subcolumns needs document-length normalization. +DEFINE_mBool(inverted_index_write_norms_for_variant_subcolumn, "false"); // G16-h: zstd levels for the SNII dict-block compression and the .prx window // auto mode. Level 9 (vs the historical 3) shrinks the two largest compressed // sections -- textbench: index -457 MB (0.918x -> 0.891x V3) -- for an import diff --git a/be/src/common/config.h b/be/src/common/config.h index f5399093dd53d9..7a7572f121d057 100644 --- a/be/src/common/config.h +++ b/be/src/common/config.h @@ -1390,6 +1390,8 @@ DECLARE_Int32(ann_index_result_cache_stale_sweep_time_sec); // inverted index DECLARE_mDouble(inverted_index_ram_buffer_size); DECLARE_mInt32(inverted_index_max_buffered_docs); +// Whether analyzed inverted indexes on variant subcolumns write BM25 norms (one byte per row). +DECLARE_mBool(inverted_index_write_norms_for_variant_subcolumn); // G16-h: zstd levels for SNII dict blocks / prx windows. Default 3 (the // all-level-3 evaluation showed level 9 buys <=6.3% index size for 17-24% // import CPU; see the DEFINEs in config.cpp). diff --git a/be/src/storage/index/inverted/inverted_index_writer.cpp b/be/src/storage/index/inverted/inverted_index_writer.cpp index 519568c28eb0bf..0c381c78bd9389 100644 --- a/be/src/storage/index/inverted/inverted_index_writer.cpp +++ b/be/src/storage/index/inverted/inverted_index_writer.cpp @@ -162,7 +162,11 @@ Status InvertedIndexColumnWriter::create_field(lucene::document::Fie (*field)->setOmitTermFreqAndPositions( !(get_parser_phrase_support_string_from_properties(_index_meta->properties()) == INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES)); - if (_should_analyzer) { + // Norms cost one byte per segment row, including rows without a value. Every variant + // subcolumn (non-empty index suffix) gets its own index, so a segment may hold thousands of + // them and their norms can dwarf the data. BM25 on such an index scores without length norms. + if (_should_analyzer && (_index_meta->get_index_suffix().empty() || + config::inverted_index_write_norms_for_variant_subcolumn)) { (*field)->setOmitNorms(false); } diff --git a/be/src/storage/index/inverted/similarity/bm25_similarity.cpp b/be/src/storage/index/inverted/similarity/bm25_similarity.cpp index 2ab946a8791409..8be1d9c200de27 100644 --- a/be/src/storage/index/inverted/similarity/bm25_similarity.cpp +++ b/be/src/storage/index/inverted/similarity/bm25_similarity.cpp @@ -17,6 +17,7 @@ #include "storage/index/inverted/similarity/bm25_similarity.h" +#include #include namespace doris::segment_v2 { @@ -41,6 +42,13 @@ BM25Similarity::BM25Similarity(float idf, float avgdl) : _idf(idf), _avgdl(avgdl } void BM25Similarity::compute_tf_cache() { + // CLucene keeps a field's token count in its .nrm header, so avgdl is 0 when no segment of the + // field stores norms (e.g. variant subcolumn indexes). Every document then has an unknown length: + // score without length normalization instead of computing 0 / 0. + if (_avgdl <= 0.0F) { + std::fill(_cache.begin(), _cache.end(), 1.0F / _k1); + return; + } for (int i = 0; i < _cache.size(); i++) { _cache[i] = 1.0F / (_k1 * ((1 - _b) + _b * LENGTH_TABLE[i] / _avgdl)); } diff --git a/be/test/storage/index/inverted/similarity/bm25_similarity_test.cpp b/be/test/storage/index/inverted/similarity/bm25_similarity_test.cpp index 9ceba92421e87f..fa3a04a70bab4b 100644 --- a/be/test/storage/index/inverted/similarity/bm25_similarity_test.cpp +++ b/be/test/storage/index/inverted/similarity/bm25_similarity_test.cpp @@ -19,6 +19,7 @@ #include +#include #include #include "common/be_mock_util.h" @@ -289,3 +290,21 @@ TEST_F(BM25SimilarityTest, CacheConsistencyTest) { ASSERT_FLOAT_EQ(similarity_->_cache[i], expected); } } + +// Indexes without norms report no token count, so avgdl is 0: scores must stay finite and ignore +// document length instead of turning into NaN. +TEST_F(BM25SimilarityTest, ZeroAvgDlScoresWithoutLengthNorm) { + mock_stats_->set_mock_idf(2.0f); + mock_stats_->set_mock_avg_dl(0.0f); + + similarity_->for_one_term(context_, L"field", L"term"); + + for (int i = 0; i < 256; ++i) { + ASSERT_FLOAT_EQ(similarity_->_cache[i], 1.0f / similarity_->_k1); + } + float score = similarity_->score(1.0f, 0); + ASSERT_FALSE(std::isnan(score)); + ASSERT_FLOAT_EQ(score, + similarity_->_weight - similarity_->_weight / (1.0f + 1.0f / similarity_->_k1)); + ASSERT_GT(similarity_->score(2.0f, 0), score); +} diff --git a/be/test/storage/segment/inverted_index_writer_test.cpp b/be/test/storage/segment/inverted_index_writer_test.cpp index 1994e226182436..6a74fe8591166c 100644 --- a/be/test/storage/segment/inverted_index_writer_test.cpp +++ b/be/test/storage/segment/inverted_index_writer_test.cpp @@ -501,7 +501,8 @@ class InvertedIndexWriterTest : public testing::Test { } // Helper method to create an inverted index with tokenization enabled - void create_tokenized_index(std::string_view rowset_id, int seg_id, bool enable_analyzer) { + void create_tokenized_index(std::string_view rowset_id, int seg_id, bool enable_analyzer, + const std::string& index_suffix = "") { auto tablet_schema = create_schema(); // Create index meta with tokenization setting @@ -525,6 +526,9 @@ class InvertedIndexWriterTest : public testing::Test { TabletIndex idx_meta; idx_meta.init_from_pb(*index_meta_pb.get()); + if (!index_suffix.empty()) { + idx_meta.set_escaped_escaped_index_suffix_path(index_suffix); + } std::string index_path_prefix {InvertedIndexDescriptor::get_index_file_path_prefix( local_segment_path(kTestDir, rowset_id, seg_id))}; @@ -1825,4 +1829,38 @@ TEST_F(InvertedIndexWriterTest, NormsFileCreationWithTokenization) { << "inverted_index_writer.cpp where .nrm file creation depends on _should_analyzer."; } +// A variant subcolumn index carries a non-empty index suffix. Its norms take one byte per segment +// row even when the path is sparse, so they are written only when +// inverted_index_write_norms_for_variant_subcolumn is enabled. +TEST_F(InvertedIndexWriterTest, NormsFileSkippedForVariantSubcolumn) { + const bool original_config_value = config::inverted_index_write_norms_for_variant_subcolumn; + Defer restore_config {[&]() { + config::inverted_index_write_norms_for_variant_subcolumn = original_config_value; + }}; + + TabletIndexPB index_meta_pb; + index_meta_pb.set_index_type(IndexType::INVERTED); + index_meta_pb.set_index_id(1); + index_meta_pb.set_index_name("test"); + index_meta_pb.add_col_unique_id(1); // c2 column id + (*index_meta_pb.mutable_properties())["parser"] = "standard"; + TabletIndex subcolumn_index_meta; + subcolumn_index_meta.init_from_pb(index_meta_pb); + subcolumn_index_meta.set_escaped_escaped_index_suffix_path("v.s_host"); + + config::inverted_index_write_norms_for_variant_subcolumn = false; + create_tokenized_index("test_variant_subcolumn_without_norms", 0, true, "v.s_host"); + std::string prefix_without_norms {InvertedIndexDescriptor::get_index_file_path_prefix( + local_segment_path(kTestDir, "test_variant_subcolumn_without_norms", 0))}; + EXPECT_FALSE(check_norms_file_exists(prefix_without_norms, &subcolumn_index_meta)) + << "a tokenized variant subcolumn index must not write .nrm by default"; + + config::inverted_index_write_norms_for_variant_subcolumn = true; + create_tokenized_index("test_variant_subcolumn_with_norms", 1, true, "v.s_host"); + std::string prefix_with_norms {InvertedIndexDescriptor::get_index_file_path_prefix( + local_segment_path(kTestDir, "test_variant_subcolumn_with_norms", 1))}; + EXPECT_TRUE(check_norms_file_exists(prefix_with_norms, &subcolumn_index_meta)) + << "inverted_index_write_norms_for_variant_subcolumn=true must restore .nrm"; +} + } // namespace doris::segment_v2 diff --git a/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out b/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out new file mode 100644 index 00000000000000..2988072d642e32 --- /dev/null +++ b/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out @@ -0,0 +1,9 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !variant_subcolumn_score -- +2 0.6931 +3 0.9531 + +-- !plain_column_score -- +1 0.5754 +3 0.8714 + diff --git a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy new file mode 100644 index 00000000000000..61783c2e5ac215 --- /dev/null +++ b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy @@ -0,0 +1,109 @@ +// 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. + +// Analyzed indexes on variant subcolumns must not write dense BM25 norms (.nrm, one byte per row), +// while analyzed indexes on ordinary columns still do. BM25 scoring keeps working on both. +suite("test_variant_subcolumn_index_norms", "p0") { + if (isCloudMode()) { + return + } + + sql """ set enable_segment_limit_pushdown = true """ + sql """ set enable_match_without_inverted_index = false """ + sql """ set default_variant_enable_typed_paths_to_sparse = false """ + sql """ set default_variant_enable_doc_mode = false """ + + sql "DROP TABLE IF EXISTS test_variant_subcolumn_index_norms" + sql """ + CREATE TABLE test_variant_subcolumn_index_norms ( + id INT, + content TEXT, + v variant< + 's_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, + INDEX idx_content (content) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true" + ), + INDEX idx_v_s (v) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "field_pattern"="s_*" + ) + ) ENGINE=OLAP DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ( + "replication_allocation" = "tag.location.default: 1", + "disable_auto_compaction" = "true", + "inverted_index_storage_format" = "V2" + ) + """ + sql """ insert into test_variant_subcolumn_index_norms values + (1, 'alpha database server', parse_to_variant('{"s_host":"alpha database server"}')), + (2, 'beta server cluster', parse_to_variant('{"s_host":"beta server cluster", "s_note":"alpha"}')), + (3, 'alpha', parse_to_variant('{"s_note":"alpha alpha beta"}')), + (4, 'gamma', parse_to_variant('{"other":"alpha"}')) + """ + sql " sync " + + // scores are printed so that a NaN (which compares greater than 0) cannot slip through + order_qt_variant_subcolumn_score """ + select id, round(score(), 4) + from test_variant_subcolumn_index_norms + where cast(v["s_note"] as string) match_phrase "alpha" + order by score() desc + limit 10 + """ + order_qt_plain_column_score """ + select id, round(score(), 4) + from test_variant_subcolumn_index_norms + where content match_phrase "alpha" + order by score() desc + limit 10 + """ + + def backendIdToIp = [:] + def backendIdToHttpPort = [:] + getBackendIpHttpPort(backendIdToIp, backendIdToHttpPort) + def tablet = sql_return_maparray("show tablets from test_variant_subcolumn_index_norms")[0] + def (code, out, err) = http_client("GET", String.format( + "http://%s:%s/api/show_nested_index_file?tablet_id=%s", + backendIdToIp.get(tablet.BackendId), backendIdToHttpPort.get(tablet.BackendId), + tablet.TabletId)) + logger.info("show_nested_index_file code=${code}, out=${out}, err=${err}") + assertEquals(0, code) + + def subcolumnIndexes = [] + def plainIndexes = [] + for (def rowset in parseJson(out.trim()).rowsets) { + for (def segment in rowset.segments) { + for (def index in segment.indices) { + def hasNorms = index.files.any { file -> file.name.endsWith(".nrm") } + if (index.index_suffix.isEmpty()) { + plainIndexes.add(hasNorms) + } else { + subcolumnIndexes.add(hasNorms) + } + } + } + } + logger.info("norms of plain indexes: ${plainIndexes}, subcolumn indexes: ${subcolumnIndexes}") + // idx_content on the single segment writes norms; idx_v_s on s_host and s_note does not + assertEquals([true], plainIndexes) + assertEquals([false, false], subcolumnIndexes) +} From b1d0aeb4ee30ea59cbe6b9db413e657c60262108 Mon Sep 17 00:00:00 2001 From: lihangyu Date: Wed, 16 Sep 2026 11:47:30 +0800 Subject: [PATCH 2/6] [fix](inverted index) Make BM25 norms a per-index "norms" property ### What problem does this PR solve? Issue Number: None Related PR: #53980, #60722 Problem Summary: The previous commit skipped norms for variant subcolumn indexes behind a BE config. Elasticsearch controls norms per field instead: `norms` is a mapping parameter, true by default for text and false for keyword, and a dynamic template can turn it off for a whole family of dynamically mapped fields. A Doris variant path is the equivalent of a dynamically mapped field, so the switch belongs on the index rather than on the BE. This commit replaces the config with an inverted index property: - PROPERTIES("norms" = "true" | "false") on the index. - The default is false when the index is on a variant path: a field_pattern index, or the copy inherited by one extracted subcolumn, which carries the path as its index suffix. - The default is true otherwise, so analyzed indexes on ordinary columns keep their norms. FE accepts the key and rejects any value other than true or false. BE reads it in InvertedIndexColumnWriter::create_field through get_index_norms_from_properties, so the decision is made per index instead of per process, and one table can keep norms on the paths that need scoring while dropping them elsewhere. ### Release note Inverted indexes accept a new "norms" property. Analyzed indexes on variant paths no longer write BM25 norms by default; PROPERTIES("norms" = "true") keeps them for one index, and "norms" = "false" drops them for an ordinary column. ### Check List (For Author) - Test: - Unit Test: BE BM25SimilarityTest.* and InvertedIndexWriterTest.*, 34 tests passed, including NormsFollowIndexNormsProperty which covers a subcolumn index by default, a subcolumn index with norms = true, a field_pattern index by default, and an ordinary column with norms = false; FE InvertedIndexPropertiesTest, 51 tests passed, including the accepted and rejected property values - Regression test: inverted_index_p0 test_variant_subcolumn_index_norms now asserts norms per index suffix (ordinary column keeps them, s_* paths drop them, a t_* pattern index with norms = true keeps them); test_bm25_score_variant and test_omit_norms pass - Behavior changed: Yes, the BE config from the previous commit is gone and the default now lives on the index. - Does this need documentation: Yes, the inverted index property list needs the new key (doc PR to follow) Co-Authored-By: Claude Opus 5 --- be/src/common/config.cpp | 4 - be/src/common/config.h | 2 - .../index/inverted/inverted_index_parser.cpp | 8 ++ .../index/inverted/inverted_index_parser.h | 9 ++ .../index/inverted/inverted_index_writer.cpp | 13 +-- .../segment/inverted_index_writer_test.cpp | 86 ++++++++++++------- .../analysis/InvertedIndexProperties.java | 4 + .../doris/analysis/InvertedIndexUtil.java | 10 +++ .../analysis/InvertedIndexPropertiesTest.java | 25 ++++++ .../test_variant_subcolumn_index_norms.groovy | 37 +++++--- 10 files changed, 141 insertions(+), 57 deletions(-) diff --git a/be/src/common/config.cpp b/be/src/common/config.cpp index ba6ea7b19b667e..96eb4362142884 100644 --- a/be/src/common/config.cpp +++ b/be/src/common/config.cpp @@ -1337,10 +1337,6 @@ DEFINE_mDouble(inverted_index_ram_buffer_size, "512"); // -1 indicates not working. // Normally we should not change this, it's useful for testing. DEFINE_mInt32(inverted_index_max_buffered_docs, "-1"); -// Norms of a variant subcolumn index are dense even when the path is sparse, so a segment with -// thousands of indexed paths pays rows * paths bytes. Off by default; enable it only when BM25 on -// variant subcolumns needs document-length normalization. -DEFINE_mBool(inverted_index_write_norms_for_variant_subcolumn, "false"); // G16-h: zstd levels for the SNII dict-block compression and the .prx window // auto mode. Level 9 (vs the historical 3) shrinks the two largest compressed // sections -- textbench: index -457 MB (0.918x -> 0.891x V3) -- for an import diff --git a/be/src/common/config.h b/be/src/common/config.h index 7a7572f121d057..f5399093dd53d9 100644 --- a/be/src/common/config.h +++ b/be/src/common/config.h @@ -1390,8 +1390,6 @@ DECLARE_Int32(ann_index_result_cache_stale_sweep_time_sec); // inverted index DECLARE_mDouble(inverted_index_ram_buffer_size); DECLARE_mInt32(inverted_index_max_buffered_docs); -// Whether analyzed inverted indexes on variant subcolumns write BM25 norms (one byte per row). -DECLARE_mBool(inverted_index_write_norms_for_variant_subcolumn); // G16-h: zstd levels for SNII dict blocks / prx windows. Default 3 (the // all-level-3 evaluation showed level 9 buys <=6.3% index size for 17-24% // import CPU; see the DEFINEs in config.cpp). diff --git a/be/src/storage/index/inverted/inverted_index_parser.cpp b/be/src/storage/index/inverted/inverted_index_parser.cpp index 3cfc3d4b0e970b..331d256700b044 100644 --- a/be/src/storage/index/inverted/inverted_index_parser.cpp +++ b/be/src/storage/index/inverted/inverted_index_parser.cpp @@ -117,6 +117,14 @@ std::string get_parser_phrase_support_string_from_properties( return INVERTED_INDEX_PARSER_PHRASE_SUPPORT_NO; } +bool get_index_norms_from_properties(const std::map& properties, + bool default_value) { + if (auto it = properties.find(INVERTED_INDEX_NORMS_KEY); it != properties.end()) { + return it->second == INVERTED_INDEX_PARSER_TRUE; + } + return default_value; +} + CharFilterMap get_parser_char_filter_map_from_properties( const std::map& properties) { if (!properties.contains(INVERTED_INDEX_PARSER_CHAR_FILTER_TYPE)) { diff --git a/be/src/storage/index/inverted/inverted_index_parser.h b/be/src/storage/index/inverted/inverted_index_parser.h index 6fd5e34d466211..e955ed1eb447cb 100644 --- a/be/src/storage/index/inverted/inverted_index_parser.h +++ b/be/src/storage/index/inverted/inverted_index_parser.h @@ -89,6 +89,9 @@ const std::string INVERTED_INDEX_PARSER_PHRASE_SUPPORT_KEY = "support_phrase"; const std::string INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES = "true"; const std::string INVERTED_INDEX_PARSER_PHRASE_SUPPORT_NO = "false"; +// Whether an analyzed index stores BM25 norms, which take one byte per row of the segment. +const std::string INVERTED_INDEX_NORMS_KEY = "norms"; + const std::string INVERTED_INDEX_PARSER_CHAR_FILTER_TYPE = "char_filter_type"; const std::string INVERTED_INDEX_PARSER_CHAR_FILTER_PATTERN = "char_filter_pattern"; const std::string INVERTED_INDEX_PARSER_CHAR_FILTER_REPLACEMENT = "char_filter_replacement"; @@ -152,6 +155,12 @@ std::string get_parser_mode_string_from_properties( std::string get_parser_phrase_support_string_from_properties( const std::map& properties); +// Whether this index writes BM25 norms. Norms cost one byte per row of the segment, including rows +// that have no value for the field, so callers pass a default of false for indexes on variant paths, +// where one segment holds one index per path. "norms" = "true" / "false" overrides the default. +bool get_index_norms_from_properties(const std::map& properties, + bool default_value); + CharFilterMap get_parser_char_filter_map_from_properties( const std::map& properties); diff --git a/be/src/storage/index/inverted/inverted_index_writer.cpp b/be/src/storage/index/inverted/inverted_index_writer.cpp index 0c381c78bd9389..87d2da44f44565 100644 --- a/be/src/storage/index/inverted/inverted_index_writer.cpp +++ b/be/src/storage/index/inverted/inverted_index_writer.cpp @@ -162,11 +162,14 @@ Status InvertedIndexColumnWriter::create_field(lucene::document::Fie (*field)->setOmitTermFreqAndPositions( !(get_parser_phrase_support_string_from_properties(_index_meta->properties()) == INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES)); - // Norms cost one byte per segment row, including rows without a value. Every variant - // subcolumn (non-empty index suffix) gets its own index, so a segment may hold thousands of - // them and their norms can dwarf the data. BM25 on such an index scores without length norms. - if (_should_analyzer && (_index_meta->get_index_suffix().empty() || - config::inverted_index_write_norms_for_variant_subcolumn)) { + // Norms cost one byte per segment row, including rows without a value. A variant path index + // (a field_pattern index, or the copy inherited by one extracted subcolumn, which carries the + // path as its index suffix) is one of possibly thousands in a segment, so its norms can dwarf + // the data: those default to no norms, and "norms" = "true" brings them back per index. + const bool variant_path_index = + !_index_meta->get_index_suffix().empty() || !_index_meta->field_pattern().empty(); + if (_should_analyzer && + get_index_norms_from_properties(_index_meta->properties(), !variant_path_index)) { (*field)->setOmitNorms(false); } diff --git a/be/test/storage/segment/inverted_index_writer_test.cpp b/be/test/storage/segment/inverted_index_writer_test.cpp index 6a74fe8591166c..f3b48165968120 100644 --- a/be/test/storage/segment/inverted_index_writer_test.cpp +++ b/be/test/storage/segment/inverted_index_writer_test.cpp @@ -502,7 +502,8 @@ class InvertedIndexWriterTest : public testing::Test { // Helper method to create an inverted index with tokenization enabled void create_tokenized_index(std::string_view rowset_id, int seg_id, bool enable_analyzer, - const std::string& index_suffix = "") { + const std::string& index_suffix = "", + const std::map& extra_properties = {}) { auto tablet_schema = create_schema(); // Create index meta with tokenization setting @@ -523,6 +524,9 @@ class InvertedIndexWriterTest : public testing::Test { // This will make should_analyzer() return true (*properties)["parser"] = "standard"; } + for (const auto& [key, value] : extra_properties) { + (*properties)[key] = value; + } TabletIndex idx_meta; idx_meta.init_from_pb(*index_meta_pb.get()); @@ -1829,38 +1833,56 @@ TEST_F(InvertedIndexWriterTest, NormsFileCreationWithTokenization) { << "inverted_index_writer.cpp where .nrm file creation depends on _should_analyzer."; } -// A variant subcolumn index carries a non-empty index suffix. Its norms take one byte per segment -// row even when the path is sparse, so they are written only when -// inverted_index_write_norms_for_variant_subcolumn is enabled. -TEST_F(InvertedIndexWriterTest, NormsFileSkippedForVariantSubcolumn) { - const bool original_config_value = config::inverted_index_write_norms_for_variant_subcolumn; - Defer restore_config {[&]() { - config::inverted_index_write_norms_for_variant_subcolumn = original_config_value; - }}; +// Norms take one byte per segment row for every indexed path, so an index on a variant path (a +// field_pattern index, or the copy inherited by one extracted subcolumn, which carries the path as +// its index suffix) writes none by default. The "norms" property overrides that per index. +TEST_F(InvertedIndexWriterTest, NormsFollowIndexNormsProperty) { + auto make_index_meta = [](const std::string& index_suffix, + const std::map& extra_properties) { + TabletIndexPB index_meta_pb; + index_meta_pb.set_index_type(IndexType::INVERTED); + index_meta_pb.set_index_id(1); + index_meta_pb.set_index_name("test"); + index_meta_pb.add_col_unique_id(1); // c2 column id + (*index_meta_pb.mutable_properties())["parser"] = "standard"; + for (const auto& [key, value] : extra_properties) { + (*index_meta_pb.mutable_properties())[key] = value; + } + TabletIndex index_meta; + index_meta.init_from_pb(index_meta_pb); + if (!index_suffix.empty()) { + index_meta.set_escaped_escaped_index_suffix_path(index_suffix); + } + return index_meta; + }; + auto path_prefix = [this](const std::string& rowset_id, int seg_id) { + return std::string {InvertedIndexDescriptor::get_index_file_path_prefix( + local_segment_path(kTestDir, rowset_id, seg_id))}; + }; - TabletIndexPB index_meta_pb; - index_meta_pb.set_index_type(IndexType::INVERTED); - index_meta_pb.set_index_id(1); - index_meta_pb.set_index_name("test"); - index_meta_pb.add_col_unique_id(1); // c2 column id - (*index_meta_pb.mutable_properties())["parser"] = "standard"; - TabletIndex subcolumn_index_meta; - subcolumn_index_meta.init_from_pb(index_meta_pb); - subcolumn_index_meta.set_escaped_escaped_index_suffix_path("v.s_host"); - - config::inverted_index_write_norms_for_variant_subcolumn = false; - create_tokenized_index("test_variant_subcolumn_without_norms", 0, true, "v.s_host"); - std::string prefix_without_norms {InvertedIndexDescriptor::get_index_file_path_prefix( - local_segment_path(kTestDir, "test_variant_subcolumn_without_norms", 0))}; - EXPECT_FALSE(check_norms_file_exists(prefix_without_norms, &subcolumn_index_meta)) - << "a tokenized variant subcolumn index must not write .nrm by default"; - - config::inverted_index_write_norms_for_variant_subcolumn = true; - create_tokenized_index("test_variant_subcolumn_with_norms", 1, true, "v.s_host"); - std::string prefix_with_norms {InvertedIndexDescriptor::get_index_file_path_prefix( - local_segment_path(kTestDir, "test_variant_subcolumn_with_norms", 1))}; - EXPECT_TRUE(check_norms_file_exists(prefix_with_norms, &subcolumn_index_meta)) - << "inverted_index_write_norms_for_variant_subcolumn=true must restore .nrm"; + create_tokenized_index("variant_subcolumn_default", 0, true, "v.s_host"); + TabletIndex subcolumn_default = make_index_meta("v.s_host", {}); + EXPECT_FALSE(check_norms_file_exists(path_prefix("variant_subcolumn_default", 0), + &subcolumn_default)) + << "a variant subcolumn index must not write .nrm by default"; + + create_tokenized_index("variant_subcolumn_norms_on", 1, true, "v.s_host", {{"norms", "true"}}); + TabletIndex subcolumn_norms_on = make_index_meta("v.s_host", {{"norms", "true"}}); + EXPECT_TRUE(check_norms_file_exists(path_prefix("variant_subcolumn_norms_on", 1), + &subcolumn_norms_on)) + << "norms = true must restore .nrm for a variant subcolumn index"; + + create_tokenized_index("field_pattern_default", 2, true, "", {{"field_pattern", "s_*"}}); + TabletIndex field_pattern_default = make_index_meta("", {{"field_pattern", "s_*"}}); + EXPECT_FALSE(check_norms_file_exists(path_prefix("field_pattern_default", 2), + &field_pattern_default)) + << "a field_pattern index must not write .nrm by default"; + + create_tokenized_index("plain_column_norms_off", 3, true, "", {{"norms", "false"}}); + TabletIndex plain_norms_off = make_index_meta("", {{"norms", "false"}}); + EXPECT_FALSE( + check_norms_file_exists(path_prefix("plain_column_norms_off", 3), &plain_norms_off)) + << "norms = false must drop .nrm for an ordinary column index"; } } // namespace doris::segment_v2 diff --git a/fe/fe-catalog/src/main/java/org/apache/doris/analysis/InvertedIndexProperties.java b/fe/fe-catalog/src/main/java/org/apache/doris/analysis/InvertedIndexProperties.java index b9b5756f17dfc0..6a3876360b289b 100644 --- a/fe/fe-catalog/src/main/java/org/apache/doris/analysis/InvertedIndexProperties.java +++ b/fe/fe-catalog/src/main/java/org/apache/doris/analysis/InvertedIndexProperties.java @@ -53,6 +53,10 @@ public class InvertedIndexProperties { public static String INVERTED_INDEX_SUPPORT_PHRASE_KEY = "support_phrase"; + // Whether an analyzed index stores BM25 norms. Indexes on variant paths default to false + // because norms cost one byte per row of the segment for every indexed path. + public static String INVERTED_INDEX_NORMS_KEY = "norms"; + public static String INVERTED_INDEX_PARSER_IGNORE_ABOVE_KEY = "ignore_above"; public static String INVERTED_INDEX_PARSER_LOWERCASE_KEY = "lower_case"; diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/InvertedIndexUtil.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/InvertedIndexUtil.java index 9f58bde9248dc8..57da08fe994c87 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/InvertedIndexUtil.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/InvertedIndexUtil.java @@ -70,6 +70,9 @@ public class InvertedIndexUtil { public static String INVERTED_INDEX_SUPPORT_PHRASE_KEY = InvertedIndexProperties.INVERTED_INDEX_SUPPORT_PHRASE_KEY; + public static String INVERTED_INDEX_NORMS_KEY = + InvertedIndexProperties.INVERTED_INDEX_NORMS_KEY; + public static String INVERTED_INDEX_PARSER_IGNORE_ABOVE_KEY = InvertedIndexProperties.INVERTED_INDEX_PARSER_IGNORE_ABOVE_KEY; @@ -202,6 +205,7 @@ private static void checkInvertedIndexProperties(Map properties, INVERTED_INDEX_PARSER_KEY_ALIAS, INVERTED_INDEX_PARSER_MODE_KEY, INVERTED_INDEX_SUPPORT_PHRASE_KEY, + INVERTED_INDEX_NORMS_KEY, INVERTED_INDEX_PARSER_CHAR_FILTER_TYPE, INVERTED_INDEX_PARSER_CHAR_FILTER_PATTERN, INVERTED_INDEX_PARSER_CHAR_FILTER_REPLACEMENT, @@ -288,6 +292,12 @@ private static void checkInvertedIndexProperties(Map properties, + ", support_phrase must be true or false"); } + String norms = properties.get(INVERTED_INDEX_NORMS_KEY); + if (norms != null && !norms.matches("true|false")) { + throw new AnalysisException("Invalid inverted index 'norms' value: " + norms + + ", norms must be true or false"); + } + checkCharFilterProperties(properties); if (ignoreAbove != null) { diff --git a/fe/fe-core/src/test/java/org/apache/doris/analysis/InvertedIndexPropertiesTest.java b/fe/fe-core/src/test/java/org/apache/doris/analysis/InvertedIndexPropertiesTest.java index a482685cb3b523..e439360a02616d 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/analysis/InvertedIndexPropertiesTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/analysis/InvertedIndexPropertiesTest.java @@ -425,6 +425,31 @@ private static void withIndexPolicyManager(IndexPolicyMgr manager, Runnable acti } } + // --- norms --- + + @Test + public void testNormsPropertyAccepted() throws AnalysisException { + for (String value : new String[] {"true", "false"}) { + Map props = new HashMap<>(); + props.put("parser", "english"); + props.put("norms", value); + InvertedIndexUtil.checkInvertedIndexParser("col1", PrimitiveType.STRING, props, + TInvertedIndexFileStorageFormat.V2); + } + } + + @Test + public void testNormsPropertyRejectsOtherValues() { + Map props = new HashMap<>(); + props.put("parser", "english"); + props.put("norms", "yes"); + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + () -> InvertedIndexUtil.checkInvertedIndexParser("col1", PrimitiveType.STRING, props, + TInvertedIndexFileStorageFormat.V2)); + Assertions.assertTrue(exception.getMessage().contains("norms must be true or false"), + exception.getMessage()); + } + // The SNII gate in checkInvertedIndexParser sees the parent VARIANT type on a whole-column // index and the sub-column type on a field_pattern index. Only the latter can be judged, // so VARIANT itself must pass. diff --git a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy index 61783c2e5ac215..4e22ba2e96ad7a 100644 --- a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy +++ b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy @@ -34,6 +34,7 @@ suite("test_variant_subcolumn_index_norms", "p0") { content TEXT, v variant< 's_*' : text, + 't_*' : text, PROPERTIES("variant_max_subcolumns_count"="0") >, INDEX idx_content (content) USING INVERTED PROPERTIES( @@ -44,6 +45,12 @@ suite("test_variant_subcolumn_index_norms", "p0") { "parser"="english", "support_phrase"="true", "field_pattern"="s_*" + ), + INDEX idx_v_t (v) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "field_pattern"="t_*", + "norms"="true" ) ) ENGINE=OLAP DUPLICATE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1 @@ -55,8 +62,8 @@ suite("test_variant_subcolumn_index_norms", "p0") { """ sql """ insert into test_variant_subcolumn_index_norms values (1, 'alpha database server', parse_to_variant('{"s_host":"alpha database server"}')), - (2, 'beta server cluster', parse_to_variant('{"s_host":"beta server cluster", "s_note":"alpha"}')), - (3, 'alpha', parse_to_variant('{"s_note":"alpha alpha beta"}')), + (2, 'beta server cluster', parse_to_variant('{"s_host":"beta server cluster", "s_note":"alpha", "t_note":"alpha"}')), + (3, 'alpha', parse_to_variant('{"s_note":"alpha alpha beta", "t_note":"alpha beta"}')), (4, 'gamma', parse_to_variant('{"other":"alpha"}')) """ sql " sync " @@ -88,22 +95,24 @@ suite("test_variant_subcolumn_index_norms", "p0") { logger.info("show_nested_index_file code=${code}, out=${out}, err=${err}") assertEquals(0, code) - def subcolumnIndexes = [] - def plainIndexes = [] + def normsBySuffix = [:] for (def rowset in parseJson(out.trim()).rowsets) { for (def segment in rowset.segments) { for (def index in segment.indices) { - def hasNorms = index.files.any { file -> file.name.endsWith(".nrm") } - if (index.index_suffix.isEmpty()) { - plainIndexes.add(hasNorms) - } else { - subcolumnIndexes.add(hasNorms) - } + normsBySuffix[index.index_suffix] = index.files.any { file -> file.name.endsWith(".nrm") } } } } - logger.info("norms of plain indexes: ${plainIndexes}, subcolumn indexes: ${subcolumnIndexes}") - // idx_content on the single segment writes norms; idx_v_s on s_host and s_note does not - assertEquals([true], plainIndexes) - assertEquals([false, false], subcolumnIndexes) + logger.info("norms by index suffix: ${normsBySuffix}") + // the suffix is the escaped variant path, e.g. v%2Es%5Fhost for v.s_host + def normsOf = { path -> + normsBySuffix.find { suffix, hasNorms -> + suffix.replace("%2E", ".").replace("%5F", "_").contains(path) + }?.value + } + // idx_content on an ordinary column keeps norms, idx_v_s drops them, idx_v_t asks for them back + assertEquals(true, normsBySuffix[""]) + assertEquals(false, normsOf("s_host")) + assertEquals(false, normsOf("s_note")) + assertEquals(true, normsOf("t_note")) } From 067dbc51b0f9ff6bb710e70b3dcb3657e055f727 Mon Sep 17 00:00:00 2001 From: lihangyu Date: Wed, 16 Sep 2026 14:46:45 +0800 Subject: [PATCH 3/6] [test](inverted index) Cover whole-column VARIANT indexes in the norms test ### What problem does this PR solve? Problem Summary: The norms regression test only covered indexes declared with a field_pattern. An inverted index built on a VARIANT column without a field_pattern is copied to every extracted subcolumn, and that copy carries the properties of the index it was copied from, so the `norms` property on the parent index decides the behaviour of all of its subcolumn copies. Add two such indexes to the test, one taking the default and one asking for norms back, so the inherited path is covered as well: norms by index suffix: [:true, v.s_host:false, v.s_note:false, v.t_note:true, vd.a_host:false, vd.other:false, vn.b_host:true, vn.other:true] ### Release note None ### Check List (For Author) - Test: Regression test - regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5 --- .../test_variant_subcolumn_index_norms.groovy | 39 +++++++++++++++++-- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy index 4e22ba2e96ad7a..1d4854300f7dfd 100644 --- a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy +++ b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy @@ -17,6 +17,8 @@ // Analyzed indexes on variant subcolumns must not write dense BM25 norms (.nrm, one byte per row), // while analyzed indexes on ordinary columns still do. BM25 scoring keeps working on both. +// This holds both for an index declared with a field_pattern and for a whole-column index on a +// VARIANT column, whose per-subcolumn copies inherit the properties of the index they come from. suite("test_variant_subcolumn_index_norms", "p0") { if (isCloudMode()) { return @@ -37,6 +39,14 @@ suite("test_variant_subcolumn_index_norms", "p0") { 't_*' : text, PROPERTIES("variant_max_subcolumns_count"="0") >, + vd variant< + 'a_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, + vn variant< + 'b_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, INDEX idx_content (content) USING INVERTED PROPERTIES( "parser"="english", "support_phrase"="true" @@ -51,6 +61,15 @@ suite("test_variant_subcolumn_index_norms", "p0") { "support_phrase"="true", "field_pattern"="t_*", "norms"="true" + ), + INDEX idx_vd (vd) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true" + ), + INDEX idx_vn (vn) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "norms"="true" ) ) ENGINE=OLAP DUPLICATE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1 @@ -61,10 +80,18 @@ suite("test_variant_subcolumn_index_norms", "p0") { ) """ sql """ insert into test_variant_subcolumn_index_norms values - (1, 'alpha database server', parse_to_variant('{"s_host":"alpha database server"}')), - (2, 'beta server cluster', parse_to_variant('{"s_host":"beta server cluster", "s_note":"alpha", "t_note":"alpha"}')), - (3, 'alpha', parse_to_variant('{"s_note":"alpha alpha beta", "t_note":"alpha beta"}')), - (4, 'gamma', parse_to_variant('{"other":"alpha"}')) + (1, 'alpha database server', parse_to_variant('{"s_host":"alpha database server"}'), + parse_to_variant('{"a_host":"alpha database server"}'), + parse_to_variant('{"b_host":"alpha database server"}')), + (2, 'beta server cluster', parse_to_variant('{"s_host":"beta server cluster", "s_note":"alpha", "t_note":"alpha"}'), + parse_to_variant('{"a_host":"beta server cluster"}'), + parse_to_variant('{"b_host":"beta server cluster"}')), + (3, 'alpha', parse_to_variant('{"s_note":"alpha alpha beta", "t_note":"alpha beta"}'), + parse_to_variant('{"a_host":"alpha"}'), + parse_to_variant('{"b_host":"alpha"}')), + (4, 'gamma', parse_to_variant('{"other":"alpha"}'), + parse_to_variant('{"other":"alpha"}'), + parse_to_variant('{"other":"alpha"}')) """ sql " sync " @@ -115,4 +142,8 @@ suite("test_variant_subcolumn_index_norms", "p0") { assertEquals(false, normsOf("s_host")) assertEquals(false, normsOf("s_note")) assertEquals(true, normsOf("t_note")) + // a whole-column index on a VARIANT column has no suffix of its own, but every subcolumn copy + // inherits its properties: idx_vd drops norms by default, idx_vn keeps them because it asks to + assertEquals(false, normsOf("a_host")) + assertEquals(true, normsOf("b_host")) } From 83dfd5391e4c14126252a5017255db7c18efbcc4 Mon Sep 17 00:00:00 2001 From: lihangyu Date: Wed, 16 Sep 2026 16:14:31 +0800 Subject: [PATCH 4/6] [fix](inverted index) Make the variant norms skip an opt-in BE config ### What problem does this PR solve? Problem Summary: The "norms" property did not mean the same thing on every index. An analyzed index on a variant path defaulted to writing no norms while an analyzed index on any other column defaulted to writing them, so the default came from the shape of the index and the property only corrected it. That also changed how variant path indexes rank, for every user, as soon as the fix landed. Give the property one meaning everywhere -- an analyzed index writes norms unless the property says otherwise, on a variant path as on an ordinary column -- and move the variant skip into the new mutable BE config inverted_index_skip_norms_for_variant, which is off by default. Turning it on drops norms for every index on a variant path (an index declared with a field_pattern, or the copy that each extracted subcolumn inherits from it) whatever that index's property says, so a cluster that cares about the space can reclaim it without rewriting its index definitions, and a cluster that cares about ranking keeps what it has. Default behaviour therefore matches the state before this PR: norms are written everywhere. ### Release note Added the BE config inverted_index_skip_norms_for_variant (off by default, mutable). Turning it on drops BM25 norms for every inverted index on a VARIANT path, whatever the index's "norms" property says. ### Check List (For Author) - Test: Unit Test / Regression test - be/test/storage/segment/inverted_index_writer_test.cpp: NormsFollowIndexNormsProperty covers the default and the property in both directions on an ordinary column, a variant subcolumn and a field_pattern index, and then the config dropping norms for variant path indexes (including one that sets "norms" = "true") while leaving an ordinary column index alone (InvertedIndexWriterTest.*: 18 passed) - regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy: the default and "norms" = "false" on field_pattern and whole-column VARIANT indexes, plus a setBeConfigTemporary case proving the config takes effect without a restart; the .out was regenerated because variant paths now keep norms, so BM25 length normalization changes those scores - test_bm25_score_variant and test_omit_norms rerun on the same build - Behavior changed: No (defaults unchanged; the variant skip is opt-in) - Does this need documentation: Yes (https://github.com/apache/doris-website/pull/4146) Co-Authored-By: Claude Opus 5 --- be/src/common/config.cpp | 4 + be/src/common/config.h | 4 + .../index/inverted/inverted_index_parser.cpp | 5 +- .../index/inverted/inverted_index_parser.h | 8 +- .../index/inverted/inverted_index_writer.cpp | 16 ++- .../segment/inverted_index_writer_test.cpp | 76 ++++++++--- .../analysis/InvertedIndexProperties.java | 6 +- .../test_variant_subcolumn_index_norms.out | 6 +- .../test_variant_subcolumn_index_norms.groovy | 123 +++++++++++++----- 9 files changed, 185 insertions(+), 63 deletions(-) diff --git a/be/src/common/config.cpp b/be/src/common/config.cpp index 96eb4362142884..b64d30e8a96013 100644 --- a/be/src/common/config.cpp +++ b/be/src/common/config.cpp @@ -1408,6 +1408,10 @@ DEFINE_mBool(debug_inverted_index_compaction, "false"); DEFINE_mBool(inverted_index_ram_dir_enable, "true"); // wheather index by RAM directory when base compaction DEFINE_mBool(inverted_index_ram_dir_enable_when_base_compaction, "true"); +// Norms cost one byte per segment row, including rows that hold no value for the field. A segment +// holds one index per variant path, so writing norms for them costs rows * paths bytes. Turn this on +// to leave norms out of indexes on a variant path, except those that set the "norms" property. +DEFINE_mBool(inverted_index_skip_norms_for_variant, "false"); // use num_broadcast_buffer blocks as buffer to do broadcast DEFINE_Int32(num_broadcast_buffer, "32"); diff --git a/be/src/common/config.h b/be/src/common/config.h index f5399093dd53d9..801f00acfb245f 100644 --- a/be/src/common/config.h +++ b/be/src/common/config.h @@ -1462,6 +1462,10 @@ DECLARE_mBool(debug_inverted_index_compaction); DECLARE_mBool(inverted_index_ram_dir_enable); // wheather index by RAM directory when base compaction DECLARE_mBool(inverted_index_ram_dir_enable_when_base_compaction); +// Norms cost one byte per segment row, including rows that hold no value for the field. A segment +// holds one index per variant path, so writing norms for them costs rows * paths bytes. Turn this on +// to leave norms out of indexes on a variant path, except those that set the "norms" property. +DECLARE_mBool(inverted_index_skip_norms_for_variant); // use num_broadcast_buffer blocks as buffer to do broadcast DECLARE_Int32(num_broadcast_buffer); diff --git a/be/src/storage/index/inverted/inverted_index_parser.cpp b/be/src/storage/index/inverted/inverted_index_parser.cpp index 331d256700b044..62628b80c2e406 100644 --- a/be/src/storage/index/inverted/inverted_index_parser.cpp +++ b/be/src/storage/index/inverted/inverted_index_parser.cpp @@ -117,12 +117,11 @@ std::string get_parser_phrase_support_string_from_properties( return INVERTED_INDEX_PARSER_PHRASE_SUPPORT_NO; } -bool get_index_norms_from_properties(const std::map& properties, - bool default_value) { +bool get_index_norms_from_properties(const std::map& properties) { if (auto it = properties.find(INVERTED_INDEX_NORMS_KEY); it != properties.end()) { return it->second == INVERTED_INDEX_PARSER_TRUE; } - return default_value; + return true; } CharFilterMap get_parser_char_filter_map_from_properties( diff --git a/be/src/storage/index/inverted/inverted_index_parser.h b/be/src/storage/index/inverted/inverted_index_parser.h index e955ed1eb447cb..44e97a1a764bf9 100644 --- a/be/src/storage/index/inverted/inverted_index_parser.h +++ b/be/src/storage/index/inverted/inverted_index_parser.h @@ -155,11 +155,9 @@ std::string get_parser_mode_string_from_properties( std::string get_parser_phrase_support_string_from_properties( const std::map& properties); -// Whether this index writes BM25 norms. Norms cost one byte per row of the segment, including rows -// that have no value for the field, so callers pass a default of false for indexes on variant paths, -// where one segment holds one index per path. "norms" = "true" / "false" overrides the default. -bool get_index_norms_from_properties(const std::map& properties, - bool default_value); +// Whether this index writes BM25 norms, which it does unless "norms" = "false" says otherwise. +// Norms cost one byte per row of the segment, including rows that have no value for the field. +bool get_index_norms_from_properties(const std::map& properties); CharFilterMap get_parser_char_filter_map_from_properties( const std::map& properties); diff --git a/be/src/storage/index/inverted/inverted_index_writer.cpp b/be/src/storage/index/inverted/inverted_index_writer.cpp index 87d2da44f44565..4306bf7550f224 100644 --- a/be/src/storage/index/inverted/inverted_index_writer.cpp +++ b/be/src/storage/index/inverted/inverted_index_writer.cpp @@ -162,14 +162,18 @@ Status InvertedIndexColumnWriter::create_field(lucene::document::Fie (*field)->setOmitTermFreqAndPositions( !(get_parser_phrase_support_string_from_properties(_index_meta->properties()) == INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES)); - // Norms cost one byte per segment row, including rows without a value. A variant path index - // (a field_pattern index, or the copy inherited by one extracted subcolumn, which carries the - // path as its index suffix) is one of possibly thousands in a segment, so its norms can dwarf - // the data: those default to no norms, and "norms" = "true" brings them back per index. + // An analyzed index writes norms unless its "norms" property says otherwise. Norms cost one byte + // per segment row, including rows without a value, and a variant path index (a field_pattern + // index, or the copy inherited by one extracted subcolumn, which carries the path as its index + // suffix) is one of possibly thousands in a segment, so their norms can dwarf the data. + // inverted_index_skip_norms_for_variant drops norms for those indexes whatever their property + // says, so that a cluster can reclaim that space without rewriting its index definitions. const bool variant_path_index = !_index_meta->get_index_suffix().empty() || !_index_meta->field_pattern().empty(); - if (_should_analyzer && - get_index_norms_from_properties(_index_meta->properties(), !variant_path_index)) { + const bool skipped_by_config = + variant_path_index && config::inverted_index_skip_norms_for_variant; + if (_should_analyzer && !skipped_by_config && + get_index_norms_from_properties(_index_meta->properties())) { (*field)->setOmitNorms(false); } diff --git a/be/test/storage/segment/inverted_index_writer_test.cpp b/be/test/storage/segment/inverted_index_writer_test.cpp index f3b48165968120..abcb235eeb2102 100644 --- a/be/test/storage/segment/inverted_index_writer_test.cpp +++ b/be/test/storage/segment/inverted_index_writer_test.cpp @@ -1860,29 +1860,71 @@ TEST_F(InvertedIndexWriterTest, NormsFollowIndexNormsProperty) { local_segment_path(kTestDir, rowset_id, seg_id))}; }; - create_tokenized_index("variant_subcolumn_default", 0, true, "v.s_host"); - TabletIndex subcolumn_default = make_index_meta("v.s_host", {}); - EXPECT_FALSE(check_norms_file_exists(path_prefix("variant_subcolumn_default", 0), - &subcolumn_default)) - << "a variant subcolumn index must not write .nrm by default"; + bool original_skip_norms_for_variant = config::inverted_index_skip_norms_for_variant; - create_tokenized_index("variant_subcolumn_norms_on", 1, true, "v.s_host", {{"norms", "true"}}); - TabletIndex subcolumn_norms_on = make_index_meta("v.s_host", {{"norms", "true"}}); - EXPECT_TRUE(check_norms_file_exists(path_prefix("variant_subcolumn_norms_on", 1), - &subcolumn_norms_on)) - << "norms = true must restore .nrm for a variant subcolumn index"; + // an analyzed index writes norms wherever it sits, and only "norms" = "false" drops them + config::inverted_index_skip_norms_for_variant = false; - create_tokenized_index("field_pattern_default", 2, true, "", {{"field_pattern", "s_*"}}); - TabletIndex field_pattern_default = make_index_meta("", {{"field_pattern", "s_*"}}); - EXPECT_FALSE(check_norms_file_exists(path_prefix("field_pattern_default", 2), - &field_pattern_default)) - << "a field_pattern index must not write .nrm by default"; + create_tokenized_index("plain_column_default", 0, true, ""); + TabletIndex plain_default = make_index_meta("", {}); + EXPECT_TRUE(check_norms_file_exists(path_prefix("plain_column_default", 0), &plain_default)) + << "an analyzed index must write .nrm by default"; - create_tokenized_index("plain_column_norms_off", 3, true, "", {{"norms", "false"}}); + create_tokenized_index("plain_column_norms_off", 1, true, "", {{"norms", "false"}}); TabletIndex plain_norms_off = make_index_meta("", {{"norms", "false"}}); EXPECT_FALSE( - check_norms_file_exists(path_prefix("plain_column_norms_off", 3), &plain_norms_off)) + check_norms_file_exists(path_prefix("plain_column_norms_off", 1), &plain_norms_off)) << "norms = false must drop .nrm for an ordinary column index"; + + create_tokenized_index("variant_subcolumn_default", 2, true, "v.s_host"); + TabletIndex subcolumn_default = make_index_meta("v.s_host", {}); + EXPECT_TRUE(check_norms_file_exists(path_prefix("variant_subcolumn_default", 2), + &subcolumn_default)) + << "a variant subcolumn index must write .nrm by default too"; + + create_tokenized_index("variant_subcolumn_norms_off", 3, true, "v.s_host", + {{"norms", "false"}}); + TabletIndex subcolumn_norms_off = make_index_meta("v.s_host", {{"norms", "false"}}); + EXPECT_FALSE(check_norms_file_exists(path_prefix("variant_subcolumn_norms_off", 3), + &subcolumn_norms_off)) + << "norms = false must drop .nrm for a variant subcolumn index"; + + create_tokenized_index("field_pattern_default", 4, true, "", {{"field_pattern", "s_*"}}); + TabletIndex field_pattern_default = make_index_meta("", {{"field_pattern", "s_*"}}); + EXPECT_TRUE(check_norms_file_exists(path_prefix("field_pattern_default", 4), + &field_pattern_default)) + << "a field_pattern index must write .nrm by default too"; + + // the config drops norms for a variant path index whatever its property says, and leaves every + // other index alone + config::inverted_index_skip_norms_for_variant = true; + + create_tokenized_index("variant_subcolumn_skipped", 5, true, "v.s_host"); + TabletIndex subcolumn_skipped = make_index_meta("v.s_host", {}); + EXPECT_FALSE(check_norms_file_exists(path_prefix("variant_subcolumn_skipped", 5), + &subcolumn_skipped)) + << "the config must drop .nrm for a variant subcolumn index"; + + create_tokenized_index("variant_subcolumn_norms_on_skipped", 6, true, "v.s_host", + {{"norms", "true"}}); + TabletIndex subcolumn_norms_on_skipped = make_index_meta("v.s_host", {{"norms", "true"}}); + EXPECT_FALSE(check_norms_file_exists(path_prefix("variant_subcolumn_norms_on_skipped", 6), + &subcolumn_norms_on_skipped)) + << "the config must win over norms = true on a variant subcolumn index"; + + create_tokenized_index("field_pattern_skipped", 7, true, "", {{"field_pattern", "s_*"}}); + TabletIndex field_pattern_skipped = make_index_meta("", {{"field_pattern", "s_*"}}); + EXPECT_FALSE(check_norms_file_exists(path_prefix("field_pattern_skipped", 7), + &field_pattern_skipped)) + << "the config must drop .nrm for a field_pattern index"; + + create_tokenized_index("plain_column_not_skipped", 8, true, ""); + TabletIndex plain_not_skipped = make_index_meta("", {}); + EXPECT_TRUE( + check_norms_file_exists(path_prefix("plain_column_not_skipped", 8), &plain_not_skipped)) + << "the config must leave an ordinary column index alone"; + + config::inverted_index_skip_norms_for_variant = original_skip_norms_for_variant; } } // namespace doris::segment_v2 diff --git a/fe/fe-catalog/src/main/java/org/apache/doris/analysis/InvertedIndexProperties.java b/fe/fe-catalog/src/main/java/org/apache/doris/analysis/InvertedIndexProperties.java index 6a3876360b289b..451e65d9c52b58 100644 --- a/fe/fe-catalog/src/main/java/org/apache/doris/analysis/InvertedIndexProperties.java +++ b/fe/fe-catalog/src/main/java/org/apache/doris/analysis/InvertedIndexProperties.java @@ -53,8 +53,10 @@ public class InvertedIndexProperties { public static String INVERTED_INDEX_SUPPORT_PHRASE_KEY = "support_phrase"; - // Whether an analyzed index stores BM25 norms. Indexes on variant paths default to false - // because norms cost one byte per row of the segment for every indexed path. + // Whether an analyzed index stores BM25 norms, which default to being stored. Norms cost one + // byte per row of the segment for every indexed path, so the BE config + // inverted_index_skip_norms_for_variant (off by default) can leave them out for every index on + // a variant path, which it does whatever this property says. public static String INVERTED_INDEX_NORMS_KEY = "norms"; public static String INVERTED_INDEX_PARSER_IGNORE_ABOVE_KEY = "ignore_above"; diff --git a/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out b/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out index 2988072d642e32..e1747721be6b9f 100644 --- a/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out +++ b/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out @@ -1,7 +1,11 @@ -- This file is automatically generated. You should know what you did if you want to edit this -- !variant_subcolumn_score -- 2 0.6931 -3 0.9531 +3 0.61 + +-- !variant_subcolumn_score_no_norms -- +2 0.6931 +3 0.6931 -- !plain_column_score -- 1 0.5754 diff --git a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy index 1d4854300f7dfd..2bda75974f2997 100644 --- a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy +++ b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy @@ -15,10 +15,12 @@ // specific language governing permissions and limitations // under the License. -// Analyzed indexes on variant subcolumns must not write dense BM25 norms (.nrm, one byte per row), -// while analyzed indexes on ordinary columns still do. BM25 scoring keeps working on both. -// This holds both for an index declared with a field_pattern and for a whole-column index on a -// VARIANT column, whose per-subcolumn copies inherit the properties of the index they come from. +// An analyzed index writes dense BM25 norms (.nrm, one byte per segment row), on a variant path as +// on any other column, and "norms" = "false" drops them. Norms on a variant path cost rows * paths +// bytes, so inverted_index_skip_norms_for_variant leaves them out there whatever the property says. +// This covers both an index declared with a field_pattern and a whole-column +// index on a VARIANT column, whose per-subcolumn copies inherit the properties of the index they +// come from. BM25 scoring keeps working with and without norms. suite("test_variant_subcolumn_index_norms", "p0") { if (isCloudMode()) { return @@ -60,7 +62,7 @@ suite("test_variant_subcolumn_index_norms", "p0") { "parser"="english", "support_phrase"="true", "field_pattern"="t_*", - "norms"="true" + "norms"="false" ), INDEX idx_vd (vd) USING INVERTED PROPERTIES( "parser"="english", @@ -69,7 +71,7 @@ suite("test_variant_subcolumn_index_norms", "p0") { INDEX idx_vn (vn) USING INVERTED PROPERTIES( "parser"="english", "support_phrase"="true", - "norms"="true" + "norms"="false" ) ) ENGINE=OLAP DUPLICATE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1 @@ -103,6 +105,13 @@ suite("test_variant_subcolumn_index_norms", "p0") { order by score() desc limit 10 """ + order_qt_variant_subcolumn_score_no_norms """ + select id, round(score(), 4) + from test_variant_subcolumn_index_norms + where cast(v["t_note"] as string) match_phrase "alpha" + order by score() desc + limit 10 + """ order_qt_plain_column_score """ select id, round(score(), 4) from test_variant_subcolumn_index_norms @@ -114,36 +123,92 @@ suite("test_variant_subcolumn_index_norms", "p0") { def backendIdToIp = [:] def backendIdToHttpPort = [:] getBackendIpHttpPort(backendIdToIp, backendIdToHttpPort) - def tablet = sql_return_maparray("show tablets from test_variant_subcolumn_index_norms")[0] - def (code, out, err) = http_client("GET", String.format( - "http://%s:%s/api/show_nested_index_file?tablet_id=%s", - backendIdToIp.get(tablet.BackendId), backendIdToHttpPort.get(tablet.BackendId), - tablet.TabletId)) - logger.info("show_nested_index_file code=${code}, out=${out}, err=${err}") - assertEquals(0, code) - - def normsBySuffix = [:] - for (def rowset in parseJson(out.trim()).rowsets) { - for (def segment in rowset.segments) { - for (def index in segment.indices) { - normsBySuffix[index.index_suffix] = index.files.any { file -> file.name.endsWith(".nrm") } + def normsBySuffixOf = { tableName -> + def tablet = sql_return_maparray("show tablets from ${tableName}")[0] + def (code, out, err) = http_client("GET", String.format( + "http://%s:%s/api/show_nested_index_file?tablet_id=%s", + backendIdToIp.get(tablet.BackendId), backendIdToHttpPort.get(tablet.BackendId), + tablet.TabletId)) + logger.info("show_nested_index_file of ${tableName} code=${code}, out=${out}, err=${err}") + assertEquals(0, code) + def norms = [:] + for (def rowset in parseJson(out.trim()).rowsets) { + for (def segment in rowset.segments) { + for (def index in segment.indices) { + norms[index.index_suffix] = index.files.any { file -> file.name.endsWith(".nrm") } + } } } + logger.info("norms by index suffix of ${tableName}: ${norms}") + return norms } - logger.info("norms by index suffix: ${normsBySuffix}") // the suffix is the escaped variant path, e.g. v%2Es%5Fhost for v.s_host - def normsOf = { path -> - normsBySuffix.find { suffix, hasNorms -> + def normsOf = { norms, path -> + norms.find { suffix, hasNorms -> suffix.replace("%2E", ".").replace("%5F", "_").contains(path) }?.value } - // idx_content on an ordinary column keeps norms, idx_v_s drops them, idx_v_t asks for them back + + def normsBySuffix = normsBySuffixOf("test_variant_subcolumn_index_norms") + // an analyzed index writes norms wherever it sits, and "norms" = "false" drops them assertEquals(true, normsBySuffix[""]) - assertEquals(false, normsOf("s_host")) - assertEquals(false, normsOf("s_note")) - assertEquals(true, normsOf("t_note")) + assertEquals(true, normsOf(normsBySuffix, "s_host")) + assertEquals(true, normsOf(normsBySuffix, "s_note")) + assertEquals(false, normsOf(normsBySuffix, "t_note")) // a whole-column index on a VARIANT column has no suffix of its own, but every subcolumn copy - // inherits its properties: idx_vd drops norms by default, idx_vn keeps them because it asks to - assertEquals(false, normsOf("a_host")) - assertEquals(true, normsOf("b_host")) + // inherits its properties: idx_vd keeps norms, idx_vn drops them because it asks to + assertEquals(true, normsOf(normsBySuffix, "a_host")) + assertEquals(false, normsOf(normsBySuffix, "b_host")) + + // the skip is a dynamic BE config: with it turned on, every index on a variant path leaves norms + // out, even one that asks for them, while an ordinary column index is untouched + setBeConfigTemporary([inverted_index_skip_norms_for_variant: true]) { + sql "DROP TABLE IF EXISTS test_variant_subcolumn_index_norms_config" + sql """ + CREATE TABLE test_variant_subcolumn_index_norms_config ( + id INT, + content TEXT, + v variant< + 's_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, + vf variant< + 'c_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, + INDEX idx_content (content) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true" + ), + INDEX idx_v_s (v) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "field_pattern"="s_*" + ), + INDEX idx_vf (vf) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "norms"="true" + ) + ) ENGINE=OLAP DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ( + "replication_allocation" = "tag.location.default: 1", + "disable_auto_compaction" = "true", + "inverted_index_storage_format" = "V2" + ) + """ + sql """ insert into test_variant_subcolumn_index_norms_config values + (1, 'alpha database server', parse_to_variant('{"s_host":"alpha database server"}'), + parse_to_variant('{"c_host":"alpha database server"}')), + (2, 'beta server cluster', parse_to_variant('{"s_host":"beta server cluster"}'), + parse_to_variant('{"c_host":"beta server cluster"}')) + """ + sql " sync " + + def configNorms = normsBySuffixOf("test_variant_subcolumn_index_norms_config") + assertEquals(false, normsOf(configNorms, "s_host")) + assertEquals(false, normsOf(configNorms, "c_host")) + assertEquals(true, configNorms[""]) + } } From c1a98a58e2a6d4eb8402b5259b1b952b23a1cd2f Mon Sep 17 00:00:00 2001 From: lihangyu Date: Wed, 16 Sep 2026 18:10:00 +0800 Subject: [PATCH 5/6] [fix](inverted index) Apply the norms policy to SNII and score indexes without norms ### What problem does this PR solve? Problem Summary: The "norms" index property and inverted_index_skip_norms_for_variant only reached the CLucene writer. SNII decided on its own: its writer set _writes_norms from the analyzer and the positions flag, and its direct compaction from the analyzer alone, so on a SNII table both knobs were silently ignored. SNII scoring also rejected any segment without norms, so it could not have honoured them anyway. - Move the rule into one function, should_write_index_norms(const TabletIndex&), and call it from the CLucene writer, the SNII writer and SNII direct compaction. SNII direct compaction rebuilds norms from the merged postings, so sources with and without norms merge correctly; the other SNII compaction path goes through the SNII writer. - Let SNII score a segment without norms. SNII keeps the token count in its stats block, so avgdl stays correct. SniiStatsProvider::encoded_norm now returns std::nullopt for such a segment, and ScorerContext::score scores that document as if its length were avgdl, the same length-neutral form the CLucene BM25 fix uses. A document with a norm goes through the same arithmetic as before. resolve_snii_scoring_segment no longer takes has_norms. - Mark both norms regression suites nonConcurrent, since they flip a BE config. ### Release note The "norms" inverted index property and the BE config inverted_index_skip_norms_for_variant now also apply to the SNII index storage format, and SNII scoring no longer fails on a segment written without norms. ### Check List (For Author) - Test: Unit Test / Regression test - SniiWriterNorms.WritesNormsFollowSharedNormsPolicy, SniiCompactionEligibilityTest.DestinationWritesNormsFollowSharedNormsPolicy, SniiScoringQuery.IndexWithoutNormsScoresWithoutLengthNormalization, CollectionStatisticsTest.SniiScoringAdmitsSegmentWithoutNorms - regression inverted_index_p0/storage_format/test_storage_format_snii_norms (new), plus test_variant_subcolumn_index_norms, test_bm25_score_variant, test_omit_norms and test_storage_format_snii - Behavior changed: Yes. A SNII segment without norms is now scored without length normalization instead of failing the query. - Does this need documentation: Yes (https://github.com/apache/doris-website/pull/4146) Co-Authored-By: Claude Opus 5 --- .../index/inverted/inverted_index_parser.cpp | 14 +- .../index/inverted/inverted_index_parser.h | 11 +- .../index/inverted/inverted_index_writer.cpp | 13 +- .../similarity/collection_statistics.cpp | 14 +- .../similarity/collection_statistics.h | 9 +- .../index/snii/compaction/eligibility.cpp | 8 +- .../storage/index/snii/query/bm25_scorer.cpp | 9 +- be/src/storage/index/snii/query/bm25_scorer.h | 11 +- .../index/snii/query/scoring_query.cpp | 5 +- .../storage/index/snii/snii_index_reader.cpp | 2 +- .../storage/index/snii/snii_index_writer.cpp | 10 +- be/src/storage/index/snii/snii_index_writer.h | 3 +- .../index/snii/stats/snii_stats_provider.cpp | 11 +- .../index/snii/stats/snii_stats_provider.h | 8 +- .../similarity/collection_statistics_test.cpp | 76 ++++---- .../snii_compaction_eligibility_test.cpp | 36 ++++ .../index/snii/query/scoring_query_test.cpp | 74 +++++++- be/test/storage/index/snii_writer_test.cpp | 44 +++++ .../test_storage_format_snii_norms.out | 29 ++++ .../test_storage_format_snii_norms.groovy | 163 ++++++++++++++++++ .../test_variant_subcolumn_index_norms.groovy | 3 +- 21 files changed, 462 insertions(+), 91 deletions(-) create mode 100644 regression-test/data/inverted_index_p0/storage_format/test_storage_format_snii_norms.out create mode 100644 regression-test/suites/inverted_index_p0/storage_format/test_storage_format_snii_norms.groovy diff --git a/be/src/storage/index/inverted/inverted_index_parser.cpp b/be/src/storage/index/inverted/inverted_index_parser.cpp index 62628b80c2e406..f92ae7eefc4a9c 100644 --- a/be/src/storage/index/inverted/inverted_index_parser.cpp +++ b/be/src/storage/index/inverted/inverted_index_parser.cpp @@ -17,6 +17,8 @@ #include "storage/index/inverted/inverted_index_parser.h" +#include "common/config.h" +#include "storage/tablet/tablet_schema.h" #include "util/string_util.h" namespace doris { @@ -117,7 +119,17 @@ std::string get_parser_phrase_support_string_from_properties( return INVERTED_INDEX_PARSER_PHRASE_SUPPORT_NO; } -bool get_index_norms_from_properties(const std::map& properties) { +bool should_write_index_norms(const TabletIndex& index_meta) { + // A variant path index (a field_pattern index, or the copy inherited by one extracted + // subcolumn, which carries the path as its index suffix) is one of possibly thousands in a + // segment, so its norms can dwarf the data. The config drops them whatever the property says, + // so that a cluster can reclaim that space without rewriting its index definitions. + const bool variant_path_index = + !index_meta.get_index_suffix().empty() || !index_meta.field_pattern().empty(); + if (variant_path_index && config::inverted_index_skip_norms_for_variant) { + return false; + } + const auto& properties = index_meta.properties(); if (auto it = properties.find(INVERTED_INDEX_NORMS_KEY); it != properties.end()) { return it->second == INVERTED_INDEX_PARSER_TRUE; } diff --git a/be/src/storage/index/inverted/inverted_index_parser.h b/be/src/storage/index/inverted/inverted_index_parser.h index 44e97a1a764bf9..329a22570e17df 100644 --- a/be/src/storage/index/inverted/inverted_index_parser.h +++ b/be/src/storage/index/inverted/inverted_index_parser.h @@ -34,6 +34,8 @@ class Analyzer; namespace doris { +class TabletIndex; + enum class InvertedIndexParserType { PARSER_UNKNOWN = 0, PARSER_NONE = 1, @@ -155,9 +157,12 @@ std::string get_parser_mode_string_from_properties( std::string get_parser_phrase_support_string_from_properties( const std::map& properties); -// Whether this index writes BM25 norms, which it does unless "norms" = "false" says otherwise. -// Norms cost one byte per row of the segment, including rows that have no value for the field. -bool get_index_norms_from_properties(const std::map& properties); +// Whether an analyzed index writes BM25 norms: the one policy shared by every index storage format +// and by index compaction. Norms cost one byte per row of the segment, including rows that have no +// value for the field. An index writes them unless its "norms" property is "false", or unless it +// is on a variant path while inverted_index_skip_norms_for_variant is on, which wins over the +// property. +bool should_write_index_norms(const TabletIndex& index_meta); CharFilterMap get_parser_char_filter_map_from_properties( const std::map& properties); diff --git a/be/src/storage/index/inverted/inverted_index_writer.cpp b/be/src/storage/index/inverted/inverted_index_writer.cpp index 4306bf7550f224..ed03225fae6f13 100644 --- a/be/src/storage/index/inverted/inverted_index_writer.cpp +++ b/be/src/storage/index/inverted/inverted_index_writer.cpp @@ -162,18 +162,7 @@ Status InvertedIndexColumnWriter::create_field(lucene::document::Fie (*field)->setOmitTermFreqAndPositions( !(get_parser_phrase_support_string_from_properties(_index_meta->properties()) == INVERTED_INDEX_PARSER_PHRASE_SUPPORT_YES)); - // An analyzed index writes norms unless its "norms" property says otherwise. Norms cost one byte - // per segment row, including rows without a value, and a variant path index (a field_pattern - // index, or the copy inherited by one extracted subcolumn, which carries the path as its index - // suffix) is one of possibly thousands in a segment, so their norms can dwarf the data. - // inverted_index_skip_norms_for_variant drops norms for those indexes whatever their property - // says, so that a cluster can reclaim that space without rewriting its index definitions. - const bool variant_path_index = - !_index_meta->get_index_suffix().empty() || !_index_meta->field_pattern().empty(); - const bool skipped_by_config = - variant_path_index && config::inverted_index_skip_norms_for_variant; - if (_should_analyzer && !skipped_by_config && - get_index_norms_from_properties(_index_meta->properties())) { + if (_should_analyzer && should_write_index_norms(*_index_meta)) { (*field)->setOmitNorms(false); } diff --git a/be/src/storage/index/inverted/similarity/collection_statistics.cpp b/be/src/storage/index/inverted/similarity/collection_statistics.cpp index 69bd4155389fed..41ea7394abf32d 100644 --- a/be/src/storage/index/inverted/similarity/collection_statistics.cpp +++ b/be/src/storage/index/inverted/similarity/collection_statistics.cpp @@ -43,11 +43,10 @@ namespace collection_statistics_detail { Result resolve_snii_scoring_segment(uint64_t index_doc_count, uint64_t sum_total_term_freq, - bool has_positions, bool has_norms) { - if (!has_positions || !has_norms) { + bool has_positions) { + if (!has_positions) { return ResultError(Status::Error( - "SNII scoring requires positions and norms; this segment was written without " - "norms -- rebuild the index or wait for compaction to rewrite it")); + "SNII scoring requires positions; this index was written without them")); } return SniiScoringSegmentStats {.doc_count = index_doc_count, .token_count = sum_total_term_freq}; @@ -246,8 +245,7 @@ Status CollectionStatistics::process_segment(const RowsetSharedPtr& rowset, const uint64_t segment_doc_count = logical_reader->stats().doc_count; RETURN_IF_ERROR(admit_snii_scoring_segment( ws_field_name, segment_doc_count, logical_reader->stats().sum_total_term_freq, - logical_reader->has_positions(), logical_reader->has_norms(), - &segment_accumulator)); + logical_reader->has_positions(), &segment_accumulator)); ::doris::snii::reader::DictBlockCache dict_block_cache; for (const auto& logical_term_bytes : collect_info.unique_terms) { @@ -335,10 +333,10 @@ Status CollectionStatistics::process_segment(const RowsetSharedPtr& rowset, Status CollectionStatistics::admit_snii_scoring_segment( const std::wstring& field_name, uint64_t index_doc_count, uint64_t sum_total_term_freq, - bool has_positions, bool has_norms, SniiScoringSegmentAccumulator* segment_accumulator) { + bool has_positions, SniiScoringSegmentAccumulator* segment_accumulator) { DORIS_CHECK(segment_accumulator != nullptr); auto segment_stats = collection_statistics_detail::resolve_snii_scoring_segment( - index_doc_count, sum_total_term_freq, has_positions, has_norms); + index_doc_count, sum_total_term_freq, has_positions); if (!segment_stats.has_value()) { clear(); return segment_stats.error(); diff --git a/be/src/storage/index/inverted/similarity/collection_statistics.h b/be/src/storage/index/inverted/similarity/collection_statistics.h index 7e93498508324a..b2221799c95916 100644 --- a/be/src/storage/index/inverted/similarity/collection_statistics.h +++ b/be/src/storage/index/inverted/similarity/collection_statistics.h @@ -85,7 +85,6 @@ class CollectionStatistics { io::IOContext* io_ctx); Status admit_snii_scoring_segment(const std::wstring& field_name, uint64_t index_doc_count, uint64_t sum_total_term_freq, bool has_positions, - bool has_norms, SniiScoringSegmentAccumulator* segment_accumulator); void commit_snii_scoring_segment(SniiScoringSegmentAccumulator&& segment_accumulator); void clear(); @@ -119,12 +118,12 @@ struct SniiScoringSegmentStats { uint64_t token_count = 0; }; -// SNII scoring requires positions (which provide term frequencies) and norms. The current writer -// emits norms for every analyzed index with positions. Older segments without norms return -// NOT_SUPPORTED until an index rebuild or compaction supplies them. +// SNII scoring requires positions, which provide term frequencies. A segment written without norms +// is still admitted: its token count comes from the stats block, and its documents are scored +// without length normalization. Result resolve_snii_scoring_segment(uint64_t index_doc_count, uint64_t sum_total_term_freq, - bool has_positions, bool has_norms); + bool has_positions); void add_term_doc_frequency( std::unordered_map>* diff --git a/be/src/storage/index/snii/compaction/eligibility.cpp b/be/src/storage/index/snii/compaction/eligibility.cpp index 7ce91bd3ab434d..9f958d9ebd598b 100644 --- a/be/src/storage/index/snii/compaction/eligibility.cpp +++ b/be/src/storage/index/snii/compaction/eligibility.cpp @@ -27,6 +27,7 @@ #include "common/config.h" #include "common/exception.h" #include "storage/index/inverted/analyzer/analyzer.h" +#include "storage/index/inverted/inverted_index_parser.h" #include "storage/index/snii/format/format_constants.h" #include "storage/index/snii/format/phrase_bigram.h" #include "storage/index/snii/reader/logical_index_reader.h" @@ -244,8 +245,11 @@ Status validate_snii_compaction_eligibility( source_ordinal)); } RETURN_IF_ERROR(validate_destination_policy(destination_index, analyzer_provider_factory)); - out->destination_writes_norms = - inverted_index::InvertedIndexAnalyzer::should_analyzer(destination_index.properties()); + // Merged norms are rebuilt from the postings, so the destination follows the same norms + // policy as a fresh write whether or not the sources carry norms. + out->destination_writes_norms = inverted_index::InvertedIndexAnalyzer::should_analyzer( + destination_index.properties()) && + should_write_index_norms(destination_index); return Status::OK(); } diff --git a/be/src/storage/index/snii/query/bm25_scorer.cpp b/be/src/storage/index/snii/query/bm25_scorer.cpp index 9cd8326deaba61..dbc74c10055c36 100644 --- a/be/src/storage/index/snii/query/bm25_scorer.cpp +++ b/be/src/storage/index/snii/query/bm25_scorer.cpp @@ -47,10 +47,13 @@ ScorerContext ScorerContext::from_idf(double idf) { return ctx; } -double ScorerContext::score(double tf, uint8_t encoded_norm, double avgdl, +double ScorerContext::score(double tf, std::optional encoded_norm, double avgdl, const Bm25Params& params) const { - const double dl = decode_norm(encoded_norm); - const double denom = tf + params.k1 * (1.0 - params.b + params.b * dl / avgdl); + // Without a norm the document length is unknown, so the document is scored as if it had the + // average length: b * dl / avgdl becomes b, and 1 - b + b leaves no length factor at all. + const double length_term = + encoded_norm.has_value() ? params.b * decode_norm(*encoded_norm) / avgdl : params.b; + const double denom = tf + params.k1 * (1.0 - params.b + length_term); return idf_ * (tf * (params.k1 + 1.0)) / denom; } diff --git a/be/src/storage/index/snii/query/bm25_scorer.h b/be/src/storage/index/snii/query/bm25_scorer.h index 51b62327a0fe1e..0c1e9c62b76c8c 100644 --- a/be/src/storage/index/snii/query/bm25_scorer.h +++ b/be/src/storage/index/snii/query/bm25_scorer.h @@ -18,6 +18,7 @@ #pragma once #include +#include // Bm25Scorer -- classic Okapi BM25 relevance scoring over SNII native stats. // @@ -27,7 +28,9 @@ // per-document contribution of a term then is: // score = idf * (tf * (k1 + 1)) / (tf + k1 * (1 - b + b * dl / avgdl)) // where tf is the in-doc term frequency, dl the document length decoded from the -// 1-byte encoded norm, and avgdl the average document length. +// 1-byte encoded norm, and avgdl the average document length. An index written +// without norms has no dl: its documents are scored as if dl were avgdl, which +// drops length normalization from the formula. // // Norm encode/decode (DOCUMENTED CONTRACT): the writer stores doc length as a // byte-quantized value floor-clamped to [1, 255]; decode is the identity map @@ -67,8 +70,10 @@ class ScorerContext { uint64_t df() const { return df_; } // Scores one document occurrence: tf is the in-doc term frequency, encoded_norm - // the doc's 1-byte length norm, avgdl the collection average length. - double score(double tf, uint8_t encoded_norm, double avgdl, const Bm25Params& params) const; + // the doc's 1-byte length norm (std::nullopt when the index stores no norms), + // avgdl the collection average length. + double score(double tf, std::optional encoded_norm, double avgdl, + const Bm25Params& params) const; private: double idf_ = 0.0; diff --git a/be/src/storage/index/snii/query/scoring_query.cpp b/be/src/storage/index/snii/query/scoring_query.cpp index cd63062cc653c4..9018edb18a24e1 100644 --- a/be/src/storage/index/snii/query/scoring_query.cpp +++ b/be/src/storage/index/snii/query/scoring_query.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -157,7 +158,7 @@ Status score_decoded(const stats::SniiStatsProvider& stats, const ScorerContext& DCHECK_EQ(docids.size(), tfs.size()); out->reserve(docids.size()); for (size_t i = 0; i < docids.size(); ++i) { - uint8_t norm = 0; + std::optional norm; RETURN_IF_ERROR(stats.encoded_norm(docids[i], &norm)); out->push_back({docids[i], ctx.score(tfs[i], norm, avgdl, params)}); } @@ -184,7 +185,7 @@ Status accumulate_decoded_candidate_scores(const stats::SniiStatsProvider& stats ++candidate_index; continue; } - uint8_t norm = 0; + std::optional norm; RETURN_IF_ERROR(stats.encoded_norm(docids[doc_index], &norm)); scores[candidate_index] += scorer.score(tfs[doc_index], norm, avgdl, params); ++doc_index; diff --git a/be/src/storage/index/snii/snii_index_reader.cpp b/be/src/storage/index/snii/snii_index_reader.cpp index bbcb78f7f8449a..80b2e2faa0b433 100644 --- a/be/src/storage/index/snii/snii_index_reader.cpp +++ b/be/src/storage/index/snii/snii_index_reader.cpp @@ -224,7 +224,7 @@ Status score_phrase_matches(const IndexQueryContextPtr& context, std::string_vie for (const auto& match : matches) { DCHECK(final_candidates.contains(match.docid)); DCHECK_NE(match.frequency, 0); - uint8_t norm = 0; + std::optional norm; RETURN_IF_ERROR(segment_stats.encoded_norm(match.docid, &norm)); scored_docs.push_back({.docid = match.docid, .score = scorer.score(match.frequency, norm, collection_avgdl, diff --git a/be/src/storage/index/snii/snii_index_writer.cpp b/be/src/storage/index/snii/snii_index_writer.cpp index 94ec6610c0bd42..5d6d3094266fa6 100644 --- a/be/src/storage/index/snii/snii_index_writer.cpp +++ b/be/src/storage/index/snii/snii_index_writer.cpp @@ -29,6 +29,7 @@ #include "common/logging.h" #include "storage/index/index_file_writer.h" #include "storage/index/inverted/analyzer/analyzer.h" +#include "storage/index/inverted/inverted_index_parser.h" #include "storage/index/inverted/query/query_info.h" #include "storage/index/snii/query/bm25_scorer.h" #include "storage/index/snii/writer/global_memory_limiter.h" @@ -115,10 +116,11 @@ Status SniiIndexColumnWriter::init() { return Status::Error( "SNII create analyzer failed: {}", e.what()); } - // A2: Analyzed indexes with positions always write norms (tokens per document, clamped to - // 1..255), matching CLucene's scoring capabilities. Keyword or positionless indexes omit - // them. Norms are an optional core-metadata region ignored by older readers. - _writes_norms = _should_analyzer && _has_positions; + // A2: Analyzed indexes with positions write norms (tokens per document, clamped to 1..255), + // matching CLucene's scoring capabilities, unless the shared norms policy turns them off. + // Keyword or positionless indexes omit them. Norms are an optional core-metadata region + // ignored by older readers. + _writes_norms = _should_analyzer && _has_positions && should_write_index_norms(*_index_meta); return Status::OK(); } diff --git a/be/src/storage/index/snii/snii_index_writer.h b/be/src/storage/index/snii/snii_index_writer.h index b17624dbfbfb7c..ed54a4657bb4d2 100644 --- a/be/src/storage/index/snii/snii_index_writer.h +++ b/be/src/storage/index/snii/snii_index_writer.h @@ -93,7 +93,8 @@ class SniiIndexColumnWriter final : public IndexColumnWriter { bool _should_analyzer = false; bool _has_positions = false; const bool _is_char; - // A2: Analyzed indexes with positions always write BM25 norms, matching CLucene. + // A2: Analyzed indexes with positions write BM25 norms, matching CLucene, unless the shared + // norms policy (should_write_index_norms) turns them off. bool _writes_norms = false; // Latch: set_direct_load() ran. The first call wins; a repeat or late call // is ignored (and logged) so one index keeps one stable compression-tier diff --git a/be/src/storage/index/snii/stats/snii_stats_provider.cpp b/be/src/storage/index/snii/stats/snii_stats_provider.cpp index 557c9c30417da7..90023cef5f0a46 100644 --- a/be/src/storage/index/snii/stats/snii_stats_provider.cpp +++ b/be/src/storage/index/snii/stats/snii_stats_provider.cpp @@ -86,14 +86,17 @@ Status SniiStatsProvider::doc_freq(std::string_view term, uint64_t* df) const { return Status::OK(); } -Status SniiStatsProvider::encoded_norm(uint32_t docid, uint8_t* out) const { +Status SniiStatsProvider::encoded_norm(uint32_t docid, std::optional* out) const { if (out == nullptr) return Status::Error("stats_provider: null out"); if (!has_norms_) { - return Status::Error( - "stats_provider: index has no norms"); + *out = std::nullopt; + return Status::OK(); } - return norms_reader_.try_encoded_norm(docid, out); + uint8_t norm = 0; + RETURN_IF_ERROR(norms_reader_.try_encoded_norm(docid, &norm)); + *out = norm; + return Status::OK(); } } // namespace doris::snii::stats diff --git a/be/src/storage/index/snii/stats/snii_stats_provider.h b/be/src/storage/index/snii/stats/snii_stats_provider.h index 28ef31c597c582..81f39acb891383 100644 --- a/be/src/storage/index/snii/stats/snii_stats_provider.h +++ b/be/src/storage/index/snii/stats/snii_stats_provider.h @@ -18,6 +18,7 @@ #pragma once #include +#include #include #include "common/status.h" @@ -59,9 +60,10 @@ class SniiStatsProvider { // Per-term document frequency. Absent term -> *df = 0 (OK status). Status doc_freq(std::string_view term, uint64_t* df) const; - // 1-byte encoded doc-length norm for docid (raw byte from the norms POD). - // Out-of-range docid -> InvalidArgument; index without norms -> InvalidArgument. - Status encoded_norm(uint32_t docid, uint8_t* out) const; + // 1-byte encoded doc-length norm for docid (raw byte from the norms POD), or + // std::nullopt when the index was written without norms. Out-of-range docid on + // an index with norms -> InvalidArgument. + Status encoded_norm(uint32_t docid, std::optional* out) const; bool has_norms() const { return has_norms_; } diff --git a/be/test/storage/index/inverted/similarity/collection_statistics_test.cpp b/be/test/storage/index/inverted/similarity/collection_statistics_test.cpp index 21b62a02f1898f..16ee72a24adc63 100644 --- a/be/test/storage/index/inverted/similarity/collection_statistics_test.cpp +++ b/be/test/storage/index/inverted/similarity/collection_statistics_test.cpp @@ -453,8 +453,9 @@ class CollectionStatisticsTest : public ::testing::Test { return file_writer.finish_close(); } - // A normal analyzed SNII segment with positions and norms, as emitted for scoring indexes. - Status write_snii_scoring_segment(const std::string& segment_path) { + // A normal analyzed SNII segment with positions, and with norms unless the index was written + // with norms turned off. + Status write_snii_scoring_segment(const std::string& segment_path, bool with_norms = true) { const std::string index_path_prefix { segment_v2::InvertedIndexDescriptor::get_index_file_path_prefix(segment_path)}; io::FileWriterPtr file_writer; @@ -483,7 +484,9 @@ class CollectionStatisticsTest : public ::testing::Test { input.index_id = 1; input.config = snii::format::IndexConfig::kDocsPositions; input.doc_count = 2; - input.encoded_norms = {snii::query::encode_norm(2), snii::query::encode_norm(1)}; + if (with_norms) { + input.encoded_norms = {snii::query::encode_norm(2), snii::query::encode_norm(1)}; + } input.terms = {std::move(alpha), std::move(beta)}; RETURN_IF_ERROR(writer.add_logical_index(input)); @@ -628,17 +631,15 @@ class CollectionStatisticsTest : public ::testing::Test { struct SniiScoringFieldInput { SniiScoringFieldInput(std::wstring field_name, uint64_t index_doc_count, - uint64_t sum_total_term_freq, bool has_norms = true) + uint64_t sum_total_term_freq) : field_name(std::move(field_name)), index_doc_count(index_doc_count), - sum_total_term_freq(sum_total_term_freq), - has_norms(has_norms) {} + sum_total_term_freq(sum_total_term_freq) {} std::wstring field_name; uint64_t index_doc_count = 0; uint64_t sum_total_term_freq = 0; bool has_positions = true; - bool has_norms = true; }; Status stage_snii_fields_for_test( @@ -647,7 +648,7 @@ class CollectionStatisticsTest : public ::testing::Test { for (const auto& field : fields) { RETURN_IF_ERROR(statistics->admit_snii_scoring_segment( field.field_name, field.index_doc_count, field.sum_total_term_freq, - field.has_positions, field.has_norms, segment_accumulator)); + field.has_positions, segment_accumulator)); } return Status::OK(); } @@ -662,9 +663,9 @@ class CollectionStatisticsTest : public ::testing::Test { Status admit_snii_segment_for_test(CollectionStatistics* statistics, const std::wstring& field_name, uint64_t index_doc_count, - uint64_t sum_total_term_freq, bool has_norms = true) { - return admit_snii_fields_for_test( - statistics, {{field_name, index_doc_count, sum_total_term_freq, has_norms}}); + uint64_t sum_total_term_freq) { + return admit_snii_fields_for_test(statistics, + {{field_name, index_doc_count, sum_total_term_freq}}); } Status stage_snii_fields_then_file_not_found_for_test( @@ -933,6 +934,32 @@ TEST_F(CollectionStatisticsTest, SniiScoringUsesPhysicalStatistics) { expect_collected_term(L"1", L"alpha", 2); } +// A segment written with norms turned off is admitted like any other: its token count comes from +// the stats block, so avgdl stays the physical average. +TEST_F(CollectionStatisticsTest, SniiScoringAdmitsSegmentWithoutNorms) { + auto tablet_schema = create_snii_schema(); + auto expr_contexts = create_match_expr_contexts("alpha"); + + const std::string segment_path = test_dir_ + "/snii_scoring_without_norms_0.dat"; + auto write_status = write_snii_scoring_segment(segment_path, /*with_norms=*/false); + ASSERT_TRUE(write_status.ok()) << write_status; + + auto rowset_meta = std::make_shared(); + auto rowset = std::make_shared(tablet_schema, rowset_meta); + rowset->set_num_segments(1); + rowset->set_segment_path(0, segment_path); + auto reader = std::make_shared(rowset); + std::vector splits {RowSetSplits(reader)}; + + auto status = + stats_->collect(runtime_state_.get(), splits, tablet_schema, expr_contexts, nullptr); + + ASSERT_TRUE(status.ok()) << status; + expect_collected_stats(L"1", 2, 3); + expect_collected_term(L"1", L"alpha", 2); + EXPECT_FLOAT_EQ(stats_->get_or_calculate_avg_dl(L"1"), 1.5F); +} + TEST_F(CollectionStatisticsTest, SniiScoringLookupUsesCallerIoContext) { snii::snii_test::ScopedEnv force_nonresident_dict("SNII_DICT_RESIDENT_MAX", "0"); auto tablet_schema = create_snii_schema(); @@ -1090,9 +1117,10 @@ class CollectionStatisticsDetailedTest : public ::testing::Test { std::unique_ptr stats_; }; -// SNII scoring requires only positions and norms; statistics come directly from the stats block. +// SNII scoring requires only positions; statistics come directly from the stats block, whether or +// not the segment carries norms. TEST(CollectionStatisticsSniiScoringTest, ResolveUsesPhysicalDocAndTokenCounts) { - auto result = resolve_snii_scoring_segment(3, 7, /*has_positions=*/true, /*has_norms=*/true); + auto result = resolve_snii_scoring_segment(3, 7, /*has_positions=*/true); ASSERT_TRUE(result.has_value()) << result.error(); EXPECT_EQ(result->doc_count, 3U); @@ -1100,22 +1128,15 @@ TEST(CollectionStatisticsSniiScoringTest, ResolveUsesPhysicalDocAndTokenCounts) } TEST(CollectionStatisticsSniiScoringTest, ResolveAcceptsEmptySegment) { - auto result = resolve_snii_scoring_segment(0, 0, /*has_positions=*/true, /*has_norms=*/true); + auto result = resolve_snii_scoring_segment(0, 0, /*has_positions=*/true); ASSERT_TRUE(result.has_value()) << result.error(); EXPECT_EQ(result->doc_count, 0U); EXPECT_EQ(result->token_count, 0U); } -TEST(CollectionStatisticsSniiScoringTest, ResolveRejectsSegmentWithoutNorms) { - auto result = resolve_snii_scoring_segment(3, 7, /*has_positions=*/true, /*has_norms=*/false); - - ASSERT_FALSE(result.has_value()); - EXPECT_EQ(result.error().code(), ErrorCode::INVERTED_INDEX_NOT_SUPPORTED); -} - TEST(CollectionStatisticsSniiScoringTest, ResolveRejectsSegmentWithoutPositions) { - auto result = resolve_snii_scoring_segment(3, 7, /*has_positions=*/false, /*has_norms=*/true); + auto result = resolve_snii_scoring_segment(3, 7, /*has_positions=*/false); ASSERT_FALSE(result.has_value()); EXPECT_EQ(result.error().code(), ErrorCode::INVERTED_INDEX_NOT_SUPPORTED); @@ -1132,17 +1153,6 @@ TEST_F(CollectionStatisticsTest, CollectionStatisticsInstancesKeepAdmissionState EXPECT_FLOAT_EQ(second.get_or_calculate_avg_dl(L"1"), 5.0F); } -// An older segment without norms disables scoring for the whole collection and clears its stats. -TEST_F(CollectionStatisticsTest, SegmentWithoutNormsRejectsWholeCollection) { - ASSERT_TRUE(admit_snii_segment_for_test(stats_.get(), L"1", 2, 6).ok()); - - auto status = admit_snii_segment_for_test(stats_.get(), L"1", 3, 7, /*has_norms=*/false); - - EXPECT_EQ(status.code(), ErrorCode::INVERTED_INDEX_NOT_SUPPORTED); - expect_no_collected_tokens(L"1"); - EXPECT_THROW(stats_->get_doc_num(), Exception); -} - TEST_F(CollectionStatisticsTest, SegmentsAccumulatePhysicalStatistics) { ASSERT_TRUE(admit_snii_segment_for_test(stats_.get(), L"1", 2, 6).ok()); ASSERT_TRUE(admit_snii_segment_for_test(stats_.get(), L"1", 3, 9).ok()); diff --git a/be/test/storage/index/snii/compaction/snii_compaction_eligibility_test.cpp b/be/test/storage/index/snii/compaction/snii_compaction_eligibility_test.cpp index bbf5c0493d308a..ab912a45557355 100644 --- a/be/test/storage/index/snii/compaction/snii_compaction_eligibility_test.cpp +++ b/be/test/storage/index/snii/compaction/snii_compaction_eligibility_test.cpp @@ -248,6 +248,42 @@ TEST(SniiCompactionEligibilityTest, DestinationWritesNormsExactlyWhenAnalyzed) { EXPECT_FALSE(keyword.destination_writes_norms); } +// The destination follows the norms policy shared with fresh writes: the "norms" property, and on +// a variant path inverted_index_skip_norms_for_variant, which wins over the property. +TEST(SniiCompactionEligibilityTest, DestinationWritesNormsFollowSharedNormsPolicy) { + const bool original_skip_norms_for_variant = + doris::config::inverted_index_skip_norms_for_variant; + auto legacy = open_index({}); + auto writes_norms = [&legacy](const std::map& properties, + const std::string& index_suffix) { + auto source_meta = make_index(properties, doris::IndexType::INVERTED, 7, index_suffix); + auto destination = make_index(properties, doris::IndexType::INVERTED, 7, index_suffix); + std::vector sources {source(*legacy, *source_meta)}; + compaction::SniiCompactionEligibility eligibility; + const Status status = compaction::validate_snii_compaction_eligibility( + sources, *destination, &eligibility); + EXPECT_TRUE(status.ok()) << status.to_string(); + return eligibility.destination_writes_norms; + }; + auto norms_off = plain_properties(); + norms_off["norms"] = "false"; + auto norms_on = plain_properties(); + norms_on["norms"] = "true"; + + doris::config::inverted_index_skip_norms_for_variant = false; + EXPECT_TRUE(writes_norms(plain_properties(), "")); + EXPECT_FALSE(writes_norms(norms_off, "")); + EXPECT_TRUE(writes_norms(plain_properties(), "v.s_host")); + EXPECT_FALSE(writes_norms(norms_off, "v.s_host")); + + doris::config::inverted_index_skip_norms_for_variant = true; + EXPECT_TRUE(writes_norms(plain_properties(), "")); + EXPECT_FALSE(writes_norms(plain_properties(), "v.s_host")); + EXPECT_FALSE(writes_norms(norms_on, "v.s_host")); + + doris::config::inverted_index_skip_norms_for_variant = original_skip_norms_for_variant; +} + TEST(SniiCompactionEligibilityTest, RejectsLegacyBigramMarkerBeforeMergeExecution) { snii_test::MemoryFile file; writer::SniiIndexInput input; diff --git a/be/test/storage/index/snii/query/scoring_query_test.cpp b/be/test/storage/index/snii/query/scoring_query_test.cpp index 96c3bc9070c267..b7fc9b3ba322ae 100644 --- a/be/test/storage/index/snii/query/scoring_query_test.cpp +++ b/be/test/storage/index/snii/query/scoring_query_test.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -288,7 +289,7 @@ TEST(SniiScoringQuery, ReferenceOracleEqualsExhaustive) { EXPECT_EQ(df, plist.size()) << term; } for (uint32_t d = 0; d < corpus.doc_count; ++d) { - uint8_t got = 0; + std::optional got; ASSERT_TRUE(stats.encoded_norm(d, &got).ok()); EXPECT_EQ(got, norms[d]) << "docid " << d; } @@ -351,10 +352,11 @@ TEST(SniiScoringQuery, StatsProviderSharesValidatedNormsAcrossQueries) { EXPECT_EQ(metered_reader.metrics().total_request_bytes, after_first.total_request_bytes); EXPECT_EQ(logical_reader.memory_usage(), memory_usage_before_load); - uint8_t first_norm = 0; - uint8_t second_norm = 0; + std::optional first_norm; + std::optional second_norm; ASSERT_TRUE(first.encoded_norm(17, &first_norm).ok()); ASSERT_TRUE(second.encoded_norm(17, &second_norm).ok()); + EXPECT_TRUE(first_norm.has_value()); EXPECT_EQ(first_norm, second_norm); std::remove(path.c_str()); @@ -382,7 +384,7 @@ TEST(SniiScoringQuery, StatsProviderSharesOneConcurrentNormsLoad) { constexpr size_t kThreadCount = 16; std::barrier start(static_cast(kThreadCount + 1)); std::vector statuses(kThreadCount); - std::vector norms(kThreadCount); + std::vector> norms(kThreadCount); std::vector threads; threads.reserve(kThreadCount); controlled_reader.reset_read_at_calls(); @@ -441,13 +443,75 @@ TEST(SniiScoringQuery, StatsProviderRetriesTransientNormsReadFailure) { SniiStatsProvider retry; ASSERT_TRUE(SniiStatsProvider::open(&logical_reader, &retry).ok()); - uint8_t norm = 0; + std::optional norm; ASSERT_TRUE(retry.encoded_norm(17, &norm).ok()); + EXPECT_TRUE(norm.has_value()); EXPECT_EQ(controlled_reader.read_at_calls(), 2U); std::remove(path.c_str()); } +// An index written without norms scores every document as if it had the average length, so the +// ranking depends on term frequency and IDF only, and no norms read is attempted. +TEST(SniiScoringQuery, IndexWithoutNormsScoresWithoutLengthNormalization) { + const Corpus corpus = MakeCorpus(); + const std::string path = TempPath(); + { + SniiIndexInput input = ToInput(corpus); + input.encoded_norms.clear(); + io::LocalFileWriter writer; + ASSERT_TRUE(writer.open(path).ok()); + SniiCompoundWriter compound_writer(&writer); + ASSERT_TRUE(compound_writer.add_logical_index(std::move(input)).ok()); + ASSERT_TRUE(compound_writer.finish().ok()); + } + + io::LocalFileReader file_reader; + ASSERT_TRUE(file_reader.open(path).ok()); + reader::SniiSegmentReader segment_reader; + ASSERT_TRUE(reader::SniiSegmentReader::open(&file_reader, &segment_reader).ok()); + reader::LogicalIndexReader logical_reader; + ASSERT_TRUE(segment_reader.open_index(1, "body", &logical_reader).ok()); + ASSERT_FALSE(logical_reader.has_norms()); + + SniiStatsProvider stats; + ASSERT_TRUE(SniiStatsProvider::open(&logical_reader, &stats).ok()); + EXPECT_FALSE(stats.has_norms()); + std::optional norm = 1; + ASSERT_TRUE(stats.encoded_norm(17, &norm).ok()); + EXPECT_FALSE(norm.has_value()); + + const Bm25Params params; + const uint32_t k = 10; + const auto& postings = corpus.postings.at("common"); + const double df = static_cast(postings.size()); + const double idf = std::log(1.0 + (corpus.doc_count - df + 0.5) / (df + 0.5)); + std::vector expected; + for (const auto& [docid, freq] : postings) { + expected.push_back({docid, idf * (freq * (params.k1 + 1.0)) / (freq + params.k1)}); + } + std::ranges::sort(expected, [](const ScoredDoc& a, const ScoredDoc& b) { + if (a.score != b.score) { + return a.score > b.score; + } + return a.docid < b.docid; + }); + expected.resize(k); + + const std::vector terms {"common"}; + std::vector scored; + ASSERT_TRUE(doris::snii::query::scoring_query_exhaustive(logical_reader, stats, terms, k, + params, &scored) + .ok()); + ASSERT_EQ(scored.size(), expected.size()); + for (size_t i = 0; i < expected.size(); ++i) { + EXPECT_EQ(scored[i].docid, expected[i].docid) << "rank " << i; + EXPECT_NEAR(scored[i].score, expected[i].score, 1e-6) << "rank " << i; + } + + std::remove(path.c_str()); +} + TEST(SniiScoringQuery, CandidatesUseCollectionStatisticsAndPreserveDuplicateClauses) { const Corpus corpus = MakeCorpus(); const std::vector norms = EncodeNorms(corpus); diff --git a/be/test/storage/index/snii_writer_test.cpp b/be/test/storage/index/snii_writer_test.cpp index 2451c8aa578ff6..bae359c434d881 100644 --- a/be/test/storage/index/snii_writer_test.cpp +++ b/be/test/storage/index/snii_writer_test.cpp @@ -333,6 +333,50 @@ TEST(SniiWriterFailureLatch, AnalyzerFailureDiscardsStateAndBlocksFinish) { EXPECT_EQ(writer.memory_reporter_for_test(), nullptr); } +// The writer follows the norms policy it shares with the CLucene writer and SNII compaction: the +// "norms" property, and on a variant path inverted_index_skip_norms_for_variant, which wins over +// the property. +TEST(SniiWriterNorms, WritesNormsFollowSharedNormsPolicy) { + const bool original_skip_norms_for_variant = + doris::config::inverted_index_skip_norms_for_variant; + auto writes_norms = [](const std::map& extra_properties, + const std::string& index_suffix) { + doris::TabletIndexPB index_pb; + index_pb.set_index_type(doris::IndexType::INVERTED); + index_pb.set_index_id(92); + index_pb.set_index_name("norms_policy"); + index_pb.add_col_unique_id(0); + index_pb.set_index_suffix_name(index_suffix); + index_pb.mutable_properties()->insert({"parser", "english"}); + index_pb.mutable_properties()->insert({"support_phrase", "true"}); + for (const auto& [key, value] : extra_properties) { + index_pb.mutable_properties()->insert({key, value}); + } + doris::TabletIndex index_meta; + index_meta.init_from_pb(index_pb); + doris::segment_v2::SniiIndexColumnWriter writer(nullptr, &index_meta, + doris::FieldType::OLAP_FIELD_TYPE_VARCHAR); + const doris::Status status = writer.init(); + EXPECT_TRUE(status.ok()) << status.to_string(); + return writer.writes_norms_for_test(); + }; + + doris::config::inverted_index_skip_norms_for_variant = false; + EXPECT_TRUE(writes_norms({}, "")); + EXPECT_FALSE(writes_norms({{"norms", "false"}}, "")); + EXPECT_TRUE(writes_norms({}, "v.s_host")); + EXPECT_FALSE(writes_norms({{"norms", "false"}}, "v.s_host")); + EXPECT_TRUE(writes_norms({{"field_pattern", "s_*"}}, "")); + + doris::config::inverted_index_skip_norms_for_variant = true; + EXPECT_TRUE(writes_norms({}, "")); + EXPECT_FALSE(writes_norms({}, "v.s_host")); + EXPECT_FALSE(writes_norms({{"norms", "true"}}, "v.s_host")); + EXPECT_FALSE(writes_norms({{"field_pattern", "s_*"}}, "")); + + doris::config::inverted_index_skip_norms_for_variant = original_skip_norms_for_variant; +} + TEST(SniiDocIdSinkGrowth, AppendRangeGrowsGeometrically) { std::vector docids; doris::snii::query::VectorDocIdSink sink(docids); diff --git a/regression-test/data/inverted_index_p0/storage_format/test_storage_format_snii_norms.out b/regression-test/data/inverted_index_p0/storage_format/test_storage_format_snii_norms.out new file mode 100644 index 00000000000000..b81a6cebfb9863 --- /dev/null +++ b/regression-test/data/inverted_index_p0/storage_format/test_storage_format_snii_norms.out @@ -0,0 +1,29 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !snii_plain_column_score -- +1 0.3541 +2 0.562 + +-- !snii_field_pattern_score -- +1 0.311 +2 0.5235 + +-- !snii_field_pattern_no_norms_score -- +1 0.47 +2 0.47 + +-- !snii_whole_column_no_norms_score -- +1 0.47 +2 0.47 + +-- !snii_config_plain_column_score -- +1 0.3541 +2 0.562 + +-- !snii_config_field_pattern_score -- +1 0.47 +2 0.47 + +-- !snii_config_norms_true_score -- +1 0.47 +2 0.47 + diff --git a/regression-test/suites/inverted_index_p0/storage_format/test_storage_format_snii_norms.groovy b/regression-test/suites/inverted_index_p0/storage_format/test_storage_format_snii_norms.groovy new file mode 100644 index 00000000000000..8d9100998b5336 --- /dev/null +++ b/regression-test/suites/inverted_index_p0/storage_format/test_storage_format_snii_norms.groovy @@ -0,0 +1,163 @@ +// 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. + +// SNII follows the same norms policy as the CLucene formats: an analyzed index writes BM25 norms +// unless "norms" = "false", and inverted_index_skip_norms_for_variant drops them for every index on +// a variant path whatever the property says. Norms are not visible from outside a SNII file, so +// this suite reads them off the scores: in every table, rows 1 and 2 match "alpha" once each, but +// row 1 is three tokens long and row 2 one. With norms the longer row scores lower; without norms +// both rows score the same, and neither is NaN. +// It flips a BE config, so it must not share the cluster with other suites. +suite("test_storage_format_snii_norms", "p0,nonConcurrent") { + sql """ set enable_match_without_inverted_index = false """ + sql """ set default_variant_enable_typed_paths_to_sparse = false """ + sql """ set default_variant_enable_doc_mode = false """ + + sql "DROP TABLE IF EXISTS test_storage_format_snii_norms" + sql """ + CREATE TABLE test_storage_format_snii_norms ( + id INT, + content TEXT, + v variant< + 's_*' : text, + 't_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, + vn variant< + 'b_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, + INDEX idx_content (content) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true" + ), + INDEX idx_v_s (v) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "field_pattern"="s_*" + ), + INDEX idx_v_t (v) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "field_pattern"="t_*", + "norms"="false" + ), + INDEX idx_vn (vn) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "norms"="false" + ) + ) ENGINE=OLAP DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ( + "replication_allocation" = "tag.location.default: 1", + "disable_auto_compaction" = "true", + "inverted_index_storage_format" = "SNII" + ) + """ + sql """ insert into test_storage_format_snii_norms values + (1, 'alpha database server', + parse_to_variant('{"s_note":"alpha database server", "t_note":"alpha database server"}'), + parse_to_variant('{"b_note":"alpha database server"}')), + (2, 'alpha', + parse_to_variant('{"s_note":"alpha", "t_note":"alpha"}'), + parse_to_variant('{"b_note":"alpha"}')), + (3, 'gamma', parse_to_variant('{"other":"gamma"}'), parse_to_variant('{"other":"gamma"}')) + """ + sql " sync " + + // an ordinary column and a field_pattern index keep norms by default: row 1 scores lower + order_qt_snii_plain_column_score """ + select id, round(score(), 4) from test_storage_format_snii_norms + where content match_any "alpha" order by score() desc limit 10 + """ + order_qt_snii_field_pattern_score """ + select id, round(score(), 4) from test_storage_format_snii_norms + where cast(v["s_note"] as string) match_any "alpha" order by score() desc limit 10 + """ + // "norms" = "false" drops them, on a field_pattern index and on the copies a whole-column + // index hands to its subcolumns: rows 1 and 2 score the same + order_qt_snii_field_pattern_no_norms_score """ + select id, round(score(), 4) from test_storage_format_snii_norms + where cast(v["t_note"] as string) match_any "alpha" order by score() desc limit 10 + """ + order_qt_snii_whole_column_no_norms_score """ + select id, round(score(), 4) from test_storage_format_snii_norms + where cast(vn["b_note"] as string) match_any "alpha" order by score() desc limit 10 + """ + + // with the config on, every index on a variant path leaves norms out, even one that asks for + // them, while an ordinary column index keeps them + setBeConfigTemporary([inverted_index_skip_norms_for_variant: true]) { + sql "DROP TABLE IF EXISTS test_storage_format_snii_norms_config" + sql """ + CREATE TABLE test_storage_format_snii_norms_config ( + id INT, + content TEXT, + v variant< + 's_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, + vf variant< + 'c_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, + INDEX idx_content (content) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true" + ), + INDEX idx_v_s (v) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "field_pattern"="s_*" + ), + INDEX idx_vf (vf) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "norms"="true" + ) + ) ENGINE=OLAP DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ( + "replication_allocation" = "tag.location.default: 1", + "disable_auto_compaction" = "true", + "inverted_index_storage_format" = "SNII" + ) + """ + sql """ insert into test_storage_format_snii_norms_config values + (1, 'alpha database server', + parse_to_variant('{"s_note":"alpha database server"}'), + parse_to_variant('{"c_note":"alpha database server"}')), + (2, 'alpha', parse_to_variant('{"s_note":"alpha"}'), parse_to_variant('{"c_note":"alpha"}')), + (3, 'gamma', parse_to_variant('{"other":"gamma"}'), parse_to_variant('{"other":"gamma"}')) + """ + sql " sync " + } + + order_qt_snii_config_plain_column_score """ + select id, round(score(), 4) from test_storage_format_snii_norms_config + where content match_any "alpha" order by score() desc limit 10 + """ + order_qt_snii_config_field_pattern_score """ + select id, round(score(), 4) from test_storage_format_snii_norms_config + where cast(v["s_note"] as string) match_any "alpha" order by score() desc limit 10 + """ + order_qt_snii_config_norms_true_score """ + select id, round(score(), 4) from test_storage_format_snii_norms_config + where cast(vf["c_note"] as string) match_any "alpha" order by score() desc limit 10 + """ +} diff --git a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy index 2bda75974f2997..d028c1bf3d7f04 100644 --- a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy +++ b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy @@ -21,7 +21,8 @@ // This covers both an index declared with a field_pattern and a whole-column // index on a VARIANT column, whose per-subcolumn copies inherit the properties of the index they // come from. BM25 scoring keeps working with and without norms. -suite("test_variant_subcolumn_index_norms", "p0") { +// It flips a BE config, so it must not share the cluster with other suites. +suite("test_variant_subcolumn_index_norms", "p0,nonConcurrent") { if (isCloudMode()) { return } From 6d36303905033dcc1dc72250ccd44402f972e2a7 Mon Sep 17 00:00:00 2001 From: lihangyu Date: Wed, 16 Sep 2026 21:27:58 +0800 Subject: [PATCH 6/6] [fix](inverted index) Refuse BM25 scoring over segments without norms ### What problem does this PR solve? Problem Summary: The previous commits of this PR let an analyzed index be written without norms ("norms" = "false", or inverted_index_skip_norms_for_variant on a variant path), and scored such an index without length normalization. That left two problems: - CLucene keeps a field's token count in its .nrm header. A segment without norms adds no tokens but still adds documents, and its rows read the fake norm encodeNorm(0), i.e. length 0. When some segments of a field have norms and others do not, as while the config is being turned on, avgdl stays positive, the zero-avgdl fallback never runs, and the rows without norms rank as the shortest possible documents, above otherwise identical rows. - SNII was changed to score such segments too, which made the two formats and the transition harder to reason about. Follow what SNII already did: BM25 scoring needs norms from every segment it reads. - CollectionStatistics now refuses an analyzed CLucene segment whose field has no norms with INVERTED_INDEX_NOT_SUPPORTED, before any score is computed, so a collection with some or all segments lacking norms fails the score() query instead of ranking inconsistently. An index that is not analyzed never writes norms and is collected as before, so SEARCH scoring over keyword fields is unchanged. - The zero-avgdl fallback in BM25Similarity is dropped: with the refusal it can no longer be reached by an analyzed index. - SNII scoring is back to rejecting segments without norms (resolve_snii_scoring_segment, SniiStatsProvider::encoded_norm, ScorerContext::score are as on master). Its message now names the property and the config instead of suggesting that compaction will add the norms back. - The writer side is unchanged: should_write_index_norms still decides for the CLucene writer, the SNII writer and SNII direct compaction. - Fix the config comment, which still said the property could override the config. MATCH filtering is not affected; only queries that compute score() need norms. ### Release note A query that computes score() on an analyzed inverted index now fails when a segment it reads was written without norms ("norms" = "false", inverted_index_skip_norms_for_variant, or a segment written before norms were supported), instead of returning NaN or inconsistent scores. ### Check List (For Author) - Test: Unit Test / Regression test - CollectionStatisticsTest.LegacyV3RejectsSegmentWrittenWithoutNorms (alone and mixed with a segment that has norms), CollectionStatisticsTest.LegacyV3KeywordIndexWithoutNormsIsStillCollected - regression test_variant_subcolumn_index_norms and test_storage_format_snii_norms: MATCH still filters, score() fails on indexes without norms and on a mixed-generation table; plus test_bm25_score_variant, test_omit_norms, test_search_score_topn_predicates, test_search_score_cache, test_search_score_topn_delete_predicate and test_storage_format_snii - Behavior changed: Yes. score() on an analyzed index with a segment that has no norms fails instead of returning NaN or skewed scores. - Does this need documentation: Yes (https://github.com/apache/doris-website/pull/4146) Co-Authored-By: Claude Opus 5 --- be/src/common/config.cpp | 3 +- be/src/common/config.h | 3 +- .../inverted/similarity/bm25_similarity.cpp | 8 -- .../similarity/collection_statistics.cpp | 34 +++-- .../similarity/collection_statistics.h | 9 +- .../storage/index/snii/query/bm25_scorer.cpp | 9 +- be/src/storage/index/snii/query/bm25_scorer.h | 11 +- .../index/snii/query/scoring_query.cpp | 5 +- .../storage/index/snii/snii_index_reader.cpp | 2 +- .../index/snii/stats/snii_stats_provider.cpp | 11 +- .../index/snii/stats/snii_stats_provider.h | 8 +- .../similarity/bm25_similarity_test.cpp | 19 --- .../similarity/collection_statistics_test.cpp | 120 ++++++++++++------ .../index/snii/query/scoring_query_test.cpp | 74 +---------- .../test_storage_format_snii_norms.out | 22 ++-- .../test_variant_subcolumn_index_norms.out | 10 +- .../test_storage_format_snii_norms.groovy | 101 ++++++++++++--- .../test_variant_subcolumn_index_norms.groovy | 80 +++++++++++- 18 files changed, 315 insertions(+), 214 deletions(-) diff --git a/be/src/common/config.cpp b/be/src/common/config.cpp index b64d30e8a96013..a0b479a4eeaefa 100644 --- a/be/src/common/config.cpp +++ b/be/src/common/config.cpp @@ -1410,7 +1410,8 @@ DEFINE_mBool(inverted_index_ram_dir_enable, "true"); DEFINE_mBool(inverted_index_ram_dir_enable_when_base_compaction, "true"); // Norms cost one byte per segment row, including rows that hold no value for the field. A segment // holds one index per variant path, so writing norms for them costs rows * paths bytes. Turn this on -// to leave norms out of indexes on a variant path, except those that set the "norms" property. +// to leave norms out of every index on a variant path, whatever its "norms" property says; BM25 +// scoring (score()) on those indexes then fails. DEFINE_mBool(inverted_index_skip_norms_for_variant, "false"); // use num_broadcast_buffer blocks as buffer to do broadcast DEFINE_Int32(num_broadcast_buffer, "32"); diff --git a/be/src/common/config.h b/be/src/common/config.h index 801f00acfb245f..6bda64a14553a2 100644 --- a/be/src/common/config.h +++ b/be/src/common/config.h @@ -1464,7 +1464,8 @@ DECLARE_mBool(inverted_index_ram_dir_enable); DECLARE_mBool(inverted_index_ram_dir_enable_when_base_compaction); // Norms cost one byte per segment row, including rows that hold no value for the field. A segment // holds one index per variant path, so writing norms for them costs rows * paths bytes. Turn this on -// to leave norms out of indexes on a variant path, except those that set the "norms" property. +// to leave norms out of every index on a variant path, whatever its "norms" property says; BM25 +// scoring (score()) on those indexes then fails. DECLARE_mBool(inverted_index_skip_norms_for_variant); // use num_broadcast_buffer blocks as buffer to do broadcast DECLARE_Int32(num_broadcast_buffer); diff --git a/be/src/storage/index/inverted/similarity/bm25_similarity.cpp b/be/src/storage/index/inverted/similarity/bm25_similarity.cpp index 8be1d9c200de27..2ab946a8791409 100644 --- a/be/src/storage/index/inverted/similarity/bm25_similarity.cpp +++ b/be/src/storage/index/inverted/similarity/bm25_similarity.cpp @@ -17,7 +17,6 @@ #include "storage/index/inverted/similarity/bm25_similarity.h" -#include #include namespace doris::segment_v2 { @@ -42,13 +41,6 @@ BM25Similarity::BM25Similarity(float idf, float avgdl) : _idf(idf), _avgdl(avgdl } void BM25Similarity::compute_tf_cache() { - // CLucene keeps a field's token count in its .nrm header, so avgdl is 0 when no segment of the - // field stores norms (e.g. variant subcolumn indexes). Every document then has an unknown length: - // score without length normalization instead of computing 0 / 0. - if (_avgdl <= 0.0F) { - std::fill(_cache.begin(), _cache.end(), 1.0F / _k1); - return; - } for (int i = 0; i < _cache.size(); i++) { _cache[i] = 1.0F / (_k1 * ((1 - _b) + _b * LENGTH_TABLE[i] / _avgdl)); } diff --git a/be/src/storage/index/inverted/similarity/collection_statistics.cpp b/be/src/storage/index/inverted/similarity/collection_statistics.cpp index 41ea7394abf32d..06f88bacbb1689 100644 --- a/be/src/storage/index/inverted/similarity/collection_statistics.cpp +++ b/be/src/storage/index/inverted/similarity/collection_statistics.cpp @@ -43,10 +43,13 @@ namespace collection_statistics_detail { Result resolve_snii_scoring_segment(uint64_t index_doc_count, uint64_t sum_total_term_freq, - bool has_positions) { - if (!has_positions) { + bool has_positions, bool has_norms) { + if (!has_positions || !has_norms) { return ResultError(Status::Error( - "SNII scoring requires positions; this index was written without them")); + "SNII scoring requires positions and norms; this segment was written without " + "positions or without norms. Norms are left out when the index sets \"norms\" = " + "\"false\" or, for a variant path, when inverted_index_skip_norms_for_variant is " + "on")); } return SniiScoringSegmentStats {.doc_count = index_doc_count, .token_count = sum_total_term_freq}; @@ -245,7 +248,8 @@ Status CollectionStatistics::process_segment(const RowsetSharedPtr& rowset, const uint64_t segment_doc_count = logical_reader->stats().doc_count; RETURN_IF_ERROR(admit_snii_scoring_segment( ws_field_name, segment_doc_count, logical_reader->stats().sum_total_term_freq, - logical_reader->has_positions(), &segment_accumulator)); + logical_reader->has_positions(), logical_reader->has_norms(), + &segment_accumulator)); ::doris::snii::reader::DictBlockCache dict_block_cache; for (const auto& logical_term_bytes : collect_info.unique_terms) { @@ -311,8 +315,22 @@ Status CollectionStatistics::process_segment(const RowsetSharedPtr& rowset, index_reader = index_searcher->getReader(); #endif total_segment_docs = std::max(total_segment_docs, index_reader->maxDoc()); - _total_num_tokens[ws_field_name] += - index_reader->sumTotalTermFreq(ws_field_name.c_str()).value_or(0); + // BM25 on an analyzed index needs the record length of every row, and CLucene keeps + // them, together with the field's token count, in the norms. A segment written without + // norms would feed a zero avgdl, or rank its rows as zero-length documents next to the + // segments that have norms, so refuse to score the collection, as SNII does. An index + // that is not analyzed never writes norms and is left as it is. + const auto token_count = index_reader->sumTotalTermFreq(ws_field_name.c_str()); + if (!token_count.has_value() && + segment_v2::inverted_index::InvertedIndexAnalyzer::should_analyzer( + collect_info.index_meta->properties())) { + return Status::Error( + "BM25 scoring requires norms, but segment {} was written without norms for " + "field {}. Norms are left out when the index sets \"norms\" = \"false\" or, " + "for a variant path, when inverted_index_skip_norms_for_variant is on", + seg_path, StringHelper::to_string(ws_field_name)); + } + _total_num_tokens[ws_field_name] += token_count.value_or(0); for (const auto& logical_term_bytes : collect_info.unique_terms) { const auto logical_term = @@ -333,10 +351,10 @@ Status CollectionStatistics::process_segment(const RowsetSharedPtr& rowset, Status CollectionStatistics::admit_snii_scoring_segment( const std::wstring& field_name, uint64_t index_doc_count, uint64_t sum_total_term_freq, - bool has_positions, SniiScoringSegmentAccumulator* segment_accumulator) { + bool has_positions, bool has_norms, SniiScoringSegmentAccumulator* segment_accumulator) { DORIS_CHECK(segment_accumulator != nullptr); auto segment_stats = collection_statistics_detail::resolve_snii_scoring_segment( - index_doc_count, sum_total_term_freq, has_positions); + index_doc_count, sum_total_term_freq, has_positions, has_norms); if (!segment_stats.has_value()) { clear(); return segment_stats.error(); diff --git a/be/src/storage/index/inverted/similarity/collection_statistics.h b/be/src/storage/index/inverted/similarity/collection_statistics.h index b2221799c95916..7e93498508324a 100644 --- a/be/src/storage/index/inverted/similarity/collection_statistics.h +++ b/be/src/storage/index/inverted/similarity/collection_statistics.h @@ -85,6 +85,7 @@ class CollectionStatistics { io::IOContext* io_ctx); Status admit_snii_scoring_segment(const std::wstring& field_name, uint64_t index_doc_count, uint64_t sum_total_term_freq, bool has_positions, + bool has_norms, SniiScoringSegmentAccumulator* segment_accumulator); void commit_snii_scoring_segment(SniiScoringSegmentAccumulator&& segment_accumulator); void clear(); @@ -118,12 +119,12 @@ struct SniiScoringSegmentStats { uint64_t token_count = 0; }; -// SNII scoring requires positions, which provide term frequencies. A segment written without norms -// is still admitted: its token count comes from the stats block, and its documents are scored -// without length normalization. +// SNII scoring requires positions (which provide term frequencies) and norms. The current writer +// emits norms for every analyzed index with positions. Older segments without norms return +// NOT_SUPPORTED until an index rebuild or compaction supplies them. Result resolve_snii_scoring_segment(uint64_t index_doc_count, uint64_t sum_total_term_freq, - bool has_positions); + bool has_positions, bool has_norms); void add_term_doc_frequency( std::unordered_map>* diff --git a/be/src/storage/index/snii/query/bm25_scorer.cpp b/be/src/storage/index/snii/query/bm25_scorer.cpp index dbc74c10055c36..9cd8326deaba61 100644 --- a/be/src/storage/index/snii/query/bm25_scorer.cpp +++ b/be/src/storage/index/snii/query/bm25_scorer.cpp @@ -47,13 +47,10 @@ ScorerContext ScorerContext::from_idf(double idf) { return ctx; } -double ScorerContext::score(double tf, std::optional encoded_norm, double avgdl, +double ScorerContext::score(double tf, uint8_t encoded_norm, double avgdl, const Bm25Params& params) const { - // Without a norm the document length is unknown, so the document is scored as if it had the - // average length: b * dl / avgdl becomes b, and 1 - b + b leaves no length factor at all. - const double length_term = - encoded_norm.has_value() ? params.b * decode_norm(*encoded_norm) / avgdl : params.b; - const double denom = tf + params.k1 * (1.0 - params.b + length_term); + const double dl = decode_norm(encoded_norm); + const double denom = tf + params.k1 * (1.0 - params.b + params.b * dl / avgdl); return idf_ * (tf * (params.k1 + 1.0)) / denom; } diff --git a/be/src/storage/index/snii/query/bm25_scorer.h b/be/src/storage/index/snii/query/bm25_scorer.h index 0c1e9c62b76c8c..51b62327a0fe1e 100644 --- a/be/src/storage/index/snii/query/bm25_scorer.h +++ b/be/src/storage/index/snii/query/bm25_scorer.h @@ -18,7 +18,6 @@ #pragma once #include -#include // Bm25Scorer -- classic Okapi BM25 relevance scoring over SNII native stats. // @@ -28,9 +27,7 @@ // per-document contribution of a term then is: // score = idf * (tf * (k1 + 1)) / (tf + k1 * (1 - b + b * dl / avgdl)) // where tf is the in-doc term frequency, dl the document length decoded from the -// 1-byte encoded norm, and avgdl the average document length. An index written -// without norms has no dl: its documents are scored as if dl were avgdl, which -// drops length normalization from the formula. +// 1-byte encoded norm, and avgdl the average document length. // // Norm encode/decode (DOCUMENTED CONTRACT): the writer stores doc length as a // byte-quantized value floor-clamped to [1, 255]; decode is the identity map @@ -70,10 +67,8 @@ class ScorerContext { uint64_t df() const { return df_; } // Scores one document occurrence: tf is the in-doc term frequency, encoded_norm - // the doc's 1-byte length norm (std::nullopt when the index stores no norms), - // avgdl the collection average length. - double score(double tf, std::optional encoded_norm, double avgdl, - const Bm25Params& params) const; + // the doc's 1-byte length norm, avgdl the collection average length. + double score(double tf, uint8_t encoded_norm, double avgdl, const Bm25Params& params) const; private: double idf_ = 0.0; diff --git a/be/src/storage/index/snii/query/scoring_query.cpp b/be/src/storage/index/snii/query/scoring_query.cpp index 9018edb18a24e1..cd63062cc653c4 100644 --- a/be/src/storage/index/snii/query/scoring_query.cpp +++ b/be/src/storage/index/snii/query/scoring_query.cpp @@ -20,7 +20,6 @@ #include #include #include -#include #include #include #include @@ -158,7 +157,7 @@ Status score_decoded(const stats::SniiStatsProvider& stats, const ScorerContext& DCHECK_EQ(docids.size(), tfs.size()); out->reserve(docids.size()); for (size_t i = 0; i < docids.size(); ++i) { - std::optional norm; + uint8_t norm = 0; RETURN_IF_ERROR(stats.encoded_norm(docids[i], &norm)); out->push_back({docids[i], ctx.score(tfs[i], norm, avgdl, params)}); } @@ -185,7 +184,7 @@ Status accumulate_decoded_candidate_scores(const stats::SniiStatsProvider& stats ++candidate_index; continue; } - std::optional norm; + uint8_t norm = 0; RETURN_IF_ERROR(stats.encoded_norm(docids[doc_index], &norm)); scores[candidate_index] += scorer.score(tfs[doc_index], norm, avgdl, params); ++doc_index; diff --git a/be/src/storage/index/snii/snii_index_reader.cpp b/be/src/storage/index/snii/snii_index_reader.cpp index 80b2e2faa0b433..bbcb78f7f8449a 100644 --- a/be/src/storage/index/snii/snii_index_reader.cpp +++ b/be/src/storage/index/snii/snii_index_reader.cpp @@ -224,7 +224,7 @@ Status score_phrase_matches(const IndexQueryContextPtr& context, std::string_vie for (const auto& match : matches) { DCHECK(final_candidates.contains(match.docid)); DCHECK_NE(match.frequency, 0); - std::optional norm; + uint8_t norm = 0; RETURN_IF_ERROR(segment_stats.encoded_norm(match.docid, &norm)); scored_docs.push_back({.docid = match.docid, .score = scorer.score(match.frequency, norm, collection_avgdl, diff --git a/be/src/storage/index/snii/stats/snii_stats_provider.cpp b/be/src/storage/index/snii/stats/snii_stats_provider.cpp index 90023cef5f0a46..557c9c30417da7 100644 --- a/be/src/storage/index/snii/stats/snii_stats_provider.cpp +++ b/be/src/storage/index/snii/stats/snii_stats_provider.cpp @@ -86,17 +86,14 @@ Status SniiStatsProvider::doc_freq(std::string_view term, uint64_t* df) const { return Status::OK(); } -Status SniiStatsProvider::encoded_norm(uint32_t docid, std::optional* out) const { +Status SniiStatsProvider::encoded_norm(uint32_t docid, uint8_t* out) const { if (out == nullptr) return Status::Error("stats_provider: null out"); if (!has_norms_) { - *out = std::nullopt; - return Status::OK(); + return Status::Error( + "stats_provider: index has no norms"); } - uint8_t norm = 0; - RETURN_IF_ERROR(norms_reader_.try_encoded_norm(docid, &norm)); - *out = norm; - return Status::OK(); + return norms_reader_.try_encoded_norm(docid, out); } } // namespace doris::snii::stats diff --git a/be/src/storage/index/snii/stats/snii_stats_provider.h b/be/src/storage/index/snii/stats/snii_stats_provider.h index 81f39acb891383..28ef31c597c582 100644 --- a/be/src/storage/index/snii/stats/snii_stats_provider.h +++ b/be/src/storage/index/snii/stats/snii_stats_provider.h @@ -18,7 +18,6 @@ #pragma once #include -#include #include #include "common/status.h" @@ -60,10 +59,9 @@ class SniiStatsProvider { // Per-term document frequency. Absent term -> *df = 0 (OK status). Status doc_freq(std::string_view term, uint64_t* df) const; - // 1-byte encoded doc-length norm for docid (raw byte from the norms POD), or - // std::nullopt when the index was written without norms. Out-of-range docid on - // an index with norms -> InvalidArgument. - Status encoded_norm(uint32_t docid, std::optional* out) const; + // 1-byte encoded doc-length norm for docid (raw byte from the norms POD). + // Out-of-range docid -> InvalidArgument; index without norms -> InvalidArgument. + Status encoded_norm(uint32_t docid, uint8_t* out) const; bool has_norms() const { return has_norms_; } diff --git a/be/test/storage/index/inverted/similarity/bm25_similarity_test.cpp b/be/test/storage/index/inverted/similarity/bm25_similarity_test.cpp index fa3a04a70bab4b..9ceba92421e87f 100644 --- a/be/test/storage/index/inverted/similarity/bm25_similarity_test.cpp +++ b/be/test/storage/index/inverted/similarity/bm25_similarity_test.cpp @@ -19,7 +19,6 @@ #include -#include #include #include "common/be_mock_util.h" @@ -290,21 +289,3 @@ TEST_F(BM25SimilarityTest, CacheConsistencyTest) { ASSERT_FLOAT_EQ(similarity_->_cache[i], expected); } } - -// Indexes without norms report no token count, so avgdl is 0: scores must stay finite and ignore -// document length instead of turning into NaN. -TEST_F(BM25SimilarityTest, ZeroAvgDlScoresWithoutLengthNorm) { - mock_stats_->set_mock_idf(2.0f); - mock_stats_->set_mock_avg_dl(0.0f); - - similarity_->for_one_term(context_, L"field", L"term"); - - for (int i = 0; i < 256; ++i) { - ASSERT_FLOAT_EQ(similarity_->_cache[i], 1.0f / similarity_->_k1); - } - float score = similarity_->score(1.0f, 0); - ASSERT_FALSE(std::isnan(score)); - ASSERT_FLOAT_EQ(score, - similarity_->_weight - similarity_->_weight / (1.0f + 1.0f / similarity_->_k1)); - ASSERT_GT(similarity_->score(2.0f, 0), score); -} diff --git a/be/test/storage/index/inverted/similarity/collection_statistics_test.cpp b/be/test/storage/index/inverted/similarity/collection_statistics_test.cpp index 16ee72a24adc63..f9c7693b67b09a 100644 --- a/be/test/storage/index/inverted/similarity/collection_statistics_test.cpp +++ b/be/test/storage/index/inverted/similarity/collection_statistics_test.cpp @@ -375,7 +375,9 @@ class CollectionStatisticsTest : public ::testing::Test { return splits; } - TabletSchemaSPtr create_legacy_v3_schema() { + TabletSchemaSPtr create_legacy_v3_schema(std::map properties = { + {"parser", "standard"}, + {"support_phrase", "true"}}) { TabletSchemaPB schema_pb; schema_pb.set_keys_type(DUP_KEYS); schema_pb.set_inverted_index_storage_format(InvertedIndexStorageFormatPB::V3); @@ -392,8 +394,7 @@ class CollectionStatisticsTest : public ::testing::Test { index._index_id = 1; index._index_type = IndexType::INVERTED; index._col_unique_ids.push_back(1); - index._properties["parser"] = "standard"; - index._properties["support_phrase"] = "true"; + index._properties = std::move(properties); tablet_schema->append_index(std::move(index)); return tablet_schema; } @@ -453,9 +454,8 @@ class CollectionStatisticsTest : public ::testing::Test { return file_writer.finish_close(); } - // A normal analyzed SNII segment with positions, and with norms unless the index was written - // with norms turned off. - Status write_snii_scoring_segment(const std::string& segment_path, bool with_norms = true) { + // A normal analyzed SNII segment with positions and norms, as emitted for scoring indexes. + Status write_snii_scoring_segment(const std::string& segment_path) { const std::string index_path_prefix { segment_v2::InvertedIndexDescriptor::get_index_file_path_prefix(segment_path)}; io::FileWriterPtr file_writer; @@ -484,9 +484,7 @@ class CollectionStatisticsTest : public ::testing::Test { input.index_id = 1; input.config = snii::format::IndexConfig::kDocsPositions; input.doc_count = 2; - if (with_norms) { - input.encoded_norms = {snii::query::encode_norm(2), snii::query::encode_norm(1)}; - } + input.encoded_norms = {snii::query::encode_norm(2), snii::query::encode_norm(1)}; input.terms = {std::move(alpha), std::move(beta)}; RETURN_IF_ERROR(writer.add_logical_index(input)); @@ -631,15 +629,17 @@ class CollectionStatisticsTest : public ::testing::Test { struct SniiScoringFieldInput { SniiScoringFieldInput(std::wstring field_name, uint64_t index_doc_count, - uint64_t sum_total_term_freq) + uint64_t sum_total_term_freq, bool has_norms = true) : field_name(std::move(field_name)), index_doc_count(index_doc_count), - sum_total_term_freq(sum_total_term_freq) {} + sum_total_term_freq(sum_total_term_freq), + has_norms(has_norms) {} std::wstring field_name; uint64_t index_doc_count = 0; uint64_t sum_total_term_freq = 0; bool has_positions = true; + bool has_norms = true; }; Status stage_snii_fields_for_test( @@ -648,7 +648,7 @@ class CollectionStatisticsTest : public ::testing::Test { for (const auto& field : fields) { RETURN_IF_ERROR(statistics->admit_snii_scoring_segment( field.field_name, field.index_doc_count, field.sum_total_term_freq, - field.has_positions, segment_accumulator)); + field.has_positions, field.has_norms, segment_accumulator)); } return Status::OK(); } @@ -663,9 +663,9 @@ class CollectionStatisticsTest : public ::testing::Test { Status admit_snii_segment_for_test(CollectionStatistics* statistics, const std::wstring& field_name, uint64_t index_doc_count, - uint64_t sum_total_term_freq) { - return admit_snii_fields_for_test(statistics, - {{field_name, index_doc_count, sum_total_term_freq}}); + uint64_t sum_total_term_freq, bool has_norms = true) { + return admit_snii_fields_for_test( + statistics, {{field_name, index_doc_count, sum_total_term_freq, has_norms}}); } Status stage_snii_fields_then_file_not_found_for_test( @@ -911,13 +911,47 @@ TEST_F(CollectionStatisticsTest, LegacyV3SkipsEmptySegmentAfterCollectingAvailab expect_collected_term(L"1", L"alpha", 1); } -TEST_F(CollectionStatisticsTest, SniiScoringUsesPhysicalStatistics) { - auto tablet_schema = create_snii_schema(); - auto expr_contexts = create_match_expr_contexts("alpha"); +// BM25 needs norms from every segment of an analyzed index: a segment written without them makes +// the whole collection refuse to score, on its own or next to segments that have norms. +TEST_F(CollectionStatisticsTest, LegacyV3RejectsSegmentWrittenWithoutNorms) { + auto with_norms_schema = create_legacy_v3_schema(); + auto without_norms_schema = create_legacy_v3_schema( + {{"parser", "standard"}, {"support_phrase", "true"}, {"norms", "false"}}); + const std::string with_norms_path = test_dir_ + "/legacy_v3_with_norms_0.dat"; + const std::string without_norms_path = test_dir_ + "/legacy_v3_without_norms_1.dat"; + ASSERT_TRUE(write_legacy_v3_segment(with_norms_schema, with_norms_path).ok()); + ASSERT_TRUE(write_legacy_v3_segment(without_norms_schema, without_norms_path).ok()); - const std::string segment_path = test_dir_ + "/snii_scoring_0.dat"; - auto write_status = write_snii_scoring_segment(segment_path); - ASSERT_TRUE(write_status.ok()) << write_status; + auto collect = [&](const std::vector& segment_paths) { + auto rowset_meta = std::make_shared(); + auto rowset = std::make_shared(without_norms_schema, + rowset_meta); + rowset->set_num_segments(static_cast(segment_paths.size())); + for (size_t i = 0; i < segment_paths.size(); ++i) { + rowset->set_segment_path(static_cast(i), segment_paths[i]); + } + auto reader = std::make_shared(rowset); + std::vector splits {RowSetSplits(reader)}; + return stats_->collect(runtime_state_.get(), splits, without_norms_schema, + create_match_expr_contexts("alpha"), nullptr); + }; + + Status status = collect({without_norms_path}); + EXPECT_EQ(status.code(), ErrorCode::INVERTED_INDEX_NOT_SUPPORTED) << status; + EXPECT_NE(status.to_string().find("written without norms"), std::string::npos) << status; + expect_no_collected_tokens(L"1"); + + status = collect({with_norms_path, without_norms_path}); + EXPECT_EQ(status.code(), ErrorCode::INVERTED_INDEX_NOT_SUPPORTED) << status; + expect_no_collected_tokens(L"1"); +} + +// An index that is not analyzed never writes norms, and its scoring statistics are collected as +// before. +TEST_F(CollectionStatisticsTest, LegacyV3KeywordIndexWithoutNormsIsStillCollected) { + auto tablet_schema = create_legacy_v3_schema({}); + const std::string segment_path = test_dir_ + "/legacy_v3_keyword_0.dat"; + ASSERT_TRUE(write_legacy_v3_segment(tablet_schema, segment_path).ok()); auto rowset_meta = std::make_shared(); auto rowset = std::make_shared(tablet_schema, rowset_meta); @@ -926,22 +960,20 @@ TEST_F(CollectionStatisticsTest, SniiScoringUsesPhysicalStatistics) { auto reader = std::make_shared(rowset); std::vector splits {RowSetSplits(reader)}; - auto status = - stats_->collect(runtime_state_.get(), splits, tablet_schema, expr_contexts, nullptr); + const Status status = stats_->collect(runtime_state_.get(), splits, tablet_schema, + create_search_contexts("TERM", "alpha beta"), nullptr); ASSERT_TRUE(status.ok()) << status; - expect_collected_stats(L"1", 2, 3); - expect_collected_term(L"1", L"alpha", 2); + expect_collected_stats(L"1", 1, 0); + expect_collected_term(L"1", L"alpha beta", 1); } -// A segment written with norms turned off is admitted like any other: its token count comes from -// the stats block, so avgdl stays the physical average. -TEST_F(CollectionStatisticsTest, SniiScoringAdmitsSegmentWithoutNorms) { +TEST_F(CollectionStatisticsTest, SniiScoringUsesPhysicalStatistics) { auto tablet_schema = create_snii_schema(); auto expr_contexts = create_match_expr_contexts("alpha"); - const std::string segment_path = test_dir_ + "/snii_scoring_without_norms_0.dat"; - auto write_status = write_snii_scoring_segment(segment_path, /*with_norms=*/false); + const std::string segment_path = test_dir_ + "/snii_scoring_0.dat"; + auto write_status = write_snii_scoring_segment(segment_path); ASSERT_TRUE(write_status.ok()) << write_status; auto rowset_meta = std::make_shared(); @@ -957,7 +989,6 @@ TEST_F(CollectionStatisticsTest, SniiScoringAdmitsSegmentWithoutNorms) { ASSERT_TRUE(status.ok()) << status; expect_collected_stats(L"1", 2, 3); expect_collected_term(L"1", L"alpha", 2); - EXPECT_FLOAT_EQ(stats_->get_or_calculate_avg_dl(L"1"), 1.5F); } TEST_F(CollectionStatisticsTest, SniiScoringLookupUsesCallerIoContext) { @@ -1117,10 +1148,9 @@ class CollectionStatisticsDetailedTest : public ::testing::Test { std::unique_ptr stats_; }; -// SNII scoring requires only positions; statistics come directly from the stats block, whether or -// not the segment carries norms. +// SNII scoring requires only positions and norms; statistics come directly from the stats block. TEST(CollectionStatisticsSniiScoringTest, ResolveUsesPhysicalDocAndTokenCounts) { - auto result = resolve_snii_scoring_segment(3, 7, /*has_positions=*/true); + auto result = resolve_snii_scoring_segment(3, 7, /*has_positions=*/true, /*has_norms=*/true); ASSERT_TRUE(result.has_value()) << result.error(); EXPECT_EQ(result->doc_count, 3U); @@ -1128,15 +1158,22 @@ TEST(CollectionStatisticsSniiScoringTest, ResolveUsesPhysicalDocAndTokenCounts) } TEST(CollectionStatisticsSniiScoringTest, ResolveAcceptsEmptySegment) { - auto result = resolve_snii_scoring_segment(0, 0, /*has_positions=*/true); + auto result = resolve_snii_scoring_segment(0, 0, /*has_positions=*/true, /*has_norms=*/true); ASSERT_TRUE(result.has_value()) << result.error(); EXPECT_EQ(result->doc_count, 0U); EXPECT_EQ(result->token_count, 0U); } +TEST(CollectionStatisticsSniiScoringTest, ResolveRejectsSegmentWithoutNorms) { + auto result = resolve_snii_scoring_segment(3, 7, /*has_positions=*/true, /*has_norms=*/false); + + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().code(), ErrorCode::INVERTED_INDEX_NOT_SUPPORTED); +} + TEST(CollectionStatisticsSniiScoringTest, ResolveRejectsSegmentWithoutPositions) { - auto result = resolve_snii_scoring_segment(3, 7, /*has_positions=*/false); + auto result = resolve_snii_scoring_segment(3, 7, /*has_positions=*/false, /*has_norms=*/true); ASSERT_FALSE(result.has_value()); EXPECT_EQ(result.error().code(), ErrorCode::INVERTED_INDEX_NOT_SUPPORTED); @@ -1153,6 +1190,17 @@ TEST_F(CollectionStatisticsTest, CollectionStatisticsInstancesKeepAdmissionState EXPECT_FLOAT_EQ(second.get_or_calculate_avg_dl(L"1"), 5.0F); } +// An older segment without norms disables scoring for the whole collection and clears its stats. +TEST_F(CollectionStatisticsTest, SegmentWithoutNormsRejectsWholeCollection) { + ASSERT_TRUE(admit_snii_segment_for_test(stats_.get(), L"1", 2, 6).ok()); + + auto status = admit_snii_segment_for_test(stats_.get(), L"1", 3, 7, /*has_norms=*/false); + + EXPECT_EQ(status.code(), ErrorCode::INVERTED_INDEX_NOT_SUPPORTED); + expect_no_collected_tokens(L"1"); + EXPECT_THROW(stats_->get_doc_num(), Exception); +} + TEST_F(CollectionStatisticsTest, SegmentsAccumulatePhysicalStatistics) { ASSERT_TRUE(admit_snii_segment_for_test(stats_.get(), L"1", 2, 6).ok()); ASSERT_TRUE(admit_snii_segment_for_test(stats_.get(), L"1", 3, 9).ok()); diff --git a/be/test/storage/index/snii/query/scoring_query_test.cpp b/be/test/storage/index/snii/query/scoring_query_test.cpp index b7fc9b3ba322ae..96c3bc9070c267 100644 --- a/be/test/storage/index/snii/query/scoring_query_test.cpp +++ b/be/test/storage/index/snii/query/scoring_query_test.cpp @@ -29,7 +29,6 @@ #include #include #include -#include #include #include #include @@ -289,7 +288,7 @@ TEST(SniiScoringQuery, ReferenceOracleEqualsExhaustive) { EXPECT_EQ(df, plist.size()) << term; } for (uint32_t d = 0; d < corpus.doc_count; ++d) { - std::optional got; + uint8_t got = 0; ASSERT_TRUE(stats.encoded_norm(d, &got).ok()); EXPECT_EQ(got, norms[d]) << "docid " << d; } @@ -352,11 +351,10 @@ TEST(SniiScoringQuery, StatsProviderSharesValidatedNormsAcrossQueries) { EXPECT_EQ(metered_reader.metrics().total_request_bytes, after_first.total_request_bytes); EXPECT_EQ(logical_reader.memory_usage(), memory_usage_before_load); - std::optional first_norm; - std::optional second_norm; + uint8_t first_norm = 0; + uint8_t second_norm = 0; ASSERT_TRUE(first.encoded_norm(17, &first_norm).ok()); ASSERT_TRUE(second.encoded_norm(17, &second_norm).ok()); - EXPECT_TRUE(first_norm.has_value()); EXPECT_EQ(first_norm, second_norm); std::remove(path.c_str()); @@ -384,7 +382,7 @@ TEST(SniiScoringQuery, StatsProviderSharesOneConcurrentNormsLoad) { constexpr size_t kThreadCount = 16; std::barrier start(static_cast(kThreadCount + 1)); std::vector statuses(kThreadCount); - std::vector> norms(kThreadCount); + std::vector norms(kThreadCount); std::vector threads; threads.reserve(kThreadCount); controlled_reader.reset_read_at_calls(); @@ -443,75 +441,13 @@ TEST(SniiScoringQuery, StatsProviderRetriesTransientNormsReadFailure) { SniiStatsProvider retry; ASSERT_TRUE(SniiStatsProvider::open(&logical_reader, &retry).ok()); - std::optional norm; + uint8_t norm = 0; ASSERT_TRUE(retry.encoded_norm(17, &norm).ok()); - EXPECT_TRUE(norm.has_value()); EXPECT_EQ(controlled_reader.read_at_calls(), 2U); std::remove(path.c_str()); } -// An index written without norms scores every document as if it had the average length, so the -// ranking depends on term frequency and IDF only, and no norms read is attempted. -TEST(SniiScoringQuery, IndexWithoutNormsScoresWithoutLengthNormalization) { - const Corpus corpus = MakeCorpus(); - const std::string path = TempPath(); - { - SniiIndexInput input = ToInput(corpus); - input.encoded_norms.clear(); - io::LocalFileWriter writer; - ASSERT_TRUE(writer.open(path).ok()); - SniiCompoundWriter compound_writer(&writer); - ASSERT_TRUE(compound_writer.add_logical_index(std::move(input)).ok()); - ASSERT_TRUE(compound_writer.finish().ok()); - } - - io::LocalFileReader file_reader; - ASSERT_TRUE(file_reader.open(path).ok()); - reader::SniiSegmentReader segment_reader; - ASSERT_TRUE(reader::SniiSegmentReader::open(&file_reader, &segment_reader).ok()); - reader::LogicalIndexReader logical_reader; - ASSERT_TRUE(segment_reader.open_index(1, "body", &logical_reader).ok()); - ASSERT_FALSE(logical_reader.has_norms()); - - SniiStatsProvider stats; - ASSERT_TRUE(SniiStatsProvider::open(&logical_reader, &stats).ok()); - EXPECT_FALSE(stats.has_norms()); - std::optional norm = 1; - ASSERT_TRUE(stats.encoded_norm(17, &norm).ok()); - EXPECT_FALSE(norm.has_value()); - - const Bm25Params params; - const uint32_t k = 10; - const auto& postings = corpus.postings.at("common"); - const double df = static_cast(postings.size()); - const double idf = std::log(1.0 + (corpus.doc_count - df + 0.5) / (df + 0.5)); - std::vector expected; - for (const auto& [docid, freq] : postings) { - expected.push_back({docid, idf * (freq * (params.k1 + 1.0)) / (freq + params.k1)}); - } - std::ranges::sort(expected, [](const ScoredDoc& a, const ScoredDoc& b) { - if (a.score != b.score) { - return a.score > b.score; - } - return a.docid < b.docid; - }); - expected.resize(k); - - const std::vector terms {"common"}; - std::vector scored; - ASSERT_TRUE(doris::snii::query::scoring_query_exhaustive(logical_reader, stats, terms, k, - params, &scored) - .ok()); - ASSERT_EQ(scored.size(), expected.size()); - for (size_t i = 0; i < expected.size(); ++i) { - EXPECT_EQ(scored[i].docid, expected[i].docid) << "rank " << i; - EXPECT_NEAR(scored[i].score, expected[i].score, 1e-6) << "rank " << i; - } - - std::remove(path.c_str()); -} - TEST(SniiScoringQuery, CandidatesUseCollectionStatisticsAndPreserveDuplicateClauses) { const Corpus corpus = MakeCorpus(); const std::vector norms = EncodeNorms(corpus); diff --git a/regression-test/data/inverted_index_p0/storage_format/test_storage_format_snii_norms.out b/regression-test/data/inverted_index_p0/storage_format/test_storage_format_snii_norms.out index b81a6cebfb9863..5f52500503f9cc 100644 --- a/regression-test/data/inverted_index_p0/storage_format/test_storage_format_snii_norms.out +++ b/regression-test/data/inverted_index_p0/storage_format/test_storage_format_snii_norms.out @@ -7,23 +7,19 @@ 1 0.311 2 0.5235 --- !snii_field_pattern_no_norms_score -- -1 0.47 -2 0.47 +-- !snii_field_pattern_no_norms_match -- +1 +2 --- !snii_whole_column_no_norms_score -- -1 0.47 -2 0.47 +-- !snii_whole_column_no_norms_match -- +1 +2 -- !snii_config_plain_column_score -- 1 0.3541 2 0.562 --- !snii_config_field_pattern_score -- -1 0.47 -2 0.47 - --- !snii_config_norms_true_score -- -1 0.47 -2 0.47 +-- !snii_mixed_match -- +1 +2 diff --git a/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out b/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out index e1747721be6b9f..9b2cf745293f8c 100644 --- a/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out +++ b/regression-test/data/inverted_index_p0/test_variant_subcolumn_index_norms.out @@ -3,11 +3,15 @@ 2 0.6931 3 0.61 --- !variant_subcolumn_score_no_norms -- -2 0.6931 -3 0.6931 +-- !variant_subcolumn_match_no_norms -- +2 +3 -- !plain_column_score -- 1 0.5754 3 0.8714 +-- !mixed_match -- +1 +2 + diff --git a/regression-test/suites/inverted_index_p0/storage_format/test_storage_format_snii_norms.groovy b/regression-test/suites/inverted_index_p0/storage_format/test_storage_format_snii_norms.groovy index 8d9100998b5336..e8888a8d41b78e 100644 --- a/regression-test/suites/inverted_index_p0/storage_format/test_storage_format_snii_norms.groovy +++ b/regression-test/suites/inverted_index_p0/storage_format/test_storage_format_snii_norms.groovy @@ -17,10 +17,11 @@ // SNII follows the same norms policy as the CLucene formats: an analyzed index writes BM25 norms // unless "norms" = "false", and inverted_index_skip_norms_for_variant drops them for every index on -// a variant path whatever the property says. Norms are not visible from outside a SNII file, so -// this suite reads them off the scores: in every table, rows 1 and 2 match "alpha" once each, but -// row 1 is three tokens long and row 2 one. With norms the longer row scores lower; without norms -// both rows score the same, and neither is NaN. +// a variant path whatever the property says. BM25 scoring needs norms, so score() on an index +// without them fails, while MATCH filtering keeps working. Norms are not visible from outside a +// SNII file, so this suite reads them off the queries: with norms, rows 1 and 2 match "alpha" once +// each and the three-token row 1 scores lower than the one-token row 2; without norms, score() +// is refused. // It flips a BE config, so it must not share the cluster with other suites. suite("test_storage_format_snii_norms", "p0,nonConcurrent") { sql """ set enable_match_without_inverted_index = false """ @@ -89,16 +90,31 @@ suite("test_storage_format_snii_norms", "p0,nonConcurrent") { select id, round(score(), 4) from test_storage_format_snii_norms where cast(v["s_note"] as string) match_any "alpha" order by score() desc limit 10 """ + // "norms" = "false" drops them, on a field_pattern index and on the copies a whole-column - // index hands to its subcolumns: rows 1 and 2 score the same - order_qt_snii_field_pattern_no_norms_score """ - select id, round(score(), 4) from test_storage_format_snii_norms - where cast(v["t_note"] as string) match_any "alpha" order by score() desc limit 10 + // index hands to its subcolumns: MATCH still filters, score() is refused + order_qt_snii_field_pattern_no_norms_match """ + select id from test_storage_format_snii_norms + where cast(v["t_note"] as string) match_any "alpha" """ - order_qt_snii_whole_column_no_norms_score """ - select id, round(score(), 4) from test_storage_format_snii_norms - where cast(vn["b_note"] as string) match_any "alpha" order by score() desc limit 10 + test { + sql """ + select id, score() from test_storage_format_snii_norms + where cast(v["t_note"] as string) match_any "alpha" order by score() desc limit 10 + """ + exception "written without" + } + order_qt_snii_whole_column_no_norms_match """ + select id from test_storage_format_snii_norms + where cast(vn["b_note"] as string) match_any "alpha" """ + test { + sql """ + select id, score() from test_storage_format_snii_norms + where cast(vn["b_note"] as string) match_any "alpha" order by score() desc limit 10 + """ + exception "written without" + } // with the config on, every index on a variant path leaves norms out, even one that asks for // them, while an ordinary column index keeps them @@ -152,12 +168,63 @@ suite("test_storage_format_snii_norms", "p0,nonConcurrent") { select id, round(score(), 4) from test_storage_format_snii_norms_config where content match_any "alpha" order by score() desc limit 10 """ - order_qt_snii_config_field_pattern_score """ - select id, round(score(), 4) from test_storage_format_snii_norms_config - where cast(v["s_note"] as string) match_any "alpha" order by score() desc limit 10 + test { + sql """ + select id, score() from test_storage_format_snii_norms_config + where cast(v["s_note"] as string) match_any "alpha" order by score() desc limit 10 + """ + exception "written without" + } + test { + sql """ + select id, score() from test_storage_format_snii_norms_config + where cast(vf["c_note"] as string) match_any "alpha" order by score() desc limit 10 + """ + exception "written without" + } + + // segments with and without norms side by side, as while the config is being turned on: + // MATCH still filters, and score() is refused rather than ranking the two kinds differently + sql "DROP TABLE IF EXISTS test_storage_format_snii_norms_mixed" + sql """ + CREATE TABLE test_storage_format_snii_norms_mixed ( + id INT, + v variant< + 's_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, + INDEX idx_v_s (v) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "field_pattern"="s_*" + ) + ) ENGINE=OLAP DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ( + "replication_allocation" = "tag.location.default: 1", + "disable_auto_compaction" = "true", + "inverted_index_storage_format" = "SNII" + ) """ - order_qt_snii_config_norms_true_score """ - select id, round(score(), 4) from test_storage_format_snii_norms_config - where cast(vf["c_note"] as string) match_any "alpha" order by score() desc limit 10 + sql """ insert into test_storage_format_snii_norms_mixed values + (1, parse_to_variant('{"s_note":"alpha database server"}')) """ + setBeConfigTemporary([inverted_index_skip_norms_for_variant: true]) { + sql """ insert into test_storage_format_snii_norms_mixed values + (2, parse_to_variant('{"s_note":"alpha"}')) + """ + } + sql " sync " + + order_qt_snii_mixed_match """ + select id from test_storage_format_snii_norms_mixed + where cast(v["s_note"] as string) match_any "alpha" + """ + test { + sql """ + select id, score() from test_storage_format_snii_norms_mixed + where cast(v["s_note"] as string) match_any "alpha" order by score() desc limit 10 + """ + exception "written without" + } } diff --git a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy index d028c1bf3d7f04..5b8d65debc2ef0 100644 --- a/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy +++ b/regression-test/suites/inverted_index_p0/test_variant_subcolumn_index_norms.groovy @@ -20,7 +20,8 @@ // bytes, so inverted_index_skip_norms_for_variant leaves them out there whatever the property says. // This covers both an index declared with a field_pattern and a whole-column // index on a VARIANT column, whose per-subcolumn copies inherit the properties of the index they -// come from. BM25 scoring keeps working with and without norms. +// come from. BM25 scoring needs norms: score() on an index without them fails, also while only +// some segments lack them, and MATCH filtering keeps working. // It flips a BE config, so it must not share the cluster with other suites. suite("test_variant_subcolumn_index_norms", "p0,nonConcurrent") { if (isCloudMode()) { @@ -106,13 +107,22 @@ suite("test_variant_subcolumn_index_norms", "p0,nonConcurrent") { order by score() desc limit 10 """ - order_qt_variant_subcolumn_score_no_norms """ - select id, round(score(), 4) + // without norms MATCH still filters, and score() is refused + order_qt_variant_subcolumn_match_no_norms """ + select id from test_variant_subcolumn_index_norms where cast(v["t_note"] as string) match_phrase "alpha" - order by score() desc - limit 10 """ + test { + sql """ + select id, score() + from test_variant_subcolumn_index_norms + where cast(v["t_note"] as string) match_phrase "alpha" + order by score() desc + limit 10 + """ + exception "written without norms" + } order_qt_plain_column_score """ select id, round(score(), 4) from test_variant_subcolumn_index_norms @@ -212,4 +222,64 @@ suite("test_variant_subcolumn_index_norms", "p0,nonConcurrent") { assertEquals(false, normsOf(configNorms, "c_host")) assertEquals(true, configNorms[""]) } + test { + sql """ + select id, score() + from test_variant_subcolumn_index_norms_config + where cast(vf["c_host"] as string) match_phrase "alpha" + order by score() desc + limit 10 + """ + exception "written without norms" + } + + // segments with and without norms side by side, as while the config is being turned on: + // MATCH still filters, and score() is refused rather than ranking the rows of the newer + // segment as zero-length documents + sql "DROP TABLE IF EXISTS test_variant_subcolumn_index_norms_mixed" + sql """ + CREATE TABLE test_variant_subcolumn_index_norms_mixed ( + id INT, + v variant< + 's_*' : text, + PROPERTIES("variant_max_subcolumns_count"="0") + >, + INDEX idx_v_s (v) USING INVERTED PROPERTIES( + "parser"="english", + "support_phrase"="true", + "field_pattern"="s_*" + ) + ) ENGINE=OLAP DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ( + "replication_allocation" = "tag.location.default: 1", + "disable_auto_compaction" = "true", + "inverted_index_storage_format" = "V2" + ) + """ + sql """ insert into test_variant_subcolumn_index_norms_mixed values + (1, parse_to_variant('{"s_note":"alpha database server"}')) + """ + setBeConfigTemporary([inverted_index_skip_norms_for_variant: true]) { + sql """ insert into test_variant_subcolumn_index_norms_mixed values + (2, parse_to_variant('{"s_note":"alpha"}')) + """ + } + sql " sync " + + order_qt_mixed_match """ + select id + from test_variant_subcolumn_index_norms_mixed + where cast(v["s_note"] as string) match_phrase "alpha" + """ + test { + sql """ + select id, score() + from test_variant_subcolumn_index_norms_mixed + where cast(v["s_note"] as string) match_phrase "alpha" + order by score() desc + limit 10 + """ + exception "written without norms" + } }