github-actions[bot] commented on code in PR #68758: URL: https://github.com/apache/doris/pull/68758#discussion_r4265873143
########## be/src/exprs/function/search/function_search.cpp: ########## @@ -0,0 +1,634 @@ +// 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 "exprs/function/search/function_search.h" + +#include <CLucene/config/repl_wchar.h> +#include <CLucene/debug/error.h> +#include <fmt/format.h> +#include <gen_cpp/Exprs_types.h> +#include <glog/logging.h> + +#include <algorithm> +#include <iterator> +#include <memory> +#include <optional> +#include <roaring/roaring.hh> +#include <string> +#include <unordered_map> +#include <utility> +#include <vector> + +#include "common/config.h" +#include "common/exception.h" +#include "common/status.h" +#include "core/block/columns_with_type_and_name.h" +#include "exprs/function/search/function_search_leaf.h" +#include "exprs/function/search/search_lowering.h" +#include "exprs/function/search/variant_inverted_index_search.h" +#include "exprs/function/simple_function_factory.h" +#include "exprs/vexpr_context.h" +#include "runtime/runtime_profile.h" +#include "storage/index/index_file_reader.h" +#include "storage/index/index_query_context.h" +#include "storage/index/inverted/analysis/analyzer/analyzer.h" +#include "storage/index/inverted/inverted_index_iterator.h" +#include "storage/index/inverted/inverted_index_reader.h" +#include "storage/index/inverted/query/engine/all_query/all_query.h" +#include "storage/index/inverted/query/engine/bit_set_query/bit_set_query.h" +#include "storage/index/inverted/query/engine/boolean_query/boolean_query_builder.h" +#include "storage/index/inverted/query/engine/boolean_query/operator.h" +#include "storage/index/inverted/query/engine/collect/doc_set_collector.h" +#include "storage/index/inverted/query/engine/collect/top_k_collector.h" +#include "storage/index/inverted/query/exec/term_pattern.h" +#include "storage/index/inverted/query/logical/node.h" +#include "storage/index/inverted/reader/collection_statistics.h" +#include "storage/olap_common.h" +#include "storage/segment/variant/nested_group_provider.h" +#include "util/thrift_util.h" + +namespace doris { + +namespace query_v2 = segment_v2::inverted_index::query_v2; + +// Serializes the whole TSearchParam, so every field of the DSL takes part in the signature, and +// adds the limit on the terms PREFIX, WILDCARD and REGEXP clauses expand to. +std::string FunctionSearch::dsl_cache_signature(const TSearchParam& search_param, + const IndexQueryContext* context) { + ThriftSerializer ser(false, 1024); + TSearchParam copy = search_param; + std::string sig; + auto st = ser.serialize(©, &sig); + if (UNLIKELY(!st.ok())) { + LOG(WARNING) << "build_dsl_signature: Thrift serialization failed: " << st.to_string() + << ", caching disabled for this query"; + return ""; + } + sig += "|max_expansions="; + sig += std::to_string( + index_query::max_expansions(context != nullptr ? context->runtime_state : nullptr)); + return sig; +} + +// Extract segment path prefix from the first available inverted index iterator. +// All fields in the same segment share the same path prefix. +static std::string extract_segment_prefix( + const std::unordered_map<std::string, IndexIterator*>& iterators) { + for (const auto& [field_name, iter] : iterators) { + auto* inv_iter = dynamic_cast<InvertedIndexIterator*>(iter); + if (!inv_iter) continue; + // Try fulltext reader first, then string type + for (auto type : + {InvertedIndexReaderType::FULLTEXT, InvertedIndexReaderType::STRING_TYPE}) { + IndexReaderType reader_type = type; + auto reader = inv_iter->get_reader(reader_type); + if (!reader) continue; + auto inv_reader = std::dynamic_pointer_cast<InvertedIndexReader>(reader); + if (!inv_reader) continue; + auto file_reader = inv_reader->get_index_file_reader(); + if (!file_reader) continue; + return file_reader->get_index_path_prefix(); + } + } + VLOG_DEBUG << "extract_segment_prefix: no suitable inverted index reader found across " + << iterators.size() << " iterators, caching disabled for this query"; + return ""; +} + +namespace { + +bool is_nested_group_search_supported() { + auto provider = segment_v2::create_nested_group_read_provider(); + return provider != nullptr && provider->should_enable_nested_group_read_path(); +} + +namespace logical = index_query::logical; + +// Lowering's view of the resolver: what index a field binds to and how that +// index analyzes values. +class SearchFieldCatalog final : public logical::FieldCatalog { +public: + SearchFieldCatalog(FieldReaderResolver& resolver, std::shared_ptr<IndexQueryContext> context) + : _resolver(resolver), _context(std::move(context)) {} + + Status resolve(const std::string& field, InvertedIndexQueryType query_type, + logical::FieldProps* out) override { + FieldReaderBinding binding; + RETURN_IF_ERROR(_resolver.resolve(field, query_type, &binding)); + if (!binding.is_bound()) { + LOG(INFO) << "search: No inverted index for field '" << field + << "' in this segment, query_type=" << static_cast<int>(query_type) + << ", returning UNKNOWN bitmap"; + *out = logical::FieldProps {}; + return Status::OK(); + } + const bool scalar = binding.inverted_reader != nullptr && + binding.inverted_reader->type() == InvertedIndexReaderType::BKD; + const bool analyzed = !scalar && inverted_index::InvertedIndexAnalyzer::should_analyzer( + binding.index_properties); + *out = logical::FieldProps {.bound = true, + .direct_index = scalar, + .analyzed = analyzed, + .binding = binding.binding_key}; + return Status::OK(); + } + + Status analyze(const logical::FieldProps& props, const std::string& value, + std::vector<logical::Token>* out) override { + const FieldReaderBinding* binding = _resolver.find_binding(props.binding); + if (binding == nullptr) { + return Status::InternalError("search: no binding '{}' to analyze with", props.binding); + } + int64_t unused_timer = 0; + SCOPED_RAW_TIMER(_context != nullptr && _context->stats != nullptr + ? &_context->stats->inverted_index_analyzer_timer + : &unused_timer); + InvertedIndexAnalyzerCtxSPtr analyzer_ctx; + RETURN_IF_ERROR(_resolver.analyzer_context_for(props.binding, &analyzer_ctx)); + return inverted_index::InvertedIndexAnalyzer::analyze(value, analyzer_ctx.get(), + binding->index_properties, out); + } + + Status normalize(const logical::FieldProps& props, const std::string& value, + std::string* out) override { + const FieldReaderBinding* binding = _resolver.find_binding(props.binding); + if (binding == nullptr) { + return Status::InternalError("search: no binding '{}' to normalize with", + props.binding); + } + try { + *out = inverted_index::InvertedIndexAnalyzer::normalize(value, + binding->index_properties); + } catch (const CLuceneError& e) { + return Status::Error<ErrorCode::INVERTED_INDEX_ANALYZER_ERROR>( + "search: normalizing '{}' failed: {}", value, e.what()); + } catch (const Exception& e) { + return Status::Error<ErrorCode::INVERTED_INDEX_ANALYZER_ERROR>( + "search: normalizing '{}' failed: {}", value, e.what()); + } + return Status::OK(); + } + +private: + FieldReaderResolver& _resolver; + std::shared_ptr<IndexQueryContext> _context; +}; + +query_v2::Occur to_query_occur(logical::Occur occur) { + switch (occur) { + case logical::Occur::kShould: + return query_v2::Occur::SHOULD; + case logical::Occur::kMustNot: + return query_v2::Occur::MUST_NOT; + case logical::Occur::kMust: + default: + return query_v2::Occur::MUST; + } +} + +Status compile_node(const logical::Node& node, const SearchLeafContext& ctx, + FieldReaderResolver& resolver, query_v2::QueryPtr* out, + std::string* binding_key); + +using Clauses = std::vector<std::pair<logical::Occur, logical::NodePtr>>; + +bool has_required_clause(const Clauses& clauses) { + return std::ranges::any_of( + clauses, [](const auto& clause) { return clause.first == logical::Occur::kMust; }); +} + +// Without scoring, optional clauses beside a required one change neither the rows nor their +// UNKNOWN state, so they are left out. +Clauses matching_clauses(const logical::Bool& boolean, bool scoring) { + if (scoring || boolean.op != logical::BoolOp::kOccur || boolean.min_should_match != 0 || + !has_required_clause(boolean.clauses)) { + return boolean.clauses; + } + Clauses kept; + std::ranges::copy_if(boolean.clauses, std::back_inserter(kept), [](const auto& clause) { + return clause.first != logical::Occur::kShould; + }); + return kept; +} + +// AND, OR and NOT ignore the per-clause occur; OCCUR keeps it and the threshold. A Boolean +// left with one clause after dropping clauses that only score is that clause. +Status compile_bool(const logical::Bool& boolean, const SearchLeafContext& ctx, + FieldReaderResolver& resolver, query_v2::QueryPtr* out) { + const Clauses clauses = matching_clauses(boolean, ctx.scoring); + if (clauses.size() == 1 && boolean.clauses.size() > 1) { + return compile_node(*clauses.front().second, ctx, resolver, out, nullptr); Review Comment: [P1] Retain the field binding when collapsing a Boolean query. Give `body` both a keyword index and a lowercase standard index, insert `body='Foo'`, and run unscored `search('body:EXACT(Foo) AND body:baz OR body:bar', '{"mode":"lucene","default_operator":"and"}')` with caching disabled. FE makes EXACT the sole MUST clause and the other terms optional, so Foo must match. Lowering resolves those optional terms too, leaving the analyzed index in `field_sources`; this shortcut then discards the EXACT leaf's binding key. TermWeight falls back to that analyzed source and searches for raw Foo instead of the keyword term, dropping the row. Propagate the retained key through compile_bool/compile_node and cover this dual-index required-leaf case. ########## be/test/storage/index/inverted/query/phrase/sloppy_phrase_matcher_test.cpp: ########## @@ -210,27 +209,23 @@ TEST(SniiSloppyPhraseMatcher, GeneratedCasesMatchV3Oracle) { plan_index.back() = plan_index.front(); positions.back() = positions.front(); } else if (iteration % 3 == 2) { - std::fill(plan_index.begin(), plan_index.end(), 0); - std::fill(positions.begin(), positions.end(), positions.front()); + std::ranges::fill(plan_index, 0); + std::ranges::fill(positions, positions.front()); } std::vector<uint32_t> offsets(clause_count); std::iota(offsets.begin(), offsets.end(), 0U); const auto spans = make_spans(positions); for (uint32_t slop : {1U, 3U, 7U}) { - SCOPED_TRACE(::testing::Message() << "iteration=" << iteration - << " clauses=" << clause_count << " slop=" << slop); SloppyPhraseMatcher unordered(plan_index, offsets, slop, false); - EXPECT_FLOAT_EQ( - unordered.match(spans, true), - v3_unordered_frequency(plan_index, positions, static_cast<int32_t>(slop))); - + mix(unordered.match(spans, true)); SloppyPhraseMatcher ordered(plan_index, offsets, slop, true); - EXPECT_FLOAT_EQ(ordered.match(spans, true), - v3_ordered_frequency(positions, static_cast<int32_t>(slop))); + mix(ordered.match(spans, true)); } } + EXPECT_EQ(count, 1536U); + EXPECT_EQ(digest, 8055439586674593017ULL); Review Comment: [P2] Make the generated corpus portable before fixing its digest. The fixed `mt19937` seed does not fix `std::uniform_int_distribution` output: [libstdc++](https://raw.githubusercontent.com/gcc-mirror/gcc/releases/gcc-12/libstdc%2B%2B-v3/include/bits/uniform_int_dist.h) and [libc++](https://raw.githubusercontent.com/llvm/llvm-project/release/18.x/libcxx/include/__random/uniform_int_distribution.h) use different bounded-draw algorithms. The supported `USE_LIBCPP` BE unit-test build already chooses 2 clauses for the first case where libstdc++ chooses 3. An arithmetic model of the current matcher and both distributions reproduces this golden on libstdc++ but yields `3665022738417980480` on libc++ for its 1,536 generated frequency values, so the assertion fails with correct matching code. The former per-case V3 oracle did not depend on identical random inputs across libraries. Check in the corpus or define the bounded draws before fixing a golden checksum. -- 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]
