This is an automated email from the ASF dual-hosted git repository.
airborne12 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new fa95a70656f [fix](be) Restore SNII proto field numbers to the shipped
layout (#67135)
fa95a70656f is described below
commit fa95a70656fefefdb63a6c0998a7536d5a77478f
Author: Jack <[email protected]>
AuthorDate: Thu Aug 27 16:06:26 2026 +0800
[fix](be) Restore SNII proto field numbers to the shipped layout (#67135)
### What problem does this PR solve?
Issue Number: None
Related PR: #66052
Problem Summary: SNII shipped before its metadata messages reached
upstream, and
the version upstreamed in #66052 had INSERTED fields in the middle of
two of
them rather than appending. protobuf identifies a field by its tag, so
the two
layouts now disagree about what a tag means, and every SNII segment
already
written decodes incorrectly under the upstream numbering:
SniiStatsPB shipped upstream (#66052)
tag 4 null_count sum_total_term_freq
tag 5 -- null_count
SniiSectionRefsPB shipped upstream (#66052)
tag 3 null_bitmap norms
tag 4 bsbf null_bitmap
tag 5 -- bsbf
Neither misread fails. SniiStatsPB tags 4 and 5 are both uint64, so a
null count
is returned as the collection token sum and feeds avgdl -- a plausible
wrong
number with no error anywhere. In SniiSectionRefsPB the null bitmap's
offset/length is taken for the norms region and bsbf is lost entirely.
This PR renumbers both messages back to the shipped layout and appends
the two
upstream additions after it: sum_total_term_freq becomes 5 and norms
becomes 5
in their respective messages. SniiCommonGramsMetadataPB needed no change
-- its
upstream fields 8-12 were appended, which is the shape every future
addition
must follow.
A new case set asserts the field numbers through protobuf reflection
rather than
through a byte digest, so the next accidental insert fails with a
message naming
the field and both tags instead of an opaque checksum mismatch.
The twelve SniiWriterGoldenBytes digests were RE-HARVESTED: field tags
and the
field-number ordering are part of the serialized Core metadata that
every SNII
segment carries, so all of them moved -- including
kGoldenKeywordDocsOnly, whose
index has neither norms nor a null bitmap. That one moving is the
evidence the
change reaches every segment shape rather than one lane.
COMPATIBILITY: this changes how existing bytes decode, deliberately.
Segments
written by an upstream build carrying #66052 are not readable afterwards
and
must be rebuilt; the format is not yet released upstream, so that
population is
development data only. Segments written by the shipped build become
readable,
which is the point.
---
.../snii/format/snii_proto_field_numbers_test.cpp | 103 +++++++++++++++++++++
.../snii/writer/snii_writer_golden_bytes_test.cpp | 17 ++--
gensrc/proto/snii.proto | 20 +++-
3 files changed, 129 insertions(+), 11 deletions(-)
diff --git
a/be/test/storage/index/snii/format/snii_proto_field_numbers_test.cpp
b/be/test/storage/index/snii/format/snii_proto_field_numbers_test.cpp
new file mode 100644
index 00000000000..5f14db77dc5
--- /dev/null
+++ b/be/test/storage/index/snii/format/snii_proto_field_numbers_test.cpp
@@ -0,0 +1,103 @@
+// 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 core metadata field numbers are ON DISK.
+//
+// protobuf identifies a field by its tag, never by its name, so renaming is
free
+// and RENUMBERING silently changes what an existing segment decodes to. The
SNII
+// format shipped before these messages reached upstream; the shipped layout is
+// the one every existing segment was written with, and it is what these cases
+// pin. Two of the fields are especially unforgiving:
+//
+// SniiStatsPB tag 4 is null_count. Upstream had inserted sum_total_term_freq
+// there. Both are uint64, so a mismatched read yields a plausible wrong
number
+// -- a null count used as a token sum, which then feeds avgdl -- with no
error
+// anywhere.
+//
+// SniiSectionRefsPB tags 3 and 4 are null_bitmap and bsbf. Upstream had
+// inserted norms at 3, which makes a reader take the null bitmap's
+// offset/length for the norms region and lose bsbf entirely.
+//
+// APPEND new fields. Never insert, never renumber, never reuse.
+
+#include <gen_cpp/snii.pb.h>
+#include <gtest/gtest.h>
+
+#include <string>
+#include <vector>
+
+namespace doris::snii {
+namespace {
+
+void expect_field_numbers(const google::protobuf::Descriptor* descriptor,
+ const std::vector<std::pair<std::string, int>>&
expected) {
+ ASSERT_NE(descriptor, nullptr);
+ for (const auto& [name, number] : expected) {
+ const auto* field = descriptor->FindFieldByName(name);
+ ASSERT_NE(field, nullptr) << descriptor->full_name() << " lost field
'" << name << "'";
+ EXPECT_EQ(field->number(), number)
+ << descriptor->full_name() << "." << name << " moved from tag
" << number << " to "
+ << field->number() << "; existing segments decode this tag as "
+ << "something else";
+ }
+}
+
+} // namespace
+
+TEST(SniiProtoFieldNumbers, StatsMatchesTheShippedLayout) {
+ expect_field_numbers(SniiStatsPB::descriptor(), {{"doc_count", 1},
+ {"indexed_doc_count", 2},
+ {"term_count", 3},
+ {"null_count", 4},
+ {"sum_total_term_freq",
5}});
+}
+
+TEST(SniiProtoFieldNumbers, SectionRefsMatchTheShippedLayout) {
+ expect_field_numbers(SniiSectionRefsPB::descriptor(), {{"dict_region", 1},
+ {"posting_region",
2},
+ {"null_bitmap", 3},
+ {"bsbf", 4},
+ {"norms", 5}});
+}
+
+TEST(SniiProtoFieldNumbers, CoreMetadataMatchesTheShippedLayout) {
+ expect_field_numbers(SniiCoreMetadataPB::descriptor(), {{"index_config",
1},
+ {"stats", 2},
+ {"section_refs",
3},
+ {"common_grams",
4},
+
{"common_grams_posting_policy", 5}});
+}
+
+// The CommonGrams block was APPENDED upstream (tags 8-12 were free in the
+// shipped layout), which is the shape every future addition must copy.
+TEST(SniiProtoFieldNumbers, CommonGramsMetadataAppendsRatherThanInserts) {
+ expect_field_numbers(SniiCommonGramsMetadataPB::descriptor(),
+ {{"plain_term_key_version", 1},
+ {"common_grams_coverage", 2},
+ {"common_grams_semantics_version", 3},
+ {"common_grams_key_version", 4},
+ {"common_grams_dictionary_identity", 5},
+ {"base_analyzer_fingerprint", 6},
+ {"common_grams_fingerprint", 7},
+ {"scoring_coverage", 8},
+ {"scoring_stats_version", 9},
+ {"norm_semantics_version", 10},
+ {"scoring_doc_count", 11},
+ {"scoring_token_count", 12}});
+}
+
+} // namespace doris::snii
diff --git
a/be/test/storage/index/snii/writer/snii_writer_golden_bytes_test.cpp
b/be/test/storage/index/snii/writer/snii_writer_golden_bytes_test.cpp
index a03e823d211..e7b19ac1c41 100644
--- a/be/test/storage/index/snii/writer/snii_writer_golden_bytes_test.cpp
+++ b/be/test/storage/index/snii/writer/snii_writer_golden_bytes_test.cpp
@@ -31,6 +31,11 @@
// If a digest changes INTENTIONALLY (format or analyzer change), re-harvest by
// running the test and copying the "actual=" value from the failure message --
// and say so loudly in the commit message.
+//
+// RE-HARVESTED when SniiStatsPB and SniiSectionRefsPB were renumbered back to
the
+// field numbers the format shipped with. Protobuf tags are part of the image,
so
+// EVERY digest moved -- including kGoldenKeywordDocsOnly, which is what tells
you
+// the change reaches all SNII segments and not just one lane.
#include <gtest/gtest.h>
@@ -303,9 +308,9 @@ private:
// Whole-image digests re-harvested for the protobuf v1 metadata layout. They
// still pin posting bytes together with every framing, directory, and metadata
// byte, so future format changes remain explicit.
-constexpr uint64_t kGoldenEnglishPhrase = 0x0adb7bf49bed5dc2ULL;
-constexpr uint64_t kGoldenUnicodePhrase = 0x2ea85ae3a736665cULL;
-constexpr uint64_t kGoldenKeywordDocsOnly = 0xcdfd89278e7a7979ULL;
+constexpr uint64_t kGoldenEnglishPhrase = 0x21fa508c4b24585eULL;
+constexpr uint64_t kGoldenUnicodePhrase = 0xf3132d01603c7613ULL;
+constexpr uint64_t kGoldenKeywordDocsOnly = 0x45e45e6f7b81c65aULL;
TEST_F(SniiWriterGoldenBytes, EnglishPhrase) {
const TabletIndex meta =
@@ -385,9 +390,9 @@ TEST_F(SniiWriterGoldenBytes,
PostingShapeMatrixCompleteImageDigest) {
"df-8192", "recut-full", "recut-tail", "docs-only",
};
constexpr std::array<uint64_t, 9> expected = {
- 0x3d00d59799c7d0adULL, 0x1ae78d4f5bcfe8b9ULL,
0x2b6f0cf4ba73bfb0ULL,
- 0xc3e82019faf77965ULL, 0x8152796902268ea2ULL,
0xe9219cd6881137a9ULL,
- 0xc7da487463843f7aULL, 0xc65096369e752f3eULL,
0x70e35f7c3b9c42a1ULL,
+ 0x5c9d83ad451240d5ULL, 0xbc57a5703b9ad0b2ULL,
0x41bbd0018cad67eeULL,
+ 0x5fd78aaa9e3d2387ULL, 0x19fb79ed197b52b1ULL,
0x53d66eb1b705907cULL,
+ 0xdae9ed00bf186842ULL, 0x72e87672c20f8cafULL,
0xc86fb412bf9c7686ULL,
};
for (size_t i = 0; i < images.size(); ++i) {
const uint64_t actual = fnv1a64(images[i].bytes);
diff --git a/gensrc/proto/snii.proto b/gensrc/proto/snii.proto
index 64a0ed4b419..e28aa0fe5cd 100644
--- a/gensrc/proto/snii.proto
+++ b/gensrc/proto/snii.proto
@@ -81,12 +81,18 @@ message SniiCoreMetadataPB {
optional uint32 common_grams_posting_policy = 5;
}
+// FIELD NUMBERS ARE ON DISK. The SNII format shipped before this message
reached
+// upstream, and those segments put null_count at 4. Upstream later inserted
+// sum_total_term_freq at 4 and pushed null_count to 5, so the two layouts
+// disagree on what tag 4 means -- and both are uint64, so a mismatched read
+// returns a plausible wrong number instead of failing. Numbering here follows
+// the shipped layout; append new fields, never insert.
message SniiStatsPB {
optional uint64 doc_count = 1;
optional uint64 indexed_doc_count = 2;
optional uint64 term_count = 3;
- optional uint64 sum_total_term_freq = 4;
- optional uint64 null_count = 5;
+ optional uint64 null_count = 4;
+ optional uint64 sum_total_term_freq = 5;
}
message SniiRegionRefPB {
@@ -94,12 +100,16 @@ message SniiRegionRefPB {
optional uint64 length = 2;
}
+// FIELD NUMBERS ARE ON DISK -- see the note on SniiStatsPB. Shipped segments
put
+// null_bitmap at 3 and bsbf at 4; upstream inserted norms at 3 and shifted
both,
+// which makes a reader take the null bitmap's offset/length for the norms
region
+// and lose bsbf entirely. Numbering here follows the shipped layout.
message SniiSectionRefsPB {
optional SniiRegionRefPB dict_region = 1;
optional SniiRegionRefPB posting_region = 2;
- optional SniiRegionRefPB norms = 3;
- optional SniiRegionRefPB null_bitmap = 4;
- optional SniiRegionRefPB bsbf = 5;
+ optional SniiRegionRefPB null_bitmap = 3;
+ optional SniiRegionRefPB bsbf = 4;
+ optional SniiRegionRefPB norms = 5;
}
message SniiCommonGramsMetadataPB {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
