airborne12 commented on code in PR #67134: URL: https://github.com/apache/doris/pull/67134#discussion_r3869979711
########## be/test/storage/index/snii/snii_plain_index_scoring_test.cpp: ########## @@ -0,0 +1,350 @@ +// 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 scoring must not depend on CommonGrams. +// +// CommonGrams is a phrase-query performance optimization. It needs a SEMANTIC +// view of the collection statistics because its physical postings hold gram +// tokens, so sum_total_term_freq and per-document length are not the numbers +// BM25 wants. That semantic view was introduced inside the CommonGrams segment +// metadata, and the scoring gate was written as "does this segment carry +// CommonGrams metadata" -- which made an ordinary analyzed index unscoreable. +// V1/V2/V3 score the same index (see regression test_bm25_score.groovy) and +// even score with norms omitted (test_omit_norms.groovy), so SNII was the +// outlier. These cases pin the aligned behaviour. + +#include <gtest/gtest.h> + +#include <memory> +#include <set> +#include <string> +#include <vector> + +#include "core/block/block.h" +#include "core/column/column_array.h" +#include "core/column/column_nullable.h" +#include "core/column/column_string.h" +#include "core/data_type/data_type_array.h" +#include "core/data_type/data_type_nullable.h" +#include "core/data_type/data_type_string.h" +#include "io/fs/local_file_system.h" +#include "storage/index/index_file_reader.h" +#include "storage/index/index_file_writer.h" +#include "storage/index/inverted/inverted_index_desc.h" +#include "storage/index/inverted/inverted_index_writer.h" +#include "storage/index/snii/query/bm25_scorer.h" +#include "storage/index/snii/stats/snii_stats_provider.h" +#include "storage/iterator/olap_data_convertor.h" +#include "storage/olap_common.h" +#include "storage/tablet/tablet_schema.h" + +namespace doris { + +using segment_v2::IndexColumnWriter; +using segment_v2::IndexFileReader; +using segment_v2::IndexFileWriter; +using segment_v2::InvertedIndexDescriptor; + +namespace { + +constexpr const char* kTestDir = "./ut_dir/snii_plain_index_scoring_test"; +constexpr int64_t kIndexId = 9101; + +// One scalar STRING column, nullable, mirroring an ordinary text table. +TabletSchemaSPtr scalar_schema() { + auto schema = std::make_shared<TabletSchema>(); + TabletSchemaPB pb; + pb.set_keys_type(DUP_KEYS); + schema->init_from_pb(pb); + TabletColumn col; + col.set_name("body"); + col.set_type(FieldType::OLAP_FIELD_TYPE_STRING); + col.set_length(INT_MAX); + col.set_is_nullable(true); + schema->append_column(col); + return schema; +} + +// ARRAY<STRING>. CommonGrams rejects ARRAY outright, so an array text column +// could never reach the scoring tier while scoring rode on CommonGrams. +TabletSchemaSPtr array_schema() { + auto schema = std::make_shared<TabletSchema>(); + TabletSchemaPB pb; + pb.set_keys_type(DUP_KEYS); + schema->init_from_pb(pb); + TabletColumn array; + array.set_name("body"); + array.set_type(FieldType::OLAP_FIELD_TYPE_ARRAY); + array.set_length(0); + array.set_index_length(0); + array.set_is_nullable(true); + TabletColumn child; + child.set_name("body_item"); + child.set_type(FieldType::OLAP_FIELD_TYPE_STRING); + child.set_length(INT_MAX); + array.add_sub_column(child); + schema->append_column(array); + return schema; +} + +// An ordinary built-in-parser index. No custom analyzer, no CommonGrams. +TabletIndex plain_index_meta(bool support_phrase) { + TabletIndexPB pb; + pb.set_index_type(IndexType::INVERTED); + pb.set_index_id(kIndexId); + pb.set_index_name("plain_idx"); + pb.add_col_unique_id(0); + (*pb.mutable_properties())["parser"] = "english"; + (*pb.mutable_properties())["lower_case"] = "true"; + (*pb.mutable_properties())["support_phrase"] = support_phrase ? "true" : "false"; + TabletIndex meta; + meta.init_from_pb(pb); + return meta; +} + +std::string prefix_for(std::string_view rowset_id) { + return std::string(InvertedIndexDescriptor::get_index_file_path_prefix( + local_segment_path(kTestDir, rowset_id, 0))); +} + +std::unique_ptr<IndexFileWriter> open_writer(const std::string& prefix, + std::string_view rowset_id) { + io::FileWriterPtr file_writer; + EXPECT_TRUE(io::global_local_filesystem() + ->create_file(InvertedIndexDescriptor::get_index_file_path_v2(prefix), + &file_writer) + .ok()); + return std::make_unique<IndexFileWriter>( + io::global_local_filesystem(), prefix, std::string(rowset_id), 0, + InvertedIndexStorageFormatPB::SNII, std::move(file_writer)); +} + +// Writes `docs` through the production scalar path and returns the index prefix. +// A row listed in `null_rows` is written as SQL NULL, so a caller can interleave +// null runs with data runs. +std::string write_scalar_segment(std::string_view rowset_id, const TabletIndex& meta, + const std::vector<std::string>& docs, + const std::set<size_t>& null_rows = {}) { + const std::string prefix = prefix_for(rowset_id); + auto inner = ColumnString::create(); + auto null_map = ColumnUInt8::create(); + for (size_t row = 0; row < docs.size(); ++row) { + const auto& doc = docs[row]; + inner->insert_data(doc.data(), doc.size()); + null_map->insert_value(null_rows.contains(row) ? 1 : 0); + } + ColumnPtr column = ColumnNullable::create(std::move(inner), std::move(null_map)); + Block block; + block.insert({column, std::make_shared<DataTypeNullable>(std::make_shared<DataTypeString>()), + "body"}); + + TabletSchemaSPtr schema = scalar_schema(); + auto index_file_writer = open_writer(prefix, rowset_id); + std::unique_ptr<IndexColumnWriter> builder; + EXPECT_TRUE( + IndexColumnWriter::create(&schema->column(0), &builder, index_file_writer.get(), &meta) + .ok()); + + OlapBlockDataConvertor convertor(schema.get(), {0}); + convertor.set_source_content(&block, 0, block.rows()); + auto [status, accessor] = convertor.convert_column_data(0); + EXPECT_TRUE(status.ok()) << status; + // Mirrors ColumnWriter::append_nullable: null runs go to add_nulls(), data + // runs to add_values(). Both must advance the norms vector by their length. + const auto* row_null_map = accessor->get_nullmap(); + const auto* data = reinterpret_cast<const uint8_t*>(accessor->get_data()); + size_t offset = 0; + while (offset < block.rows()) { + const bool is_null = row_null_map != nullptr && row_null_map[offset] != 0; + size_t run = 1; + while (offset + run < block.rows() && + ((row_null_map != nullptr && row_null_map[offset + run] != 0) == is_null)) { + ++run; + } + if (is_null) { + EXPECT_TRUE(builder->add_nulls(static_cast<uint32_t>(run)).ok()); + } else { + EXPECT_TRUE(builder->add_values("body", data + offset * sizeof(Slice), run).ok()); + } + offset += run; + } + EXPECT_TRUE(builder->finish().ok()); + EXPECT_TRUE(index_file_writer->begin_close().ok()); + EXPECT_TRUE(index_file_writer->finish_close().ok()); + return prefix; +} + +class SniiPlainIndexScoring : public testing::Test { +protected: + void SetUp() override { + ASSERT_TRUE(io::global_local_filesystem()->delete_directory(kTestDir).ok()); + ASSERT_TRUE(io::global_local_filesystem()->create_directory(kTestDir).ok()); + } +}; + +} // namespace + +// The whole point: an ordinary analyzed SNII index carries scoring data. +TEST_F(SniiPlainIndexScoring, PlainAnalyzedIndexOpensTheScoringStatsProvider) { + const TabletIndex meta = plain_index_meta(/*support_phrase=*/true); + // Four documents, 24 tokens total -> avgdl 6. + const std::string prefix = write_scalar_segment( + "plain_rs", meta, + {"alpha beta gamma delta epsilon zeta", "alpha beta gamma delta epsilon zeta", + "alpha beta gamma delta epsilon zeta", "alpha beta gamma delta epsilon zeta"}); + + IndexFileReader reader(io::global_local_filesystem(), prefix, + InvertedIndexStorageFormatPB::SNII); + ASSERT_TRUE(reader.init().ok()); + auto logical = reader.open_snii_index(&meta); + ASSERT_TRUE(logical.has_value()) << logical.error(); + + doris::snii::stats::SniiStatsProvider stats; + const Status status = + doris::snii::stats::SniiStatsProvider::open(logical.value().get(), &stats); + ASSERT_TRUE(status.ok()) << status; + + EXPECT_TRUE(stats.has_norms()) << "an analyzed index must persist per-document norms"; + EXPECT_DOUBLE_EQ(stats.avgdl(), 6.0); + uint64_t df = 0; + ASSERT_TRUE(stats.doc_freq("alpha", &df).ok()); + EXPECT_EQ(df, 4U); + + uint8_t norm = 0; + ASSERT_TRUE(stats.encoded_norm(0, &norm).ok()); + EXPECT_EQ(norm, 6U) << "the norm must encode the document's token count"; +} + +// The riskiest invariant this change touches: norms are per ROW, and a null run +// goes through add_nulls() rather than the token path. One missed push and the +// vector desyncs -- every later document would be scored with a neighbour's length. +TEST_F(SniiPlainIndexScoring, NullRunsKeepOneNormPerDocument) { + const TabletIndex meta = plain_index_meta(/*support_phrase=*/true); + // 6 rows: data, NULL, NULL, data, NULL, data -- runs on both sides. + const std::string prefix = write_scalar_segment( + "nulls_rs", meta, + {"alpha beta gamma", "", "", "alpha beta", "", "alpha beta gamma delta"}, + /*null_rows=*/ {1, 2, 4}); + + IndexFileReader reader(io::global_local_filesystem(), prefix, + InvertedIndexStorageFormatPB::SNII); + ASSERT_TRUE(reader.init().ok()); + auto logical = reader.open_snii_index(&meta); + ASSERT_TRUE(logical.has_value()) << logical.error(); + + doris::snii::stats::SniiStatsProvider stats; + const Status status = + doris::snii::stats::SniiStatsProvider::open(logical.value().get(), &stats); + ASSERT_TRUE(status.ok()) << status; + EXPECT_TRUE(stats.has_norms()); + + const std::vector<uint64_t> expected {3, 0, 0, 2, 0, 4}; + for (uint32_t docid = 0; docid < expected.size(); ++docid) { + uint8_t norm = 0; + ASSERT_TRUE(stats.encoded_norm(docid, &norm).ok()) << "docid " << docid; + EXPECT_EQ(norm, doris::snii::query::encode_norm(expected[docid])) + << "norm desynced at docid " << docid; + } + uint8_t past_end = 0; + EXPECT_FALSE(stats.encoded_norm(static_cast<uint32_t>(expected.size()), &past_end).ok()) + << "the norms vector outlives the document count"; + + // avgdl divides by ALL rows, the same rows the norms span: 9 tokens / 6 docs. + EXPECT_EQ(logical.value()->stats().doc_count, 6); + EXPECT_EQ(logical.value()->stats().sum_total_term_freq, 9); + EXPECT_DOUBLE_EQ(stats.avgdl(), 1.5); +} + +// CommonGrams rejects ARRAY fields, so arrays were unscoreable on SNII while +// V1/V2/V3 scored them. +TEST_F(SniiPlainIndexScoring, PlainAnalyzedArrayIndexOpensTheScoringStatsProvider) { + const TabletIndex meta = plain_index_meta(/*support_phrase=*/true); + const std::string prefix = prefix_for("array_rs"); + + DataTypePtr item = std::make_shared<DataTypeNullable>(std::make_shared<DataTypeString>()); + DataTypePtr array_type = std::make_shared<DataTypeArray>(item); + MutableColumnPtr nested = array_type->create_column(); + for (int row = 0; row < 4; ++row) { + Array value; + value.push_back(Field::create_field<TYPE_STRING>(std::string("alpha beta"))); + value.push_back(Field::create_field<TYPE_STRING>(std::string("gamma delta"))); + nested->insert(Field::create_field<TYPE_ARRAY>(value)); + } + auto null_map = ColumnUInt8::create(); + for (int row = 0; row < 4; ++row) { + null_map->insert_value(0); + } + ColumnPtr column = ColumnNullable::create(std::move(nested), std::move(null_map)); + Block block; + block.insert({column, std::make_shared<DataTypeNullable>(array_type), "body"}); + + TabletSchemaSPtr schema = array_schema(); + auto index_file_writer = open_writer(prefix, "array_rs"); + std::unique_ptr<IndexColumnWriter> builder; + ASSERT_TRUE( + IndexColumnWriter::create(&schema->column(0), &builder, index_file_writer.get(), &meta) + .ok()); + + OlapBlockDataConvertor convertor(schema.get(), {0}); + convertor.set_source_content(&block, 0, block.rows()); + auto [status, accessor] = convertor.convert_column_data(0); + ASSERT_TRUE(status.ok()) << status; + const auto* data_ptr = reinterpret_cast<const uint64_t*>(accessor->get_data()); + ASSERT_TRUE( + builder->add_array_values(field_type_size(schema->column(0).get_sub_column(0).type()), Review Comment: Verified through the real IndexBuilder ADD INDEX path. With 4064 empty ARRAY rows followed by ["alpha"], the RED test stored alpha at docid 0 instead of 4064 and reported doc_count 1 instead of 4065. IndexBuilder::_add_data now forwards all-empty batches for text ARRAY indexes so SNII advances row ids and norms. The existing non-null element-buffer guard remains for numeric and ANN writers. SniiBuildIndexAdvancesAcrossAllEmptyArrayBatches is GREEN with alpha at docid 4064 and doc_count 4065. Assessment: severity 9/10, scenario confidence 10/10, production likelihood 5.5/10; normalized score 8.65/10, so this is fixed. The follow-up is currently in local commit 6ab16d07f22 and has not been pushed yet. ########## be/src/storage/index/snii/snii_index_writer.cpp: ########## @@ -171,12 +171,20 @@ Status SniiIndexColumnWriter::init() { close_on_error(); return status; } - _config = ::doris::snii::format::IndexConfig::kDocsPositionsScoring; } else if (_common_grams_metadata_seed.has_value()) { close_on_error(); return Status::Error<ErrorCode::INVERTED_INDEX_ANALYZER_ERROR>( "SNII CommonGrams metadata cannot be attached to a plain analyzer"); } + // Scoring rides on ANALYSIS, not on CommonGrams. Any analyzed index + // with positions persists per-document norms and therefore reaches + // the scoring tier -- CommonGrams only changes what the semantic + // token count means, not whether one exists. Gating this on + // _uses_common_grams is what made an ordinary SNII index + // unscoreable while V1/V2/V3 scored the same index. + if (_has_positions) { Review Comment: Verified with a production-path RED test using ArrayColumnWriter::append_nullable, not only a direct SNII writer call. An outer-NULL row retaining the nested payload "poison poison poison" produced TTF=6 instead of 3, avgdl=2 instead of 1, poison df=1 instead of 0, and a nonzero norm for the NULL document. The fix adds a combined nullable-ARRAY writer path. SNII now suppresses nested payload for an outer-NULL row while still advancing one document id, recording the null row, and appending encode_norm(0). OuterNullArrayPayloadDoesNotAffectScoring is GREEN. Assessment: severity 8.5/10, scenario confidence 10/10, production likelihood 5.5/10; normalized score 8.43/10, so this is fixed rather than deferred. The follow-up is currently in local commit 6ab16d07f22 and has not been pushed yet. ########## be/src/storage/index/snii/stats/snii_stats_provider.h: ########## @@ -71,7 +71,9 @@ class SniiStatsProvider { Status total_term_freq(std::string_view term, uint64_t* ttf) const; // 1-byte encoded doc-length norm for docid (raw byte from the norms POD). - // Out-of-range docid -> InvalidArgument; index without norms -> InvalidArgument. + // Out-of-range docid -> InvalidArgument. An index WITHOUT norms yields the Review Comment: Verified the implementation and open-time contract. encoded_norm returns InvalidArgument when norms are absent, and SniiStatsProvider::open rejects a normless scoring shape, so the new neutral-byte comment was incorrect; there is no implemented fallback to preserve. I kept the runtime behavior and corrected the header contract, plus the test-file introduction that implied normless SNII scoring was supported. Assessment: severity 2/10, scenario confidence 10/10, production likelihood 2/10; normalized score 4.80/10. This falls below the production-code threshold, so the low-risk documentation-only correction is used. The follow-up is currently in local commit 6ab16d07f22 and has not been pushed yet. ########## be/src/storage/index/snii/format/core_metadata.cpp: ########## @@ -207,13 +207,16 @@ Status decode_core_pb(const doris::snii::SniiCoreMetadataPB& input, CoreMetadata return corrupted("core metadata: scoring index requires a norms region"); } } - if (has_scoring_tier || - (out->common_grams_metadata.has_value() && - out->common_grams_metadata->scoring_coverage == ScoringCoverage::kComplete)) { + // The scoring tier no longer implies CommonGrams: an ordinary analyzed index Review Comment: Re-evaluated this against the actual production baseline rather than the unpublished Apache master baseline. The original claim remains technically reproducible against current Apache master `1197ae1fa9800b7026317e3bd2fd2f26b527053b`: that decoder validates metadata for every scoring T3 and rejects a metadata-free segment. However, that reader is not deployed. The actual production baseline is SelectDB [`branch-hotfix-selectdb-cloud-4.1.7-minimax-rows`](https://github.com/selectdb/selectdb-core/commits/branch-hotfix-selectdb-cloud-4.1.7-minimax-rows/) at `ef649d804bc5a214829474715b4a3c4a2c696cd6`, with CommonGrams disabled. Its reader accepts only `IndexConfig` 0/1, while this PR writes plain scoring as config 2, so it rejects the new rowset before scoring metadata can affect compatibility. The latest-master rebase restores the shipped protobuf field numbers, but that does not remove this decisive index-config boundary. Persisting plain metadata therefore still cannot make a new rowset readable by the actual production reader. Revised assessment under this production premise: - Severity if the stated scenario existed: **9/10** - Confidence that the comment scenario matches the real rollout: **1/10** - Probability of encountering that scenario in production: **0/10** - Normalized priority: **4.40/10** (`0.45×9 + 0.35×1 + 0.20×0`), below the fix threshold The proposed metadata/fingerprint/compaction identity work is therefore not a low-cost compatibility correction. I removed the plain scoring metadata, builtin analyzer fingerprint, plain-T3 compaction identity changes, and the test that used the wrong old-reader baseline. CommonGrams-disabled plain scoring remains metadata-free, and the original SNII compaction behavior is unchanged. If a future mixed-version rollout from `ef649d8` is required, it needs a separate format/capability strategy, such as a writer gate, old-reader backport, or format-version bump, plus a cross-version golden test. Plain metadata alone is insufficient. The amended follow-up is commit `6ab16d07f22`, rebased onto current master and pushed to this PR. Verification after rebase and rollback: 52 focused tests pass; clang-format/check-format and the ASAN BE build pass. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
