eldenmoon commented on code in PR #69761:
URL: https://github.com/apache/doris/pull/69761#discussion_r4500359230
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/IndexDefinition.java:
##########
@@ -562,6 +575,66 @@ public Map<String, String> getProperties() {
return properties;
}
+ public boolean isVariantValuesIndex() {
+ return indexType == IndexType.INVERTED &&
InvertedIndexProperties.isVariantValuesIndex(properties);
+ }
+
+ private void validateVariantValuesIndex(DataType columnType,
+ TInvertedIndexFileStorageFormat invertedIndexFileStorageFormat) {
+ String scope =
properties.get(InvertedIndexUtil.VARIANT_INDEX_SCOPE_KEY);
+ if (scope == null) {
+ return;
+ }
+ if (!isVariantValuesIndex()) {
+ throw new AnalysisException("variant_index_scope must be values,
found: " + scope);
+ }
+ if (!columnType.isVariantType()) {
+ throw new AnalysisException(
+ "variant_index_scope=values can only be used on VARIANT
columns");
+ }
+ // Declared Decimal256 can be stored, but full-root assembly cannot
decode it.
+ // Reject this combination before accepting a table whose legal
inserts would fail.
+ VariantType variantType = (VariantType) columnType;
+ if (variantType.getEnableNestedGroup()) {
+ throw new AnalysisException("VARIANT values index does not support
NestedGroup");
+ }
+ if (variantType.getPredefinedFields().stream()
+ .anyMatch(field -> containsDecimal256(field.getDataType()))) {
+ throw new AnalysisException("VARIANT values index does not support
declared Decimal256 paths");
+ }
+ if (invertedIndexFileStorageFormat !=
TInvertedIndexFileStorageFormat.SNII) {
Review Comment:
Fixed in 3b12c163494 by a BE rule instead of per-path FE checks: a values
index is stored only in SNII index files. A segment whose index file is V2/V3
is written without the values index (`VariantV2ColumnWriter::init`), and
reading it registers no values-index reader (`ColumnReader::_load_index`), so
MATCH evaluates rows, exactly as for segments written before a new-data-only
ADD. A RECOVERed or RESTOREd legacy partition, including the cloud path where
MS copies the backup's tablet meta, therefore writes and reads correctly
without the index; it no longer reaches the SNII writer. The FE lifecycle
checks from 4000def679a are reverted. `variant_values_index_test` covers
V2/V3/SNII writes, reads and full compaction.
##########
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:
`doris_be_test` is compiled with `-fno-access-control`
(be/test/CMakeLists.txt), so the test compiled and ran. The test is replaced in
31a977fedb8 and asserts through segment writes and reads instead.
##########
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:
`checkValuesIndexCompatibility()` is removed in 3b12c163494 together with
the rest of 4000def679a's RESTORE changes; RESTORE is back to the base behavior
and its signature check.
##########
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:
`checkValuesIndexCompatibility()` is removed in 3b12c163494 together with
the rest of 4000def679a's RESTORE changes; RESTORE is back to the base behavior
and its signature check.
--
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]
