airborne12 commented on code in PR #67538:
URL: https://github.com/apache/doris/pull/67538#discussion_r4000025470
##########
be/src/storage/index/snii/format/core_metadata.cpp:
##########
@@ -137,6 +219,21 @@ Status encode_core_metadata(const CoreMetadata& metadata,
ByteSink* out) {
}
encode_region_ref(metadata.section_refs.null_bitmap,
refs->mutable_null_bitmap());
encode_region_ref(metadata.section_refs.bsbf, refs->mutable_bsbf());
+ if (metadata.gram_scheme.has_value()) {
Review Comment:
Not changing this in the PR, but the mechanism is real, so here is what I
checked.
An older BE does mis-read such a segment: master's `NGramTokenizerFactory`
reads only `min_gram`, `max_gram` and `token_chars`, and refuses only `max_gram
- min_gram > 1`. A gram policy that keeps the default 3/4, or sets nothing but
`mode`, is therefore accepted and built as a legacy ngram, and MATCH would be
answered from the wrong dictionary -- exactly your `abcd` example.
Getting there takes an older BE reading a segment this PR wrote. A
pre-feature FE cannot create the policy (`mode` is not in master's
`NGramTokenizerValidator.ALLOWED_PROPS`), and Doris upgrades BEs before FEs, so
on a supported path every BE already understands gram mode by the time any FE
accepts the DDL. What remains is a downgrade after gram indexes exist, which
the PR description declares unsupported. The feature is unreleased, so there is
no data written by an older version to stay compatible with; a fleet-capability
gate plus a frozen-old-reader test would be machinery for a path Doris does not
support.
##########
be/src/exprs/function/like.cpp:
##########
@@ -1107,6 +1114,78 @@ Status FunctionRegexpLike::open(FunctionContext* context,
return Status::OK();
}
+// R8 (unity build): file-scope helpers use a namespace private to this file.
+namespace like_gram_index_detail {
+
+// Index acceleration may be skipped, but cancellation and memory failures
stop the query.
+Status dispatch_query(bool is_like, const std::string& pattern,
segment_v2::IndexIterator* iter,
+ const IndexFieldNameAndTypePair& data_type_with_name,
uint32_t num_rows,
+ segment_v2::InvertedIndexResultBitmap* bitmap_result) {
+ segment_v2::InvertedIndexParam param;
+ param.column_name = data_type_with_name.first;
+ param.column_type = data_type_with_name.second;
+ param.query_value = Field::create_field<TYPE_STRING>(pattern);
+ param.query_type = is_like ?
segment_v2::InvertedIndexQueryType::LIKE_GRAM_QUERY
Review Comment:
Confirmed and fixed in 98ec7f53029.
Reproduced on a live cluster: 20,000 rows, the same LIKE / REGEXP on two
tables that differ only in index order. With the gram index declared first the
profile showed `GramIndexCandidateRows=20, RowsGramIndexFiltered=19980`; with
an english index declared first both were 0 -- the english reader got the
query, answered SKIPPED, and `dispatch_query` took that as success. The rows
were right both ways; the acceleration was simply gone.
Readers now answer `is_gram_family()` from the index properties and the
in-memory policy (`resolve_gram_scheme`), never by opening the index. For
LIKE_GRAM / REGEXP_GRAM, `InvertedIndexIterator::select_best_reader` takes the
lowest-id FULLTEXT reader that is gram-family and falls back to the previous
choice when none is, so a column without a gram index behaves exactly as
before, and nothing is resolved when the column has a single index. I chose
this over retrying readers on SKIPPED because that answer comes from the
segment's persisted scheme: a retry would open every ordinary index on every
segment before reaching the gram one.
Tests:
`InvertedIndexIteratorTest.SelectBestReader_GramQueryPrefersGramFamilyOverOlderIndex`
and `...GramQueryKeepsIdOrderAndFallsBack`, and the new
`test_gram_index_order`, which declares an english index before the gram index
and requires `RowsGramIndexFiltered > 0` for LIKE and REGEXP on top of index-on
/ index-off parity. Both unit tests fail against the previous selection, and
the suite failed on the previous build with `RowsGramIndexFiltered=0`; on the
fixed build it reports 18 of 19 rows filtered for LIKE and for REGEXP.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3314,6 +3314,12 @@ private boolean processAddIndex(CreateIndexOp
createIndexOp, OlapTable olapTable
}
}
+ // CreateIndexOp#validate materializes the IndexDefinition into
alterIndex before
+ // checkColumn runs, so the property defaults that are only filled in
during checkColumn
+ // (currently the gram family's support_phrase=false) have to be
written back here, or the
+ // index that is persisted and shipped to BE would lose them.
+ indexDef.applyPropertiesTo(alterIndex);
Review Comment:
Not changing this in this PR: the gap predates it and is not specific to
gram.
Measured on a live cluster, UNIQUE KEY with merge-on-write disabled:
`parser=english` is refused by both CREATE TABLE and ALTER ADD INDEX, while
`analyzer=<gram>` and, as a non-gram control, `analyzer=<custom standard>` are
refused by CREATE TABLE and accepted by ALTER ADD INDEX, whose build job
finishes. The CREATE overload learned `analyzer` / `normalizer` in 6a7e1e5f8c3
(#58178, normalizer support); the catalog-`Column` overload ALTER uses was
never updated.
On what the accepted index does, for both analyzers: across two overlapping
loads, a full compaction into a non-overlapping base followed by another
overwrite, and `enable_mor_value_predicate_pushdown_tables` naming the table,
LIKE / REGEXP / MATCH_ANY / MATCH_ALL returned the expected rows with the index
on and off, and every index counter stayed 0. That matches the scan path:
value-column expressions stay above the merge
(`_should_push_down_common_expr`), and a pushed value predicate keeps its
residual conjunct. So the index is dead weight -- build time and storage --
rather than a wrong-result risk. Making the ALTER overload check the same four
keys is the right fix, but it belongs in its own change against master, since
every analyzer and normalizer index has the gap today.
##########
be/src/storage/index/snii/snii_index_reader.cpp:
##########
@@ -651,6 +741,16 @@ Status SniiIndexReader::_query(const IndexQueryContextPtr&
context, const std::s
const ::doris::snii::reader::LogicalIndexReader* logical_reader = nullptr;
RETURN_IF_ERROR(_get_logical_reader(context, &searcher_cache_handle,
&uncached_reader,
&logical_reader));
+ // Compare the two optionals, not just two schemes: a segment written by a
legacy ngram
+ // tokenizer carries no scheme at all, and its dictionary holds that
tokenizer's terms. Once
+ // the current analyzer cuts grams, looking those grams up in that
dictionary answers a
+ // different question, so an absent persisted scheme is a mismatch like
any other.
+ if (analyzed_query && current_gram_scheme.has_value() &&
Review Comment:
Confirmed and fixed in 627dae482bc.
Reproduced on a live cluster: a dense-3 table dropped without FORCE, its
analyzer recreated under the same name as a legacy `ngram(2,2)`, then RECOVER.
With the index on vs off: `MATCH_ANY 'abcd'` `[]` vs `[1,2,4,5]`, `MATCH_ALL
'abcd'` `[]` vs `[1,2]`, `MATCH_REGEXP '^ab$'` `[]` vs `[1,2,4]`, and a second
index-on run stayed empty.
The fence now compares the two optionals whichever side is empty, so a
persisted scheme under a scheme-less analyzer steps aside like any other
mismatch. On the cache: the lookup for this case still happens before the
segment is opened, but there is nothing mismatched in the cache for it to find.
The fence returns before a bitmap is computed, so no attempt under a
scheme-less analyzer ever stores one for a gram segment, and while the analyzer
still had a scheme the result cache was not consulted at all.
Tests:
`SniiGramCacheTest.AnalyzedQueriesDeclineWhenTheAnalyzerNoLongerCutsGrams`
writes a real dense-3 segment, replaces the policy with a legacy bigram
tokenizer under the same name, and requires MATCH_ANY / MATCH_ALL /
MATCH_REGEXP to decline twice in a row with neither a cache insert nor a hit;
`test_gram_policy_recovery` gained the gram-to-legacy recovery with the bigram
answers pinned. Against the previous fence the unit test saw `OK`, a cache
insert on the first attempt and a cache hit on the second, and the suite failed
with `expected: <[1, 2, 4, 5]> but was: <[]>`; both pass on the fixed build.
--
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]
