From 575d490af61054add5061259af21f62c4b7c0583 Mon Sep 17 00:00:00 2001 From: Spade A <71589810+SpadeA-Tang@users.noreply.github.com> Date: Tue, 9 Sep 2025 19:05:56 +0800 Subject: [PATCH] fix: ngram index is mistakenly used for unsopported operations 2 (#44142) issue: https://github.com/milvus-io/milvus/issues/44020 https://github.com/milvus-io/milvus/pull/43955 only fixed unary expression This fixes all expressions and add more tests. --------- Signed-off-by: SpadeA-Tang Signed-off-by: SpadeA --- .../expression/BinaryArithOpEvalRangeExpr.h | 2 +- .../src/exec/expression/BinaryRangeExpr.cpp | 9 +- .../core/src/exec/expression/ExistsExpr.cpp | 2 +- internal/core/src/exec/expression/Expr.h | 31 +- .../src/exec/expression/JsonContainsExpr.cpp | 4 +- .../core/src/exec/expression/NullExpr.cpp | 8 +- .../core/src/exec/expression/TermExpr.cpp | 4 +- .../core/src/exec/expression/UnaryExpr.cpp | 25 +- internal/core/src/exec/expression/UnaryExpr.h | 5 +- .../core/src/index/NgramInvertedIndexTest.cpp | 741 ++++++++++++++++++ .../core/unittest/test_utils/GenExprProto.h | 11 + 11 files changed, 799 insertions(+), 43 deletions(-) diff --git a/internal/core/src/exec/expression/BinaryArithOpEvalRangeExpr.h b/internal/core/src/exec/expression/BinaryArithOpEvalRangeExpr.h index 999b3cfc23..4d661a88a8 100644 --- a/internal/core/src/exec/expression/BinaryArithOpEvalRangeExpr.h +++ b/internal/core/src/exec/expression/BinaryArithOpEvalRangeExpr.h @@ -515,7 +515,7 @@ class PhyBinaryArithOpEvalRangeExpr : public SegmentExpr { template bool CanUseIndex() { - if (is_index_mode_ && IndexHasRawData()) { + if (SegmentExpr::CanUseIndex() && IndexHasRawData()) { use_index_ = true; return true; } diff --git a/internal/core/src/exec/expression/BinaryRangeExpr.cpp b/internal/core/src/exec/expression/BinaryRangeExpr.cpp index 4a8e1bc8e3..1c226e6925 100644 --- a/internal/core/src/exec/expression/BinaryRangeExpr.cpp +++ b/internal/core/src/exec/expression/BinaryRangeExpr.cpp @@ -68,7 +68,7 @@ PhyBinaryRangeFilterExpr::Eval(EvalCtx& context, VectorPtr& result) { } case DataType::JSON: { auto value_type = expr_->lower_val_.val_case(); - if (is_index_mode_ && !has_offset_input_) { + if (SegmentExpr::CanUseIndex() && !has_offset_input_) { switch (value_type) { case proto::plan::GenericValue::ValCase::kInt64Val: { proto::plan::GenericValue double_lower_val; @@ -163,7 +163,7 @@ PhyBinaryRangeFilterExpr::Eval(EvalCtx& context, VectorPtr& result) { template VectorPtr PhyBinaryRangeFilterExpr::ExecRangeVisitorImpl(EvalCtx& context) { - if (is_index_mode_ && !has_offset_input_) { + if (SegmentExpr::CanUseIndex() && !has_offset_input_) { return ExecRangeVisitorImplForIndex(); } else { return ExecRangeVisitorImplForData(context); @@ -200,8 +200,9 @@ PhyBinaryRangeFilterExpr::PreCheckOverflow(HighPrecisionType& val1, } auto valid_res = (input != nullptr) - ? ProcessChunksForValidByOffsets(is_index_mode_, *input) - : ProcessChunksForValid(is_index_mode_); + ? ProcessChunksForValidByOffsets(SegmentExpr::CanUseIndex(), + *input) + : ProcessChunksForValid(SegmentExpr::CanUseIndex()); auto res_vec = std::make_shared(TargetBitmap(batch_size), std::move(valid_res)); diff --git a/internal/core/src/exec/expression/ExistsExpr.cpp b/internal/core/src/exec/expression/ExistsExpr.cpp index 59e8be36ff..26f82f4510 100644 --- a/internal/core/src/exec/expression/ExistsExpr.cpp +++ b/internal/core/src/exec/expression/ExistsExpr.cpp @@ -32,7 +32,7 @@ PhyExistsFilterExpr::Eval(EvalCtx& context, VectorPtr& result) { SetHasOffsetInput((input != nullptr)); switch (expr_->column_.data_type_) { case DataType::JSON: { - if (is_index_mode_ && !has_offset_input_) { + if (SegmentExpr::CanUseIndex() && !has_offset_input_) { result = EvalJsonExistsForIndex(); } else { result = EvalJsonExistsForDataSegment(context); diff --git a/internal/core/src/exec/expression/Expr.h b/internal/core/src/exec/expression/Expr.h index 40582a565c..f44b047361 100644 --- a/internal/core/src/exec/expression/Expr.h +++ b/internal/core/src/exec/expression/Expr.h @@ -182,7 +182,6 @@ class SegmentExpr : public Expr { allow_any_json_cast_type_, is_json_contains_); if (pinned_index_.size() > 0) { - is_index_mode_ = true; num_index_chunk_ = pinned_index_.size(); } // if index not include raw data, also need load data @@ -276,7 +275,9 @@ class SegmentExpr : public Expr { MoveCursor() override { // when we specify input, do not maintain states if (!has_offset_input_) { - if (is_index_mode_) { + // CanUseIndex excludes ngram index and this is true even ngram index is used as ExecNgramMatch + // uses data cursor. + if (SegmentExpr::CanUseIndex()) { MoveCursorForIndex(); if (segment_->HasFieldData(field_id_)) { MoveCursorForData(); @@ -303,15 +304,16 @@ class SegmentExpr : public Expr { int64_t GetNextBatchSize() { - auto current_chunk = is_index_mode_ && use_index_ ? current_index_chunk_ - : current_data_chunk_; - auto current_chunk_pos = is_index_mode_ && use_index_ + auto current_chunk = SegmentExpr::CanUseIndex() && use_index_ + ? current_index_chunk_ + : current_data_chunk_; + auto current_chunk_pos = SegmentExpr::CanUseIndex() && use_index_ ? current_index_chunk_pos_ : current_data_chunk_pos_; auto current_rows = 0; if (segment_->is_chunked()) { current_rows = - is_index_mode_ && use_index_ && + SegmentExpr::CanUseIndex() && use_index_ && segment_->type() == SegmentType::Sealed ? current_chunk_pos : segment_->num_rows_until_chunk(field_id_, current_chunk) + @@ -482,7 +484,7 @@ class SegmentExpr : public Expr { int64_t processed_size = 0; // index reverse lookup - if (is_index_mode_ && num_data_chunk_ == 0) { + if (SegmentExpr::CanUseIndex() && num_data_chunk_ == 0) { return ProcessIndexLookupByOffsets( func, skip_func, input, res, valid_res, values...); } @@ -1218,9 +1220,16 @@ class SegmentExpr : public Expr { } } + bool + CanUseIndex() const { + // Ngram index should be used in specific execution path (CanUseNgramIndex -> ExecNgramMatch). + // TODO: if multiple indexes are supported, this logic should be changed + return num_index_chunk_ != 0 && !CanUseNgramIndex(); + } + template bool - CanUseIndex(OpType op) const { + CanUseIndexForOp(OpType op) const { typedef std:: conditional_t, std::string, T> IndexInnerType; @@ -1288,6 +1297,11 @@ class SegmentExpr : public Expr { return PlanUseJsonStats(context) && HasJsonStats(field_id); } + virtual bool + CanUseNgramIndex() const { + return false; + }; + protected: const segcore::SegmentInternalInterface* segment_; const FieldId field_id_; @@ -1300,7 +1314,6 @@ class SegmentExpr : public Expr { DataType value_type_; bool allow_any_json_cast_type_{false}; bool is_json_contains_{false}; - bool is_index_mode_{false}; bool is_data_mode_{false}; // sometimes need to skip index and using raw data // default true means use index as much as possible diff --git a/internal/core/src/exec/expression/JsonContainsExpr.cpp b/internal/core/src/exec/expression/JsonContainsExpr.cpp index b841038428..5245cd45fe 100644 --- a/internal/core/src/exec/expression/JsonContainsExpr.cpp +++ b/internal/core/src/exec/expression/JsonContainsExpr.cpp @@ -48,7 +48,7 @@ PhyJsonContainsFilterExpr::Eval(EvalCtx& context, VectorPtr& result) { switch (expr_->column_.data_type_) { case DataType::ARRAY: { - if (is_index_mode_ && !has_offset_input_) { + if (SegmentExpr::CanUseIndex() && !has_offset_input_) { result = EvalArrayContainsForIndexSegment( expr_->column_.element_type_); } else { @@ -57,7 +57,7 @@ PhyJsonContainsFilterExpr::Eval(EvalCtx& context, VectorPtr& result) { break; } case DataType::JSON: { - if (is_index_mode_ && !has_offset_input_) { + if (SegmentExpr::CanUseIndex() && !has_offset_input_) { result = EvalArrayContainsForIndexSegment( value_type_ == DataType::INT64 ? DataType::DOUBLE : value_type_); diff --git a/internal/core/src/exec/expression/NullExpr.cpp b/internal/core/src/exec/expression/NullExpr.cpp index 42a95a3de3..8a47c93a39 100644 --- a/internal/core/src/exec/expression/NullExpr.cpp +++ b/internal/core/src/exec/expression/NullExpr.cpp @@ -88,10 +88,10 @@ PhyNullExpr::ExecVisitorImpl(OffsetVector* input) { if (auto res = PreCheckNullable(input)) { return res; } - auto valid_res = - (input != nullptr) - ? ProcessChunksForValidByOffsets(is_index_mode_, *input) - : ProcessChunksForValid(is_index_mode_); + auto valid_res = (input != nullptr) + ? ProcessChunksForValidByOffsets( + SegmentExpr::CanUseIndex(), *input) + : ProcessChunksForValid(SegmentExpr::CanUseIndex()); TargetBitmap res = valid_res.clone(); if (expr_->op_ == proto::plan::NullExpr_NullOp_IsNull) { res.flip(); diff --git a/internal/core/src/exec/expression/TermExpr.cpp b/internal/core/src/exec/expression/TermExpr.cpp index f5d40209cf..488bb40ee1 100644 --- a/internal/core/src/exec/expression/TermExpr.cpp +++ b/internal/core/src/exec/expression/TermExpr.cpp @@ -224,7 +224,7 @@ PhyTermFilterExpr::ExecVisitorImplTemplateJson(EvalCtx& context) { if (expr_->is_in_field_) { return ExecTermJsonVariableInField(context); } else { - if (is_index_mode_ && !has_offset_input_) { + if (SegmentExpr::CanUseIndex() && !has_offset_input_) { // we create double index for json int64 field for now using GetType = std::conditional_t, @@ -814,7 +814,7 @@ PhyTermFilterExpr::ExecTermJsonFieldInVariable(EvalCtx& context) { template VectorPtr PhyTermFilterExpr::ExecVisitorImpl(EvalCtx& context) { - if (is_index_mode_ && !has_offset_input_) { + if (SegmentExpr::CanUseIndex() && !has_offset_input_) { return ExecVisitorImplForIndex(); } else { return ExecVisitorImplForData(context); diff --git a/internal/core/src/exec/expression/UnaryExpr.cpp b/internal/core/src/exec/expression/UnaryExpr.cpp index b6481f7f46..888864dda7 100644 --- a/internal/core/src/exec/expression/UnaryExpr.cpp +++ b/internal/core/src/exec/expression/UnaryExpr.cpp @@ -53,7 +53,7 @@ template <> bool PhyUnaryRangeFilterExpr::CanUseIndexForArray() { bool res; - if (!is_index_mode_) { + if (!SegmentExpr::CanUseIndex()) { use_index_ = res = false; return res; } @@ -200,7 +200,7 @@ PhyUnaryRangeFilterExpr::Eval(EvalCtx& context, VectorPtr& result) { case DataType::JSON: { auto val_type = expr_->val_.val_case(); auto val_type_inner = FromValCase(val_type); - if (CanExecNgramMatchForJson() && !has_offset_input_) { + if (CanUseNgramIndex() && !has_offset_input_) { auto res = ExecNgramMatch(); // If nullopt is returned, it means the query cannot be // optimized by ngram index. Forward it to the normal path. @@ -1248,7 +1248,7 @@ PhyUnaryRangeFilterExpr::ExecRangeVisitorImpl(EvalCtx& context) { fmt::format("match query does not support iterative filter")); } return ExecTextMatch(); - } else if (CanExecNgramMatch()) { + } else if (CanUseNgramIndex()) { auto res = ExecNgramMatch(); // If nullopt is returned, it means the query cannot be // optimized by ngram index. Forward it to the normal path. @@ -1441,8 +1441,9 @@ PhyUnaryRangeFilterExpr::PreCheckOverflow(OffsetVector* input) { } auto valid = (input != nullptr) - ? ProcessChunksForValidByOffsets(is_index_mode_, *input) - : ProcessChunksForValid(is_index_mode_); + ? ProcessChunksForValidByOffsets( + SegmentExpr::CanUseIndex(), *input) + : ProcessChunksForValid(SegmentExpr::CanUseIndex()); auto res_vec = std::make_shared( TargetBitmap(batch_size), std::move(valid)); TargetBitmapView res(res_vec->GetRawData(), batch_size); @@ -1699,11 +1700,8 @@ PhyUnaryRangeFilterExpr::ExecRangeVisitorImplForData(EvalCtx& context) { template bool PhyUnaryRangeFilterExpr::CanUseIndex() { - use_index_ = - is_index_mode_ && SegmentExpr::CanUseIndex(expr_->op_type_) && - // Ngram index should be used in specific execution path (CanExecNgramMatch -> ExecNgramMatch). - // TODO: if multiple indexes are supported, this logic should be changed - pinned_ngram_index_.get() == nullptr; + use_index_ = SegmentExpr::CanUseIndex() && + SegmentExpr::CanUseIndexForOp(expr_->op_type_); return use_index_; } @@ -1827,12 +1825,7 @@ PhyUnaryRangeFilterExpr::ExecTextMatch() { }; bool -PhyUnaryRangeFilterExpr::CanExecNgramMatch() { - return pinned_ngram_index_.get() != nullptr && !has_offset_input_; -} - -bool -PhyUnaryRangeFilterExpr::CanExecNgramMatchForJson() { +PhyUnaryRangeFilterExpr::CanUseNgramIndex() const { return pinned_ngram_index_.get() != nullptr && !has_offset_input_; } diff --git a/internal/core/src/exec/expression/UnaryExpr.h b/internal/core/src/exec/expression/UnaryExpr.h index e5360038f5..46a9529d1c 100644 --- a/internal/core/src/exec/expression/UnaryExpr.h +++ b/internal/core/src/exec/expression/UnaryExpr.h @@ -878,10 +878,7 @@ class PhyUnaryRangeFilterExpr : public SegmentExpr { ExecTextMatch(); bool - CanExecNgramMatch(); - - bool - CanExecNgramMatchForJson(); + CanUseNgramIndex() const override; std::optional ExecNgramMatch(); diff --git a/internal/core/src/index/NgramInvertedIndexTest.cpp b/internal/core/src/index/NgramInvertedIndexTest.cpp index 61dfd7252d..04279fbc40 100644 --- a/internal/core/src/index/NgramInvertedIndexTest.cpp +++ b/internal/core/src/index/NgramInvertedIndexTest.cpp @@ -343,6 +343,413 @@ TEST(NgramIndex, TestNgramSimple) { std::vector(10000, true)); } +// Test that ngram index should only be used for like operations +// (Match, InnerMatch, PrefixMatch, PostfixMatch) +// and NOT for other operations (Equal, NotEqual, In, NotIn, etc.) +// Issue: https://github.com/milvus-io/milvus/issues/44020 +TEST(NgramIndex, TestNonLikeExpressionsWithNgram) { + boost::container::vector data = {"apple", + "banana", + "cherry", + "date", + "elderberry", + "fig", + "grape", + "honeydew", + "kiwi", + "lemon"}; + + int64_t collection_id = 1; + int64_t partition_id = 2; + int64_t segment_id = 3; + int64_t index_build_id = 4000; + int64_t index_version = 4000; + int64_t index_id = 5000; + + auto schema = std::make_shared(); + auto field_id = schema->AddDebugField("ngram", DataType::VARCHAR); + + auto field_meta = milvus::segcore::gen_field_meta(collection_id, + partition_id, + segment_id, + field_id.get(), + DataType::VARCHAR, + DataType::NONE, + false); + auto index_meta = gen_index_meta( + segment_id, field_id.get(), index_build_id, index_version); + + std::string root_path = "/tmp/test-inverted-index/"; + auto storage_config = gen_local_storage_config(root_path); + auto cm = CreateChunkManager(storage_config); + + size_t nb = data.size(); + + auto field_data = + storage::CreateFieldData(DataType::VARCHAR, DataType::NONE, false); + field_data->FillFieldData(data.data(), data.size()); + + auto segment = CreateSealedSegment(schema); + auto field_data_info = PrepareSingleFieldInsertBinlog(collection_id, + partition_id, + segment_id, + field_id.get(), + {field_data}, + cm); + segment->LoadFieldData(field_data_info); + + auto payload_reader = + std::make_shared(field_data); + storage::InsertData insert_data(payload_reader); + insert_data.SetFieldDataMeta(field_meta); + insert_data.SetTimestamps(0, 100); + + auto serialized_bytes = insert_data.Serialize(storage::Remote); + + auto get_binlog_path = [=](int64_t log_id) { + return fmt::format("{}/{}/{}/{}/{}", + collection_id, + partition_id, + segment_id, + field_id.get(), + log_id); + }; + + auto log_path = get_binlog_path(0); + + auto cm_w = ChunkManagerWrapper(cm); + cm_w.Write(log_path, serialized_bytes.data(), serialized_bytes.size()); + + storage::FileManagerContext ctx(field_meta, index_meta, cm); + std::vector index_files; + + // Build ngram index + { + Config config; + config[milvus::index::INDEX_TYPE] = milvus::index::INVERTED_INDEX_TYPE; + config[INSERT_FILES_KEY] = std::vector{log_path}; + + auto ngram_params = index::NgramParams{ + .loading_index = false, + .min_gram = 2, + .max_gram = 4, + }; + auto index = + std::make_shared(ctx, ngram_params); + index->Build(config); + + auto create_index_result = index->Upload(); + index_files = create_index_result->GetIndexFiles(); + } + + // Load index and test + { + std::map index_params{ + {milvus::index::INDEX_TYPE, milvus::index::NGRAM_INDEX_TYPE}, + {milvus::index::MIN_GRAM, "2"}, + {milvus::index::MAX_GRAM, "4"}, + {milvus::LOAD_PRIORITY, "HIGH"}, + }; + milvus::segcore::LoadIndexInfo load_index_info{ + .collection_id = collection_id, + .partition_id = partition_id, + .segment_id = segment_id, + .field_id = field_id.get(), + .field_type = DataType::VARCHAR, + .enable_mmap = true, + .mmap_dir_path = "/tmp/test-ngram-index-mmap-dir", + .index_id = index_id, + .index_build_id = index_build_id, + .index_version = index_version, + .index_params = index_params, + .index_files = index_files, + .schema = field_meta.field_schema, + .index_size = 1024 * 1024 * 1024, + }; + + uint8_t trace_id[16] = {0}; + uint8_t span_id[8] = {0}; + trace_id[0] = 1; + span_id[0] = 2; + CTraceContext trace{ + .traceID = trace_id, + .spanID = span_id, + .traceFlags = 0, + }; + auto cload_index_info = static_cast(&load_index_info); + AppendIndexV2(trace, cload_index_info); + UpdateSealedSegmentIndex(segment.get(), cload_index_info); + + // Test: TermFilterExpr (IN operator) + { + std::vector values; + proto::plan::GenericValue val1; + val1.set_string_val("apple"); + values.push_back(val1); + proto::plan::GenericValue val2; + val2.set_string_val("banana"); + values.push_back(val2); + proto::plan::GenericValue val3; + val3.set_string_val("cherry"); + values.push_back(val3); + + auto term_expr = std::make_shared( + milvus::expr::ColumnInfo(field_id, DataType::VARCHAR), values); + auto plan = std::make_shared( + DEFAULT_PLANNODE_ID, term_expr); + + BitsetType final = + ExecuteQueryExpr(plan, segment.get(), nb, MAX_TIMESTAMP); + // Only apple, banana, cherry should match + for (size_t i = 0; i < nb; i++) { + if (i < 3) { + ASSERT_TRUE(final[i]) << "Expected true at index " << i; + } else { + ASSERT_FALSE(final[i]) << "Expected false at index " << i; + } + } + } + + // Test: UnaryRangeExpr with Equal operator + { + auto unary_range_expr = + test::GenUnaryRangeExpr(proto::plan::OpType::Equal, "apple"); + auto column_info = test::GenColumnInfo( + field_id.get(), proto::schema::DataType::VarChar, false, false); + unary_range_expr->set_allocated_column_info(column_info); + auto expr = test::GenExpr(); + expr->set_allocated_unary_range_expr(unary_range_expr); + auto parser = ProtoParser(schema); + auto typed_expr = parser.ParseExprs(*expr); + auto parsed = std::make_shared( + DEFAULT_PLANNODE_ID, typed_expr); + BitsetType final = + ExecuteQueryExpr(parsed, segment.get(), nb, MAX_TIMESTAMP); + // Only apple should match (exact match) + for (size_t i = 0; i < nb; i++) { + if (i == 0) { + ASSERT_TRUE(final[i]) << "Expected true at index " << i; + } else { + ASSERT_FALSE(final[i]) << "Expected false at index " << i; + } + } + } + + // Test: BinaryRangeFilterExpr + { + proto::plan::GenericValue lower_val; + lower_val.set_string_val("cherry"); + proto::plan::GenericValue upper_val; + upper_val.set_string_val("grape"); + + auto binary_range_expr = + std::make_shared( + milvus::expr::ColumnInfo(field_id, DataType::VARCHAR), + lower_val, + upper_val, + true, + true); + auto plan = std::make_shared( + DEFAULT_PLANNODE_ID, binary_range_expr); + + BitsetType final = + ExecuteQueryExpr(plan, segment.get(), nb, MAX_TIMESTAMP); + // Strings between "cherry" and "grape" inclusive: cherry, date, elderberry, fig, grape + for (size_t i = 0; i < nb; i++) { + if (i >= 2 && i <= 6) { + ASSERT_TRUE(final[i]) << "Expected true at index " << i; + } else { + ASSERT_FALSE(final[i]) << "Expected false at index " << i; + } + } + } + + // Test: LogicalBinaryExpr with AND + { + // Create Equal expression + auto unary_range_expr1 = + test::GenUnaryRangeExpr(proto::plan::OpType::Equal, "apple"); + auto column_info1 = test::GenColumnInfo( + field_id.get(), proto::schema::DataType::VarChar, false, false); + unary_range_expr1->set_allocated_column_info(column_info1); + auto expr1 = test::GenExpr(); + expr1->set_allocated_unary_range_expr(unary_range_expr1); + auto parser1 = ProtoParser(schema); + auto typed_expr1 = parser1.ParseExprs(*expr1); + + // Create NotEqual expression + auto unary_range_expr2 = test::GenUnaryRangeExpr( + proto::plan::OpType::NotEqual, "banana"); + auto column_info2 = test::GenColumnInfo( + field_id.get(), proto::schema::DataType::VarChar, false, false); + unary_range_expr2->set_allocated_column_info(column_info2); + auto expr2 = test::GenExpr(); + expr2->set_allocated_unary_range_expr(unary_range_expr2); + auto parser2 = ProtoParser(schema); + auto typed_expr2 = parser2.ParseExprs(*expr2); + + // Create LogicalBinaryExpr with AND + auto logical_and_expr = + std::make_shared( + milvus::expr::LogicalBinaryExpr::OpType::And, + typed_expr1, + typed_expr2); + auto plan = std::make_shared( + DEFAULT_PLANNODE_ID, logical_and_expr); + + BitsetType final = + ExecuteQueryExpr(plan, segment.get(), nb, MAX_TIMESTAMP); + // Only apple should match (apple == "apple" AND apple != "banana") + for (size_t i = 0; i < nb; i++) { + if (i == 0) { + ASSERT_TRUE(final[i]) << "Expected true at index " << i; + } else { + ASSERT_FALSE(final[i]) << "Expected false at index " << i; + } + } + } + + // Test: LogicalUnaryExpr with NOT + { + // Create Equal expression + auto unary_range_expr = + test::GenUnaryRangeExpr(proto::plan::OpType::Equal, "apple"); + auto column_info = test::GenColumnInfo( + field_id.get(), proto::schema::DataType::VarChar, false, false); + unary_range_expr->set_allocated_column_info(column_info); + auto expr = test::GenExpr(); + expr->set_allocated_unary_range_expr(unary_range_expr); + auto parser = ProtoParser(schema); + auto typed_expr = parser.ParseExprs(*expr); + + // Create LogicalUnaryExpr with NOT + auto logical_not_expr = + std::make_shared( + milvus::expr::LogicalUnaryExpr::OpType::LogicalNot, + typed_expr); + auto plan = std::make_shared( + DEFAULT_PLANNODE_ID, logical_not_expr); + + BitsetType final = + ExecuteQueryExpr(plan, segment.get(), nb, MAX_TIMESTAMP); + // All except apple should match (NOT (field == "apple")) + for (size_t i = 0; i < nb; i++) { + if (i != 0) { + ASSERT_TRUE(final[i]) << "Expected true at index " << i; + } else { + ASSERT_FALSE(final[i]) << "Expected false at index " << i; + } + } + } + + // Test: LogicalBinaryExpr with OR + { + // Create Equal expression + auto unary_range_expr1 = + test::GenUnaryRangeExpr(proto::plan::OpType::Equal, "apple"); + auto column_info1 = test::GenColumnInfo( + field_id.get(), proto::schema::DataType::VarChar, false, false); + unary_range_expr1->set_allocated_column_info(column_info1); + auto expr1 = test::GenExpr(); + expr1->set_allocated_unary_range_expr(unary_range_expr1); + auto parser1 = ProtoParser(schema); + auto typed_expr1 = parser1.ParseExprs(*expr1); + + // Create Equal expression for "banana" + auto unary_range_expr2 = + test::GenUnaryRangeExpr(proto::plan::OpType::Equal, "banana"); + auto column_info2 = test::GenColumnInfo( + field_id.get(), proto::schema::DataType::VarChar, false, false); + unary_range_expr2->set_allocated_column_info(column_info2); + auto expr2 = test::GenExpr(); + expr2->set_allocated_unary_range_expr(unary_range_expr2); + auto parser2 = ProtoParser(schema); + auto typed_expr2 = parser2.ParseExprs(*expr2); + + // Create LogicalBinaryExpr with OR + auto logical_or_expr = + std::make_shared( + milvus::expr::LogicalBinaryExpr::OpType::Or, + typed_expr1, + typed_expr2); + auto plan = std::make_shared( + DEFAULT_PLANNODE_ID, logical_or_expr); + + BitsetType final = + ExecuteQueryExpr(plan, segment.get(), nb, MAX_TIMESTAMP); + // Apple and banana should match (apple == "apple" OR field == "banana") + for (size_t i = 0; i < nb; i++) { + if (i == 0 || i == 1) { + ASSERT_TRUE(final[i]) << "Expected true at index " << i; + } else { + ASSERT_FALSE(final[i]) << "Expected false at index " << i; + } + } + } + + // Test: NullExpr with IS_NULL + { + auto null_expr = std::make_shared( + milvus::expr::ColumnInfo(field_id, DataType::VARCHAR), + proto::plan::NullExpr_NullOp_IsNull); + auto plan = std::make_shared( + DEFAULT_PLANNODE_ID, null_expr); + + BitsetType final = + ExecuteQueryExpr(plan, segment.get(), nb, MAX_TIMESTAMP); + // None should match since we have no null values + for (size_t i = 0; i < nb; i++) { + ASSERT_FALSE(final[i]) << "Expected false at index " << i; + } + } + + // Test: NullExpr with IS_NOT_NULL + { + auto null_expr = std::make_shared( + milvus::expr::ColumnInfo(field_id, DataType::VARCHAR), + proto::plan::NullExpr_NullOp_IsNotNull); + auto plan = std::make_shared( + DEFAULT_PLANNODE_ID, null_expr); + + BitsetType final = + ExecuteQueryExpr(plan, segment.get(), nb, MAX_TIMESTAMP); + // All should match since we have no null values + for (size_t i = 0; i < nb; i++) { + ASSERT_TRUE(final[i]) << "Expected true at index " << i; + } + } + + // // Test: ExistsExpr + // { + // auto exists_expr = std::make_shared( + // milvus::expr::ColumnInfo(field_id, DataType::VARCHAR)); + // auto plan = std::make_shared( + // DEFAULT_PLANNODE_ID, exists_expr); + + // BitsetType final = ExecuteQueryExpr(plan, segment.get(), nb, MAX_TIMESTAMP); + // // All should match since the field exists for all rows + // for (size_t i = 0; i < nb; i++) { + // ASSERT_TRUE(final[i]) << "Expected true at index " << i; + // } + // } + + // Test: AlwaysTrueExpr + { + auto always_true_expr = + std::make_shared(); + auto plan = std::make_shared( + DEFAULT_PLANNODE_ID, always_true_expr); + + BitsetType final = + ExecuteQueryExpr(plan, segment.get(), nb, MAX_TIMESTAMP); + // All should match + for (size_t i = 0; i < nb; i++) { + ASSERT_TRUE(final[i]) << "Expected true at index " << i; + } + } + } +} + TEST(NgramIndex, TestNgramJson) { std::vector json_raw_data = { R"(1)", @@ -479,4 +886,338 @@ TEST(NgramIndex, TestNgramJson) { EXPECT_TRUE(result[id]); } } +} + +// Test that ngram index should only be used for like operations on JSON fields +// and NOT for other operations (Equal, NotEqual, In, etc.) +TEST(NgramIndex, TestJsonNonLikeExpressionsWithNgram) { + std::vector json_raw_data = {R"({"name": "apple"})", + R"({"name": "banana"})", + R"({"name": "cherry"})", + R"({"name": "date"})", + R"({"name": "elderberry"})", + R"({"name": "fig"})", + R"({"name": "grape"})", + R"({"name": "honeydew"})", + R"({"name": "kiwi"})", + R"({"name": "lemon"})"}; + + auto json_path = "/name"; + auto schema = std::make_shared(); + auto json_fid = schema->AddDebugField("json", DataType::JSON); + + auto file_manager_ctx = storage::FileManagerContext(); + file_manager_ctx.fieldDataMeta.field_schema.set_data_type( + milvus::proto::schema::JSON); + file_manager_ctx.fieldDataMeta.field_schema.set_fieldid(json_fid.get()); + file_manager_ctx.fieldDataMeta.field_id = json_fid.get(); + + index::CreateIndexInfo create_index_info{ + .index_type = index::INVERTED_INDEX_TYPE, + .json_cast_type = JsonCastType::FromString("VARCHAR"), + .json_path = json_path, + .ngram_params = std::optional{index::NgramParams{ + .loading_index = false, + .min_gram = 2, + .max_gram = 4, + }}, + }; + auto inv_index = index::IndexFactory::GetInstance().CreateJsonIndex( + create_index_info, file_manager_ctx); + + auto ngram_index = std::unique_ptr( + static_cast(inv_index.release())); + + std::vector jsons; + for (auto& json : json_raw_data) { + jsons.push_back(milvus::Json(simdjson::padded_string(json))); + } + + auto json_field = + std::make_shared>(DataType::JSON, false); + json_field->add_json_data(jsons); + ngram_index->BuildWithFieldData({json_field}); + ngram_index->finish(); + ngram_index->create_reader(milvus::index::SetBitsetSealed); + + auto segment = segcore::CreateSealedSegment(schema); + segcore::LoadIndexInfo load_index_info; + load_index_info.field_id = json_fid.get(); + load_index_info.field_type = DataType::JSON; + load_index_info.cache_index = + CreateTestCacheIndex("", std::move(ngram_index)); + + std::map index_params{ + {milvus::index::INDEX_TYPE, milvus::index::NGRAM_INDEX_TYPE}, + {milvus::index::MIN_GRAM, "2"}, + {milvus::index::MAX_GRAM, "4"}, + {milvus::LOAD_PRIORITY, "HIGH"}, + {JSON_PATH, json_path}, + {JSON_CAST_TYPE, "VARCHAR"}}; + load_index_info.index_params = index_params; + + segment->LoadIndex(load_index_info); + + auto cm = milvus::storage::RemoteChunkManagerSingleton::GetInstance() + .GetRemoteChunkManager(); + auto load_info = PrepareSingleFieldInsertBinlog( + 0, 0, 0, json_fid.get(), {json_field}, cm); + segment->LoadFieldData(load_info); + + size_t nb = json_raw_data.size(); + + // Test: JSON Equal operation + { + proto::plan::GenericValue value; + value.set_string_val("apple"); + auto expr = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true), + proto::plan::OpType::Equal, + value, + std::vector{}); + + auto plan = + std::make_shared(DEFAULT_PLANNODE_ID, expr); + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // Only first record should match (exact match for "apple") + EXPECT_EQ(result.count(), 1); + EXPECT_TRUE(result[0]); + } + + // Test: JSON NotEqual operation + { + proto::plan::GenericValue value; + value.set_string_val("apple"); + auto expr = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true), + proto::plan::OpType::NotEqual, + value, + std::vector{}); + + auto plan = + std::make_shared(DEFAULT_PLANNODE_ID, expr); + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // All except first record should match + EXPECT_EQ(result.count(), 9); + EXPECT_FALSE(result[0]); + for (size_t i = 1; i < nb; i++) { + EXPECT_TRUE(result[i]); + } + } + + // Test: JSON GreaterThan operation + { + proto::plan::GenericValue value; + value.set_string_val("fig"); + auto expr = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true), + proto::plan::OpType::GreaterThan, + value, + std::vector{}); + + auto plan = + std::make_shared(DEFAULT_PLANNODE_ID, expr); + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // Records with names > "fig": grape, honeydew, kiwi, lemon + EXPECT_EQ(result.count(), 4); + for (size_t i = 6; i < nb; i++) { + EXPECT_TRUE(result[i]); + } + } + + // Test: JSON LessThan operation + { + proto::plan::GenericValue value; + value.set_string_val("date"); + auto expr = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true), + proto::plan::OpType::LessThan, + value, + std::vector{}); + + auto plan = + std::make_shared(DEFAULT_PLANNODE_ID, expr); + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // Records with names < "date": apple, banana, cherry + EXPECT_EQ(result.count(), 3); + for (size_t i = 0; i < 3; i++) { + EXPECT_TRUE(result[i]); + } + } + + // Test: JSON TermFilterExpr (IN operation) + { + std::vector values; + proto::plan::GenericValue val1, val2, val3; + val1.set_string_val("apple"); + val2.set_string_val("cherry"); + val3.set_string_val("grape"); + values.push_back(val1); + values.push_back(val2); + values.push_back(val3); + + auto term_expr = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true), + values); + auto plan = std::make_shared(DEFAULT_PLANNODE_ID, + term_expr); + + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // Only apple, cherry, grape should match + EXPECT_EQ(result.count(), 3); + EXPECT_TRUE(result[0]); // apple + EXPECT_TRUE(result[2]); // cherry + EXPECT_TRUE(result[6]); // grape + } + + // Test: JSON BinaryRangeFilterExpr + { + proto::plan::GenericValue lower_val; + lower_val.set_string_val("cherry"); + proto::plan::GenericValue upper_val; + upper_val.set_string_val("grape"); + + auto binary_range_expr = + std::make_shared( + milvus::expr::ColumnInfo( + json_fid, DataType::JSON, {"name"}, true), + lower_val, + upper_val, + true, + true); + auto plan = std::make_shared(DEFAULT_PLANNODE_ID, + binary_range_expr); + + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // Strings between "cherry" and "grape" inclusive: cherry, date, elderberry, fig, grape + EXPECT_EQ(result.count(), 5); + for (size_t i = 2; i <= 6; i++) { + EXPECT_TRUE(result[i]); + } + } + + // Test: JSON NullExpr IS_NULL + { + auto null_expr = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true), + proto::plan::NullExpr_NullOp_IsNull); + auto plan = std::make_shared(DEFAULT_PLANNODE_ID, + null_expr); + + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // None should match since all have non-null names + EXPECT_EQ(result.count(), 0); + } + + // Test: JSON NullExpr IS_NOT_NULL + { + auto null_expr = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true), + proto::plan::NullExpr_NullOp_IsNotNull); + auto plan = std::make_shared(DEFAULT_PLANNODE_ID, + null_expr); + + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // All should match since all have non-null names + EXPECT_EQ(result.count(), 10); + for (size_t i = 0; i < nb; i++) { + EXPECT_TRUE(result[i]); + } + } + + // Test: JSON ExistsExpr + { + auto exists_expr = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true)); + auto plan = std::make_shared(DEFAULT_PLANNODE_ID, + exists_expr); + + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // All should match since all have the "name" field + EXPECT_EQ(result.count(), 10); + for (size_t i = 0; i < nb; i++) { + EXPECT_TRUE(result[i]); + } + } + + // Test: JSON LogicalBinaryExpr with AND + { + // Create Equal expression for "apple" + proto::plan::GenericValue val1; + val1.set_string_val("apple"); + auto expr1 = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true), + proto::plan::OpType::Equal, + val1, + std::vector{}); + + // Create NotEqual expression for "banana" + proto::plan::GenericValue val2; + val2.set_string_val("banana"); + auto expr2 = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true), + proto::plan::OpType::NotEqual, + val2, + std::vector{}); + + // Create LogicalBinaryExpr with AND + auto logical_and_expr = + std::make_shared( + milvus::expr::LogicalBinaryExpr::OpType::And, expr1, expr2); + auto plan = std::make_shared(DEFAULT_PLANNODE_ID, + logical_and_expr); + + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // Only apple should match (name == "apple" AND name != "banana") + EXPECT_EQ(result.count(), 1); + EXPECT_TRUE(result[0]); + } + + // Test: JSON LogicalUnaryExpr with NOT + { + proto::plan::GenericValue value; + value.set_string_val("apple"); + auto equal_expr = std::make_shared( + milvus::expr::ColumnInfo(json_fid, DataType::JSON, {"name"}, true), + proto::plan::OpType::Equal, + value, + std::vector{}); + + // Create LogicalUnaryExpr with NOT + auto logical_not_expr = + std::make_shared( + milvus::expr::LogicalUnaryExpr::OpType::LogicalNot, equal_expr); + auto plan = std::make_shared(DEFAULT_PLANNODE_ID, + logical_not_expr); + + auto result = milvus::query::ExecuteQueryExpr( + plan, segment.get(), nb, MAX_TIMESTAMP); + + // All except apple should match (NOT (name == "apple")) + EXPECT_EQ(result.count(), 9); + EXPECT_FALSE(result[0]); + for (size_t i = 1; i < nb; i++) { + EXPECT_TRUE(result[i]); + } + } } \ No newline at end of file diff --git a/internal/core/unittest/test_utils/GenExprProto.h b/internal/core/unittest/test_utils/GenExprProto.h index c337f570be..a2c3a16a1a 100644 --- a/internal/core/unittest/test_utils/GenExprProto.h +++ b/internal/core/unittest/test_utils/GenExprProto.h @@ -12,6 +12,7 @@ #pragma once #include +#include #include #include "common/Consts.h" @@ -21,6 +22,10 @@ #include "plan/PlanNode.h" namespace milvus::test { + +template +inline constexpr bool always_false = false; + inline auto GenColumnInfo( int64_t field_id, @@ -51,6 +56,12 @@ GenGenericValue(T value) { generic->set_float_val(static_cast(value)); } else if constexpr (std::is_same_v) { generic->set_string_val(static_cast(value)); + } else if constexpr (std::is_same_v || + std::is_same_v, const char*> || + (std::is_array_v && + std::is_same_v, + const char>)) { + generic->set_string_val(std::string(value)); } else { static_assert(always_false); }