github-actions[bot] commented on code in PR #67917:
URL: https://github.com/apache/doris/pull/67917#discussion_r4000732697
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/invertedindex/AnalyzerIdentityBuilder.java:
##########
@@ -177,6 +179,11 @@ private static String resolveComponentIdentity(String
name, IndexPolicyTypeEnum
// Build identity from sorted properties
TreeMap<String, String> sortedProps = new TreeMap<>(props);
+ if (expectedType == IndexPolicyTypeEnum.TOKENIZER
+ && "ngram".equals(sortedProps.get(IndexPolicy.PROP_TYPE)))
{
+ // This setting only limits policy creation; it does not
change emitted tokens.
+ sortedProps.remove(PROP_MAX_NGRAM_DIFF);
Review Comment:
[P2] Keep replayed invalid policies out of valid identity equivalence
The base FE admitted and journaled n-gram policies without a difference
check, so after a supported upgrade a legacy `{min_gram=1,max_gram=8}` policy
can remain referenced by an index even though current BE construction rejects
it at the missing property's default limit of 1. A new `1..8,max_ngram_diff=7`
policy is usable, but removing the property here gives both tokenizers the same
identity, so ALTER ADD INDEX rejects the working replacement as a duplicate of
the broken index. The new unit test uses exactly this invalid no-limit control
and therefore codifies the collision. Please preserve ceiling-insensitive
identity for valid policies while marking replayed-invalid policies unusable
and giving each a distinct fallback identity, then cover replay followed by
replacement.
##########
be/src/storage/index/inverted/tokenizer/ngram/ngram_tokenizer_factory.cpp:
##########
@@ -26,12 +26,23 @@ std::unordered_map<std::string, CharMatcherPtr>
NGramTokenizerFactory::MATCHERS;
void NGramTokenizerFactory::initialize(const Settings& settings) {
_min_gram = settings.get_int("min_gram",
NGramTokenizer::DEFAULT_MIN_NGRAM_SIZE);
_max_gram = settings.get_int("max_gram",
NGramTokenizer::DEFAULT_MAX_NGRAM_SIZE);
+ int32_t max_ngram_diff = settings.get_int("max_ngram_diff", 1);
+ if (max_ngram_diff < 0) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT,
+ "max_ngram_diff must be greater than or equal to 0");
+ }
+ if (max_ngram_diff > MAX_NGRAM_DIFF) {
+ throw Exception(
+ ErrorCode::INVALID_ARGUMENT,
+ "max_ngram_diff must be less than or equal to " +
std::to_string(MAX_NGRAM_DIFF));
+ }
int32_t ngram_diff = _max_gram - _min_gram;
- if (ngram_diff > 1) {
+ if (ngram_diff > max_ngram_diff) {
Review Comment:
[P2] Bound absolute gram sizes before constructing the tokenizer
This caps only the difference, so
`min_gram=536870910,max_gram=536870912,max_ngram_diff=2` now passes FE and this
check (the old fixed limit rejected that width). On first use,
`NGramTokenizer::init` evaluates `_buffer.resize(4 * max_gram + 1024)`, which
overflows signed 32-bit arithmetic; even
`min_gram=268435455,max_gram=268435457,max_ngram_diff=2` requests about 4 GiB
of `UChar32` storage before looking at the input. Because policy installation
is lazy, CREATE TOKENIZER/ANALYZER and BE sync succeed, then TOKENIZE or
classic/SNII index writing can OOM or fail. Please enforce a practical absolute
min/max bound in both FE and BE, use checked `size_t` arithmetic for the
buffer, and add rejection/boundary tests.
--
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]
