github-actions[bot] commented on code in PR #68051:
URL: https://github.com/apache/doris/pull/68051#discussion_r4022782762
##########
be/src/exprs/function/function_ip.h:
##########
@@ -708,6 +708,11 @@ class FunctionIsIPAddressInRange : public IFunction {
}
// apply for inverted index
std::shared_ptr<roaring::Roaring> null_bitmap =
std::make_shared<roaring::Roaring>();
+ if (iter->has_null()) {
Review Comment:
**[P2] Unwrap `has_null()` and avoid the cold double-open**
`has_null()` returns `Result<bool>`, but using it directly as the condition
tests only whether the result has a value (`expected::operator bool()` returns
`has_value`); a successful `false` still enters this branch, while an error is
silently treated as false. This standalone fetch also runs before the first BKD
lookup: on cold NULL/searcher caches it opens and closes the index directory,
then the range query opens it again even though `handle_searcher_cache()`
intentionally populates the NULL cache through its already-open directory.
Please propagate/unpack the result and acquire NULLs through or after the first
selected BKD query (for example by attaching a NULL-bitmap handle to the min
query), so the cold path uses one directory open.
##########
regression-test/suites/inverted_index_p0/test_ip_cidr_not_null.groovy:
##########
@@ -0,0 +1,63 @@
+// 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_ip_cidr_not_null", "nonConcurrent") {
+ sql "DROP TABLE IF EXISTS test_ip_cidr_not_null"
+ sql """
+ CREATE TABLE test_ip_cidr_not_null (
+ id INT NOT NULL,
+ address IPV4 NULL,
+ INDEX address_index (address) USING INVERTED
+ ) ENGINE=OLAP
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES ('replication_num' = '1')
+ """
+ sql """
+ INSERT INTO test_ip_cidr_not_null VALUES
+ (1, '192.168.1.1'),
+ (2, NULL),
+ (3, '192.168.2.1'),
+ (4, '192.168.1.255')
+ """
+
+ sql "SET debug_skip_fold_constant = true"
+ sql "SET inverted_index_skip_threshold = 0"
+ sql "SET enable_segment_limit_pushdown = true"
+
+ sql "SET enable_inverted_index_query = false"
+ qt_sql_without_inverted_index """
+ SELECT id
+ FROM test_ip_cidr_not_null
+ WHERE NOT is_ip_address_in_range(address, '192.168.1.0/24')
+ ORDER BY id
+ """
+
+ sql "SET enable_inverted_index_query = true"
Review Comment:
**[P2] Make the regression prove the index evaluator ran**
These assertions compare only final rows, so a planner miss or an allowed
inverted-index bypass falls back to row evaluation and still returns `3`; the
test can therefore pass without executing the production lines changed by this
PR. The existing IP inverted-index suite enables `ip.inverted_index_filtered`
around supported queries: the index evaluator handles that point, while row
`execute_impl()` fails, making fallback visible. Please use the same oracle
around both enabled queries (and explicitly pin/instrument the query cache if
the repeat is intended to prove a cache hit).
##########
be/src/exprs/function/function_ip.h:
##########
@@ -708,6 +708,11 @@ class FunctionIsIPAddressInRange : public IFunction {
}
// apply for inverted index
std::shared_ptr<roaring::Roaring> null_bitmap =
std::make_shared<roaring::Roaring>();
+ if (iter->has_null()) {
+ segment_v2::InvertedIndexQueryCacheHandle null_bitmap_cache_handle;
+ RETURN_IF_ERROR(iter->read_null_bitmap(&null_bitmap_cache_handle));
+ null_bitmap = null_bitmap_cache_handle.get_bitmap();
Review Comment:
**[P1] Preserve UNKNOWN rows through indexed AND before outer NOT**
A reachable case is:
```text
NOT
AND
is_ip_address_in_range(address, cidr) -- address is NULL
flag = 1 -- flag is 0 (FALSE)
```
SQL evaluates `NULL AND FALSE` to `FALSE`, then outer `NOT` to `TRUE`. With
this new NULL bitmap, the first child is `(data={}, null={row})`, but
`VCompoundPred::evaluate_inverted_index` breaks the AND loop solely because the
data bitmap is empty. It never evaluates the FALSE child that would clear the
row from the NULL bitmap, and outer `op_not()` subtracts that stale NULL row,
incorrectly filtering it out. Please make the AND shortcut continue while
UNKNOWN rows can still be resolved (the safe empty terminal state requires both
TRUE/data and NULL to be empty), and add an index-forced regression for this
shape.
--
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]
