airborne12 commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4068726939
##########
be/src/storage/index/inverted/char_filter/icu_normalizer_char_filter.cpp:
##########
@@ -59,33 +63,97 @@ void ICUNormalizerCharFilter::fill() {
input.resize(_reader->size());
_reader->readCopy(input.data(), 0, static_cast<int32_t>(input.size()));
normalize_text(input, _buf);
+ build_source_byte_offset_runs();
_transformed_input.init(_buf.data(), static_cast<int32_t>(_buf.size()),
false);
}
void ICUNormalizerCharFilter::normalize_text(const std::string& input,
std::string& output) {
output.clear();
+ _edits.reset();
if (input.empty()) {
return;
}
UErrorCode status = U_ZERO_ERROR;
- icu::UnicodeString src16 = icu::UnicodeString::fromUTF8(input);
- UNormalizationCheckResult quick_result = _normalizer->quickCheck(src16,
status);
- if (U_SUCCESS(status) && quick_result == UNORM_YES) {
+ icu::StringByteSink<std::string> sink(&output);
+ _normalizer->normalizeUTF8(0, icu::StringPiece(input), sink, &_edits,
status);
+ if (U_FAILURE(status)) {
+ LOG(WARNING) << "ICU normalize failed: " << u_errorName(status) << ",
using original text";
output = input;
+ _edits.reset();
+ _edits.addUnchanged(static_cast<int32_t>(input.size()));
return;
}
+}
- icu::UnicodeString result16;
- status = U_ZERO_ERROR;
- _normalizer->normalize(src16, result16, status);
+void ICUNormalizerCharFilter::build_source_byte_offset_runs() {
+ _offset_correction_runs.clear();
+ UErrorCode status = U_ZERO_ERROR;
+ auto iterator = _edits.getFineChangesIterator();
+ while (iterator.next(status)) {
+ if (U_FAILURE(status)) {
+ _offset_correction_runs.clear();
+ return;
+ }
+
+ const int32_t source_start = iterator.sourceIndex();
+ const int32_t destination_start = iterator.destinationIndex();
+ const int32_t source_length = iterator.oldLength();
+ const int32_t destination_length = iterator.newLength();
+ if (!_offset_correction_runs.empty()) {
+ auto& previous = _offset_correction_runs.back();
+ const int64_t previous_source_end =
+ static_cast<int64_t>(previous.source_start) +
+ static_cast<int64_t>(previous.source_length) *
previous.repeat_count;
+ const int64_t previous_destination_end =
+ static_cast<int64_t>(previous.destination_start) +
+ static_cast<int64_t>(previous.destination_length) *
previous.repeat_count;
+ if (previous.source_length == source_length &&
+ previous.destination_length == destination_length &&
+ previous_source_end == source_start &&
+ previous_destination_end == destination_start) {
+ ++previous.repeat_count;
+ continue;
+ }
+ }
+ _offset_correction_runs.push_back(
Review Comment:
Fixed in 98adc064ea0. Measured first: 100k alternating `aＡ` pairs (400 KB)
left a 2,621,440-byte `OffsetCorrectionRun` table after `fill()`, and that
capacity survived `clear()` on reuse.
The side table is gone. `fill()` now keeps only ICU's `Edits` (16-bit units,
so roughly 2 bytes per change or unchanged run) plus a cached fine
`Edits::Iterator`, and `correct_offset()` answers through
`sourceIndexFromDestinationIndex()`, which has the same semantics as the
previous run arithmetic (start of an edit -> its source start, inside an edit
-> its source end, unchanged text keeps its relative position, deletions map to
the following source position). ICU's `findIndex` walks forward from the cached
position and searches backward for nearby smaller offsets, so the increasing
per-rune queries of the tokenizers stay O(1) amortized and n-gram style
back-steps stay O(distance). `DenseAlternatingEditsMapOffsetsWithoutSideTable`
covers the dense alternating case with forward and backward queries; the
existing sparse and ligature cases still pass.
##########
be/src/runtime/index_policy/index_policy_mgr.cpp:
##########
@@ -142,50 +186,58 @@ AnalyzerPtr IndexPolicyMgr::get_policy_by_name(const
std::string& name) {
AnalyzerPtr IndexPolicyMgr::get_analyzer_by_name(const std::string& name) {
std::shared_lock lock(_mutex);
const std::string normalized_name = normalize_name(name);
- auto name_it = _name_to_id.find(normalized_name);
- if (name_it == _name_to_id.end()) {
+ const auto* index_policy = find_policy_by_name_locked(name);
+ if (index_policy == nullptr) {
if (is_builtin_normalizer(normalized_name)) {
return build_builtin_normalizer(name);
}
throw Exception(ErrorCode::INVALID_ARGUMENT, "Policy not found with
name: " + name);
}
- auto policy_it = _policys.find(name_it->second);
- if (policy_it == _policys.end()) {
- throw Exception(ErrorCode::INVALID_ARGUMENT, "Policy not found with
id: " + name);
- }
- if (policy_it->second.type == TIndexPolicyType::ANALYZER) {
- return build_analyzer_provider_from_config(
- build_analyzer_config_from_policy(policy_it->second),
{})
+ if (index_policy->type == TIndexPolicyType::ANALYZER) {
+ return
build_analyzer_provider_from_config(build_analyzer_config_from_policy(*index_policy),
+ {})
->get_analyzer();
}
- if (policy_it->second.type == TIndexPolicyType::NORMALIZER) {
- return build_normalizer_from_policy(policy_it->second);
+ if (index_policy->type == TIndexPolicyType::NORMALIZER) {
+ return build_normalizer_from_policy(*index_policy);
}
throw Exception(ErrorCode::INVALID_ARGUMENT, "Analyzer policy not found: "
+ name);
}
AnalyzerProviderPtr IndexPolicyMgr::get_analyzer_provider_by_name(
- const std::string& name, const std::map<std::string, std::string>&
outer_char_filter_map) {
+ const std::string& name, const std::map<std::string, std::string>&
outer_char_filter_map,
+ std::string* resolved_name, std::string* legacy_name) {
std::shared_lock lock(_mutex);
+ if (resolved_name != nullptr) {
+ *resolved_name = name;
+ }
+ if (legacy_name != nullptr) {
+ legacy_name->clear();
+ }
const std::string normalized_name = normalize_name(name);
- auto name_it = _name_to_id.find(normalized_name);
- if (name_it == _name_to_id.end()) {
+ const auto* index_policy = find_policy_by_name_locked(name);
Review Comment:
Fixed in 41ed29c02fd. Reproduced first: with only a replayed exact
`LOWERCASE` token-filter policy, `get_policy_by_name("lowercase")` threw `[E33]
Policy not found with type: lowercase` because `find_policy_by_name_locked()`
fell back to the normalized name before the built-in check.
The three top-level lookups (`get_policy_by_name`, `get_analyzer_by_name`,
`get_analyzer_provider_by_name`) now resolve through
`find_top_level_policy_locked()`: exact policy -> canonical built-in normalizer
-> normalized fallback, which is the order FE's
`validateNormalizerExists`/`resolveAnalyzerName` use. Nested tokenizer/filter
references keep the existing `find_policy_by_name_locked()` behavior.
`BuiltinNormalizerWinsOverNormalizedLegacyPolicy` covers the replayed
`LOWERCASE` collision for all three entry points and checks that the exact
spelling still binds the legacy policy.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/InvertedIndexUtil.java:
##########
@@ -393,16 +420,40 @@ public static boolean
canHaveMultipleInvertedIndexes(DataType colType, List<Inde
}
Set<String> analyzerKeys = new HashSet<>();
+ Set<String> analyzerSelectors = new HashSet<>();
for (IndexDefinition indexDef : indexDefs) {
- String key = buildAnalyzerIdentity(indexDef.getProperties());
+ Map<String, String> properties = indexDef.getProperties();
+ String key = buildAnalyzerIdentity(properties);
// HashSet.add() returns false if element already exists
if (!analyzerKeys.add(key)) {
return false;
}
+ String selector = getAnalyzerSelector(properties);
+ if (!INVERTED_INDEX_PARSER_IK.equals(selector) &&
!analyzerSelectors.add(selector)) {
Review Comment:
Checked against the pre-PR behavior and keeping this as is. At the merge
base, `canHaveMultipleInvertedIndexes` only deduplicated by analyzer identity
and had no selector fence at all, so identity-distinct IK configurations
(smart/max-word, lowercase on/off) were already admitted on one column, and an
unqualified MATCH already picked the first analyzed index
(`OlapTable.getInvertedIndex` -> `findFirst()` over analyzed indexes). Neither
the admission nor that fallback is introduced by this PR.
What this PR changed is the opposite direction: the new same-selector fence
rejects aliases that MATCH cannot tell apart, and the IK exemption exists only
so that DDL which was valid before the PR stays valid. For the selectable case,
`USING ANALYZER ik` is now deterministic: it binds the default max-word +
lowercase identity (`matchesBuiltinIkDefaults`, covered by
`testExplicitBuiltinIkSelectsMatchingModeAndLowercase` and
`InvertedIndexIteratorTest.MatchBindsDistinctIkModesAndLowercaseStates` from
the earlier thread), instead of whichever index came first.
Rejecting the additional IK identities would tighten pre-existing DDL, and
making the selector carry mode/lowercase is new query syntax; both are beyond
this PR. If we want MATCH to reach the other IK variants, I would rather do
that as a follow-up (for example selecting by the identity of a named analyzer)
than change admission here. Resolving on that basis; happy to reopen if you see
a case that the PR itself made worse.
--
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]
