airborne12 commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4080703717
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/invertedindex/AnalyzerIdentityBuilder.java:
##########
@@ -17,22 +17,89 @@
package org.apache.doris.analysis.invertedindex;
+import org.apache.doris.analysis.InvertedIndexProperties;
import org.apache.doris.catalog.Env;
import org.apache.doris.indexpolicy.IndexPolicy;
import org.apache.doris.indexpolicy.IndexPolicyTypeEnum;
import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableSet;
+import com.ibm.icu.lang.UCharacter;
+import com.ibm.icu.text.UnicodeSet;
import org.apache.logging.log4j.Logger;
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Deque;
+import java.util.List;
+import java.util.Locale;
import java.util.Map;
+import java.util.Set;
import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.regex.Pattern;
public final class AnalyzerIdentityBuilder {
private static final String PROP_MAX_NGRAM_DIFF = "max_ngram_diff";
+ private static final String KEYWORD_TOKENIZER = "keyword";
+ private static final String CHAR_REPLACE_FILTER = "char_replace";
+ private static final String PROP_PATTERN = "pattern";
+ private static final String PROP_REPLACEMENT = "replacement";
+ // Defaults CharReplaceCharFilterFactory applies to a bare built-in
reference.
+ private static final String CHAR_REPLACE_DEFAULT_PATTERN = ",._";
+ private static final String CHAR_REPLACE_DEFAULT_REPLACEMENT = " ";
+ // Token filters that emit the same terms, offsets and provenance when
applied twice in a row.
+ private static final Set<String> IDEMPOTENT_TOKEN_FILTERS =
ImmutableSet.of("lowercase", "asciifolding");
Review Comment:
Half of this holds and is fixed in ce8505fa6e7; the other half does not, and
we would rather show you why than quietly skip it.
**icu_normalizer: agreed, collapsed.** A bare reference is `name=nfkc_cf`
with an empty `unicode_set_filter`, and an empty set means the base normalizer
is used directly rather than wrapped in a `FilteredNormalizer2`.
`ICUNormalizerFilter` sets `_text_changed` by comparing the actual input and
output bytes, so on the second pass it is false and both the exact
per-code-point map and the conservative span are delegated upstream unchanged.
NFKC_CF being a fixed point is pinned by a test that runs every non-surrogate
code point through `Normalizer2.getNFKCCasefoldInstance()` twice and asserts
the second pass never changes anything.
**word_delimiter: not idempotent, so it stays out.** The two scans disagree
about malformed bytes. `WordDelimiterIterator::next()` hits `c < 0` and does
`_end++; continue;`, so a malformed byte is absorbed into the current word,
while the backward scan in `set_bounds()` does `pos--; continue;`, so
`_end_bounds` stops before it. A subword that ends in a malformed byte
therefore fails the `_iterator->_end == term_length` passthrough on the next
pass and gets re-cut by `generate_part()`, losing that byte.
Measured, not just read:
`WordDelimiterFilterTest.RepeatedFilterDropsTrailingMalformedByte` feeds
`"abc\xFF def"` through a keyword tokenizer and asserts one pass emits
`abc\xFF`, `def` while two passes emit `abc`, `def`. This is not a hypothetical
input for this PR - ce8505fa6e7's sibling commit exists precisely to keep
indexing malformed bytes. Collapsing the two identities would make the fences
reject a pair of indexes that really do produce different terms.
Coverage: `testAdjacentDuplicateIcuNormalizerFiltersCollapse` for the
positive side plus non-adjacent, `name=nfc` and `unicode_set_filter=[a-z]`
negatives, `testAdjacentDuplicateWordDelimiterFiltersKeepDistinctIdentities` to
pin the exclusion, and both DDL paths in `InvertedIndexPropertiesTest` and
`SchemaChangeHandlerTest`.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/invertedindex/AnalyzerIdentityBuilder.java:
##########
@@ -226,26 +925,400 @@ private static String resolveTokenFilterIdentity(String
filterList) {
* IMPORTANT: Order is preserved because filter order is semantically
significant.
*/
private static String resolveCharFilterIdentity(String filterList) {
+ return resolveCharFilterIdentity(filterList, null);
+ }
+
+ private static String resolveCharFilterIdentity(String filterList,
FoldContext downstreamFold) {
+ ArrayDeque<String> identities = new ArrayDeque<>();
+ walkCharFilters(filterList, downstreamFold, identities);
+ return String.join(",", identities);
+ }
+
+ /**
+ * Resolve the chain from its last filter to its first, collecting
identities, and return the
+ * case-folding context that a filter placed in front of the chain would
run in.
+ */
+ private static FoldContext walkCharFilters(
+ String filterList, FoldContext downstreamFold, Deque<String>
identities) {
+ FoldContext fold = downstreamFold;
if (Strings.isNullOrEmpty(filterList)) {
- return "";
+ return fold;
}
- StringBuilder sb = new StringBuilder();
String[] filters = filterList.split(",\\s*");
// DO NOT sort - filter order is semantically significant
- for (int i = 0; i < filters.length; i++) {
- String filter = filters[i].trim();
- if (i > 0) {
- sb.append(",");
+ for (int i = filters.length - 1; i >= 0; --i) {
+ String filterName = filters[i].trim();
+ String filter = resolveComponentIdentity(filterName,
IndexPolicyTypeEnum.CHAR_FILTER, fold);
+ if (Strings.isNullOrEmpty(filter)) {
+ continue;
}
+ identities.addFirst(filter);
Review Comment:
Confirmed and fixed in ce8505fa6e7.
`CharReplaceCharFilter::process_pattern()` walks the buffer and does `if
(_patterns.test(uc)) c = _replacement[0];` - an independent per-byte
substitution, one byte for one byte, with the replacement constrained to a
single byte by the factory. That makes it idempotent for any configuration,
including the case you call out: after the first pass the only pattern byte
that can remain is the replacement byte itself, and it maps to itself, so the
second pass is the identity. The filter does not override `correct_offset` or
`correct_start_offset`, so both layers use the default delegation and a
two-layer chain corrects offsets exactly like one layer.
The collapse is in `walkCharFilters()` rather than the token filter path,
keyed on the canonical identity of the immediately preceding entry and limited
to a usable `char_replace` (`charReplaceSourceBytes` returning non-null). Order
is preserved, and the skipped entry still contributes its fold context, which
is harmless because blocking the same bytes twice is idempotent.
Test: `testAdjacentDuplicateCharReplaceFiltersCollapse`, covering bare
repeats, a named policy that restates the factory defaults, and a named
non-default pair, with three negatives - separated by another filter, a
different replacement, and a repeated `icu_normalizer` char filter, which is
not proven idempotent on this path.
--
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]
