github-actions[bot] commented on code in PR #61200: URL: https://github.com/apache/doris/pull/61200#discussion_r2923834113
########## regression-test/suites/search/test_search_not_null_bitmap.groovy: ########## @@ -0,0 +1,222 @@ +// 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. + +suite("test_search_not_null_bitmap", "p0") { + // Regression test for DORIS-24681: + // search('NOT field:value') was incorrectly including NULL rows because + // ExcludeScorer did not handle null bitmaps. The fix enhances ExcludeScorer + // with null bitmap awareness while keeping lazy seek-based exclusion. + // ExcludeScorer now implements SQL three-valued logic: NOT(NULL) = NULL, + // so NULL rows are excluded from the result set. + + def tableName = "search_not_null_bitmap" + Review Comment: **[Minor]** Per regression test standards in AGENTS.md: "For ordinary single test tables, do not use `def tableName` form; instead hardcode your table name in all SQL." This applies to `tableName`, `allNullTable`, and `mixedTable` — all three should be hardcoded directly in the SQL statements rather than using `def` variables. ########## be/src/storage/index/inverted/query_v2/boolean_query/occur_boolean_weight.cpp: ########## @@ -21,19 +21,27 @@ #include "storage/index/inverted/query_v2/disjunction_scorer.h" #include "storage/index/inverted/query_v2/exclude_scorer.h" #include "storage/index/inverted/query_v2/intersection.h" +#include "storage/index/inverted/query_v2/intersection_scorer.h" #include "storage/index/inverted/query_v2/reqopt_scorer.h" #include "storage/index/inverted/query_v2/union/buffered_union.h" namespace doris::segment_v2::inverted_index::query_v2 { template <typename ScoreCombinerPtrT> OccurBooleanWeight<ScoreCombinerPtrT>::OccurBooleanWeight( - std::vector<std::pair<Occur, WeightPtr>> sub_weights, size_t minimum_number_should_match, - bool enable_scoring, ScoreCombinerPtrT score_combiner) + std::vector<std::pair<Occur, WeightPtr>> sub_weights, std::vector<std::string> binding_keys, + size_t minimum_number_should_match, bool enable_scoring, ScoreCombinerPtrT score_combiner) : _sub_weights(std::move(sub_weights)), + _binding_keys(std::move(binding_keys)), _minimum_number_should_match(minimum_number_should_match), _enable_scoring(enable_scoring), - _score_combiner(std::move(score_combiner)) {} + _score_combiner(std::move(score_combiner)) { + DCHECK(_binding_keys.empty() || _binding_keys.size() == _sub_weights.size()) + << "binding_keys size (" << _binding_keys.size() << ") must match sub_weights size (" Review Comment: **[Minor]** Per AGENTS.md coding standards: "Any `if` check for errors must have a clearly known inevitable failure path... you may use the `DORIS_CHECK` macro for precondition assertions." `DCHECK` disappears in RELEASE builds, leaving no invariant protection in production. Since this is a precondition assertion (binding_keys size must match sub_weights size), it should use `DORIS_CHECK` instead: ```cpp DORIS_CHECK(_binding_keys.empty() || _binding_keys.size() == _sub_weights.size()) << "binding_keys size (" << _binding_keys.size() << ") must match sub_weights size (" << _sub_weights.size() << ") when non-empty"; ``` Note: The subsequent `_binding_keys.resize(_sub_weights.size())` makes this safe in practice (it will silently pad even if the sizes mismatch), so the DCHECK is effectively dead in RELEASE builds where size mismatches would silently produce wrong binding_key associations rather than crashing. `DORIS_CHECK` would catch this. -- 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]
