airborne12 commented on code in PR #67134:
URL: https://github.com/apache/doris/pull/67134#discussion_r3869979718
##########
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 61842c50d41 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 61842c50d41 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:
Verified against the unchanged scoring metadata validator. The metadata-free
plain T3 image written by the PR failed with "SNII semantic scoring metadata is
missing", confirming the rolling-upgrade incompatibility.
Plain scoring T3 now persists complete plain semantic metadata: raw
non-internal term keys, CommonGrams coverage none, complete scoring versions,
and an immutable base-analyzer fingerprint. The new image passes the previous
reader validation. Streamed compaction also accepts a mix of legacy
metadata-free and current plain T3 sources, then writes the current metadata
while preserving norms and semantic token count. The compatibility,
eligibility, and byte-identical compaction tests are GREEN.
Assessment: severity 9/10, scenario confidence 10/10, production likelihood
7/10; normalized score 8.95/10, so this is fixed. The follow-up is currently in
local commit 61842c50d41 and has not been pushed yet.
##########
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 61842c50d41 and has not been pushed yet.
--
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]
