airborne12 commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4090583940
##########
fe/fe-core/src/main/java/org/apache/doris/indexpolicy/IndexPolicyMgr.java:
##########
@@ -55,13 +56,65 @@ public class IndexPolicyMgr implements Writable,
GsonPostProcessable {
private final Map<Long, IndexPolicy> idToIndexPolicy = Maps.newHashMap();
// Keys are normalized to lowercase for case-insensitive lookup
private final Map<String, IndexPolicy> nameToIndexPolicy =
Maps.newHashMap();
+ // Legacy metadata can contain case-distinct names that share a normalized
key. Keep exact
+ // bindings separately so a saved analyzer continues to resolve its
original component.
+ private final transient Map<String, IndexPolicy> exactNameToIndexPolicy =
Maps.newHashMap();
/**
* Normalize policy name to lowercase for case-insensitive lookup.
* Policy names are case-insensitive in Doris.
*/
private static String normalizeKey(String name) {
- return name == null ? null : name.trim().toLowerCase();
+ return name == null ? null : name.trim().toLowerCase(Locale.ROOT);
+ }
+
+ private static String exactKey(String name) {
+ return name == null ? null : name.trim();
+ }
+
+ // Callers hold either the read or write lock. Prefer an exact legacy name
binding and
+ // retain normalized lookup only for interactive case-insensitive fallback.
+ private IndexPolicy getPolicyByNameLocked(String name) {
+ IndexPolicy exactPolicy = exactNameToIndexPolicy.get(exactKey(name));
+ return exactPolicy != null ? exactPolicy :
nameToIndexPolicy.get(normalizeKey(name));
+ }
+
+ // Callers hold either the read or write lock. BE dispatches a canonical
built-in analyzer, then an
+ // exact policy, then a built-in by normalized name; return a spelling
that reaches that built-in,
+ // or null for a policy.
+ private String resolveTopLevelBuiltinLocked(String name, Set<String>
builtins) {
+ String exactName = exactKey(name);
+ if (IndexPolicy.BUILTIN_ANALYZERS.contains(exactName) &&
builtins.contains(exactName)) {
Review Comment:
Confirmed and fixed in 50156ba8684.
`get_analyzer_name_from_properties()` takes the analyzer name and, when it
is empty, falls back to the normalizer property and returns it unchanged
(`inverted_index_parser.cpp:203-216`). `create_analyzer()` and
`create_analyzer_provider()` then test `is_builtin_analyzer()` before they
consult the policy manager (`analyzer.cpp:168-186,187-208`), so a normalizer
policy named `ik` is stepped over and the index is built with the built-in
analyzer, without anything telling the user.
One correction to the thread: the create path is already covered. The
`conflicts with built-in ...` checks in `createIndexPolicy` are not split by
policy type, so `CREATE INVERTED INDEX NORMALIZER ik` is rejected today. The
gap is the binding check, which a policy replayed from an older edit log
reaches without ever passing those checks. That is where the fix went - adding
it to the create path too would be dead code.
`validateNormalizerExists()` now rejects a binding whose policy name is a
built-in analyzer name, and the error says to rename the policy or use the
built-in instead. It deliberately does not silently treat the binding as the
built-in, since that is the behaviour this thread is about.
The comparison is exact and case sensitive, matching
`is_builtin_analyzer()`, which compares against the nine lower-case constants
directly (`analyzer.cpp:83-93`). A policy named `IK` is genuinely reachable
through BE's exact-name lookup, so it stays valid - there is a test for that.
Tests:
`PolicyValidatorTests.testNormalizerNamedAfterBuiltinAnalyzerIsUnreachable`
with `testExactCaseDistinctNormalizerPolicyRemainsReachable` as the negative,
plus
`InvertedIndexPropertiesTest.testCreateTableRejectsNormalizerNamedAfterBuiltinAnalyzer`
and
`SchemaChangeHandlerTest.testNormalizerNamedAfterBuiltinAnalyzerIsRejectedInDdl`
for CREATE TABLE and ALTER, each keeping a custom normalizer and the real
built-in `lowercase` working.
One limit worth stating: this rejects the binding at DDL analysis time. An
index created before this change keeps its metadata, and BE keeps reading it
with the built-in analyzer; migrating those is not something this change does.
##########
be/src/storage/index/inverted/inverted_index_iterator.cpp:
##########
@@ -164,8 +166,9 @@ Result<InvertedIndexReaderPtr>
InvertedIndexIterator::select_best_reader(
}
field_type = get_inverted_index_leaf_field_type(column_type);
}
- auto selection =
select_best_inverted_index_candidate(_selection_candidates, _key_to_entries,
- field_type,
query_type, normalized_key);
+ auto selection =
+ select_best_inverted_index_candidate(_selection_candidates,
_key_to_entries, field_type,
Review Comment:
Confirmed and fixed in 50156ba8684.
`InvertedIndexIterator::get_reader()` walks `_selection_candidates` and
returns the first entry whose `reader_type` matches, never looking at
`analyzer_key` (`inverted_index_iterator.cpp:209-221`), while the query itself
runs on whatever `select_best_reader(column_type, query_type, analyzer_key,
legacy_analyzer_key)` picks. So the preflight and the execution could look at
different indexes, and with two full-text indexes disagreeing about
`support_phrase` the index order alone decided whether a phrase query was
refused or ran against an index with no positions.
Rather than making the preflight analyzer-aware, the check moved to where
the executing reader is already known: `read_from_index()` applies it right
after `select_best_reader()` returns, for the three query types that read
positions (`is_phrase_query()`). The old check in
`FunctionMatchBase::evaluate_inverted_index()` is gone. Putting the invariant
at the single point that knows the selected reader means no caller can route
around it.
Test: `InvertedIndexIteratorTest.PhraseSupportIsCheckedOnTheSelectedReader`
builds the two indexes, adds them in both orders, and asserts that the
analyzer-selected reader carries the expected `support_phrase` either way -
while also asserting that the first candidate of the type, which is what the
old preflight read, flips with the ordering.
--
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]
