github-actions[bot] commented on code in PR #69761:
URL: https://github.com/apache/doris/pull/69761#discussion_r4499318267
##########
fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java:
##########
@@ -776,6 +779,16 @@ private void checkAndPrepareMeta() {
}
}
+ // The historical signature omits index properties. A
path index with the
+ // same name is not compatible with a values index,
even when IDs differ.
+ if (!isAtomicRestore) {
+ st = checkValuesIndexCompatibility(localOlapTbl,
remoteOlapTbl, tblInfo);
Review Comment:
[P1] Do not run values-index compatibility checks during FORCE schema
replacement
When table signatures differ, `isForceReplace` sets `isSchemaChanged=true`
at line 766 so the remote schema can replace the local one. The new block at
lines 784-789 still calls `checkValuesIndexCompatibility()` before that
replacement. If the local table has a values index absent from the backup,
`remoteIndexes.get(...)` is null and line 1452 throws
`Preconditions.checkNotNull` instead of returning a normal status; a same-name
property difference is also rejected even though the local schema will be
discarded. Skip this compatibility check for intentional schema replacement (or
make it conditional on matching schemas) and add a FORCE restore test.
##########
fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java:
##########
@@ -1419,12 +1438,60 @@ private boolean
genFileMappingWhenBackupReplicasEqual(PartitionInfo localPartInf
return false;
}
+ protected Status checkValuesIndexCompatibility(OlapTable localTbl,
OlapTable remoteTbl,
+ BackupOlapTableInfo tableInfo) {
+ boolean hasValuesIndex =
localTbl.getIndexes().stream().anyMatch(Index::isVariantValuesIndex)
+ ||
remoteTbl.getIndexes().stream().anyMatch(Index::isVariantValuesIndex);
+ if (!hasValuesIndex) {
+ return Status.OK;
+ }
+ Map<String, Index> remoteIndexes = remoteTbl.getIndexesMap();
+ for (Index index : localTbl.getIndexes()) {
+ Index remote = remoteIndexes.get(index.getIndexName());
+ // The historical signature already compares index names, types
and columns.
+ Preconditions.checkNotNull(remote);
+ if (!Objects.equals(index.getProperties(),
remote.getProperties())) {
Review Comment:
[P2] Keep ordinary path-index restore compatibility
`checkValuesIndexCompatibility()` is enabled when either table has a values
index, but lines 1449-1453 then require identical properties for every local
index. A table with the same values index plus an ordinary path index can
therefore fail restore solely because the ordinary path index changed
parser/analyzer properties, even though `OlapTable.getSignature()`
intentionally ignores those properties and the same mismatch remains accepted
for ordinary-index-only tables. Restrict the strict comparison to values-index
entries and add a mixed-index restore case.
##########
be/test/storage/index/inverted/snii/variant_values_index_test.cpp:
##########
@@ -0,0 +1,319 @@
+// 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.
+
+#include <gtest/gtest.h>
+
+#include <algorithm>
+#include <map>
+#include <set>
+#include <sstream>
+#include <string>
+#include <vector>
+
+#include "core/assert_cast.h"
+#include "core/column/column_nullable.h"
+#include "core/column/variant_v2/column_variant_v2.h"
+#include "core/data_type/data_type_nullable.h"
+#include "core/data_type/data_type_string.h"
+#include "core/data_type/data_type_variant_v2.h"
+#include "core/value/variant/variant_leaf_visitor.h"
+#include "exprs/function/match.h"
+#include "exprs/function/parse/variant_string_parse.h"
+#include "io/fs/local_file_system.h"
+#include "io/io_common.h"
+#include "runtime/runtime_state.h"
+#include "storage/index/index_file_reader.h"
+#include "storage/index/index_file_writer.h"
+#include "storage/index/index_query_context.h"
+#include "storage/index/inverted/analysis/analyzer/analyzer.h"
+#include "storage/index/inverted/common/inverted_index_desc.h"
+#include "storage/index/inverted/snii/snii_index_reader.h"
+#include "storage/index/inverted/snii/snii_query_oracle.h"
+#include "storage/segment/variant/v2/variant_values_index_writer.h"
+#include "testutil/index_storage_test_util.h"
+
+namespace doris::segment_v2 {
+namespace {
+
+TEST(VariantValuesIndexTest, ScopeIsExactAndCaseInsensitive) {
+ for (const auto& [scope, expected] : {std::pair {"values", true},
+ {"VALUES", true},
+ {" values ", false},
+ {"paths", false},
+ {"", false}}) {
+ TabletIndexPB pb;
+ pb.set_index_type(IndexType::INVERTED);
+ (*pb.mutable_properties())["variant_index_scope"] = scope;
+ TabletIndex index;
+ index.init_from_pb(pb);
+ EXPECT_EQ(index.is_variant_values_index(), expected) << scope;
+ pb.set_index_type(IndexType::NGRAM_BF);
+ index.init_from_pb(pb);
+ EXPECT_FALSE(index.is_variant_values_index());
+ }
+}
+
+ColumnVariantV2::MutablePtr encode_documents(const std::vector<std::string>&
docs) {
+ JsonStringToVariantEncoder encoder;
+ for (const auto& json : docs) {
+ encoder.add_json({json.data(), json.size()});
+ }
+ auto column = ColumnVariantV2::create();
+ column->insert_encoded_batch(encoder.finish_batch());
+ return column;
+}
+
+class VariantValuesSegmentTest : public
index_storage_test::IndexStorageTestFixture {
+protected:
+ void check_layout(bool doc, int buckets, int limit, int64_t id);
+ void check_match(const ColumnPtr& values, const TabletIndex* meta,
+ const std::shared_ptr<IndexFileReader>& file);
+};
+
+TEST_F(VariantValuesSegmentTest, InitRequiresSniiContainerBeforeOpeningIndex) {
+ TabletIndexPB pb;
+ pb.set_index_id(72);
+ pb.set_index_type(IndexType::INVERTED);
+ (*pb.mutable_properties())["variant_index_scope"] = "values";
+ (*pb.mutable_properties())["parser"] = "english";
+ (*pb.mutable_properties())["support_phrase"] = "false";
+ TabletIndex index;
+ index.init_from_pb(pb);
+ for (auto format : {InvertedIndexStorageFormatPB::V2,
InvertedIndexStorageFormatPB::V3,
+ InvertedIndexStorageFormatPB::SNII}) {
+ IndexFileWriter file(io::global_local_filesystem(), _test_dir +
"/format_guard", "rowset",
+ 0, format);
+ VariantValuesIndexWriter writer(&file, &index, false);
+ Status status = writer.init();
+ if (format == InvertedIndexStorageFormatPB::SNII) {
+ ASSERT_TRUE(status.ok()) << status;
+ } else {
+ EXPECT_TRUE(status.is<ErrorCode::INVALID_ARGUMENT>()) << status;
+ EXPECT_EQ(writer._writer, nullptr);
Review Comment:
[P1] Fix the new SNII format-guard test's private-member access
`VariantValuesIndexWriter::_writer` is private, and the test also reads
private `IndexFileWriter::_indices_dirs` and `_snii_file_writer` at lines
108-109. There is no friend declaration, `BE_TEST` seam, or public accessor in
this test/header, so compiling the new `variant_values_index_test` target fails
with C++ private-access errors. Assert the post-init state through public
behavior/accessors or add a narrowly scoped test seam.
--
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]
