github-actions[bot] commented on code in PR #67538:
URL: https://github.com/apache/doris/pull/67538#discussion_r3998863391
##########
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:
[P1] Reapply the MOR value-column rule before persisting these analyzer
defaults. This ADD INDEX path called the catalog-Column `checkColumn` overload,
whose UNIQUE/MOW-disabled guard recognizes only `parser`; a gram index is
identified by `analyzer`, so ALTER ADD passes here while CREATE TABLE's
overload rejects the same analyzed value-column index. The write-back then
persists an index that the scan path deliberately cannot push below UNIQUE
merging. Please make the catalog overload use `isAnalyzedInvertedIndex()` (or
the same four keys) and add a UNIQUE_KEYS, MOW-disabled ADD INDEX test; the
current test uses DUP_KEYS.
##########
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:
[P1] Select a gram-capable reader before dispatching this query. The param
carries no analyzer/capability key, so gram types are treated as match queries
and the iterator picks the lowest-ID FULLTEXT reader. In a supported migration
layout with an older ordinary English/SNII or CLucene index and a later gram
index, the ordinary reader wins, returns SKIPPED/NOT_SUPPORTED, and
`dispatch_query` converts that to success without ever trying the gram reader.
Scalar evaluation preserves results, but the new acceleration is silently
disabled. Please require/prefer gram capability or retry compatible readers,
and cover ordinary-first plus gram-second ordering.
##########
be/src/storage/index/snii/writer/logical_index_writer.cpp:
##########
@@ -505,6 +562,93 @@ struct LogicalIndexWriter::BlockState {
// Out-of-line so unique_ptr<BlockState> sees the complete type (see header).
LogicalIndexWriter::~LogicalIndexWriter() = default;
+// Offers one term to the high-df digest. The digest keeps the highest-df
terms seen, as a
+// min-heap on df so the cheapest entry to evict is always at the front, which
makes the
+// whole pass O(terms log K) time and O(K) space -- no second sort over the
vocabulary.
+//
+// Terms below the floor are dropped outright: the digest exists to let a
query GIVE UP
+// early, and a term rare enough to sit below the floor is one no cost gate
would give up on.
+void LogicalIndexWriter::note_high_df_term(uint64_t term_hash, uint32_t df) {
+ // A zero floor means no digest for this index (see high_df_floor_for),
and it has to be
+ // tested separately: `df < 0` is never true, so the comparison below
would let every
+ // term through on exactly the indexes that want none.
+ if (high_df_floor_ == 0 || df < high_df_floor_) {
+ return;
+ }
+ const auto by_df = [](const HighDfEntry& lhs, const HighDfEntry& rhs) {
+ return lhs.df > rhs.df; // greater-than gives a MIN-heap on df
+ };
+ if (high_df_terms_.size() < format::kMaxHighDfDigestTerms) {
+ high_df_terms_.push_back({df, term_hash});
Review Comment:
[P2] Charge this retained digest heap to the writer's `MemoryReporter`. It
grows to 4,096 aligned entries (64 KiB) per logical index without a
reservation, is kept in the compound writer until teardown, and
`finish_high_df_digest` creates another heap copy plus parallel hash/df vectors
before encoding. Those bytes appear in the task allocator tracker, but they are
absent from both the dedicated SNII build metric and
`snii_registered_build_bytes()`, the only build-share signal used by the global
limiter; concurrent multi-index builds can therefore bypass that share by the
sum of these heaps. Please pre-charge capacity growth, account the finalization
buffers (or serialize through tracked storage), release retained digest storage
once metadata is materialized, and add reporter-balance coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/InvertedIndexUtil.java:
##########
@@ -353,12 +358,70 @@ private static void checkAnalyzerName(String
analyzerName, PrimitiveType colType
+ " is not supported for column of type " + colType);
}
try {
-
Env.getCurrentEnv().getIndexPolicyMgr().validateAnalyzerExists(analyzerName);
+ IndexPolicyMgr indexPolicyMgr =
Env.getCurrentEnv().getIndexPolicyMgr();
+ indexPolicyMgr.validateAnalyzerExists(analyzerName);
+ // Gram-family analyzer (an ngram tokenizer carrying mode, see
+ // IndexPolicyMgr#resolveGramTokenizerMode): BE builds
sparse/dense gram postings for it
+ // only on SNII, and those postings carry no positions, so phrase
queries are impossible.
+ Optional<String> gramMode =
indexPolicyMgr.resolveGramTokenizerMode(analyzerName);
+ if (gramMode.isPresent()) {
+ if (colType.isArrayType()) {
+ throw new AnalysisException("gram tokenizer (mode=" +
gramMode.get()
+ + ") analyzer '" + analyzerName + "' does not
support ARRAY columns");
+ }
+ if (!colType.isCharFamily()) {
+ throw new AnalysisException("gram tokenizer (mode=" +
gramMode.get()
+ + ") analyzer '" + analyzerName
+ + "' is supported only on scalar CHAR, VARCHAR, or
STRING columns");
+ }
+ if (storageFormat != TInvertedIndexFileStorageFormat.SNII) {
+ throw new AnalysisException("gram tokenizer (mode=" +
gramMode.get()
+ + ") requires inverted_index_storage_format =
SNII");
+ }
+ if ("true".equals(supportPhrase)) {
+ throw new AnalysisException(
+ "gram tokenizer index does not support phrase
(support_phrase must be false)");
+ }
+ }
} catch (DdlException e) {
throw new AnalysisException("Invalid custom analyzer: " +
e.getMessage());
}
}
+ /**
+ * Constraints a gram-family analyzer (an ngram tokenizer carrying mode)
enforces at the index
+ * property level:
+ * 1) an index-level char_filter (char_filter_type/pattern/replacement)
conflicts with the
+ * semantics of gram split boundaries (the character replacement
happens after the tokenizer
+ * has already split by the gram rule, which breaks reproducibility),
so it is rejected;
+ * 2) when support_phrase is not given explicitly it defaults to "false",
overriding the general
+ * rule of the {@link Index} constructor that "an analyzer implies
true" -- a gram index is
+ * forced to docs-only on the BE side and has no positions for a phrase
query to use.
+ *
+ * <p>The {@code properties} held by the caller ({@link
#checkInvertedIndexProperties}) and the
+ * {@link IndexDefinition} field are the same mutable Map reference, so
the defaults written
+ * here are seen when {@code IndexDefinition#translateToCatalogStyle}
builds the {@link Index}.
+ */
+ private static void applyGramFamilyIndexDefaults(String analyzerName,
Map<String, String> properties)
+ throws AnalysisException {
+ if (analyzerName == null || analyzerName.isEmpty()) {
+ return;
+ }
+ Optional<String> gramMode =
Env.getCurrentEnv().getIndexPolicyMgr().resolveGramTokenizerMode(analyzerName);
+ if (!gramMode.isPresent()) {
+ return;
+ }
+ if (properties.get(INVERTED_INDEX_PARSER_CHAR_FILTER_TYPE) != null
+ || properties.get(INVERTED_INDEX_PARSER_CHAR_FILTER_PATTERN)
!= null
+ ||
properties.get(INVERTED_INDEX_PARSER_CHAR_FILTER_REPLACEMENT) != null) {
+ throw new AnalysisException("char_filter cannot be used with gram
tokenizer (mode="
+ + gramMode.get() + ")");
+ }
+ if (properties.get(INVERTED_INDEX_SUPPORT_PHRASE_KEY) == null) {
+ properties.put(INVERTED_INDEX_SUPPORT_PHRASE_KEY, "false");
Review Comment:
[P1] Check phrase capability on the analyzer-selected reader. This default
makes a gram-family index docs-only, and a supported column can have that index
first plus a later positional English/standard index. For `MATCH_PHRASE ...
USING ANALYZER` targeting the later index, the expression carries the right
analyzer key and `read_from_index` would select it, but `FunctionMatchBase`
first calls analyzer-blind `get_reader(FULLTEXT)`, sees the leading gram
reader's `support_phrase=false`, and returns `INDEX_INVALID_PARAMETERS`. That
status is not downgraded, so the valid query fails solely by index order.
Please perform the capability check on the keyed selected reader (and reuse it
for execution), with gram-first/positional-second phrase and phrase-prefix
coverage.
##########
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:
[P1] Fence gram writes from pre-feature readers even when no posting is
dropped. This optional protobuf field is the only marker on a small gram
segment: format/min-reader stay at v1, the directory raises no required feature
unless a posting is physically dropped, and an old factory simply ignores
`mode` and uses legacy ngram semantics. For example, a new dense min=3/max=4
segment for `abcd` stores `abc,bcd`; an old legacy reader also requires `abcd`
for MATCH_ALL and can return an empty exact bitmap although scalar legacy
analysis matches. The existing stopped-posting feature cannot protect
sub-2,000-row/no-drop segments. Please gate gram DDL/writes on fleet capability
(and/or introduce a reader fence with a safe unsupported downgrade), preserve
that contract through inheritance, and add a frozen-old-reader test.
##########
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:
[P1] Handle the reverse optional mismatch as well. A dense/sparse gram
segment can be recovered after its same-named analyzer is recreated as legacy
ngram (no `mode`), leaving `current_gram_scheme=nullopt` while the persisted
scheme is present. This guard then skips: current bigram MATCH terms are looked
up in a dense-trigram dictionary, so an exact empty bitmap can remove rows that
scalar current-analyzer MATCH accepts; MATCH_REGEXP is affected too. The cache
is also re-enabled and consulted before the persisted scheme is known, making
that bitmap reusable. Compare the full optionals after opening the reader (and
keep this case out of the scheme-blind cache), with gram-to-legacy recovery
coverage.
##########
be/src/storage/index/snii/snii_index_writer.cpp:
##########
@@ -377,6 +638,7 @@ void SniiIndexColumnWriter::close_on_error() {
// Balance the observation-tracker mirror before dropping the reporter.
_report_null_docids_capacity(/*release_all=*/true);
_report_encoded_norms_capacity(/*release_all=*/true);
+ _report_gram_buffers_capacity(/*release_all=*/true);
_memory_reporter.reset();
Review Comment:
[P2] Release the retained density sample before resetting this reporter. If
a held row fails analysis during `_finish_density_calibration()`, the latch
calls `close_on_error()` from inside replay; this path clears other buffers,
but leaves `_density_sample` and `_density_sample_bytes` owned by the
still-live column writer. `MemoryReporter`'s destructor then drains the
sample's charge from the process-wide build-RAM counter even though the
vector/strings (up to the configured budget plus the final row) remain resident
until the segment writer is cleared. Please swap/clear the sample and report
its release before dropping the reporter, with a replay-failure cleanup test.
--
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]
