github-actions[bot] commented on code in PR #68021:
URL: https://github.com/apache/doris/pull/68021#discussion_r4015549765
##########
be/src/storage/tablet/tablet_reader.cpp:
##########
@@ -368,7 +368,8 @@ Status TabletReader::_init_column_predicates(const
ReaderParams& read_params) {
}
const auto& col = *_read_schema->column(predicate->column_id());
const auto* tablet_index =
_tablet_schema->get_ngram_bf_index(col.unique_id());
- if (tablet_index && config::enable_query_like_bloom_filter) {
+ if (tablet_index && config::enable_query_like_bloom_filter &&
+ predicate->can_do_bloom_filter(true)) {
Review Comment:
[P2] Preserve safe LIKE pruning inside mixed conjunctions
This per-predicate `false` is later aggregated by
`AndBlockColumnPredicate::can_do_bloom_filter(true)`, which requires every
same-column predicate to be eligible. Thus a safe `v LIKE '%plain%'` combined
with a backslash-bearing LIKE causes the latter to disable the NGRAM index for
the safe predicate too, even though this method already built its query bloom
filter. On large scans that turns a selective page-pruned query into an
unpruned scan. Please keep ineligible LIKEs out of the NGRAM predicate group,
or otherwise let eligible children drive BF evaluation while ineligible
children accept, so only the unsafe predicate loses the index.
##########
regression-test/suites/index_p0/test_ngram_bloomfilter_index_like_escape.groovy:
##########
@@ -0,0 +1,96 @@
+// 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_ngram_bloomfilter_index_like_escape") {
+ // An NGRAM_BF index prunes a page whose bloom filter misses a token of
the LIKE pattern, so
+ // the tokens have to occur in every string the pattern matches. Deriving
them means reading
+ // the pattern's escapes exactly the way LIKE does, which the storage
layer does not attempt;
+ // it leaves the index alone instead. Whatever the reason, turning the
index on must never
+ // change a result set.
+
+ // One backslash. Backslashes go through SQL string-literal unescaping
before reaching LIKE,
+ // so each one is written twice in the statement text.
+ def bs = '\\'
+ def quote = { String raw -> raw.replace(bs, bs + bs) }
+
+ def values = [
+ 'a' + bs + 'bc', // one literal backslash
+ 'xxab' + bs + bs + 'cdyy', // two consecutive literal
backslashes
+ 'zab' + bs + 'Zcdz',
+ 'a' + bs + 'Zbb', // backslash in front of an
ordinary character
+ 'a' + bs + '中zz', // backslash in front of a
multi-byte character
+ '100%off', // literal percent in the data
+ 'x_y', // literal underscore in the data
+ 'qa%bz', // for the custom-escape pattern
below
+ 'plain ascii row',
+ '中文测试行',
+ ]
+
+ // [pattern, escape clause]. The escape-free entries must keep using the
index; the rest are
Review Comment:
[P2] Assert the NGRAM path, not only result equality
The stated invariant here is two-sided: escaped/custom patterns must bypass
NGRAM, while escape-free patterns must keep using it. The assertions below only
compare result rows, so an implementation that returns `false` from
`can_do_bloom_filter(true)` for every LIKE passes the entire suite, including
`%plain%` and `%中文%`. Please add a deterministic path-use oracle—for example
the existing `RowsBloomFilterFiltered` profile technique from
`test_ngram_bloomfilter_index_change.groovy` (with controlled page/compaction
layout)—that proves positive pruning for an escape-free control and no BF
pruning for the escaped/custom cases.
##########
be/src/storage/predicate/like_column_predicate.h:
##########
@@ -89,7 +90,17 @@ class LikeColumnPredicate final : public ColumnPredicate {
}
return true;
}
- bool can_do_bloom_filter(bool ngram) const override { return ngram; }
+ bool can_do_bloom_filter(bool ngram) const override {
+ if (!ngram) {
+ return false;
+ }
+ // A pattern that can carry an escape is not supported by the ngram
index.
+ if (_state->has_custom_escape) {
Review Comment:
[P1] Make three-argument LIKE reach this gate in checked builds
This new branch relies on a custom-`ESCAPE` LIKE reaching
`LikeColumnPredicate`, but the upstream
`OlapScanLocalState::_should_push_down_function_filter` still executes
`DCHECK(children.size() == 2)` (`olap_scan_operator.cpp:544`). Nereids
preserves the third escape argument, and the new cases at lines 55-57 enable
function pushdown, so ASAN/Debug builds (which do not define `NDEBUG`) abort
before `has_custom_escape` can be consulted. Please make the normalizer
explicitly accept/validate both two- and three-argument LIKE (or reject
three-argument pushdown cleanly) so these cases actually exercise the fix in
checked builds.
--
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]
