[GitHub] [lucene] magibney opened a new pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney opened a new pull request #15:
URL: https://github.com/apache/lucene/pull/15


   Continuation of: [lucene-solr PR #892](https://github.com/apache/lucene-solr/pull/892)
   See also: [LUCENE-8972](https://issues.apache.org/jira/browse/LUCENE-8972)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-832365501


   That makes sense; apologies for the rough state wrt precommit (though fwiw the _tests_  have been my focus, and those should be solid). I'll get precommit passing with any necessary comment formatting handled manually.
   
   Unless you suggest otherwise I'll also rip out all the "rollback"-approach stuff (related to the original approach taken in this PR). It was helpful during development to have that as a point of reference, but it ultimately should not be committed, and at this point I'm confident enough in the streaming approach that the "rollback" stuff has probably outlived its usefulness (and it'll be in the commit history if anyone feels a need to crosscheck against it).


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney edited a comment on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney edited a comment on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-799579642


   Thanks for looking at this, @rmuir! And no worries wrt "questions already answered" -- it's been long enough that this all feels fresh to me again, for better or worse :-)
   
   The Traditional->Simplified use case is a good example of where this type of functionality (however accomplished) is really necessary, because of the significance of dictionary-based tokenization for these scripts. It sounds promising to map the transliteration file onto a file for MappingCharFilter. Based on what you say, it looks like that would be viable for Traditional->Simplified. I think that approach would also address some of the weird issues this PR had to work around wrt offset resolution in composite transliterators. I wonder whether the same approach could apply to other scripts that commonly use dictionary-based tokenization?
   
   Perhaps this is what you were suggesting, but if the "MappingCharFilter" approach were used as an optimization, while still preserving generic ICUTransformCharFilter directly backed by ICU Transliterator, it'd be possible to test for consistency between the two approaches, while preserving "vanilla" ICUTransformCharFilter for unanticipated use cases, etc. ...
   
   True, `assumeExternalUnicodeNormalization` gets you into "shoot yourself in the foot" territory for sure, but results in roughly 4x performance improvement (based on some naive but I think representative benchmarks). [This comment](https://github.com/apache/lucene-solr/pull/892#issuecomment-537538822) describes the approach, and the [comment immediately following](https://github.com/apache/lucene-solr/pull/892#issuecomment-537601926) confirms results (using `Cyrillic-Latin` as an example). Regarding which transformations this applies to (from the first linked comment):
   >NFC, as a trailing transformation step, is both very common and very active -- active in the sense that it will in many common contexts block output waiting for combining diacritics for literally almost every character
   
   I'm inclined to think the significant performance gain for such a common case is worth it -- as a user I'd certainly not want that type of functionality hidden from me. I wonder if there's a way to "have cake and eat it too" ...
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r596889938



##########
File path: lucene/analysis/icu/src/java/org/apache/lucene/analysis/icu/ICUTransformCharFilter.java
##########
@@ -0,0 +1,686 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.analysis.icu;
+
+import com.ibm.icu.impl.Utility;
+import com.ibm.icu.text.ReplaceableString;
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.Transliterator.Position;
+import com.ibm.icu.text.UTF16;
+import java.io.IOException;
+import java.io.Reader;
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.WeakHashMap;
+import org.apache.lucene.analysis.CharFilter;
+import org.apache.lucene.analysis.charfilter.BaseCharFilter;
+import org.apache.lucene.util.ArrayUtil;
+
+/**
+ * A {@link CharFilter} that transforms text with ICU.
+ *
+ * <p>ICU provides text-transformation functionality via its Transliteration API. Although script
+ * conversion is its most common use, a Transliterator can actually perform a more general class of
+ * tasks. In fact, Transliterator defines a very general API which specifies only that a segment of
+ * the input text is replaced by new text. The particulars of this conversion are determined
+ * entirely by subclasses of Transliterator.
+ *
+ * <p>Some useful transformations for search are built-in:
+ *
+ * <ul>
+ *   <li>Conversion from Traditional to Simplified Chinese characters
+ *   <li>Conversion from Hiragana to Katakana
+ *   <li>Conversion from Fullwidth to Halfwidth forms.
+ *   <li>Script conversions, for example Serbian Cyrillic to Latin
+ * </ul>
+ *
+ * <p>Example usage:
+ *
+ * <blockquote>
+ *
+ * stream = new ICUTransformCharFilter(reader,
+ * Transliterator.getInstance("Traditional-Simplified"));
+ *
+ * </blockquote>
+ *
+ * <br>
+ * For more details, see the <a href="http://userguide.icu-project.org/transforms/general">ICU User
+ * Guide</a>.
+ */
+public final class ICUTransformCharFilter extends BaseCharFilter {
+
+  // Transliterator to transform the text
+  private final Transliterator transform;
+
+  // Reusable position object
+  private final Position position = new Position();
+
+  private static final int READ_BUFFER_SIZE = 1024;
+  private final char[] tmpBuffer = new char[READ_BUFFER_SIZE];
+
+  private static final int INITIAL_TRANSLITERATE_BUFFER_SIZE = 1024;
+  private final StringBuffer buffer = new StringBuffer(INITIAL_TRANSLITERATE_BUFFER_SIZE);
+  private final ReplaceableString replaceable = new ReplaceableString(buffer);
+
+  private static final int BUFFER_PRUNE_THRESHOLD = 1024;
+
+  private int outputCursor = 0;
+  private boolean inputFinished = false;
+  private int charCount = 0;
+  private int offsetDiffAdjust = 0;
+
+  private static final int HARD_MAX_ROLLBACK_BUFFER_CAPACITY =
+      Integer.highestOneBit(Integer.MAX_VALUE);
+  static final int DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY = 8192;
+  private final int maxRollbackBufferCapacity;
+
+  private static final int DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY = 4; // must be power of 2
+  private char[] rollbackBuffer;
+  private int rollbackBufferSize = 0;
+
+  static final boolean DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW = true;
+  private final boolean failOnRollbackBufferOverflow;
+
+  ICUTransformCharFilter(Reader in, Transliterator transform) {
+    this(
+        in,
+        transform,
+        DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+        DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW);
+  }
+
+  /**
+   * Construct new {@link ICUTransformCharFilter} with the specified {@link Transliterator}, backed
+   * by the specified {@link Reader}.
+   *
+   * @param in input source
+   * @param transform used to perform transliteration
+   * @param maxRollbackBufferCapacityHint used to control the maximum size to which this {@link
+   *     ICUTransformCharFilter} will buffer and rollback partial transliteration of input
+   *     sequences. The provided hint will be converted to an enforced limit of "the greatest power
+   *     of 2 (excluding '1') less than or equal to the specified value". It is illegal to specify a
+   *     negative value. There is no power of 2 greater than <code>
+   *     Integer.highestOneBit(Integer.MAX_VALUE))</code>, so to prevent overflow, values in this
+   *     range will resolve to an enforced limit of <code>Integer.highestOneBit(Integer.MAX_VALUE))
+   *     </code>. Specifying "0" (or "1", in practice) disables rollback. Larger values can in some
+   *     cases yield more accurate transliteration, at the cost of performance and
+   *     resolution/accuracy of offset correction. This is intended primarily as a failsafe, with a
+   *     relatively large default value of {@value
+   *     ICUTransformCharFilter#DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY}. See comments "To understand
+   *     the need for rollback" in private method: {@link
+   *     Transliterator#filteredTransliterate(com.ibm.icu.text.Replaceable, Position, boolean,
+   *     boolean)}
+   * @param failOnRollbackBufferOverflow if true, transliteration failing to complete within the
+   *     specified rollbackBuffer window will fail with an exception. If false, transliteration
+   *     forges ahead with possibly-inconsistent results.
+   */
+  ICUTransformCharFilter(
+      Reader in,
+      Transliterator transform,
+      int maxRollbackBufferCapacityHint,
+      boolean failOnRollbackBufferOverflow) {
+    super(in);
+    this.transform = ICUTransformFilter.optimizeForCommonCase(transform);
+    this.failOnRollbackBufferOverflow = failOnRollbackBufferOverflow;
+    if (maxRollbackBufferCapacityHint < 0) {
+      throw new IllegalArgumentException(
+          "It is illegal to request negative rollback buffer max capacity");
+    } else if (maxRollbackBufferCapacityHint >= HARD_MAX_ROLLBACK_BUFFER_CAPACITY) {
+      // arg is positive, so user wants the largest possible buffer capacity limit
+      // we know what that is (static), protecting for overflow.
+      this.maxRollbackBufferCapacity = HARD_MAX_ROLLBACK_BUFFER_CAPACITY;
+      this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+    } else {
+      // greatest power of 2 (excluding "1") less than or equal to the specified hint
+      this.maxRollbackBufferCapacity =
+          Integer.highestOneBit(maxRollbackBufferCapacityHint - 1) << 1;
+      if (this.maxRollbackBufferCapacity == 0) {
+        this.rollbackBuffer = null;
+      } else {
+        this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+      }
+    }
+  }
+
+  /**
+   * Reads characters into a portion of an array. This method will block until some input is
+   * available, an I/O error occurs, or the end of the stream is reached.
+   *
+   * @param cbuf Destination buffer
+   * @param off Offset at which to start storing characters
+   * @param len Maximum number of characters to read
+   * @return The number of characters read, or -1 if the end of the stream has been reached
+   * @throws IOException If an I/O error occurs
+   */
+  @Override
+  public int read(char[] cbuf, int off, int len) throws IOException {
+    while (!inputFinished || position.start > outputCursor) {
+      // (expecting more input) || (output remains that has not been flushed)
+      if (position.start > outputCursor) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      int resLen = transliterateBufferContents();
+      if (resLen > 0) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      if (!readInputToBuffer()) {
+        final int preStart = position.start;
+        final int preLimit;
+        final int bufferLength = buffer.length();
+        if (preStart < bufferLength) {
+          if (UTF16.isLeadSurrogate(buffer.charAt(bufferLength - 1))) {
+            // if last char is a lead surrogate, transform won't handle it properly
+            // preLimit = bufferLength - 1; // could simply pass the malformed input along
+            throw new RuntimeException(
+                "malformed input; final UTF16 character is a lead surrogate");
+          } else {
+            preLimit = bufferLength;
+          }
+          position.contextLimit = preLimit;
+          position.limit = preLimit;
+          transform.finishTransliteration(replaceable, position);
+        } else if (offsetDiffAdjust == 0) {
+          break;
+        } else {
+          preLimit = bufferLength;
+        }
+        cursorAdvanced(preStart, preLimit);
+      }
+    }
+
+    return -1;
+  }
+
+  private void rollback(int preStart, int preLimit) {
+    buffer.delete(preStart, position.limit); // delete uncommitted chars
+    buffer.insert(preStart, rollbackBuffer, 0, rollbackBufferSize);
+    position.start = preStart;
+    position.contextLimit = preLimit;
+    position.limit = preLimit;
+  }
+
+  /**
+   * Grow rollback buffer if necessary, within constraints of {@link #maxRollbackBufferCapacity}.
+   * This imposes an arbitrary failsafe to prevent the possibility that the rollback buffer could
+   * grow indefinitely.
+   *
+   * @return true if upon return, rollback buffer has sufficient capacity for new input chars,
+   *     otherwise false.
+   */
+  private boolean ensureRollbackBufferCapacity() {
+    // ensure space for at least 2 chars (surrogate pair, max possible space needed)
+    if (rollbackBuffer.length - rollbackBufferSize < 2) {
+      if (rollbackBuffer.length < maxRollbackBufferCapacity) {
+        rollbackBuffer = ArrayUtil.growExact(rollbackBuffer, rollbackBuffer.length << 1);
+      } else {
+        // hit threshold; not going to increase the buffer size
+        if (failOnRollbackBufferOverflow) {
+          throw new RuntimeException(
+              "input could not be transliterated without overflowing maxRollbackBufferCapacity ("
+                  + maxRollbackBufferCapacity
+                  + "); "
+                  + "try increasing maxRollbackBufferCapacity, or setting failOnRollbackBufferOverflow=false");
+        }
+        return false;
+      }
+    }
+    return true;
+  }
+
+  private int pushRollbackBuffer(int idx, int bufferLength) {
+    if (idx >= bufferLength) {
+      return 0;
+    } else {
+      // note: we have already ensured sufficient buffer capacity
+      final char candidate = buffer.charAt(idx);
+      if (!UTF16.isLeadSurrogate(candidate)) {
+        if (rollbackBuffer != null) {
+          rollbackBuffer[rollbackBufferSize++] = candidate;
+        }
+        return 1;
+      } else {
+        if (++idx < bufferLength) {
+          if (rollbackBuffer != null) {
+            rollbackBuffer[rollbackBufferSize++] = candidate;
+            rollbackBuffer[rollbackBufferSize++] = buffer.charAt(idx);
+          }
+          return 2;
+        } else {
+          // we don't yet have the high surrogate
+          if (inputFinished) {
+            // no more input available; proceed with the char we have; rollback not needed
+            return 1;
+          } else {
+            return 0; // wait for more input to be available
+          }
+        }
+      }
+    }
+  }
+
+  /**
+   * Transliterate as much of the contents of {@link #buffer} as possible.
+   *
+   * @return number of output characters transliterated (possibly 0)
+   */
+  private int transliterateBufferContents() {
+    int nextCharLength = pushRollbackBuffer(position.limit, buffer.length());
+    if (nextCharLength == 0) {
+      return 0;
+    }
+    final int preCharCount = charCount;
+    int preStart = position.start;
+    int preLimit;
+    do {
+      position.limit += nextCharLength;
+      preLimit = position.limit;
+      position.contextLimit = preLimit;
+      transform.filteredTransliterate(replaceable, position, true);
+      boolean rollbackSizeWithinBounds = true;
+      if (rollbackBuffer != null
+          && position.start < position.limit
+          && (rollbackSizeWithinBounds = ensureRollbackBufferCapacity())) {
+        // complete pass not transliterated, and not yet at rollback buffer threshold cap.
+        // N.b.: rollback buffer threshold cap is arbitrary, so we check here (somewhat
+        // counterintuitively, *before* rolling back, as opposed to before pushing to
+        // the buffer) so that we can fall through to the else clause, clear the rollback
+        // buffer, and proceed.
+        rollback(preStart, preLimit);
+      } else {
+        rollbackBufferSize = 0;
+        if (position.start > preStart) {
+          // cursor advanced
+          cursorAdvanced(preStart, preLimit);
+
+          // N.b.: advancing contextStart precludes support for quantifiers, but is crucial for
+          // streaming,
+          // so we'll do it anyway.
+          // See comments in Transliterator source code:
+          // TODO
+          // This doesn't work once we add quantifier support.  Need to rewrite
+          // this code to support quantifiers and 'use maximum backup <n>;'.
+          //
+          //         index.contextStart = Math.max(index.start - getMaximumContextLength(),
+          //                                       originalStart);
+          position.contextStart =
+              Math.max(position.start - transform.getMaximumContextLength(), preStart);
+          preStart = position.start;
+          if (!rollbackSizeWithinBounds) {
+            // prepopulate newly cleared rollback buffer with all top-level uncommitted characters
+            rollbackBufferSize = position.limit - preStart;
+            if (rollbackBuffer.length - rollbackBufferSize < 2) {
+              // even after flushing all committed text, there's not enough space in the rollback
+              // buffer.
+              // This is an edge case of an edge case, when the last char32 read into the rollback
+              // buffer
+              // is a surrogate pair (completely filling the rollback buffer), *and* the last
+              // transliteration pass advanced position.start by exactly one char16 (not a surrogate
+              // pair).
+              preStart = forceAdvance(preStart, preLimit);
+              rollbackBufferSize = position.limit - preStart;
+            }
+            buffer.getChars(preStart, position.limit, rollbackBuffer, 0);
+          }
+        } else if (preLimit != position.limit) {
+          // cursor hasn't advanced; incoming characters have probably been deleted
+          offsetDiffAdjust += preLimit - position.limit;
+          // edge case of !rollbackSizeWithinBounds needs no special handling here, since input
+          // characters *are* being
+          // processed (deleted) -- we *are* progressing through the input stream, although the
+          // output stream hasn't changed.
+        } else if (!rollbackSizeWithinBounds) {
+          // cursor hasn't advanced, no incoming characters have been deleted, and the rollback
+          // buffer is full.
+          preStart = forceAdvance(preStart, preLimit);
+        }
+      }
+    } while ((nextCharLength = pushRollbackBuffer(position.limit, buffer.length())) > 0);
+
+    return charCount - preCharCount;
+  }
+
+  private static final int FORCE_THRESHOLD = 2; // conservative; the length of a surrogate pair
+
+  private int forceAdvance(int preStart, int preLimit) {
+    int shift;
+    if (maxRollbackBufferCapacity == 2) {
+      rollbackBufferSize = 0;
+      shift = 2;
+    } else {
+      shift = 0;
+      do {
+        shift += UTF16.isLeadSurrogate(rollbackBuffer[shift]) ? 2 : 1;
+      } while (shift < FORCE_THRESHOLD && shift < maxRollbackBufferCapacity);
+      rollbackBufferSize -= shift;
+      System.arraycopy(rollbackBuffer, shift, rollbackBuffer, 0, rollbackBufferSize);
+    }
+    position.start += shift; // mock transliterator advance
+    cursorAdvanced(preStart, preLimit);
+    position.contextStart =
+        Math.max(position.start - transform.getMaximumContextLength(), preStart);
+    return position.start;
+  }
+
+  private void cursorAdvanced(int preStart, int preLimit) {
+    final int outputLength = position.start - preStart;
+    final int diff = preLimit - position.limit + offsetDiffAdjust;
+    offsetDiffAdjust = 0;
+    if (diff == 0) {
+      // increment charCount; no offset correction necessary
+      charCount += outputLength;
+    } else {
+      // limit change indicates change in length of replacement text; correct offsets accordingly
+      // we correct offsets as frequently as possible, for every incremental change. This will
+      // sometimes be for a range of characters, but that's the best we can do as far as the level
+      // of granularity that's available to us via the Transliterator API.
+      recordOffsetDiff(diff, outputLength);
+    }
+  }
+
+  private void recordOffsetDiff(int diff, int outputLength) {
+    final int cumuDiff = getLastCumulativeDiff();
+    if (diff < 0) {
+      // positive diff indicates an increase in character count wrt input
+      for (int i = 1; i <= -diff; ++i) {
+        addOffCorrectMap(charCount + i, cumuDiff - i);
+      }
+    } else {
+      // positive diff indicates an decrease in character count wrt input
+      addOffCorrectMap(charCount + outputLength, cumuDiff + diff);
+    }
+    charCount += outputLength;
+  }
+
+  private boolean readInputToBuffer() throws IOException {
+    int res = input.read(tmpBuffer, 0, tmpBuffer.length);
+    if (res == -1) {
+      inputFinished = true;
+      return false;
+    } else {
+      buffer.append(tmpBuffer, 0, res);
+      return true;
+    }
+  }
+
+  private int outputFromResultBuffer(char[] cbuf, int begin, int len) {
+    len = Math.min(position.start - outputCursor, len);
+    buffer.getChars(outputCursor, outputCursor + len, cbuf, begin);
+    outputCursor += len;
+    if (outputCursor > BUFFER_PRUNE_THRESHOLD && position.contextStart > BUFFER_PRUNE_THRESHOLD) {
+      final int pruneSize = Math.min(outputCursor, position.contextStart);
+      buffer.delete(0, pruneSize);
+      position.contextStart -= pruneSize;
+      position.start -= pruneSize;
+      position.limit -= pruneSize;
+      position.contextLimit -= pruneSize;
+      outputCursor -= pruneSize;
+    }
+    return len;
+  }
+
+  /**
+   * Strips off Unicode character normalization form rules from a Transliterator. Do this if, as is
+   * typical, your analysis chain already includes another component that does normalization, like
+   * {@link ICUNormalizer2CharFilter}.
+   *
+   * @return a new Transliterator with no normalization, or the original Transliterator if it
+   *     already did no normalization.
+   */
+  public static Transliterator withoutUnicodeNormalization(Transliterator transliterator) {
+    final String modifiedRules = modifyRules(false, transliterator);
+    if (modifiedRules == null) {
+      return transliterator;
+    }
+    String baseId = transliterator.getID();
+    String modId = baseId.concat(baseId.lastIndexOf('/') < 0 ? "/X_NO_NORM_IO" : "_X_NO_NORM_IO");
+    return Transliterator.createFromRules(modId, modifiedRules, Transliterator.FORWARD);
+  }

Review comment:
       Before embarking on this, I have one question: in considering a similar approach [here](https://github.com/apache/lucene-solr/pull/892#issuecomment-563293051), I noted that I couldn't find any precedent for this type of under-the-hood analysis-chain rewriting. Am I wrong about the lack of precedent for an analysis chain component factory basically rewriting the analysis chain to introduce extra components? I'd like to follow precedent if it exists, particularly wrt whether the factory introduces multiple components _directly_ on the analysis chain, or attempts to "black box" wrap multiple components into a single "composite" component (from the perspective of the analysis chain). Do you have any preference/suggestions?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r598735719



##########
File path: lucene/analysis/icu/src/java/org/apache/lucene/analysis/icu/ICUTransformCharFilterFactory.java
##########
@@ -0,0 +1,391 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.icu;
+
+import com.ibm.icu.impl.Utility;
+import com.ibm.icu.text.Normalizer2;
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.UTF16;
+import java.io.Reader;
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.analysis.CharFilterFactory;
+
+/**
+ * Factory for {@link ICUTransformCharFilter}.
+ *
+ * <p>Supports the following attributes:
+ *
+ * <ul>
+ *   <li>id (mandatory): A Transliterator ID, one from {@link Transliterator#getAvailableIDs()}
+ *   <li>direction (optional): Either 'forward' or 'reverse'. Default is forward.
+ * </ul>
+ *
+ * @see Transliterator
+ * @since 8.3.0
+ * @lucene.spi {@value #NAME}
+ */
+public class ICUTransformCharFilterFactory extends CharFilterFactory {
+
+  /** SPI name */
+  public static final String NAME = "icuTransform";
+
+  static final String MAX_ROLLBACK_BUFFER_CAPACITY_ARGNAME = "maxRollbackBufferCapacity";
+  static final String FAIL_ON_ROLLBACK_BUFFER_OVERFLOW_ARGNAME = "failOnRollbackBufferOverflow";
+  static final String SUPPRESS_UNICODE_NORMALIZATION_EXTERNALIZATION_ARGNAME =
+      "suppressUnicodeNormalizationExternalization";
+  private final NormType leading;
+  private final Transliterator transliterator;
+  private final NormType trailing;
+  private final int maxRollbackBufferCapacity;
+  private final boolean failOnRollbackBufferOverflow;
+
+  // TODO: add support for custom rules
+  /** Creates a new ICUTransformFilterFactory */
+  public ICUTransformCharFilterFactory(Map<String, String> args) {
+    super(args);
+    String id = require(args, "id");
+    String direction =
+        get(args, "direction", Arrays.asList("forward", "reverse"), "forward", false);
+    int dir = "forward".equals(direction) ? Transliterator.FORWARD : Transliterator.REVERSE;
+    int tmpCapacityHint =
+        getInt(
+            args,
+            MAX_ROLLBACK_BUFFER_CAPACITY_ARGNAME,
+            ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY);
+    this.maxRollbackBufferCapacity = tmpCapacityHint == -1 ? Integer.MAX_VALUE : tmpCapacityHint;
+    this.failOnRollbackBufferOverflow =
+        getBoolean(
+            args,
+            FAIL_ON_ROLLBACK_BUFFER_OVERFLOW_ARGNAME,
+            ICUTransformCharFilter.DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW);
+    boolean suppressUnicodeNormalizationExternalization =
+        getBoolean(args, SUPPRESS_UNICODE_NORMALIZATION_EXTERNALIZATION_ARGNAME, false);
+    Transliterator stockTransliterator = Transliterator.getInstance(id, dir);
+    if (suppressUnicodeNormalizationExternalization) {
+      this.leading = null;
+      this.transliterator = stockTransliterator;
+      this.trailing = null;
+    } else {
+      ExternalNormalization ext = externalizeUnicodeNormalization(stockTransliterator);
+      this.leading = ext.leading;
+      this.transliterator = ext.t;
+      this.trailing = ext.trailing;
+    }
+    if (!args.isEmpty()) {
+      throw new IllegalArgumentException("Unknown parameters: " + args);
+    }
+  }
+
+  private static final Reader wrapReader(NormType normType, Reader r) {
+    if (normType == null) {
+      return r;
+    }
+    switch (normType) {
+      case NFC:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFCInstance());
+      case NFD:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFDInstance());
+      case NFKC:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFKCInstance());
+      case NFKD:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFKDInstance());
+      default:
+        throw new UnsupportedOperationException(
+            "test not yet able to compensate externally for normalization type \""
+                + normType
+                + "\"");
+    }
+  }
+
+  /** Default ctor for compatibility with SPI */
+  public ICUTransformCharFilterFactory() {
+    throw defaultCtorException();
+  }
+
+  @Override
+  public Reader create(Reader input) {
+    input = wrapReader(leading, input);
+    input =
+        new ICUTransformCharFilter(
+            input, transliterator, maxRollbackBufferCapacity, failOnRollbackBufferOverflow);
+    return wrapReader(trailing, input);
+  }
+
+  private static final boolean ESCAPE_UNPRINTABLE = false;
+
+  /**
+   * Attempts to detect and externalize any leading or trailing top-level Unicode normalization. In
+   * the event that such normalization is detected, a new "core" Transliterator (with any detected
+   * pre-/post-normalization removed) is created and returned.
+   *
+   * <p>The creation of any new "core" Transliterator (and much of the actual code in this method)
+   * is based on the {@link com.ibm.icu.text.CompoundTransliterator#toRules(boolean)} method (with
+   * the boolean arg replaced by {@link #ESCAPE_UNPRINTABLE} -- always <code>false</code> in this
+   * context).
+   *
+   * @param t the Transliterator to base modified rules on.
+   * @return simple ExternalNormalization struct containing a non-null Transliterator, if possible
+   *     with any leading and trailing Unicode normalization externalized. The effect of applying
+   *     the resulting leading Unicode norm, Transliterator, and trailing Unicode norm, should be
+   *     equivalent to the effect of applying the input Transliterator t.
+   */
+  private static ExternalNormalization externalizeUnicodeNormalization(Transliterator t) {
+    final Transliterator[] trans = t.getElements();
+    final String topLevelId = t.getID();
+    final int start;
+    final int limit;
+    final NormType leading;
+    final NormType trailing;
+    if (trans.length == 1) {
+      warnNestedUnicodeNormalization(trans[0], topLevelId, true);
+      return new ExternalNormalization(null, t, null);
+    } else {
+      final int lastIndex;
+      if ((leading = unicodeNormalizationType(trans[0].getID())) != null) {
+        start = 1;
+        limit =
+            (trailing = unicodeNormalizationType(trans[lastIndex = trans.length - 1].getID()))
+                    != null
+                ? lastIndex
+                : trans.length;
+      } else if (warnNestedUnicodeNormalization(trans[0], topLevelId, true)
+          && (trailing = unicodeNormalizationType(trans[lastIndex = trans.length - 1].getID()))
+              != null) {
+        start = 0;
+        limit = lastIndex;
+      } else {
+        warnNestedUnicodeNormalization(trans[trans.length - 1], topLevelId, false);
+        return new ExternalNormalization(null, t, null);
+      }
+    }
+    // We do NOT call toRules() on our component transliterators, in
+    // general. If we have several rule-based transliterators, this
+    // yields a concatenation of the rules -- not what we want. We do
+    // handle compound RBT transliterators specially -- those for which
+    // compoundRBTIndex >= 0. For the transliterator at compoundRBTIndex,
+    // we do call toRules() recursively.
+    StringBuilder rulesSource = new StringBuilder();
+    if (t.getFilter() != null) {
+      // We might be a compound RBT and if we have a global
+      // filter, then emit it at the top.
+      rulesSource.append("::").append(t.getFilter().toPattern(ESCAPE_UNPRINTABLE)).append(ID_DELIM);
+    }
+    final int globalFilterEnd = rulesSource.length();
+    boolean hasAnonymousRBTs = false;
+    for (int i = start; i < limit; ++i) {
+      String rule;
+
+      // Anonymous RuleBasedTransliterators (inline rules and
+      // ::BEGIN/::END blocks) are given IDs that begin with
+      // "%Pass": use toRules() to write all the rules to the output
+      // (and insert "::Null;" if we have two in a row)
+      if (trans[i].getID().startsWith("%Pass")) {
+        hasAnonymousRBTs = true;
+        rule = trans[i].toRules(ESCAPE_UNPRINTABLE);
+        if (i > start && trans[i - 1].getID().startsWith("%Pass")) rule = "::Null;" + rule;
+
+        // we also use toRules() on CompoundTransliterators (which we
+        // check for by looking for a semicolon in the ID)-- this gets
+        // the list of their child transliterators output in the right
+        // format
+      } else if (trans[i].getID().indexOf(';') >= 0) {
+        rule = trans[i].toRules(ESCAPE_UNPRINTABLE);
+
+        // for everything else, use baseToRules()
+      } else {
+        rule = baseToRules(ESCAPE_UNPRINTABLE, trans[i]);
+      }
+      _smartAppend(rulesSource, '\n');
+      rulesSource.append(rule);
+      _smartAppend(rulesSource, ID_DELIM);
+    }
+    // Analogous to the contract for {@link com.ibm.icu.text.Transliterator#toRules(boolean)}, the
+    // modified rules String should be sufficient to recreate a Transliterator based on the
+    // specified input Transliterator, via {@link
+    // com.ibm.icu.text.Transliterator#createFromRules(String, String, int)}.
+    final String modifiedRules =
+        hasAnonymousRBTs ? rulesSource.toString() : rulesSource.substring(globalFilterEnd);
+    String baseId = t.getID();
+    String modId = baseId.concat(baseId.lastIndexOf('/') < 0 ? "/X_NO_NORM_IO" : "_X_NO_NORM_IO");
+    Transliterator replacement =
+        Transliterator.createFromRules(modId, modifiedRules, Transliterator.FORWARD);
+    return new ExternalNormalization(leading, replacement, trailing);
+  }
+
+  static class ExternalNormalization {
+    private final NormType leading;
+    private final Transliterator t;
+    private final NormType trailing;
+
+    private ExternalNormalization(NormType leading, Transliterator t, NormType trailing) {
+      this.leading = leading;
+      this.t = t;
+      this.trailing = trailing;
+    }
+  }
+
+  /**
+   * It is possible that leading and trailing (or singleton) Transliterators might apply nested
+   * Unicode normalization, thus acting in the capacity of a Normalizer, without qualifying as a
+   * top-level Normalizer as currently defined in {@link #unicodeNormalizationType(String)}. For
+   * now, we will emit a warning if users request stripping of i/o unicode normalization in such a
+   * case (to facilitate reporting and more nuanced handling in the future, if necessary).
+   *
+   * @param levelOne a level one component Transliterator to test for nested unicode normalization.
+   * @param topLevelId top level Transliterator parent id.
+   * @param leading true if leading component transliterator; false implies trailing component
+   * @return always return true, for easy integration with control flow.
+   */
+  private static boolean warnNestedUnicodeNormalization(

Review comment:
       Yes, that makes sense. This portion of the code was left over from the "assumeExternalUnicodeNormalization" approach (managed by the user) that we're now moving away from. There are probably other parts of the code that should be similarly removed or modified as a consequence of this change, but the commits through c86aa55 really just focused on initial refactoring.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-800745487


   I looked, I don't think the normalization optimizations here are safe. Many rulesets assume a certain form for a reason, because the rules work that way. For example, Hangul-Latin works by decomposing into Jamo. Rules are only there for Jamo and then it composes back. If you remove the normalization rules, Hangul won't get transliterated.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-801001464


   > My conception of this arg is as an "expert" option that can significantly improve performance, but requires users to know (and externally compensate for, with specific ICUNormalizer2CharFilters in the top-level analysis chain) the specific type of input and output unicode normalization applied by a particular transliterator.
   
   No, most users using this stuff have brains of rocks. We can't have such unsafe traps like this.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-799147946


   thanks for updating the PR! Its late tonight, but I will look at this this week. I think I haven't looked since the patch so I am sure there are some "interesting" things that had to be done for any kind of good performance... Sorry if I ask dumb questions you have already answered...!
   
   I think its good to have the charfilter especially for the more efficient transforms like `XYZ>Latin`... I think we should mainly optimize the transliterator code for those type of transforms.
   
   The JIRA Issue discusses stuff like Traditional->Simplified as the use-case, but I am not sure I would make major sacrifices to try to speedup Traditional->Simplified. At least in the past this one was slow as a transliterator, but looking at the rules, maybe it really shouldnt be a transliterator at all: https://github.com/unicode-org/cldr/blob/master/common/transforms/Simplified-Traditional.xml. This file is like 4k lines of simple mappings (longest match) and out of thousands, only 6 context-based rules are used (ScDigit: 16 chars in the set). So as an alternative, we could process that file, expand those 6 rules (resulting in 66 additional "lines"), and produce a text file suitable for MappingCharFilter. It could be part of the ICU upgrade automation in the gradle build so that it gets regenerated with new ICU versions.
   
   I can see why disabling normalization in the output might improve performance here, why normalize twice?  But its a bit scary to provide such an option that makes things "faster" but might result in crazy output and confuse users or even make tokenizers behave worse, if they don't understand what it means. Even then its still problematic, as the ICUNormalizer2CharFilter has buggy offset handling, that the tests will find if you re-enable testRandom and run it enough times: https://issues.apache.org/jira/browse/LUCENE-5595 . So I'm not a fan of e.g. returning unexpected decomposed output from this thing right now. For which transforms does this optimization impact the performance and how much?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r600505701



##########
File path: lucene/analysis/icu/src/java/org/apache/lucene/analysis/icu/ICUTransformCharFilterFactory.java
##########
@@ -0,0 +1,338 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.icu;
+
+import com.ibm.icu.impl.Utility;
+import com.ibm.icu.text.Normalizer2;
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.UTF16;
+import java.io.Reader;
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.Map;
+import org.apache.lucene.analysis.CharFilterFactory;
+
+/**
+ * Factory for {@link ICUTransformCharFilter}.
+ *
+ * <p>Supports the following attributes:
+ *
+ * <ul>
+ *   <li>id (mandatory): A Transliterator ID, one from {@link Transliterator#getAvailableIDs()}
+ *   <li>direction (optional): Either 'forward' or 'reverse'. Default is forward.
+ * </ul>
+ *
+ * @see Transliterator
+ * @since 8.3.0
+ * @lucene.spi {@value #NAME}
+ */
+public class ICUTransformCharFilterFactory extends CharFilterFactory {
+
+  /** SPI name */
+  public static final String NAME = "icuTransform";
+
+  static final String MAX_ROLLBACK_BUFFER_CAPACITY_ARGNAME = "maxRollbackBufferCapacity";
+  static final String FAIL_ON_ROLLBACK_BUFFER_OVERFLOW_ARGNAME = "failOnRollbackBufferOverflow";
+  private final NormType leading;
+  private final Transliterator transliterator;
+  private final NormType trailing;
+  private final int maxRollbackBufferCapacity;
+  private final boolean failOnRollbackBufferOverflow;
+
+  // TODO: add support for custom rules
+  /** Creates a new ICUTransformFilterFactory */
+  public ICUTransformCharFilterFactory(Map<String, String> args) {
+    this(args, false);
+  }
+
+  /** package access, to allow tests to suppress unicode normalization externalization */
+  ICUTransformCharFilterFactory(
+      Map<String, String> args, boolean suppressUnicodeNormalizationExternalization) {
+    super(args);
+    String id = require(args, "id");
+    String direction =
+        get(args, "direction", Arrays.asList("forward", "reverse"), "forward", false);
+    int dir = "forward".equals(direction) ? Transliterator.FORWARD : Transliterator.REVERSE;
+    int tmpCapacityHint =
+        getInt(
+            args,
+            MAX_ROLLBACK_BUFFER_CAPACITY_ARGNAME,
+            ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY);
+    this.maxRollbackBufferCapacity = tmpCapacityHint == -1 ? Integer.MAX_VALUE : tmpCapacityHint;
+    this.failOnRollbackBufferOverflow =
+        getBoolean(
+            args,
+            FAIL_ON_ROLLBACK_BUFFER_OVERFLOW_ARGNAME,
+            ICUTransformCharFilter.DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW);
+    Transliterator stockTransliterator = Transliterator.getInstance(id, dir);
+    if (suppressUnicodeNormalizationExternalization) {
+      this.leading = null;
+      this.transliterator = stockTransliterator;
+      this.trailing = null;
+    } else {
+      ExternalNormalization ext = externalizeUnicodeNormalization(stockTransliterator);
+      this.leading = ext.leading;
+      this.transliterator = ext.t;
+      this.trailing = ext.trailing;
+    }
+    if (!args.isEmpty()) {
+      throw new IllegalArgumentException("Unknown parameters: " + args);
+    }
+  }
+
+  /**
+   * for tests; check whether unicode normalization externalization optimization is actually applied
+   */
+  boolean externalizedUnicodeNormalization() {
+    return leading != null || trailing != null;
+  }
+
+  private static final Reader wrapReader(NormType normType, Reader r) {
+    if (normType == null) {
+      return r;
+    }
+    switch (normType) {
+      case NFC:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFCInstance());
+      case NFD:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFDInstance());
+      case NFKC:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFKCInstance());
+      case NFKD:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFKDInstance());
+      default:
+        throw new UnsupportedOperationException(
+            "test not yet able to compensate externally for normalization type \""
+                + normType
+                + "\"");
+    }
+  }
+
+  /** Default ctor for compatibility with SPI */
+  public ICUTransformCharFilterFactory() {
+    throw defaultCtorException();
+  }
+
+  @Override
+  public Reader create(Reader input) {
+    input = wrapReader(leading, input);
+    input =
+        new ICUTransformCharFilter(
+            input, transliterator, maxRollbackBufferCapacity, failOnRollbackBufferOverflow);
+    return wrapReader(trailing, input);
+  }
+
+  private static final boolean ESCAPE_UNPRINTABLE = true;
+
+  /**
+   * Attempts to detect and externalize any leading or trailing top-level Unicode normalization. In
+   * the event that such normalization is detected, a new "core" Transliterator (with any detected
+   * pre-/post-normalization removed) is created and returned.
+   *
+   * <p>The creation of any new "core" Transliterator (and much of the actual code in this method)
+   * is based on the {@link com.ibm.icu.text.CompoundTransliterator#toRules(boolean)} method (with
+   * the boolean arg replaced by {@link #ESCAPE_UNPRINTABLE} -- always <code>true</code> in this
+   * context).
+   *
+   * @param t the Transliterator to base modified rules on.
+   * @return simple ExternalNormalization struct containing a non-null Transliterator, if possible
+   *     with any leading and trailing Unicode normalization externalized. The effect of applying
+   *     the resulting leading Unicode norm, Transliterator, and trailing Unicode norm, should be
+   *     equivalent to the effect of applying the input Transliterator t.
+   */
+  private static ExternalNormalization externalizeUnicodeNormalization(Transliterator t) {
+    final Transliterator[] trans = t.getElements();
+    final String topLevelId = t.getID();

Review comment:
       this line is dead code (if you merge up to `main` the ECJ compiler will find it)




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-832355590


   Yeah; the heavy hand of spotlessApply was the main reason I didn't fuss with getting the precommit checks to pass. I understand if you want to wait for the build checks to pass before digging into this, and would be happy to (as you suggest) work that out manually.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-806055837


   Great! yes this is looking pretty good from my perspective too. (I forgot to address the `hoistFilter` method rename -- thanks for catching that).
   
   I'm curious what you'll make of 0d8c001 ... the previous state was kind of weird because we were ostensibly "detecting" norm ids that never actually cropped up in practice, but then throwing an `UnsupportedOperationException` if we had ever come to the point of trying to replace them. This worked because of the fact that they never cropped up in practice. I'm pretty sure that the change introduced in 0d8c001 would work fine, but at the moment it's definitely not covered by tests.
   
   Alternatives to 0d8c001 would be:
   
   1. stop detecting the strings FCC, FCD, and NFKC_CF (i.e. don't recognize them as candidates for replacement/optimization)
   2. continue detecting FCC/FCD/NFKC_CF and leave in place the new code that purports to handle them, but `assert` (either in a test over `availableIDs`, or in live code so that the check would be a noop unless assertions were enabled) that we should never actually _encounter_ these types of leading/trailing normalization.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r595948260



##########
File path: lucene/analysis/icu/src/java/org/apache/lucene/analysis/icu/ICUTransformFilter.java
##########
@@ -77,17 +77,26 @@
   @SuppressWarnings("deprecation")
   public ICUTransformFilter(TokenStream input, Transliterator transform) {
     super(input);
-    this.transform = transform;
+    this.transform = optimizeForCommonCase(transform);
+  }
 
-    /*
-     * This is cheating, but speeds things up a lot.
-     * If we wanted to use pkg-private APIs we could probably do better.
-     */
+  /**
+   * Configures the specified {@link Transliterator} to optimize common use case when the input
+   * doesn't match the filter anyway.
+   *
+   * <p>This is cheating, but speeds things up a lot. If we wanted to use pkg-private APIs we could
+   * probably do better.
+   *
+   * @param transform {@link Transliterator} to be optimized
+   * @return the input argument, optionally configured to optimize common case.
+   */
+  static Transliterator optimizeForCommonCase(Transliterator transform) {

Review comment:
       I'd suggest a more descriptive name such as `hoistFilter`: that is all that happens here, we apply the same filter from the rules to transform itself, to save cycles for chars that don't match it.

##########
File path: lucene/analysis/icu/src/java/org/apache/lucene/analysis/icu/ICUTransformCharFilter.java
##########
@@ -0,0 +1,686 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.analysis.icu;
+
+import com.ibm.icu.impl.Utility;
+import com.ibm.icu.text.ReplaceableString;
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.Transliterator.Position;
+import com.ibm.icu.text.UTF16;
+import java.io.IOException;
+import java.io.Reader;
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.WeakHashMap;
+import org.apache.lucene.analysis.CharFilter;
+import org.apache.lucene.analysis.charfilter.BaseCharFilter;
+import org.apache.lucene.util.ArrayUtil;
+
+/**
+ * A {@link CharFilter} that transforms text with ICU.
+ *
+ * <p>ICU provides text-transformation functionality via its Transliteration API. Although script
+ * conversion is its most common use, a Transliterator can actually perform a more general class of
+ * tasks. In fact, Transliterator defines a very general API which specifies only that a segment of
+ * the input text is replaced by new text. The particulars of this conversion are determined
+ * entirely by subclasses of Transliterator.
+ *
+ * <p>Some useful transformations for search are built-in:
+ *
+ * <ul>
+ *   <li>Conversion from Traditional to Simplified Chinese characters
+ *   <li>Conversion from Hiragana to Katakana
+ *   <li>Conversion from Fullwidth to Halfwidth forms.
+ *   <li>Script conversions, for example Serbian Cyrillic to Latin
+ * </ul>
+ *
+ * <p>Example usage:
+ *
+ * <blockquote>
+ *
+ * stream = new ICUTransformCharFilter(reader,
+ * Transliterator.getInstance("Traditional-Simplified"));
+ *
+ * </blockquote>
+ *
+ * <br>
+ * For more details, see the <a href="http://userguide.icu-project.org/transforms/general">ICU User
+ * Guide</a>.
+ */
+public final class ICUTransformCharFilter extends BaseCharFilter {
+
+  // Transliterator to transform the text
+  private final Transliterator transform;
+
+  // Reusable position object
+  private final Position position = new Position();
+
+  private static final int READ_BUFFER_SIZE = 1024;
+  private final char[] tmpBuffer = new char[READ_BUFFER_SIZE];
+
+  private static final int INITIAL_TRANSLITERATE_BUFFER_SIZE = 1024;
+  private final StringBuffer buffer = new StringBuffer(INITIAL_TRANSLITERATE_BUFFER_SIZE);
+  private final ReplaceableString replaceable = new ReplaceableString(buffer);
+
+  private static final int BUFFER_PRUNE_THRESHOLD = 1024;
+
+  private int outputCursor = 0;
+  private boolean inputFinished = false;
+  private int charCount = 0;
+  private int offsetDiffAdjust = 0;
+
+  private static final int HARD_MAX_ROLLBACK_BUFFER_CAPACITY =
+      Integer.highestOneBit(Integer.MAX_VALUE);
+  static final int DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY = 8192;
+  private final int maxRollbackBufferCapacity;
+
+  private static final int DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY = 4; // must be power of 2
+  private char[] rollbackBuffer;
+  private int rollbackBufferSize = 0;
+
+  static final boolean DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW = true;
+  private final boolean failOnRollbackBufferOverflow;
+
+  ICUTransformCharFilter(Reader in, Transliterator transform) {
+    this(
+        in,
+        transform,
+        DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+        DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW);
+  }
+
+  /**
+   * Construct new {@link ICUTransformCharFilter} with the specified {@link Transliterator}, backed
+   * by the specified {@link Reader}.
+   *
+   * @param in input source
+   * @param transform used to perform transliteration
+   * @param maxRollbackBufferCapacityHint used to control the maximum size to which this {@link
+   *     ICUTransformCharFilter} will buffer and rollback partial transliteration of input
+   *     sequences. The provided hint will be converted to an enforced limit of "the greatest power
+   *     of 2 (excluding '1') less than or equal to the specified value". It is illegal to specify a
+   *     negative value. There is no power of 2 greater than <code>
+   *     Integer.highestOneBit(Integer.MAX_VALUE))</code>, so to prevent overflow, values in this
+   *     range will resolve to an enforced limit of <code>Integer.highestOneBit(Integer.MAX_VALUE))
+   *     </code>. Specifying "0" (or "1", in practice) disables rollback. Larger values can in some
+   *     cases yield more accurate transliteration, at the cost of performance and
+   *     resolution/accuracy of offset correction. This is intended primarily as a failsafe, with a
+   *     relatively large default value of {@value
+   *     ICUTransformCharFilter#DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY}. See comments "To understand
+   *     the need for rollback" in private method: {@link
+   *     Transliterator#filteredTransliterate(com.ibm.icu.text.Replaceable, Position, boolean,
+   *     boolean)}
+   * @param failOnRollbackBufferOverflow if true, transliteration failing to complete within the
+   *     specified rollbackBuffer window will fail with an exception. If false, transliteration
+   *     forges ahead with possibly-inconsistent results.
+   */
+  ICUTransformCharFilter(
+      Reader in,
+      Transliterator transform,
+      int maxRollbackBufferCapacityHint,
+      boolean failOnRollbackBufferOverflow) {
+    super(in);
+    this.transform = ICUTransformFilter.optimizeForCommonCase(transform);
+    this.failOnRollbackBufferOverflow = failOnRollbackBufferOverflow;
+    if (maxRollbackBufferCapacityHint < 0) {
+      throw new IllegalArgumentException(
+          "It is illegal to request negative rollback buffer max capacity");
+    } else if (maxRollbackBufferCapacityHint >= HARD_MAX_ROLLBACK_BUFFER_CAPACITY) {
+      // arg is positive, so user wants the largest possible buffer capacity limit
+      // we know what that is (static), protecting for overflow.
+      this.maxRollbackBufferCapacity = HARD_MAX_ROLLBACK_BUFFER_CAPACITY;
+      this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+    } else {
+      // greatest power of 2 (excluding "1") less than or equal to the specified hint
+      this.maxRollbackBufferCapacity =
+          Integer.highestOneBit(maxRollbackBufferCapacityHint - 1) << 1;
+      if (this.maxRollbackBufferCapacity == 0) {
+        this.rollbackBuffer = null;
+      } else {
+        this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+      }
+    }
+  }
+
+  /**
+   * Reads characters into a portion of an array. This method will block until some input is
+   * available, an I/O error occurs, or the end of the stream is reached.
+   *
+   * @param cbuf Destination buffer
+   * @param off Offset at which to start storing characters
+   * @param len Maximum number of characters to read
+   * @return The number of characters read, or -1 if the end of the stream has been reached
+   * @throws IOException If an I/O error occurs
+   */
+  @Override
+  public int read(char[] cbuf, int off, int len) throws IOException {
+    while (!inputFinished || position.start > outputCursor) {
+      // (expecting more input) || (output remains that has not been flushed)
+      if (position.start > outputCursor) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      int resLen = transliterateBufferContents();
+      if (resLen > 0) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      if (!readInputToBuffer()) {
+        final int preStart = position.start;
+        final int preLimit;
+        final int bufferLength = buffer.length();
+        if (preStart < bufferLength) {
+          if (UTF16.isLeadSurrogate(buffer.charAt(bufferLength - 1))) {
+            // if last char is a lead surrogate, transform won't handle it properly
+            // preLimit = bufferLength - 1; // could simply pass the malformed input along
+            throw new RuntimeException(
+                "malformed input; final UTF16 character is a lead surrogate");
+          } else {
+            preLimit = bufferLength;
+          }
+          position.contextLimit = preLimit;
+          position.limit = preLimit;
+          transform.finishTransliteration(replaceable, position);
+        } else if (offsetDiffAdjust == 0) {
+          break;
+        } else {
+          preLimit = bufferLength;
+        }
+        cursorAdvanced(preStart, preLimit);
+      }
+    }
+
+    return -1;
+  }
+
+  private void rollback(int preStart, int preLimit) {
+    buffer.delete(preStart, position.limit); // delete uncommitted chars
+    buffer.insert(preStart, rollbackBuffer, 0, rollbackBufferSize);
+    position.start = preStart;
+    position.contextLimit = preLimit;
+    position.limit = preLimit;
+  }
+
+  /**
+   * Grow rollback buffer if necessary, within constraints of {@link #maxRollbackBufferCapacity}.
+   * This imposes an arbitrary failsafe to prevent the possibility that the rollback buffer could
+   * grow indefinitely.
+   *
+   * @return true if upon return, rollback buffer has sufficient capacity for new input chars,
+   *     otherwise false.
+   */
+  private boolean ensureRollbackBufferCapacity() {
+    // ensure space for at least 2 chars (surrogate pair, max possible space needed)
+    if (rollbackBuffer.length - rollbackBufferSize < 2) {
+      if (rollbackBuffer.length < maxRollbackBufferCapacity) {
+        rollbackBuffer = ArrayUtil.growExact(rollbackBuffer, rollbackBuffer.length << 1);
+      } else {
+        // hit threshold; not going to increase the buffer size
+        if (failOnRollbackBufferOverflow) {
+          throw new RuntimeException(
+              "input could not be transliterated without overflowing maxRollbackBufferCapacity ("
+                  + maxRollbackBufferCapacity
+                  + "); "
+                  + "try increasing maxRollbackBufferCapacity, or setting failOnRollbackBufferOverflow=false");
+        }
+        return false;
+      }
+    }
+    return true;
+  }
+
+  private int pushRollbackBuffer(int idx, int bufferLength) {
+    if (idx >= bufferLength) {
+      return 0;
+    } else {
+      // note: we have already ensured sufficient buffer capacity
+      final char candidate = buffer.charAt(idx);
+      if (!UTF16.isLeadSurrogate(candidate)) {
+        if (rollbackBuffer != null) {
+          rollbackBuffer[rollbackBufferSize++] = candidate;
+        }
+        return 1;
+      } else {
+        if (++idx < bufferLength) {
+          if (rollbackBuffer != null) {
+            rollbackBuffer[rollbackBufferSize++] = candidate;
+            rollbackBuffer[rollbackBufferSize++] = buffer.charAt(idx);
+          }
+          return 2;
+        } else {
+          // we don't yet have the high surrogate
+          if (inputFinished) {
+            // no more input available; proceed with the char we have; rollback not needed
+            return 1;
+          } else {
+            return 0; // wait for more input to be available
+          }
+        }
+      }
+    }
+  }
+
+  /**
+   * Transliterate as much of the contents of {@link #buffer} as possible.
+   *
+   * @return number of output characters transliterated (possibly 0)
+   */
+  private int transliterateBufferContents() {
+    int nextCharLength = pushRollbackBuffer(position.limit, buffer.length());
+    if (nextCharLength == 0) {
+      return 0;
+    }
+    final int preCharCount = charCount;
+    int preStart = position.start;
+    int preLimit;
+    do {
+      position.limit += nextCharLength;
+      preLimit = position.limit;
+      position.contextLimit = preLimit;
+      transform.filteredTransliterate(replaceable, position, true);
+      boolean rollbackSizeWithinBounds = true;
+      if (rollbackBuffer != null
+          && position.start < position.limit
+          && (rollbackSizeWithinBounds = ensureRollbackBufferCapacity())) {
+        // complete pass not transliterated, and not yet at rollback buffer threshold cap.
+        // N.b.: rollback buffer threshold cap is arbitrary, so we check here (somewhat
+        // counterintuitively, *before* rolling back, as opposed to before pushing to
+        // the buffer) so that we can fall through to the else clause, clear the rollback
+        // buffer, and proceed.
+        rollback(preStart, preLimit);
+      } else {
+        rollbackBufferSize = 0;
+        if (position.start > preStart) {
+          // cursor advanced
+          cursorAdvanced(preStart, preLimit);
+
+          // N.b.: advancing contextStart precludes support for quantifiers, but is crucial for
+          // streaming,
+          // so we'll do it anyway.
+          // See comments in Transliterator source code:
+          // TODO
+          // This doesn't work once we add quantifier support.  Need to rewrite
+          // this code to support quantifiers and 'use maximum backup <n>;'.
+          //
+          //         index.contextStart = Math.max(index.start - getMaximumContextLength(),
+          //                                       originalStart);
+          position.contextStart =
+              Math.max(position.start - transform.getMaximumContextLength(), preStart);
+          preStart = position.start;
+          if (!rollbackSizeWithinBounds) {
+            // prepopulate newly cleared rollback buffer with all top-level uncommitted characters
+            rollbackBufferSize = position.limit - preStart;
+            if (rollbackBuffer.length - rollbackBufferSize < 2) {
+              // even after flushing all committed text, there's not enough space in the rollback
+              // buffer.
+              // This is an edge case of an edge case, when the last char32 read into the rollback
+              // buffer
+              // is a surrogate pair (completely filling the rollback buffer), *and* the last
+              // transliteration pass advanced position.start by exactly one char16 (not a surrogate
+              // pair).
+              preStart = forceAdvance(preStart, preLimit);
+              rollbackBufferSize = position.limit - preStart;
+            }
+            buffer.getChars(preStart, position.limit, rollbackBuffer, 0);
+          }
+        } else if (preLimit != position.limit) {
+          // cursor hasn't advanced; incoming characters have probably been deleted
+          offsetDiffAdjust += preLimit - position.limit;
+          // edge case of !rollbackSizeWithinBounds needs no special handling here, since input
+          // characters *are* being
+          // processed (deleted) -- we *are* progressing through the input stream, although the
+          // output stream hasn't changed.
+        } else if (!rollbackSizeWithinBounds) {
+          // cursor hasn't advanced, no incoming characters have been deleted, and the rollback
+          // buffer is full.
+          preStart = forceAdvance(preStart, preLimit);
+        }
+      }
+    } while ((nextCharLength = pushRollbackBuffer(position.limit, buffer.length())) > 0);
+
+    return charCount - preCharCount;
+  }
+
+  private static final int FORCE_THRESHOLD = 2; // conservative; the length of a surrogate pair
+
+  private int forceAdvance(int preStart, int preLimit) {
+    int shift;
+    if (maxRollbackBufferCapacity == 2) {
+      rollbackBufferSize = 0;
+      shift = 2;
+    } else {
+      shift = 0;
+      do {
+        shift += UTF16.isLeadSurrogate(rollbackBuffer[shift]) ? 2 : 1;
+      } while (shift < FORCE_THRESHOLD && shift < maxRollbackBufferCapacity);
+      rollbackBufferSize -= shift;
+      System.arraycopy(rollbackBuffer, shift, rollbackBuffer, 0, rollbackBufferSize);
+    }
+    position.start += shift; // mock transliterator advance
+    cursorAdvanced(preStart, preLimit);
+    position.contextStart =
+        Math.max(position.start - transform.getMaximumContextLength(), preStart);
+    return position.start;
+  }
+
+  private void cursorAdvanced(int preStart, int preLimit) {
+    final int outputLength = position.start - preStart;
+    final int diff = preLimit - position.limit + offsetDiffAdjust;
+    offsetDiffAdjust = 0;
+    if (diff == 0) {
+      // increment charCount; no offset correction necessary
+      charCount += outputLength;
+    } else {
+      // limit change indicates change in length of replacement text; correct offsets accordingly
+      // we correct offsets as frequently as possible, for every incremental change. This will
+      // sometimes be for a range of characters, but that's the best we can do as far as the level
+      // of granularity that's available to us via the Transliterator API.
+      recordOffsetDiff(diff, outputLength);
+    }
+  }
+
+  private void recordOffsetDiff(int diff, int outputLength) {
+    final int cumuDiff = getLastCumulativeDiff();
+    if (diff < 0) {
+      // positive diff indicates an increase in character count wrt input
+      for (int i = 1; i <= -diff; ++i) {
+        addOffCorrectMap(charCount + i, cumuDiff - i);
+      }
+    } else {
+      // positive diff indicates an decrease in character count wrt input
+      addOffCorrectMap(charCount + outputLength, cumuDiff + diff);
+    }
+    charCount += outputLength;
+  }
+
+  private boolean readInputToBuffer() throws IOException {
+    int res = input.read(tmpBuffer, 0, tmpBuffer.length);
+    if (res == -1) {
+      inputFinished = true;
+      return false;
+    } else {
+      buffer.append(tmpBuffer, 0, res);
+      return true;
+    }
+  }
+
+  private int outputFromResultBuffer(char[] cbuf, int begin, int len) {
+    len = Math.min(position.start - outputCursor, len);
+    buffer.getChars(outputCursor, outputCursor + len, cbuf, begin);
+    outputCursor += len;
+    if (outputCursor > BUFFER_PRUNE_THRESHOLD && position.contextStart > BUFFER_PRUNE_THRESHOLD) {
+      final int pruneSize = Math.min(outputCursor, position.contextStart);
+      buffer.delete(0, pruneSize);
+      position.contextStart -= pruneSize;
+      position.start -= pruneSize;
+      position.limit -= pruneSize;
+      position.contextLimit -= pruneSize;
+      outputCursor -= pruneSize;
+    }
+    return len;
+  }
+
+  /**
+   * Strips off Unicode character normalization form rules from a Transliterator. Do this if, as is
+   * typical, your analysis chain already includes another component that does normalization, like
+   * {@link ICUNormalizer2CharFilter}.
+   *
+   * @return a new Transliterator with no normalization, or the original Transliterator if it
+   *     already did no normalization.
+   */
+  public static Transliterator withoutUnicodeNormalization(Transliterator transliterator) {
+    final String modifiedRules = modifyRules(false, transliterator);
+    if (modifiedRules == null) {
+      return transliterator;
+    }
+    String baseId = transliterator.getID();
+    String modId = baseId.concat(baseId.lastIndexOf('/') < 0 ? "/X_NO_NORM_IO" : "_X_NO_NORM_IO");
+    return Transliterator.createFromRules(modId, modifiedRules, Transliterator.FORWARD);
+  }

Review comment:
       btw I also think its ok to extend the optimization for the case where we know for sure previous stream gives us the input normalization we already need, so in this example:
   
   ```
   if (!(charstream instanceof ICUNormalizer2CharFilter and in [NFD, NFKD])) {
     charstream = new ICUNormalizer2CharFilter(charstream, NFD);
   }
   charstream = new ICUTransformCharCharter(charstream, X);
   charstream = new ICUNormalizer2CharFilter(charstream, NFC);
   return charstream;
   ```
   
   The filters don't even have to know about such optimizations and we can write tests for them too.

##########
File path: lucene/analysis/icu/src/java/org/apache/lucene/analysis/icu/ICUTransformCharFilter.java
##########
@@ -0,0 +1,686 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.analysis.icu;
+
+import com.ibm.icu.impl.Utility;
+import com.ibm.icu.text.ReplaceableString;
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.Transliterator.Position;
+import com.ibm.icu.text.UTF16;
+import java.io.IOException;
+import java.io.Reader;
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.WeakHashMap;
+import org.apache.lucene.analysis.CharFilter;
+import org.apache.lucene.analysis.charfilter.BaseCharFilter;
+import org.apache.lucene.util.ArrayUtil;
+
+/**
+ * A {@link CharFilter} that transforms text with ICU.
+ *
+ * <p>ICU provides text-transformation functionality via its Transliteration API. Although script
+ * conversion is its most common use, a Transliterator can actually perform a more general class of
+ * tasks. In fact, Transliterator defines a very general API which specifies only that a segment of
+ * the input text is replaced by new text. The particulars of this conversion are determined
+ * entirely by subclasses of Transliterator.
+ *
+ * <p>Some useful transformations for search are built-in:
+ *
+ * <ul>
+ *   <li>Conversion from Traditional to Simplified Chinese characters
+ *   <li>Conversion from Hiragana to Katakana
+ *   <li>Conversion from Fullwidth to Halfwidth forms.
+ *   <li>Script conversions, for example Serbian Cyrillic to Latin
+ * </ul>
+ *
+ * <p>Example usage:
+ *
+ * <blockquote>
+ *
+ * stream = new ICUTransformCharFilter(reader,
+ * Transliterator.getInstance("Traditional-Simplified"));
+ *
+ * </blockquote>
+ *
+ * <br>
+ * For more details, see the <a href="http://userguide.icu-project.org/transforms/general">ICU User
+ * Guide</a>.
+ */
+public final class ICUTransformCharFilter extends BaseCharFilter {
+
+  // Transliterator to transform the text
+  private final Transliterator transform;
+
+  // Reusable position object
+  private final Position position = new Position();
+
+  private static final int READ_BUFFER_SIZE = 1024;
+  private final char[] tmpBuffer = new char[READ_BUFFER_SIZE];
+
+  private static final int INITIAL_TRANSLITERATE_BUFFER_SIZE = 1024;
+  private final StringBuffer buffer = new StringBuffer(INITIAL_TRANSLITERATE_BUFFER_SIZE);
+  private final ReplaceableString replaceable = new ReplaceableString(buffer);
+
+  private static final int BUFFER_PRUNE_THRESHOLD = 1024;
+
+  private int outputCursor = 0;
+  private boolean inputFinished = false;
+  private int charCount = 0;
+  private int offsetDiffAdjust = 0;
+
+  private static final int HARD_MAX_ROLLBACK_BUFFER_CAPACITY =
+      Integer.highestOneBit(Integer.MAX_VALUE);
+  static final int DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY = 8192;
+  private final int maxRollbackBufferCapacity;
+
+  private static final int DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY = 4; // must be power of 2
+  private char[] rollbackBuffer;
+  private int rollbackBufferSize = 0;
+
+  static final boolean DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW = true;
+  private final boolean failOnRollbackBufferOverflow;
+
+  ICUTransformCharFilter(Reader in, Transliterator transform) {
+    this(
+        in,
+        transform,
+        DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+        DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW);
+  }
+
+  /**
+   * Construct new {@link ICUTransformCharFilter} with the specified {@link Transliterator}, backed
+   * by the specified {@link Reader}.
+   *
+   * @param in input source
+   * @param transform used to perform transliteration
+   * @param maxRollbackBufferCapacityHint used to control the maximum size to which this {@link
+   *     ICUTransformCharFilter} will buffer and rollback partial transliteration of input
+   *     sequences. The provided hint will be converted to an enforced limit of "the greatest power
+   *     of 2 (excluding '1') less than or equal to the specified value". It is illegal to specify a
+   *     negative value. There is no power of 2 greater than <code>
+   *     Integer.highestOneBit(Integer.MAX_VALUE))</code>, so to prevent overflow, values in this
+   *     range will resolve to an enforced limit of <code>Integer.highestOneBit(Integer.MAX_VALUE))
+   *     </code>. Specifying "0" (or "1", in practice) disables rollback. Larger values can in some
+   *     cases yield more accurate transliteration, at the cost of performance and
+   *     resolution/accuracy of offset correction. This is intended primarily as a failsafe, with a
+   *     relatively large default value of {@value
+   *     ICUTransformCharFilter#DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY}. See comments "To understand
+   *     the need for rollback" in private method: {@link
+   *     Transliterator#filteredTransliterate(com.ibm.icu.text.Replaceable, Position, boolean,
+   *     boolean)}
+   * @param failOnRollbackBufferOverflow if true, transliteration failing to complete within the
+   *     specified rollbackBuffer window will fail with an exception. If false, transliteration
+   *     forges ahead with possibly-inconsistent results.
+   */
+  ICUTransformCharFilter(
+      Reader in,
+      Transliterator transform,
+      int maxRollbackBufferCapacityHint,
+      boolean failOnRollbackBufferOverflow) {
+    super(in);
+    this.transform = ICUTransformFilter.optimizeForCommonCase(transform);
+    this.failOnRollbackBufferOverflow = failOnRollbackBufferOverflow;
+    if (maxRollbackBufferCapacityHint < 0) {
+      throw new IllegalArgumentException(
+          "It is illegal to request negative rollback buffer max capacity");
+    } else if (maxRollbackBufferCapacityHint >= HARD_MAX_ROLLBACK_BUFFER_CAPACITY) {
+      // arg is positive, so user wants the largest possible buffer capacity limit
+      // we know what that is (static), protecting for overflow.
+      this.maxRollbackBufferCapacity = HARD_MAX_ROLLBACK_BUFFER_CAPACITY;
+      this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+    } else {
+      // greatest power of 2 (excluding "1") less than or equal to the specified hint
+      this.maxRollbackBufferCapacity =
+          Integer.highestOneBit(maxRollbackBufferCapacityHint - 1) << 1;
+      if (this.maxRollbackBufferCapacity == 0) {
+        this.rollbackBuffer = null;
+      } else {
+        this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+      }
+    }
+  }
+
+  /**
+   * Reads characters into a portion of an array. This method will block until some input is
+   * available, an I/O error occurs, or the end of the stream is reached.
+   *
+   * @param cbuf Destination buffer
+   * @param off Offset at which to start storing characters
+   * @param len Maximum number of characters to read
+   * @return The number of characters read, or -1 if the end of the stream has been reached
+   * @throws IOException If an I/O error occurs
+   */
+  @Override
+  public int read(char[] cbuf, int off, int len) throws IOException {
+    while (!inputFinished || position.start > outputCursor) {
+      // (expecting more input) || (output remains that has not been flushed)
+      if (position.start > outputCursor) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      int resLen = transliterateBufferContents();
+      if (resLen > 0) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      if (!readInputToBuffer()) {
+        final int preStart = position.start;
+        final int preLimit;
+        final int bufferLength = buffer.length();
+        if (preStart < bufferLength) {
+          if (UTF16.isLeadSurrogate(buffer.charAt(bufferLength - 1))) {
+            // if last char is a lead surrogate, transform won't handle it properly
+            // preLimit = bufferLength - 1; // could simply pass the malformed input along
+            throw new RuntimeException(
+                "malformed input; final UTF16 character is a lead surrogate");
+          } else {
+            preLimit = bufferLength;
+          }
+          position.contextLimit = preLimit;
+          position.limit = preLimit;
+          transform.finishTransliteration(replaceable, position);
+        } else if (offsetDiffAdjust == 0) {
+          break;
+        } else {
+          preLimit = bufferLength;
+        }
+        cursorAdvanced(preStart, preLimit);
+      }
+    }
+
+    return -1;
+  }
+
+  private void rollback(int preStart, int preLimit) {
+    buffer.delete(preStart, position.limit); // delete uncommitted chars
+    buffer.insert(preStart, rollbackBuffer, 0, rollbackBufferSize);
+    position.start = preStart;
+    position.contextLimit = preLimit;
+    position.limit = preLimit;
+  }
+
+  /**
+   * Grow rollback buffer if necessary, within constraints of {@link #maxRollbackBufferCapacity}.
+   * This imposes an arbitrary failsafe to prevent the possibility that the rollback buffer could
+   * grow indefinitely.
+   *
+   * @return true if upon return, rollback buffer has sufficient capacity for new input chars,
+   *     otherwise false.
+   */
+  private boolean ensureRollbackBufferCapacity() {
+    // ensure space for at least 2 chars (surrogate pair, max possible space needed)
+    if (rollbackBuffer.length - rollbackBufferSize < 2) {
+      if (rollbackBuffer.length < maxRollbackBufferCapacity) {
+        rollbackBuffer = ArrayUtil.growExact(rollbackBuffer, rollbackBuffer.length << 1);
+      } else {
+        // hit threshold; not going to increase the buffer size
+        if (failOnRollbackBufferOverflow) {
+          throw new RuntimeException(
+              "input could not be transliterated without overflowing maxRollbackBufferCapacity ("
+                  + maxRollbackBufferCapacity
+                  + "); "
+                  + "try increasing maxRollbackBufferCapacity, or setting failOnRollbackBufferOverflow=false");
+        }
+        return false;
+      }
+    }
+    return true;
+  }
+
+  private int pushRollbackBuffer(int idx, int bufferLength) {
+    if (idx >= bufferLength) {
+      return 0;
+    } else {
+      // note: we have already ensured sufficient buffer capacity
+      final char candidate = buffer.charAt(idx);
+      if (!UTF16.isLeadSurrogate(candidate)) {
+        if (rollbackBuffer != null) {
+          rollbackBuffer[rollbackBufferSize++] = candidate;
+        }
+        return 1;
+      } else {
+        if (++idx < bufferLength) {
+          if (rollbackBuffer != null) {
+            rollbackBuffer[rollbackBufferSize++] = candidate;
+            rollbackBuffer[rollbackBufferSize++] = buffer.charAt(idx);
+          }
+          return 2;
+        } else {
+          // we don't yet have the high surrogate
+          if (inputFinished) {
+            // no more input available; proceed with the char we have; rollback not needed
+            return 1;
+          } else {
+            return 0; // wait for more input to be available
+          }
+        }
+      }
+    }
+  }
+
+  /**
+   * Transliterate as much of the contents of {@link #buffer} as possible.
+   *
+   * @return number of output characters transliterated (possibly 0)
+   */
+  private int transliterateBufferContents() {
+    int nextCharLength = pushRollbackBuffer(position.limit, buffer.length());
+    if (nextCharLength == 0) {
+      return 0;
+    }
+    final int preCharCount = charCount;
+    int preStart = position.start;
+    int preLimit;
+    do {
+      position.limit += nextCharLength;
+      preLimit = position.limit;
+      position.contextLimit = preLimit;
+      transform.filteredTransliterate(replaceable, position, true);
+      boolean rollbackSizeWithinBounds = true;
+      if (rollbackBuffer != null
+          && position.start < position.limit
+          && (rollbackSizeWithinBounds = ensureRollbackBufferCapacity())) {
+        // complete pass not transliterated, and not yet at rollback buffer threshold cap.
+        // N.b.: rollback buffer threshold cap is arbitrary, so we check here (somewhat
+        // counterintuitively, *before* rolling back, as opposed to before pushing to
+        // the buffer) so that we can fall through to the else clause, clear the rollback
+        // buffer, and proceed.
+        rollback(preStart, preLimit);
+      } else {
+        rollbackBufferSize = 0;
+        if (position.start > preStart) {
+          // cursor advanced
+          cursorAdvanced(preStart, preLimit);
+
+          // N.b.: advancing contextStart precludes support for quantifiers, but is crucial for
+          // streaming,
+          // so we'll do it anyway.
+          // See comments in Transliterator source code:
+          // TODO
+          // This doesn't work once we add quantifier support.  Need to rewrite
+          // this code to support quantifiers and 'use maximum backup <n>;'.
+          //
+          //         index.contextStart = Math.max(index.start - getMaximumContextLength(),
+          //                                       originalStart);
+          position.contextStart =
+              Math.max(position.start - transform.getMaximumContextLength(), preStart);
+          preStart = position.start;
+          if (!rollbackSizeWithinBounds) {
+            // prepopulate newly cleared rollback buffer with all top-level uncommitted characters
+            rollbackBufferSize = position.limit - preStart;
+            if (rollbackBuffer.length - rollbackBufferSize < 2) {
+              // even after flushing all committed text, there's not enough space in the rollback
+              // buffer.
+              // This is an edge case of an edge case, when the last char32 read into the rollback
+              // buffer
+              // is a surrogate pair (completely filling the rollback buffer), *and* the last
+              // transliteration pass advanced position.start by exactly one char16 (not a surrogate
+              // pair).
+              preStart = forceAdvance(preStart, preLimit);
+              rollbackBufferSize = position.limit - preStart;
+            }
+            buffer.getChars(preStart, position.limit, rollbackBuffer, 0);
+          }
+        } else if (preLimit != position.limit) {
+          // cursor hasn't advanced; incoming characters have probably been deleted
+          offsetDiffAdjust += preLimit - position.limit;
+          // edge case of !rollbackSizeWithinBounds needs no special handling here, since input
+          // characters *are* being
+          // processed (deleted) -- we *are* progressing through the input stream, although the
+          // output stream hasn't changed.
+        } else if (!rollbackSizeWithinBounds) {
+          // cursor hasn't advanced, no incoming characters have been deleted, and the rollback
+          // buffer is full.
+          preStart = forceAdvance(preStart, preLimit);
+        }
+      }
+    } while ((nextCharLength = pushRollbackBuffer(position.limit, buffer.length())) > 0);
+
+    return charCount - preCharCount;
+  }
+
+  private static final int FORCE_THRESHOLD = 2; // conservative; the length of a surrogate pair
+
+  private int forceAdvance(int preStart, int preLimit) {
+    int shift;
+    if (maxRollbackBufferCapacity == 2) {
+      rollbackBufferSize = 0;
+      shift = 2;
+    } else {
+      shift = 0;
+      do {
+        shift += UTF16.isLeadSurrogate(rollbackBuffer[shift]) ? 2 : 1;
+      } while (shift < FORCE_THRESHOLD && shift < maxRollbackBufferCapacity);
+      rollbackBufferSize -= shift;
+      System.arraycopy(rollbackBuffer, shift, rollbackBuffer, 0, rollbackBufferSize);
+    }
+    position.start += shift; // mock transliterator advance
+    cursorAdvanced(preStart, preLimit);
+    position.contextStart =
+        Math.max(position.start - transform.getMaximumContextLength(), preStart);
+    return position.start;
+  }
+
+  private void cursorAdvanced(int preStart, int preLimit) {
+    final int outputLength = position.start - preStart;
+    final int diff = preLimit - position.limit + offsetDiffAdjust;
+    offsetDiffAdjust = 0;
+    if (diff == 0) {
+      // increment charCount; no offset correction necessary
+      charCount += outputLength;
+    } else {
+      // limit change indicates change in length of replacement text; correct offsets accordingly
+      // we correct offsets as frequently as possible, for every incremental change. This will
+      // sometimes be for a range of characters, but that's the best we can do as far as the level
+      // of granularity that's available to us via the Transliterator API.
+      recordOffsetDiff(diff, outputLength);
+    }
+  }
+
+  private void recordOffsetDiff(int diff, int outputLength) {
+    final int cumuDiff = getLastCumulativeDiff();
+    if (diff < 0) {
+      // positive diff indicates an increase in character count wrt input
+      for (int i = 1; i <= -diff; ++i) {
+        addOffCorrectMap(charCount + i, cumuDiff - i);
+      }
+    } else {
+      // positive diff indicates an decrease in character count wrt input
+      addOffCorrectMap(charCount + outputLength, cumuDiff + diff);
+    }
+    charCount += outputLength;
+  }
+
+  private boolean readInputToBuffer() throws IOException {
+    int res = input.read(tmpBuffer, 0, tmpBuffer.length);
+    if (res == -1) {
+      inputFinished = true;
+      return false;
+    } else {
+      buffer.append(tmpBuffer, 0, res);
+      return true;
+    }
+  }
+
+  private int outputFromResultBuffer(char[] cbuf, int begin, int len) {
+    len = Math.min(position.start - outputCursor, len);
+    buffer.getChars(outputCursor, outputCursor + len, cbuf, begin);
+    outputCursor += len;
+    if (outputCursor > BUFFER_PRUNE_THRESHOLD && position.contextStart > BUFFER_PRUNE_THRESHOLD) {
+      final int pruneSize = Math.min(outputCursor, position.contextStart);
+      buffer.delete(0, pruneSize);
+      position.contextStart -= pruneSize;
+      position.start -= pruneSize;
+      position.limit -= pruneSize;
+      position.contextLimit -= pruneSize;
+      outputCursor -= pruneSize;
+    }
+    return len;
+  }
+
+  /**
+   * Strips off Unicode character normalization form rules from a Transliterator. Do this if, as is
+   * typical, your analysis chain already includes another component that does normalization, like
+   * {@link ICUNormalizer2CharFilter}.
+   *
+   * @return a new Transliterator with no normalization, or the original Transliterator if it
+   *     already did no normalization.
+   */
+  public static Transliterator withoutUnicodeNormalization(Transliterator transliterator) {
+    final String modifiedRules = modifyRules(false, transliterator);
+    if (modifiedRules == null) {
+      return transliterator;
+    }
+    String baseId = transliterator.getID();
+    String modId = baseId.concat(baseId.lastIndexOf('/') < 0 ? "/X_NO_NORM_IO" : "_X_NO_NORM_IO");
+    return Transliterator.createFromRules(modId, modifiedRules, Transliterator.FORWARD);
+  }

Review comment:
       I would move this logic to the factory for now. If we see `NFD; X; NFC` we can instead do this:
   
   ```
   charstream = new ICUNormalizer2CharFilter(charstream, NFD);
   charstream = new ICUTransformCharCharter(charstream, X);
   charstream = new ICUNormalizer2CharFilter(charstream, NFC);
   return charstream;
   ```
   
   This way, the performance is improved automatically but in a safe way, without any option: we just "hoist" normalization out of the transform and implement it with faster component behind the scenes.

##########
File path: lucene/analysis/icu/src/java/org/apache/lucene/analysis/icu/ICUTransformCharFilter.java
##########
@@ -0,0 +1,686 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.analysis.icu;
+
+import com.ibm.icu.impl.Utility;
+import com.ibm.icu.text.ReplaceableString;
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.Transliterator.Position;
+import com.ibm.icu.text.UTF16;
+import java.io.IOException;
+import java.io.Reader;
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.WeakHashMap;
+import org.apache.lucene.analysis.CharFilter;
+import org.apache.lucene.analysis.charfilter.BaseCharFilter;
+import org.apache.lucene.util.ArrayUtil;
+
+/**
+ * A {@link CharFilter} that transforms text with ICU.
+ *
+ * <p>ICU provides text-transformation functionality via its Transliteration API. Although script
+ * conversion is its most common use, a Transliterator can actually perform a more general class of
+ * tasks. In fact, Transliterator defines a very general API which specifies only that a segment of
+ * the input text is replaced by new text. The particulars of this conversion are determined
+ * entirely by subclasses of Transliterator.
+ *
+ * <p>Some useful transformations for search are built-in:
+ *
+ * <ul>
+ *   <li>Conversion from Traditional to Simplified Chinese characters
+ *   <li>Conversion from Hiragana to Katakana
+ *   <li>Conversion from Fullwidth to Halfwidth forms.
+ *   <li>Script conversions, for example Serbian Cyrillic to Latin
+ * </ul>
+ *
+ * <p>Example usage:
+ *
+ * <blockquote>
+ *
+ * stream = new ICUTransformCharFilter(reader,
+ * Transliterator.getInstance("Traditional-Simplified"));
+ *
+ * </blockquote>
+ *
+ * <br>
+ * For more details, see the <a href="http://userguide.icu-project.org/transforms/general">ICU User
+ * Guide</a>.
+ */
+public final class ICUTransformCharFilter extends BaseCharFilter {
+
+  // Transliterator to transform the text
+  private final Transliterator transform;
+
+  // Reusable position object
+  private final Position position = new Position();
+
+  private static final int READ_BUFFER_SIZE = 1024;
+  private final char[] tmpBuffer = new char[READ_BUFFER_SIZE];
+
+  private static final int INITIAL_TRANSLITERATE_BUFFER_SIZE = 1024;
+  private final StringBuffer buffer = new StringBuffer(INITIAL_TRANSLITERATE_BUFFER_SIZE);
+  private final ReplaceableString replaceable = new ReplaceableString(buffer);
+
+  private static final int BUFFER_PRUNE_THRESHOLD = 1024;
+
+  private int outputCursor = 0;
+  private boolean inputFinished = false;
+  private int charCount = 0;
+  private int offsetDiffAdjust = 0;
+
+  private static final int HARD_MAX_ROLLBACK_BUFFER_CAPACITY =
+      Integer.highestOneBit(Integer.MAX_VALUE);
+  static final int DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY = 8192;
+  private final int maxRollbackBufferCapacity;
+
+  private static final int DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY = 4; // must be power of 2
+  private char[] rollbackBuffer;
+  private int rollbackBufferSize = 0;
+
+  static final boolean DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW = true;
+  private final boolean failOnRollbackBufferOverflow;
+
+  ICUTransformCharFilter(Reader in, Transliterator transform) {
+    this(
+        in,
+        transform,
+        DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+        DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW);
+  }
+
+  /**
+   * Construct new {@link ICUTransformCharFilter} with the specified {@link Transliterator}, backed
+   * by the specified {@link Reader}.
+   *
+   * @param in input source
+   * @param transform used to perform transliteration
+   * @param maxRollbackBufferCapacityHint used to control the maximum size to which this {@link
+   *     ICUTransformCharFilter} will buffer and rollback partial transliteration of input
+   *     sequences. The provided hint will be converted to an enforced limit of "the greatest power
+   *     of 2 (excluding '1') less than or equal to the specified value". It is illegal to specify a
+   *     negative value. There is no power of 2 greater than <code>
+   *     Integer.highestOneBit(Integer.MAX_VALUE))</code>, so to prevent overflow, values in this
+   *     range will resolve to an enforced limit of <code>Integer.highestOneBit(Integer.MAX_VALUE))
+   *     </code>. Specifying "0" (or "1", in practice) disables rollback. Larger values can in some
+   *     cases yield more accurate transliteration, at the cost of performance and
+   *     resolution/accuracy of offset correction. This is intended primarily as a failsafe, with a
+   *     relatively large default value of {@value
+   *     ICUTransformCharFilter#DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY}. See comments "To understand
+   *     the need for rollback" in private method: {@link
+   *     Transliterator#filteredTransliterate(com.ibm.icu.text.Replaceable, Position, boolean,
+   *     boolean)}
+   * @param failOnRollbackBufferOverflow if true, transliteration failing to complete within the
+   *     specified rollbackBuffer window will fail with an exception. If false, transliteration
+   *     forges ahead with possibly-inconsistent results.
+   */
+  ICUTransformCharFilter(
+      Reader in,
+      Transliterator transform,
+      int maxRollbackBufferCapacityHint,
+      boolean failOnRollbackBufferOverflow) {
+    super(in);
+    this.transform = ICUTransformFilter.optimizeForCommonCase(transform);
+    this.failOnRollbackBufferOverflow = failOnRollbackBufferOverflow;
+    if (maxRollbackBufferCapacityHint < 0) {
+      throw new IllegalArgumentException(
+          "It is illegal to request negative rollback buffer max capacity");
+    } else if (maxRollbackBufferCapacityHint >= HARD_MAX_ROLLBACK_BUFFER_CAPACITY) {
+      // arg is positive, so user wants the largest possible buffer capacity limit
+      // we know what that is (static), protecting for overflow.
+      this.maxRollbackBufferCapacity = HARD_MAX_ROLLBACK_BUFFER_CAPACITY;
+      this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+    } else {
+      // greatest power of 2 (excluding "1") less than or equal to the specified hint
+      this.maxRollbackBufferCapacity =
+          Integer.highestOneBit(maxRollbackBufferCapacityHint - 1) << 1;
+      if (this.maxRollbackBufferCapacity == 0) {
+        this.rollbackBuffer = null;
+      } else {
+        this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+      }
+    }
+  }
+
+  /**
+   * Reads characters into a portion of an array. This method will block until some input is
+   * available, an I/O error occurs, or the end of the stream is reached.
+   *
+   * @param cbuf Destination buffer
+   * @param off Offset at which to start storing characters
+   * @param len Maximum number of characters to read
+   * @return The number of characters read, or -1 if the end of the stream has been reached
+   * @throws IOException If an I/O error occurs
+   */
+  @Override
+  public int read(char[] cbuf, int off, int len) throws IOException {
+    while (!inputFinished || position.start > outputCursor) {
+      // (expecting more input) || (output remains that has not been flushed)
+      if (position.start > outputCursor) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      int resLen = transliterateBufferContents();
+      if (resLen > 0) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      if (!readInputToBuffer()) {
+        final int preStart = position.start;
+        final int preLimit;
+        final int bufferLength = buffer.length();
+        if (preStart < bufferLength) {
+          if (UTF16.isLeadSurrogate(buffer.charAt(bufferLength - 1))) {
+            // if last char is a lead surrogate, transform won't handle it properly
+            // preLimit = bufferLength - 1; // could simply pass the malformed input along
+            throw new RuntimeException(
+                "malformed input; final UTF16 character is a lead surrogate");
+          } else {
+            preLimit = bufferLength;
+          }
+          position.contextLimit = preLimit;
+          position.limit = preLimit;
+          transform.finishTransliteration(replaceable, position);
+        } else if (offsetDiffAdjust == 0) {
+          break;
+        } else {
+          preLimit = bufferLength;
+        }
+        cursorAdvanced(preStart, preLimit);
+      }
+    }
+
+    return -1;
+  }
+
+  private void rollback(int preStart, int preLimit) {
+    buffer.delete(preStart, position.limit); // delete uncommitted chars
+    buffer.insert(preStart, rollbackBuffer, 0, rollbackBufferSize);
+    position.start = preStart;
+    position.contextLimit = preLimit;
+    position.limit = preLimit;
+  }
+
+  /**
+   * Grow rollback buffer if necessary, within constraints of {@link #maxRollbackBufferCapacity}.
+   * This imposes an arbitrary failsafe to prevent the possibility that the rollback buffer could
+   * grow indefinitely.
+   *
+   * @return true if upon return, rollback buffer has sufficient capacity for new input chars,
+   *     otherwise false.
+   */
+  private boolean ensureRollbackBufferCapacity() {
+    // ensure space for at least 2 chars (surrogate pair, max possible space needed)
+    if (rollbackBuffer.length - rollbackBufferSize < 2) {
+      if (rollbackBuffer.length < maxRollbackBufferCapacity) {
+        rollbackBuffer = ArrayUtil.growExact(rollbackBuffer, rollbackBuffer.length << 1);
+      } else {
+        // hit threshold; not going to increase the buffer size
+        if (failOnRollbackBufferOverflow) {
+          throw new RuntimeException(
+              "input could not be transliterated without overflowing maxRollbackBufferCapacity ("
+                  + maxRollbackBufferCapacity
+                  + "); "
+                  + "try increasing maxRollbackBufferCapacity, or setting failOnRollbackBufferOverflow=false");
+        }
+        return false;
+      }
+    }
+    return true;
+  }
+
+  private int pushRollbackBuffer(int idx, int bufferLength) {
+    if (idx >= bufferLength) {
+      return 0;
+    } else {
+      // note: we have already ensured sufficient buffer capacity
+      final char candidate = buffer.charAt(idx);
+      if (!UTF16.isLeadSurrogate(candidate)) {
+        if (rollbackBuffer != null) {
+          rollbackBuffer[rollbackBufferSize++] = candidate;
+        }
+        return 1;
+      } else {
+        if (++idx < bufferLength) {
+          if (rollbackBuffer != null) {
+            rollbackBuffer[rollbackBufferSize++] = candidate;
+            rollbackBuffer[rollbackBufferSize++] = buffer.charAt(idx);
+          }
+          return 2;
+        } else {
+          // we don't yet have the high surrogate
+          if (inputFinished) {
+            // no more input available; proceed with the char we have; rollback not needed
+            return 1;
+          } else {
+            return 0; // wait for more input to be available
+          }
+        }
+      }
+    }
+  }
+
+  /**
+   * Transliterate as much of the contents of {@link #buffer} as possible.
+   *
+   * @return number of output characters transliterated (possibly 0)
+   */
+  private int transliterateBufferContents() {
+    int nextCharLength = pushRollbackBuffer(position.limit, buffer.length());
+    if (nextCharLength == 0) {
+      return 0;
+    }
+    final int preCharCount = charCount;
+    int preStart = position.start;
+    int preLimit;
+    do {
+      position.limit += nextCharLength;
+      preLimit = position.limit;
+      position.contextLimit = preLimit;
+      transform.filteredTransliterate(replaceable, position, true);
+      boolean rollbackSizeWithinBounds = true;
+      if (rollbackBuffer != null
+          && position.start < position.limit
+          && (rollbackSizeWithinBounds = ensureRollbackBufferCapacity())) {
+        // complete pass not transliterated, and not yet at rollback buffer threshold cap.
+        // N.b.: rollback buffer threshold cap is arbitrary, so we check here (somewhat
+        // counterintuitively, *before* rolling back, as opposed to before pushing to
+        // the buffer) so that we can fall through to the else clause, clear the rollback
+        // buffer, and proceed.
+        rollback(preStart, preLimit);
+      } else {
+        rollbackBufferSize = 0;
+        if (position.start > preStart) {
+          // cursor advanced
+          cursorAdvanced(preStart, preLimit);
+
+          // N.b.: advancing contextStart precludes support for quantifiers, but is crucial for
+          // streaming,
+          // so we'll do it anyway.
+          // See comments in Transliterator source code:
+          // TODO
+          // This doesn't work once we add quantifier support.  Need to rewrite
+          // this code to support quantifiers and 'use maximum backup <n>;'.
+          //
+          //         index.contextStart = Math.max(index.start - getMaximumContextLength(),
+          //                                       originalStart);
+          position.contextStart =
+              Math.max(position.start - transform.getMaximumContextLength(), preStart);
+          preStart = position.start;
+          if (!rollbackSizeWithinBounds) {
+            // prepopulate newly cleared rollback buffer with all top-level uncommitted characters
+            rollbackBufferSize = position.limit - preStart;
+            if (rollbackBuffer.length - rollbackBufferSize < 2) {
+              // even after flushing all committed text, there's not enough space in the rollback
+              // buffer.
+              // This is an edge case of an edge case, when the last char32 read into the rollback
+              // buffer
+              // is a surrogate pair (completely filling the rollback buffer), *and* the last
+              // transliteration pass advanced position.start by exactly one char16 (not a surrogate
+              // pair).
+              preStart = forceAdvance(preStart, preLimit);
+              rollbackBufferSize = position.limit - preStart;
+            }
+            buffer.getChars(preStart, position.limit, rollbackBuffer, 0);
+          }
+        } else if (preLimit != position.limit) {
+          // cursor hasn't advanced; incoming characters have probably been deleted
+          offsetDiffAdjust += preLimit - position.limit;
+          // edge case of !rollbackSizeWithinBounds needs no special handling here, since input
+          // characters *are* being
+          // processed (deleted) -- we *are* progressing through the input stream, although the
+          // output stream hasn't changed.
+        } else if (!rollbackSizeWithinBounds) {
+          // cursor hasn't advanced, no incoming characters have been deleted, and the rollback
+          // buffer is full.
+          preStart = forceAdvance(preStart, preLimit);
+        }
+      }
+    } while ((nextCharLength = pushRollbackBuffer(position.limit, buffer.length())) > 0);
+
+    return charCount - preCharCount;
+  }
+
+  private static final int FORCE_THRESHOLD = 2; // conservative; the length of a surrogate pair
+
+  private int forceAdvance(int preStart, int preLimit) {
+    int shift;
+    if (maxRollbackBufferCapacity == 2) {
+      rollbackBufferSize = 0;
+      shift = 2;
+    } else {
+      shift = 0;
+      do {
+        shift += UTF16.isLeadSurrogate(rollbackBuffer[shift]) ? 2 : 1;
+      } while (shift < FORCE_THRESHOLD && shift < maxRollbackBufferCapacity);
+      rollbackBufferSize -= shift;
+      System.arraycopy(rollbackBuffer, shift, rollbackBuffer, 0, rollbackBufferSize);
+    }
+    position.start += shift; // mock transliterator advance
+    cursorAdvanced(preStart, preLimit);
+    position.contextStart =
+        Math.max(position.start - transform.getMaximumContextLength(), preStart);
+    return position.start;
+  }
+
+  private void cursorAdvanced(int preStart, int preLimit) {
+    final int outputLength = position.start - preStart;
+    final int diff = preLimit - position.limit + offsetDiffAdjust;
+    offsetDiffAdjust = 0;
+    if (diff == 0) {
+      // increment charCount; no offset correction necessary
+      charCount += outputLength;
+    } else {
+      // limit change indicates change in length of replacement text; correct offsets accordingly
+      // we correct offsets as frequently as possible, for every incremental change. This will
+      // sometimes be for a range of characters, but that's the best we can do as far as the level
+      // of granularity that's available to us via the Transliterator API.
+      recordOffsetDiff(diff, outputLength);
+    }
+  }
+
+  private void recordOffsetDiff(int diff, int outputLength) {
+    final int cumuDiff = getLastCumulativeDiff();
+    if (diff < 0) {
+      // positive diff indicates an increase in character count wrt input
+      for (int i = 1; i <= -diff; ++i) {
+        addOffCorrectMap(charCount + i, cumuDiff - i);
+      }
+    } else {
+      // positive diff indicates an decrease in character count wrt input
+      addOffCorrectMap(charCount + outputLength, cumuDiff + diff);
+    }
+    charCount += outputLength;
+  }
+
+  private boolean readInputToBuffer() throws IOException {
+    int res = input.read(tmpBuffer, 0, tmpBuffer.length);
+    if (res == -1) {
+      inputFinished = true;
+      return false;
+    } else {
+      buffer.append(tmpBuffer, 0, res);
+      return true;
+    }
+  }
+
+  private int outputFromResultBuffer(char[] cbuf, int begin, int len) {
+    len = Math.min(position.start - outputCursor, len);
+    buffer.getChars(outputCursor, outputCursor + len, cbuf, begin);
+    outputCursor += len;
+    if (outputCursor > BUFFER_PRUNE_THRESHOLD && position.contextStart > BUFFER_PRUNE_THRESHOLD) {
+      final int pruneSize = Math.min(outputCursor, position.contextStart);
+      buffer.delete(0, pruneSize);
+      position.contextStart -= pruneSize;
+      position.start -= pruneSize;
+      position.limit -= pruneSize;
+      position.contextLimit -= pruneSize;
+      outputCursor -= pruneSize;
+    }
+    return len;
+  }
+
+  /**
+   * Strips off Unicode character normalization form rules from a Transliterator. Do this if, as is
+   * typical, your analysis chain already includes another component that does normalization, like
+   * {@link ICUNormalizer2CharFilter}.
+   *
+   * @return a new Transliterator with no normalization, or the original Transliterator if it
+   *     already did no normalization.
+   */
+  public static Transliterator withoutUnicodeNormalization(Transliterator transliterator) {
+    final String modifiedRules = modifyRules(false, transliterator);
+    if (modifiedRules == null) {
+      return transliterator;
+    }
+    String baseId = transliterator.getID();
+    String modId = baseId.concat(baseId.lastIndexOf('/') < 0 ? "/X_NO_NORM_IO" : "_X_NO_NORM_IO");
+    return Transliterator.createFromRules(modId, modifiedRules, Transliterator.FORWARD);
+  }

Review comment:
       And finally for the ICUNormalizer2CharFilterFactory I am ok with it inspecting the input stream and just returning it bare to complete the optimization. For example if you ask to normalize to NFC:
   
   ```
   if (!(charstream instanceof ICUNormalizer2CharFilter and in [NFC, NFKC]])) {
     charstream = new ICUNormalizer2CharFilter(charstream, NFC);
   }
   return charstream;
   ```
   
   It is just a safe optimization to do, to remove double normalization in this way. Although really it shouldn't be that expensive especially on already-normalized strings, as normalization is optimized for that.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-801000190


   I think it would be best if we removed it from this PR and look at it as a followup. There are other alternatives that can be done in a safe or transparent way. For example we can "look" at the transform and if it wants NFD we can wrap charstream with additional normalization filter before/after ourselves. 
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-805850393


   @magibney I think this is really close to being merged? When testing against current `main`, the new static analysis checks complain about a piece of dead code in the Factory (I added a comment). I also would like to still rename that `optimizeForCommonCase` to something more descriptive such as `hoistFilter` or similar. Finally, the new test (testNormalizationOptimizationOnAvailableIDs) should be marked `@Nightly` because it takes a very long time (not your fault, just the reality of what it is doing). It will still do what we need, as it will still detect any problems on ICU upgrade.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-799752136


   Yeah; it's been a while since I thought about this, but I think there's an inherent challenge in the way Transliterators are composable (can be chained together), but also kind of atomic/black-box (assume nothing about input or output and so internally do whatever manipulation they need in order to get the text in/out of a form they're designed to work with). iirc the "black box" Transliterators that internally construct composite Transliterators don't have their component parts cleanly exposed, so if one wants the functionality provided by a given Transliterator, the only (practical) option is to accept whatever higher-level baggage is bundled with it.
   
   More accurate offsets, and probably better performance, could I think be achieved by "decomposing" composite transliterators (and some fancy footwork). But I remember feeling convinced that it would be really hard to do this "right" (see [last comment on parent PR](https://github.com/apache/lucene-solr/pull/892#issuecomment-567644194)) -- and that although it probably _could_ be done in Lucene, it would probably be more appropriately done in ICU. I think this would amount to a substantial change (addition?) to the ICU Transliterator API/implementation, which currently doesn't track offsets at all, nor the way filters can kind of "bypass" elements of the decomposed transliterator chain.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] uschindler commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

uschindler commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r600864899



##########
File path: lucene/analysis/icu/src/test/org/apache/lucene/analysis/icu/TestICUTransformCharFilter.java
##########
@@ -0,0 +1,423 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.icu;
+
+import static org.apache.lucene.analysis.icu.ICUTransformCharFilterFactory.FAIL_ON_ROLLBACK_BUFFER_OVERFLOW_ARGNAME;
+import static org.apache.lucene.analysis.icu.ICUTransformCharFilterFactory.MAX_ROLLBACK_BUFFER_CAPACITY_ARGNAME;
+
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.UnicodeSet;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.StringReader;
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.lucene.analysis.Analyzer;
+import org.apache.lucene.analysis.BaseTokenStreamTestCase;
+import org.apache.lucene.analysis.CharFilter;
+import org.apache.lucene.analysis.MockTokenizer;
+import org.apache.lucene.analysis.Tokenizer;
+import org.apache.lucene.analysis.core.KeywordTokenizer;
+
+/** Test the ICUTransformCharFilter with some basic examples. */
+public class TestICUTransformCharFilter extends BaseTokenStreamTestCase {
+
+  public void testBasicFunctionality() throws Exception {
+    checkToken("Traditional-Simplified", "簡化字", "简化字");
+    checkToken("Katakana-Hiragana", "ヒラガナ", "ひらがな");
+    checkToken("Fullwidth-Halfwidth", "アルアノリウ", "ｱﾙｱﾉﾘｳ");
+    checkToken("Any-Latin", "Αλφαβητικός Κατάλογος", "Alphabētikós Katálogos");
+    checkToken(
+        "NFD; [:Nonspacing Mark:] Remove", "Alphabētikós Katálogos", "Alphabetikos Katalogos");
+    checkToken("Han-Latin", "中国", "zhōng guó");
+  }
+
+  public void testRollbackBuffer() throws Exception {
+    checkToken("Cyrillic-Latin", "яяяяя", "âââââ", false); // final NFC transform applied
+    checkToken(
+        "Cyrillic-Latin",
+        0,
+        false,
+        "яяяяя",
+        "a\u0302a\u0302a\u0302a\u0302a\u0302",
+        true); // final NFC transform never applied
+    checkToken(
+        "Cyrillic-Latin",
+        0,
+        false,
+        "яяяяя",
+        "âââââ",
+        false); // final NFC transform disabled internally, applied externally
+    checkToken("Cyrillic-Latin", 2, false, "яяяяя", "ââa\u0302a\u0302a\u0302", true);
+    checkToken(
+        "Cyrillic-Latin",
+        4,
+        false,
+        "яяяяяяяяяя",
+        "ââa\u0302a\u0302a\u0302a\u0302a\u0302a\u0302a\u0302a\u0302",
+        true);
+    checkToken(
+        "Cyrillic-Latin",
+        8,
+        false,
+        "яяяяяяяяяяяяяяяяяяяя",
+        "ââââââa\u0302ââââa\u0302ââââa\u0302âââ",
+        true);
+    try {
+      checkToken(
+          "Cyrillic-Latin",
+          8,
+          true,
+          "яяяяяяяяяяяяяяяяяяяя",
+          "ââââââa\u0302ââââa\u0302ââââa\u0302âââ",
+          true);
+      fail("with failOnRollbackBufferOverflow=true, we expect to throw a RuntimeException");
+    } catch (RuntimeException ex) {
+      // this is expected.
+    }
+  }
+
+  /**
+   * Sanity check all top-level prepackaged Transliterators to make sure that no trivial errors are
+   * thrown on instantiation. We're not really checking anything in particular here, but under the
+   * hood this will at least make a cursory check for consistency between the "stock"
+   * Transliterator, and any potential "optimized" version with externalized unicode normalization.
+   */
+  @Nightly
+  public void testNormalizationOptimizationOnAvailableIDs() throws Exception {
+    Enumeration<String> ids = Transliterator.getAvailableIDs();
+    List<String> idsWithNestedNormalization = new ArrayList<>();
+    while (ids.hasMoreElements()) {
+      String id = ids.nextElement();
+      @SuppressWarnings("unused")
+      boolean hasNestedUnicodeNormalization =
+          accumulateNestedUnicodeNormalization(id, idsWithNestedNormalization);
+      try {
+        // set `iterationsDefault=0` because in this test we simply want to ignore the non-optimized
+        //  case.
+        // set `iterationsOptimized=1` because `testRandomStrings` can be slow; we want this as a
+        //  sanity check, but as a matter of course we should not need many iterations.
+        @SuppressWarnings("unused")
+        boolean optimized = testRandomStrings(id, 0, 1);
+
+        // We can't really _do_ much with `hasNestedUnicodeNormalization` or `optimized`, in terms
+        // of validation.
+        // They are completely independent (i.e. not mutually exclusive). We leave these checks
+        // stubbed out here because we're initially optimizing normalization that can be easily
+        // detected at the top level, but we want some plumbing in place to be more transparent
+        // about what we're optimizing and what we're not (and to some extent _why_).
+        // TODO: We're aware that this is probably leaving "on the table" a bunch of potential
+        //  optimization of the "_nested_ unicode normalization" case. It may be worth optimizing
+        //  these nested cases (and addressing any additional complexity specific to that case)
+        //  as a separate, follow-up issue.
+      } catch (Exception ex) {
+        // wrap the exception so that we can report the offending `id`
+        throw new RuntimeException("problem for id: " + id, ex);
+      }
+    }
+  }
+
+  /**
+   * It is possible that leading and trailing (or singleton) Transliterators might apply nested
+   * Unicode normalization, thus acting in the capacity of a Normalizer, without qualifying as a
+   * top-level Normalizer as currently defined in {@link
+   * ICUTransformCharFilterFactory#unicodeNormalizationType(String)}. For now, simply detect these
+   * cases (to facilitate more nuanced handling in the future, if necessary).
+   *
+   * @param topLevelId top level Transliterator parent id.
+   * @param ids list to which the topLevelId will be added if nested unicode normalization is
+   *     detected
+   */
+  private static boolean accumulateNestedUnicodeNormalization(String topLevelId, List<String> ids) {
+    Transliterator levelOne = Transliterator.getInstance(topLevelId);
+    Transliterator[] topLevelElements = levelOne.getElements();
+    if (topLevelElements.length == 1 && levelOne == topLevelElements[0]) {
+      // A leaf Transliterator; shortcircuit
+      return false;
+    }
+    ArrayDeque<Transliterator> elements =
+        new ArrayDeque<>(topLevelElements.length << 2); // oversize
+    elements.addAll(Arrays.asList(topLevelElements));
+    do {
+      final Transliterator t = elements.removeFirst();
+      if (ICUTransformCharFilterFactory.unicodeNormalizationType(t.getID()) != null) {
+        ids.add(topLevelId);
+        return true;
+      }
+      Transliterator[] subElements = t.getElements();
+      if (subElements.length > 1 || t != subElements[0]) {
+        for (Transliterator sub : subElements) {
+          elements.addFirst(sub);
+        }
+      }
+    } while (!elements.isEmpty());
+    return false;
+  }
+
+  public void testCustomFunctionality() throws Exception {
+    String rules = "a > b; b > c;"; // convert a's to b's and b's to c's
+    checkToken(
+        Transliterator.createFromRules("test", rules, Transliterator.FORWARD),
+        new StringReader("abacadaba"),
+        "bcbcbdbcb");
+  }
+
+  public void testCustomFunctionality2() throws Exception {
+    String rules = "c { a > b; a > d;"; // convert a's to b's and b's to c's
+    checkToken(
+        Transliterator.createFromRules("test", rules, Transliterator.FORWARD),
+        new StringReader("caa"),
+        "cbd");
+  }
+
+  public void testOptimizer() throws Exception {
+    String rules = "a > b; b > c;"; // convert a's to b's and b's to c's
+    Transliterator custom = Transliterator.createFromRules("test", rules, Transliterator.FORWARD);
+    assertTrue(custom.getFilter() == null);
+    new ICUTransformCharFilter(new StringReader(""), custom);
+    assertTrue(custom.getFilter().equals(new UnicodeSet("[ab]")));
+  }
+
+  public void testOptimizer2() throws Exception {
+    checkToken("Traditional-Simplified; CaseFold", "ABCDE", "abcde");
+  }
+
+  public void testOptimizerSurrogate() throws Exception {
+    String rules = "\\U00020087 > x;"; // convert CJK UNIFIED IDEOGRAPH-20087 to an x
+    Transliterator custom = Transliterator.createFromRules("test", rules, Transliterator.FORWARD);
+    assertTrue(custom.getFilter() == null);
+    new ICUTransformCharFilter(new StringReader(""), custom);
+    assertTrue(custom.getFilter().equals(new UnicodeSet("[\\U00020087]")));
+  }
+
+  private void checkToken(Transliterator transliterator, Reader input, String expected)
+      throws IOException {
+    input =
+        new ICUTransformCharFilter(
+            input,
+            transliterator,
+            ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+            false);
+    final KeywordTokenizer input1 = new KeywordTokenizer();
+    input1.setReader(input);
+    assertTokenStreamContents(input1, new String[] {expected});
+  }
+
+  private void checkToken(String id, String input, String expected) throws IOException {
+    checkToken(
+        id, ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY, false, input, expected);
+  }
+
+  private void checkToken(
+      String id, String input, String expected, boolean suppressUnicodeNormExternalization)
+      throws IOException {
+    checkToken(
+        id,
+        ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+        false,
+        input,
+        expected,
+        suppressUnicodeNormExternalization);
+  }
+
+  private void checkToken(
+      String id,
+      int maxRollbackBufferCapacity,
+      boolean failOnRollbackBufferOverflow,
+      String input,
+      String expected)
+      throws IOException {
+    checkToken(
+        getTransliteratingFilter(
+            id, new StringReader(input), maxRollbackBufferCapacity, failOnRollbackBufferOverflow),
+        expected);
+  }
+
+  private void checkToken(
+      String id,
+      int maxRollbackBufferCapacity,
+      boolean failOnRollbackBufferOverflow,
+      String input,
+      String expected,
+      boolean suppressUnicodeNormalizationExternalization)
+      throws IOException {
+    checkToken(
+        getTransliteratingFilter(
+            null,
+            id,
+            new StringReader(input),
+            suppressUnicodeNormalizationExternalization,
+            maxRollbackBufferCapacity,
+            failOnRollbackBufferOverflow),
+        expected);
+  }
+
+  private void checkToken(CharFilter input, String expected) throws IOException {
+    final KeywordTokenizer input1 = new KeywordTokenizer();
+    input1.setReader(input);
+    assertTokenStreamContents(input1, new String[] {expected});
+  }
+
+  public void testRandomStringsLatinToKatakana() throws Exception {
+    // this Transliterator often decreases character length wrt input
+    // we _don't_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Latin-Katakana", 1000, -1);
+  }
+
+  public void testRandomStringsAnyToLatin() throws Exception {
+    // this Transliterator often increases character length wrt input
+    // we _don't_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Any-Latin", 1000, -1);
+  }
+
+  public void testRandomStringsKatakanaToHiragana() throws Exception {
+    // this Transliterator often increases character length wrt input
+    // we _do_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Katakana-Hiragana", -1, 1000);
+  }
+
+  private static Analyzer getAnalyzer(String id, boolean suppressExternalize, boolean[] optimized) {
+    if (optimized != null && !suppressExternalize) {
+      getTransliteratingFilter(optimized, id, new StringReader("dummy"), suppressExternalize);
+    }
+    return new Analyzer() {
+      @Override
+      protected TokenStreamComponents createComponents(String fieldName) {
+        Tokenizer tokenizer = new MockTokenizer(MockTokenizer.WHITESPACE, false);
+        return new TokenStreamComponents(tokenizer, tokenizer);
+      }
+
+      @Override
+      protected Reader initReader(String fieldName, Reader reader) {
+        return super.initReader(
+            fieldName, getTransliteratingFilter(null, id, reader, suppressExternalize));
+      }
+
+      @Override
+      protected Reader initReaderForNormalization(String fieldName, Reader reader) {
+        return super.initReaderForNormalization(
+            fieldName, getTransliteratingFilter(null, id, reader, suppressExternalize));
+      }
+    };
+  }
+
+  /** blast some random strings through the analyzer */
+  private boolean testRandomStrings(final String id, int iterationsDefault, int iterationsOptimized)
+      throws Exception {
+    final boolean firstSuppressExternalize = random().nextBoolean();
+    boolean[] optimized = new boolean[] {false};
+    Analyzer a = getAnalyzer(id, firstSuppressExternalize, optimized);
+    Analyzer b = getAnalyzer(id, !firstSuppressExternalize, optimized);
+    final int iterations;
+    if (optimized[0]) {
+      iterations = iterationsOptimized;
+    } else {
+      iterations = iterationsDefault;
+      // there's no optimization; it doesn't matter which analyzer we use.
+      b.close();
+      b = null;
+    }
+    assertTrue(
+        "implicitly expected optimized=" + optimized[0] + "; requested iterations=" + iterations,
+        iterations >= 0);
+    checkRandomData(random(), a, b, iterations * RANDOM_MULTIPLIER);

Review comment:
       Hi,
   ok thanks for the clarification. I was very busy today to verify the BaseTokenStreamTestCase changes and missed this small detail, because it was hidden in all other changes! I think that it might not be a good idea to make this class more complex, by adding yet another method with again another possibly null parameter. This class is much too complicated already, so we would soon need a test to test the tester!
   
   So I would really like to suggest to undo the changes to BaseTokenStreamTestCase and leave this class alone, as almost every tokenstream/analyzer is tested through it. It's way to risky to change it and make it even more complex - just for one test!
   
   Would it not be enough here to just check random terms or analyzer consistency with the current code sequentially (as suggested) and just write some additional test, that compares positions and offsets for this tokenstream? I am not sure if random data is needed for this, but you can easily generate it. Then just compare the offsets and positions one by one, ignoring all other attributes.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-832362947


   yes, it is much easier for me to help out if the build and tests are working, I can't really review otherwise because I rarely write java these days. So to suggest something I usually have to test it out locally


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-832732406


   @uschindler FYI all modifications to `BaseTokenStreamTestCase` (from earlier iterations of this PR) have now been reverted.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r596904385



##########
File path: lucene/analysis/icu/src/java/org/apache/lucene/analysis/icu/ICUTransformCharFilter.java
##########
@@ -0,0 +1,686 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.analysis.icu;
+
+import com.ibm.icu.impl.Utility;
+import com.ibm.icu.text.ReplaceableString;
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.Transliterator.Position;
+import com.ibm.icu.text.UTF16;
+import java.io.IOException;
+import java.io.Reader;
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.WeakHashMap;
+import org.apache.lucene.analysis.CharFilter;
+import org.apache.lucene.analysis.charfilter.BaseCharFilter;
+import org.apache.lucene.util.ArrayUtil;
+
+/**
+ * A {@link CharFilter} that transforms text with ICU.
+ *
+ * <p>ICU provides text-transformation functionality via its Transliteration API. Although script
+ * conversion is its most common use, a Transliterator can actually perform a more general class of
+ * tasks. In fact, Transliterator defines a very general API which specifies only that a segment of
+ * the input text is replaced by new text. The particulars of this conversion are determined
+ * entirely by subclasses of Transliterator.
+ *
+ * <p>Some useful transformations for search are built-in:
+ *
+ * <ul>
+ *   <li>Conversion from Traditional to Simplified Chinese characters
+ *   <li>Conversion from Hiragana to Katakana
+ *   <li>Conversion from Fullwidth to Halfwidth forms.
+ *   <li>Script conversions, for example Serbian Cyrillic to Latin
+ * </ul>
+ *
+ * <p>Example usage:
+ *
+ * <blockquote>
+ *
+ * stream = new ICUTransformCharFilter(reader,
+ * Transliterator.getInstance("Traditional-Simplified"));
+ *
+ * </blockquote>
+ *
+ * <br>
+ * For more details, see the <a href="http://userguide.icu-project.org/transforms/general">ICU User
+ * Guide</a>.
+ */
+public final class ICUTransformCharFilter extends BaseCharFilter {
+
+  // Transliterator to transform the text
+  private final Transliterator transform;
+
+  // Reusable position object
+  private final Position position = new Position();
+
+  private static final int READ_BUFFER_SIZE = 1024;
+  private final char[] tmpBuffer = new char[READ_BUFFER_SIZE];
+
+  private static final int INITIAL_TRANSLITERATE_BUFFER_SIZE = 1024;
+  private final StringBuffer buffer = new StringBuffer(INITIAL_TRANSLITERATE_BUFFER_SIZE);
+  private final ReplaceableString replaceable = new ReplaceableString(buffer);
+
+  private static final int BUFFER_PRUNE_THRESHOLD = 1024;
+
+  private int outputCursor = 0;
+  private boolean inputFinished = false;
+  private int charCount = 0;
+  private int offsetDiffAdjust = 0;
+
+  private static final int HARD_MAX_ROLLBACK_BUFFER_CAPACITY =
+      Integer.highestOneBit(Integer.MAX_VALUE);
+  static final int DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY = 8192;
+  private final int maxRollbackBufferCapacity;
+
+  private static final int DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY = 4; // must be power of 2
+  private char[] rollbackBuffer;
+  private int rollbackBufferSize = 0;
+
+  static final boolean DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW = true;
+  private final boolean failOnRollbackBufferOverflow;
+
+  ICUTransformCharFilter(Reader in, Transliterator transform) {
+    this(
+        in,
+        transform,
+        DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+        DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW);
+  }
+
+  /**
+   * Construct new {@link ICUTransformCharFilter} with the specified {@link Transliterator}, backed
+   * by the specified {@link Reader}.
+   *
+   * @param in input source
+   * @param transform used to perform transliteration
+   * @param maxRollbackBufferCapacityHint used to control the maximum size to which this {@link
+   *     ICUTransformCharFilter} will buffer and rollback partial transliteration of input
+   *     sequences. The provided hint will be converted to an enforced limit of "the greatest power
+   *     of 2 (excluding '1') less than or equal to the specified value". It is illegal to specify a
+   *     negative value. There is no power of 2 greater than <code>
+   *     Integer.highestOneBit(Integer.MAX_VALUE))</code>, so to prevent overflow, values in this
+   *     range will resolve to an enforced limit of <code>Integer.highestOneBit(Integer.MAX_VALUE))
+   *     </code>. Specifying "0" (or "1", in practice) disables rollback. Larger values can in some
+   *     cases yield more accurate transliteration, at the cost of performance and
+   *     resolution/accuracy of offset correction. This is intended primarily as a failsafe, with a
+   *     relatively large default value of {@value
+   *     ICUTransformCharFilter#DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY}. See comments "To understand
+   *     the need for rollback" in private method: {@link
+   *     Transliterator#filteredTransliterate(com.ibm.icu.text.Replaceable, Position, boolean,
+   *     boolean)}
+   * @param failOnRollbackBufferOverflow if true, transliteration failing to complete within the
+   *     specified rollbackBuffer window will fail with an exception. If false, transliteration
+   *     forges ahead with possibly-inconsistent results.
+   */
+  ICUTransformCharFilter(
+      Reader in,
+      Transliterator transform,
+      int maxRollbackBufferCapacityHint,
+      boolean failOnRollbackBufferOverflow) {
+    super(in);
+    this.transform = ICUTransformFilter.optimizeForCommonCase(transform);
+    this.failOnRollbackBufferOverflow = failOnRollbackBufferOverflow;
+    if (maxRollbackBufferCapacityHint < 0) {
+      throw new IllegalArgumentException(
+          "It is illegal to request negative rollback buffer max capacity");
+    } else if (maxRollbackBufferCapacityHint >= HARD_MAX_ROLLBACK_BUFFER_CAPACITY) {
+      // arg is positive, so user wants the largest possible buffer capacity limit
+      // we know what that is (static), protecting for overflow.
+      this.maxRollbackBufferCapacity = HARD_MAX_ROLLBACK_BUFFER_CAPACITY;
+      this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+    } else {
+      // greatest power of 2 (excluding "1") less than or equal to the specified hint
+      this.maxRollbackBufferCapacity =
+          Integer.highestOneBit(maxRollbackBufferCapacityHint - 1) << 1;
+      if (this.maxRollbackBufferCapacity == 0) {
+        this.rollbackBuffer = null;
+      } else {
+        this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+      }
+    }
+  }
+
+  /**
+   * Reads characters into a portion of an array. This method will block until some input is
+   * available, an I/O error occurs, or the end of the stream is reached.
+   *
+   * @param cbuf Destination buffer
+   * @param off Offset at which to start storing characters
+   * @param len Maximum number of characters to read
+   * @return The number of characters read, or -1 if the end of the stream has been reached
+   * @throws IOException If an I/O error occurs
+   */
+  @Override
+  public int read(char[] cbuf, int off, int len) throws IOException {
+    while (!inputFinished || position.start > outputCursor) {
+      // (expecting more input) || (output remains that has not been flushed)
+      if (position.start > outputCursor) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      int resLen = transliterateBufferContents();
+      if (resLen > 0) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      if (!readInputToBuffer()) {
+        final int preStart = position.start;
+        final int preLimit;
+        final int bufferLength = buffer.length();
+        if (preStart < bufferLength) {
+          if (UTF16.isLeadSurrogate(buffer.charAt(bufferLength - 1))) {
+            // if last char is a lead surrogate, transform won't handle it properly
+            // preLimit = bufferLength - 1; // could simply pass the malformed input along
+            throw new RuntimeException(
+                "malformed input; final UTF16 character is a lead surrogate");
+          } else {
+            preLimit = bufferLength;
+          }
+          position.contextLimit = preLimit;
+          position.limit = preLimit;
+          transform.finishTransliteration(replaceable, position);
+        } else if (offsetDiffAdjust == 0) {
+          break;
+        } else {
+          preLimit = bufferLength;
+        }
+        cursorAdvanced(preStart, preLimit);
+      }
+    }
+
+    return -1;
+  }
+
+  private void rollback(int preStart, int preLimit) {
+    buffer.delete(preStart, position.limit); // delete uncommitted chars
+    buffer.insert(preStart, rollbackBuffer, 0, rollbackBufferSize);
+    position.start = preStart;
+    position.contextLimit = preLimit;
+    position.limit = preLimit;
+  }
+
+  /**
+   * Grow rollback buffer if necessary, within constraints of {@link #maxRollbackBufferCapacity}.
+   * This imposes an arbitrary failsafe to prevent the possibility that the rollback buffer could
+   * grow indefinitely.
+   *
+   * @return true if upon return, rollback buffer has sufficient capacity for new input chars,
+   *     otherwise false.
+   */
+  private boolean ensureRollbackBufferCapacity() {
+    // ensure space for at least 2 chars (surrogate pair, max possible space needed)
+    if (rollbackBuffer.length - rollbackBufferSize < 2) {
+      if (rollbackBuffer.length < maxRollbackBufferCapacity) {
+        rollbackBuffer = ArrayUtil.growExact(rollbackBuffer, rollbackBuffer.length << 1);
+      } else {
+        // hit threshold; not going to increase the buffer size
+        if (failOnRollbackBufferOverflow) {
+          throw new RuntimeException(
+              "input could not be transliterated without overflowing maxRollbackBufferCapacity ("
+                  + maxRollbackBufferCapacity
+                  + "); "
+                  + "try increasing maxRollbackBufferCapacity, or setting failOnRollbackBufferOverflow=false");
+        }
+        return false;
+      }
+    }
+    return true;
+  }
+
+  private int pushRollbackBuffer(int idx, int bufferLength) {
+    if (idx >= bufferLength) {
+      return 0;
+    } else {
+      // note: we have already ensured sufficient buffer capacity
+      final char candidate = buffer.charAt(idx);
+      if (!UTF16.isLeadSurrogate(candidate)) {
+        if (rollbackBuffer != null) {
+          rollbackBuffer[rollbackBufferSize++] = candidate;
+        }
+        return 1;
+      } else {
+        if (++idx < bufferLength) {
+          if (rollbackBuffer != null) {
+            rollbackBuffer[rollbackBufferSize++] = candidate;
+            rollbackBuffer[rollbackBufferSize++] = buffer.charAt(idx);
+          }
+          return 2;
+        } else {
+          // we don't yet have the high surrogate
+          if (inputFinished) {
+            // no more input available; proceed with the char we have; rollback not needed
+            return 1;
+          } else {
+            return 0; // wait for more input to be available
+          }
+        }
+      }
+    }
+  }
+
+  /**
+   * Transliterate as much of the contents of {@link #buffer} as possible.
+   *
+   * @return number of output characters transliterated (possibly 0)
+   */
+  private int transliterateBufferContents() {
+    int nextCharLength = pushRollbackBuffer(position.limit, buffer.length());
+    if (nextCharLength == 0) {
+      return 0;
+    }
+    final int preCharCount = charCount;
+    int preStart = position.start;
+    int preLimit;
+    do {
+      position.limit += nextCharLength;
+      preLimit = position.limit;
+      position.contextLimit = preLimit;
+      transform.filteredTransliterate(replaceable, position, true);
+      boolean rollbackSizeWithinBounds = true;
+      if (rollbackBuffer != null
+          && position.start < position.limit
+          && (rollbackSizeWithinBounds = ensureRollbackBufferCapacity())) {
+        // complete pass not transliterated, and not yet at rollback buffer threshold cap.
+        // N.b.: rollback buffer threshold cap is arbitrary, so we check here (somewhat
+        // counterintuitively, *before* rolling back, as opposed to before pushing to
+        // the buffer) so that we can fall through to the else clause, clear the rollback
+        // buffer, and proceed.
+        rollback(preStart, preLimit);
+      } else {
+        rollbackBufferSize = 0;
+        if (position.start > preStart) {
+          // cursor advanced
+          cursorAdvanced(preStart, preLimit);
+
+          // N.b.: advancing contextStart precludes support for quantifiers, but is crucial for
+          // streaming,
+          // so we'll do it anyway.
+          // See comments in Transliterator source code:
+          // TODO
+          // This doesn't work once we add quantifier support.  Need to rewrite
+          // this code to support quantifiers and 'use maximum backup <n>;'.
+          //
+          //         index.contextStart = Math.max(index.start - getMaximumContextLength(),
+          //                                       originalStart);
+          position.contextStart =
+              Math.max(position.start - transform.getMaximumContextLength(), preStart);
+          preStart = position.start;
+          if (!rollbackSizeWithinBounds) {
+            // prepopulate newly cleared rollback buffer with all top-level uncommitted characters
+            rollbackBufferSize = position.limit - preStart;
+            if (rollbackBuffer.length - rollbackBufferSize < 2) {
+              // even after flushing all committed text, there's not enough space in the rollback
+              // buffer.
+              // This is an edge case of an edge case, when the last char32 read into the rollback
+              // buffer
+              // is a surrogate pair (completely filling the rollback buffer), *and* the last
+              // transliteration pass advanced position.start by exactly one char16 (not a surrogate
+              // pair).
+              preStart = forceAdvance(preStart, preLimit);
+              rollbackBufferSize = position.limit - preStart;
+            }
+            buffer.getChars(preStart, position.limit, rollbackBuffer, 0);
+          }
+        } else if (preLimit != position.limit) {
+          // cursor hasn't advanced; incoming characters have probably been deleted
+          offsetDiffAdjust += preLimit - position.limit;
+          // edge case of !rollbackSizeWithinBounds needs no special handling here, since input
+          // characters *are* being
+          // processed (deleted) -- we *are* progressing through the input stream, although the
+          // output stream hasn't changed.
+        } else if (!rollbackSizeWithinBounds) {
+          // cursor hasn't advanced, no incoming characters have been deleted, and the rollback
+          // buffer is full.
+          preStart = forceAdvance(preStart, preLimit);
+        }
+      }
+    } while ((nextCharLength = pushRollbackBuffer(position.limit, buffer.length())) > 0);
+
+    return charCount - preCharCount;
+  }
+
+  private static final int FORCE_THRESHOLD = 2; // conservative; the length of a surrogate pair
+
+  private int forceAdvance(int preStart, int preLimit) {
+    int shift;
+    if (maxRollbackBufferCapacity == 2) {
+      rollbackBufferSize = 0;
+      shift = 2;
+    } else {
+      shift = 0;
+      do {
+        shift += UTF16.isLeadSurrogate(rollbackBuffer[shift]) ? 2 : 1;
+      } while (shift < FORCE_THRESHOLD && shift < maxRollbackBufferCapacity);
+      rollbackBufferSize -= shift;
+      System.arraycopy(rollbackBuffer, shift, rollbackBuffer, 0, rollbackBufferSize);
+    }
+    position.start += shift; // mock transliterator advance
+    cursorAdvanced(preStart, preLimit);
+    position.contextStart =
+        Math.max(position.start - transform.getMaximumContextLength(), preStart);
+    return position.start;
+  }
+
+  private void cursorAdvanced(int preStart, int preLimit) {
+    final int outputLength = position.start - preStart;
+    final int diff = preLimit - position.limit + offsetDiffAdjust;
+    offsetDiffAdjust = 0;
+    if (diff == 0) {
+      // increment charCount; no offset correction necessary
+      charCount += outputLength;
+    } else {
+      // limit change indicates change in length of replacement text; correct offsets accordingly
+      // we correct offsets as frequently as possible, for every incremental change. This will
+      // sometimes be for a range of characters, but that's the best we can do as far as the level
+      // of granularity that's available to us via the Transliterator API.
+      recordOffsetDiff(diff, outputLength);
+    }
+  }
+
+  private void recordOffsetDiff(int diff, int outputLength) {
+    final int cumuDiff = getLastCumulativeDiff();
+    if (diff < 0) {
+      // positive diff indicates an increase in character count wrt input
+      for (int i = 1; i <= -diff; ++i) {
+        addOffCorrectMap(charCount + i, cumuDiff - i);
+      }
+    } else {
+      // positive diff indicates an decrease in character count wrt input
+      addOffCorrectMap(charCount + outputLength, cumuDiff + diff);
+    }
+    charCount += outputLength;
+  }
+
+  private boolean readInputToBuffer() throws IOException {
+    int res = input.read(tmpBuffer, 0, tmpBuffer.length);
+    if (res == -1) {
+      inputFinished = true;
+      return false;
+    } else {
+      buffer.append(tmpBuffer, 0, res);
+      return true;
+    }
+  }
+
+  private int outputFromResultBuffer(char[] cbuf, int begin, int len) {
+    len = Math.min(position.start - outputCursor, len);
+    buffer.getChars(outputCursor, outputCursor + len, cbuf, begin);
+    outputCursor += len;
+    if (outputCursor > BUFFER_PRUNE_THRESHOLD && position.contextStart > BUFFER_PRUNE_THRESHOLD) {
+      final int pruneSize = Math.min(outputCursor, position.contextStart);
+      buffer.delete(0, pruneSize);
+      position.contextStart -= pruneSize;
+      position.start -= pruneSize;
+      position.limit -= pruneSize;
+      position.contextLimit -= pruneSize;
+      outputCursor -= pruneSize;
+    }
+    return len;
+  }
+
+  /**
+   * Strips off Unicode character normalization form rules from a Transliterator. Do this if, as is
+   * typical, your analysis chain already includes another component that does normalization, like
+   * {@link ICUNormalizer2CharFilter}.
+   *
+   * @return a new Transliterator with no normalization, or the original Transliterator if it
+   *     already did no normalization.
+   */
+  public static Transliterator withoutUnicodeNormalization(Transliterator transliterator) {
+    final String modifiedRules = modifyRules(false, transliterator);
+    if (modifiedRules == null) {
+      return transliterator;
+    }
+    String baseId = transliterator.getID();
+    String modId = baseId.concat(baseId.lastIndexOf('/') < 0 ? "/X_NO_NORM_IO" : "_X_NO_NORM_IO");
+    return Transliterator.createFromRules(modId, modifiedRules, Transliterator.FORWARD);
+  }

Review comment:
       i would just wrap the stream with multiple filters similar to the review comment. No, there is usually factories wrapping things with multiple streams, but nothing prevents it.
   
   I would not introduce any single composite thing here as I don't see the benefits, e.g. start simple. It is really just replacing the previous option with an automatic optimization to me.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r598135382



##########
File path: lucene/analysis/icu/src/java/org/apache/lucene/analysis/icu/ICUTransformCharFilterFactory.java
##########
@@ -0,0 +1,391 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.icu;
+
+import com.ibm.icu.impl.Utility;
+import com.ibm.icu.text.Normalizer2;
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.UTF16;
+import java.io.Reader;
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.analysis.CharFilterFactory;
+
+/**
+ * Factory for {@link ICUTransformCharFilter}.
+ *
+ * <p>Supports the following attributes:
+ *
+ * <ul>
+ *   <li>id (mandatory): A Transliterator ID, one from {@link Transliterator#getAvailableIDs()}
+ *   <li>direction (optional): Either 'forward' or 'reverse'. Default is forward.
+ * </ul>
+ *
+ * @see Transliterator
+ * @since 8.3.0
+ * @lucene.spi {@value #NAME}
+ */
+public class ICUTransformCharFilterFactory extends CharFilterFactory {
+
+  /** SPI name */
+  public static final String NAME = "icuTransform";
+
+  static final String MAX_ROLLBACK_BUFFER_CAPACITY_ARGNAME = "maxRollbackBufferCapacity";
+  static final String FAIL_ON_ROLLBACK_BUFFER_OVERFLOW_ARGNAME = "failOnRollbackBufferOverflow";
+  static final String SUPPRESS_UNICODE_NORMALIZATION_EXTERNALIZATION_ARGNAME =
+      "suppressUnicodeNormalizationExternalization";
+  private final NormType leading;
+  private final Transliterator transliterator;
+  private final NormType trailing;
+  private final int maxRollbackBufferCapacity;
+  private final boolean failOnRollbackBufferOverflow;
+
+  // TODO: add support for custom rules
+  /** Creates a new ICUTransformFilterFactory */
+  public ICUTransformCharFilterFactory(Map<String, String> args) {
+    super(args);
+    String id = require(args, "id");
+    String direction =
+        get(args, "direction", Arrays.asList("forward", "reverse"), "forward", false);
+    int dir = "forward".equals(direction) ? Transliterator.FORWARD : Transliterator.REVERSE;
+    int tmpCapacityHint =
+        getInt(
+            args,
+            MAX_ROLLBACK_BUFFER_CAPACITY_ARGNAME,
+            ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY);
+    this.maxRollbackBufferCapacity = tmpCapacityHint == -1 ? Integer.MAX_VALUE : tmpCapacityHint;
+    this.failOnRollbackBufferOverflow =
+        getBoolean(
+            args,
+            FAIL_ON_ROLLBACK_BUFFER_OVERFLOW_ARGNAME,
+            ICUTransformCharFilter.DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW);
+    boolean suppressUnicodeNormalizationExternalization =
+        getBoolean(args, SUPPRESS_UNICODE_NORMALIZATION_EXTERNALIZATION_ARGNAME, false);
+    Transliterator stockTransliterator = Transliterator.getInstance(id, dir);
+    if (suppressUnicodeNormalizationExternalization) {
+      this.leading = null;
+      this.transliterator = stockTransliterator;
+      this.trailing = null;
+    } else {
+      ExternalNormalization ext = externalizeUnicodeNormalization(stockTransliterator);
+      this.leading = ext.leading;
+      this.transliterator = ext.t;
+      this.trailing = ext.trailing;
+    }
+    if (!args.isEmpty()) {
+      throw new IllegalArgumentException("Unknown parameters: " + args);
+    }
+  }
+
+  private static final Reader wrapReader(NormType normType, Reader r) {
+    if (normType == null) {
+      return r;
+    }
+    switch (normType) {
+      case NFC:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFCInstance());
+      case NFD:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFDInstance());
+      case NFKC:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFKCInstance());
+      case NFKD:
+        return new ICUNormalizer2CharFilter(r, Normalizer2.getNFKDInstance());
+      default:
+        throw new UnsupportedOperationException(
+            "test not yet able to compensate externally for normalization type \""
+                + normType
+                + "\"");
+    }
+  }
+
+  /** Default ctor for compatibility with SPI */
+  public ICUTransformCharFilterFactory() {
+    throw defaultCtorException();
+  }
+
+  @Override
+  public Reader create(Reader input) {
+    input = wrapReader(leading, input);
+    input =
+        new ICUTransformCharFilter(
+            input, transliterator, maxRollbackBufferCapacity, failOnRollbackBufferOverflow);
+    return wrapReader(trailing, input);
+  }
+
+  private static final boolean ESCAPE_UNPRINTABLE = false;
+
+  /**
+   * Attempts to detect and externalize any leading or trailing top-level Unicode normalization. In
+   * the event that such normalization is detected, a new "core" Transliterator (with any detected
+   * pre-/post-normalization removed) is created and returned.
+   *
+   * <p>The creation of any new "core" Transliterator (and much of the actual code in this method)
+   * is based on the {@link com.ibm.icu.text.CompoundTransliterator#toRules(boolean)} method (with
+   * the boolean arg replaced by {@link #ESCAPE_UNPRINTABLE} -- always <code>false</code> in this
+   * context).
+   *
+   * @param t the Transliterator to base modified rules on.
+   * @return simple ExternalNormalization struct containing a non-null Transliterator, if possible
+   *     with any leading and trailing Unicode normalization externalized. The effect of applying
+   *     the resulting leading Unicode norm, Transliterator, and trailing Unicode norm, should be
+   *     equivalent to the effect of applying the input Transliterator t.
+   */
+  private static ExternalNormalization externalizeUnicodeNormalization(Transliterator t) {
+    final Transliterator[] trans = t.getElements();
+    final String topLevelId = t.getID();
+    final int start;
+    final int limit;
+    final NormType leading;
+    final NormType trailing;
+    if (trans.length == 1) {
+      warnNestedUnicodeNormalization(trans[0], topLevelId, true);
+      return new ExternalNormalization(null, t, null);
+    } else {
+      final int lastIndex;
+      if ((leading = unicodeNormalizationType(trans[0].getID())) != null) {
+        start = 1;
+        limit =
+            (trailing = unicodeNormalizationType(trans[lastIndex = trans.length - 1].getID()))
+                    != null
+                ? lastIndex
+                : trans.length;
+      } else if (warnNestedUnicodeNormalization(trans[0], topLevelId, true)
+          && (trailing = unicodeNormalizationType(trans[lastIndex = trans.length - 1].getID()))
+              != null) {
+        start = 0;
+        limit = lastIndex;
+      } else {
+        warnNestedUnicodeNormalization(trans[trans.length - 1], topLevelId, false);
+        return new ExternalNormalization(null, t, null);
+      }
+    }
+    // We do NOT call toRules() on our component transliterators, in
+    // general. If we have several rule-based transliterators, this
+    // yields a concatenation of the rules -- not what we want. We do
+    // handle compound RBT transliterators specially -- those for which
+    // compoundRBTIndex >= 0. For the transliterator at compoundRBTIndex,
+    // we do call toRules() recursively.
+    StringBuilder rulesSource = new StringBuilder();
+    if (t.getFilter() != null) {
+      // We might be a compound RBT and if we have a global
+      // filter, then emit it at the top.
+      rulesSource.append("::").append(t.getFilter().toPattern(ESCAPE_UNPRINTABLE)).append(ID_DELIM);
+    }
+    final int globalFilterEnd = rulesSource.length();
+    boolean hasAnonymousRBTs = false;
+    for (int i = start; i < limit; ++i) {
+      String rule;
+
+      // Anonymous RuleBasedTransliterators (inline rules and
+      // ::BEGIN/::END blocks) are given IDs that begin with
+      // "%Pass": use toRules() to write all the rules to the output
+      // (and insert "::Null;" if we have two in a row)
+      if (trans[i].getID().startsWith("%Pass")) {
+        hasAnonymousRBTs = true;
+        rule = trans[i].toRules(ESCAPE_UNPRINTABLE);
+        if (i > start && trans[i - 1].getID().startsWith("%Pass")) rule = "::Null;" + rule;
+
+        // we also use toRules() on CompoundTransliterators (which we
+        // check for by looking for a semicolon in the ID)-- this gets
+        // the list of their child transliterators output in the right
+        // format
+      } else if (trans[i].getID().indexOf(';') >= 0) {
+        rule = trans[i].toRules(ESCAPE_UNPRINTABLE);
+
+        // for everything else, use baseToRules()
+      } else {
+        rule = baseToRules(ESCAPE_UNPRINTABLE, trans[i]);
+      }
+      _smartAppend(rulesSource, '\n');
+      rulesSource.append(rule);
+      _smartAppend(rulesSource, ID_DELIM);
+    }
+    // Analogous to the contract for {@link com.ibm.icu.text.Transliterator#toRules(boolean)}, the
+    // modified rules String should be sufficient to recreate a Transliterator based on the
+    // specified input Transliterator, via {@link
+    // com.ibm.icu.text.Transliterator#createFromRules(String, String, int)}.
+    final String modifiedRules =
+        hasAnonymousRBTs ? rulesSource.toString() : rulesSource.substring(globalFilterEnd);
+    String baseId = t.getID();
+    String modId = baseId.concat(baseId.lastIndexOf('/') < 0 ? "/X_NO_NORM_IO" : "_X_NO_NORM_IO");
+    Transliterator replacement =
+        Transliterator.createFromRules(modId, modifiedRules, Transliterator.FORWARD);
+    return new ExternalNormalization(leading, replacement, trailing);
+  }
+
+  static class ExternalNormalization {
+    private final NormType leading;
+    private final Transliterator t;
+    private final NormType trailing;
+
+    private ExternalNormalization(NormType leading, Transliterator t, NormType trailing) {
+      this.leading = leading;
+      this.t = t;
+      this.trailing = trailing;
+    }
+  }
+
+  /**
+   * It is possible that leading and trailing (or singleton) Transliterators might apply nested
+   * Unicode normalization, thus acting in the capacity of a Normalizer, without qualifying as a
+   * top-level Normalizer as currently defined in {@link #unicodeNormalizationType(String)}. For
+   * now, we will emit a warning if users request stripping of i/o unicode normalization in such a
+   * case (to facilitate reporting and more nuanced handling in the future, if necessary).
+   *
+   * @param levelOne a level one component Transliterator to test for nested unicode normalization.
+   * @param topLevelId top level Transliterator parent id.
+   * @param leading true if leading component transliterator; false implies trailing component
+   * @return always return true, for easy integration with control flow.
+   */
+  private static boolean warnNestedUnicodeNormalization(

Review comment:
       It is not helpful to print/warn/log/populate-weak-maps, instead let's replace with a junit test that loops through Transliterator.getAvailableIDs or similar and asserts stuff about each one, failing if it doesn't meet expectations. This way, tests fail on icu version upgrade if something is newly introduced that must be dealt with.
   




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r600788632



##########
File path: lucene/analysis/icu/src/test/org/apache/lucene/analysis/icu/TestICUTransformCharFilter.java
##########
@@ -0,0 +1,423 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.icu;
+
+import static org.apache.lucene.analysis.icu.ICUTransformCharFilterFactory.FAIL_ON_ROLLBACK_BUFFER_OVERFLOW_ARGNAME;
+import static org.apache.lucene.analysis.icu.ICUTransformCharFilterFactory.MAX_ROLLBACK_BUFFER_CAPACITY_ARGNAME;
+
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.UnicodeSet;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.StringReader;
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.lucene.analysis.Analyzer;
+import org.apache.lucene.analysis.BaseTokenStreamTestCase;
+import org.apache.lucene.analysis.CharFilter;
+import org.apache.lucene.analysis.MockTokenizer;
+import org.apache.lucene.analysis.Tokenizer;
+import org.apache.lucene.analysis.core.KeywordTokenizer;
+
+/** Test the ICUTransformCharFilter with some basic examples. */
+public class TestICUTransformCharFilter extends BaseTokenStreamTestCase {
+
+  public void testBasicFunctionality() throws Exception {
+    checkToken("Traditional-Simplified", "簡化字", "简化字");
+    checkToken("Katakana-Hiragana", "ヒラガナ", "ひらがな");
+    checkToken("Fullwidth-Halfwidth", "アルアノリウ", "ｱﾙｱﾉﾘｳ");
+    checkToken("Any-Latin", "Αλφαβητικός Κατάλογος", "Alphabētikós Katálogos");
+    checkToken(
+        "NFD; [:Nonspacing Mark:] Remove", "Alphabētikós Katálogos", "Alphabetikos Katalogos");
+    checkToken("Han-Latin", "中国", "zhōng guó");
+  }
+
+  public void testRollbackBuffer() throws Exception {
+    checkToken("Cyrillic-Latin", "яяяяя", "âââââ", false); // final NFC transform applied
+    checkToken(
+        "Cyrillic-Latin",
+        0,
+        false,
+        "яяяяя",
+        "a\u0302a\u0302a\u0302a\u0302a\u0302",
+        true); // final NFC transform never applied
+    checkToken(
+        "Cyrillic-Latin",
+        0,
+        false,
+        "яяяяя",
+        "âââââ",
+        false); // final NFC transform disabled internally, applied externally
+    checkToken("Cyrillic-Latin", 2, false, "яяяяя", "ââa\u0302a\u0302a\u0302", true);
+    checkToken(
+        "Cyrillic-Latin",
+        4,
+        false,
+        "яяяяяяяяяя",
+        "ââa\u0302a\u0302a\u0302a\u0302a\u0302a\u0302a\u0302a\u0302",
+        true);
+    checkToken(
+        "Cyrillic-Latin",
+        8,
+        false,
+        "яяяяяяяяяяяяяяяяяяяя",
+        "ââââââa\u0302ââââa\u0302ââââa\u0302âââ",
+        true);
+    try {
+      checkToken(
+          "Cyrillic-Latin",
+          8,
+          true,
+          "яяяяяяяяяяяяяяяяяяяя",
+          "ââââââa\u0302ââââa\u0302ââââa\u0302âââ",
+          true);
+      fail("with failOnRollbackBufferOverflow=true, we expect to throw a RuntimeException");
+    } catch (RuntimeException ex) {
+      // this is expected.
+    }
+  }
+
+  /**
+   * Sanity check all top-level prepackaged Transliterators to make sure that no trivial errors are
+   * thrown on instantiation. We're not really checking anything in particular here, but under the
+   * hood this will at least make a cursory check for consistency between the "stock"
+   * Transliterator, and any potential "optimized" version with externalized unicode normalization.
+   */
+  @Nightly
+  public void testNormalizationOptimizationOnAvailableIDs() throws Exception {
+    Enumeration<String> ids = Transliterator.getAvailableIDs();
+    List<String> idsWithNestedNormalization = new ArrayList<>();
+    while (ids.hasMoreElements()) {
+      String id = ids.nextElement();
+      @SuppressWarnings("unused")
+      boolean hasNestedUnicodeNormalization =
+          accumulateNestedUnicodeNormalization(id, idsWithNestedNormalization);
+      try {
+        // set `iterationsDefault=0` because in this test we simply want to ignore the non-optimized
+        //  case.
+        // set `iterationsOptimized=1` because `testRandomStrings` can be slow; we want this as a
+        //  sanity check, but as a matter of course we should not need many iterations.
+        @SuppressWarnings("unused")
+        boolean optimized = testRandomStrings(id, 0, 1);
+
+        // We can't really _do_ much with `hasNestedUnicodeNormalization` or `optimized`, in terms
+        // of validation.
+        // They are completely independent (i.e. not mutually exclusive). We leave these checks
+        // stubbed out here because we're initially optimizing normalization that can be easily
+        // detected at the top level, but we want some plumbing in place to be more transparent
+        // about what we're optimizing and what we're not (and to some extent _why_).
+        // TODO: We're aware that this is probably leaving "on the table" a bunch of potential
+        //  optimization of the "_nested_ unicode normalization" case. It may be worth optimizing
+        //  these nested cases (and addressing any additional complexity specific to that case)
+        //  as a separate, follow-up issue.
+      } catch (Exception ex) {
+        // wrap the exception so that we can report the offending `id`
+        throw new RuntimeException("problem for id: " + id, ex);
+      }
+    }
+  }
+
+  /**
+   * It is possible that leading and trailing (or singleton) Transliterators might apply nested
+   * Unicode normalization, thus acting in the capacity of a Normalizer, without qualifying as a
+   * top-level Normalizer as currently defined in {@link
+   * ICUTransformCharFilterFactory#unicodeNormalizationType(String)}. For now, simply detect these
+   * cases (to facilitate more nuanced handling in the future, if necessary).
+   *
+   * @param topLevelId top level Transliterator parent id.
+   * @param ids list to which the topLevelId will be added if nested unicode normalization is
+   *     detected
+   */
+  private static boolean accumulateNestedUnicodeNormalization(String topLevelId, List<String> ids) {
+    Transliterator levelOne = Transliterator.getInstance(topLevelId);
+    Transliterator[] topLevelElements = levelOne.getElements();
+    if (topLevelElements.length == 1 && levelOne == topLevelElements[0]) {
+      // A leaf Transliterator; shortcircuit
+      return false;
+    }
+    ArrayDeque<Transliterator> elements =
+        new ArrayDeque<>(topLevelElements.length << 2); // oversize
+    elements.addAll(Arrays.asList(topLevelElements));
+    do {
+      final Transliterator t = elements.removeFirst();
+      if (ICUTransformCharFilterFactory.unicodeNormalizationType(t.getID()) != null) {
+        ids.add(topLevelId);
+        return true;
+      }
+      Transliterator[] subElements = t.getElements();
+      if (subElements.length > 1 || t != subElements[0]) {
+        for (Transliterator sub : subElements) {
+          elements.addFirst(sub);
+        }
+      }
+    } while (!elements.isEmpty());
+    return false;
+  }
+
+  public void testCustomFunctionality() throws Exception {
+    String rules = "a > b; b > c;"; // convert a's to b's and b's to c's
+    checkToken(
+        Transliterator.createFromRules("test", rules, Transliterator.FORWARD),
+        new StringReader("abacadaba"),
+        "bcbcbdbcb");
+  }
+
+  public void testCustomFunctionality2() throws Exception {
+    String rules = "c { a > b; a > d;"; // convert a's to b's and b's to c's
+    checkToken(
+        Transliterator.createFromRules("test", rules, Transliterator.FORWARD),
+        new StringReader("caa"),
+        "cbd");
+  }
+
+  public void testOptimizer() throws Exception {
+    String rules = "a > b; b > c;"; // convert a's to b's and b's to c's
+    Transliterator custom = Transliterator.createFromRules("test", rules, Transliterator.FORWARD);
+    assertTrue(custom.getFilter() == null);
+    new ICUTransformCharFilter(new StringReader(""), custom);
+    assertTrue(custom.getFilter().equals(new UnicodeSet("[ab]")));
+  }
+
+  public void testOptimizer2() throws Exception {
+    checkToken("Traditional-Simplified; CaseFold", "ABCDE", "abcde");
+  }
+
+  public void testOptimizerSurrogate() throws Exception {
+    String rules = "\\U00020087 > x;"; // convert CJK UNIFIED IDEOGRAPH-20087 to an x
+    Transliterator custom = Transliterator.createFromRules("test", rules, Transliterator.FORWARD);
+    assertTrue(custom.getFilter() == null);
+    new ICUTransformCharFilter(new StringReader(""), custom);
+    assertTrue(custom.getFilter().equals(new UnicodeSet("[\\U00020087]")));
+  }
+
+  private void checkToken(Transliterator transliterator, Reader input, String expected)
+      throws IOException {
+    input =
+        new ICUTransformCharFilter(
+            input,
+            transliterator,
+            ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+            false);
+    final KeywordTokenizer input1 = new KeywordTokenizer();
+    input1.setReader(input);
+    assertTokenStreamContents(input1, new String[] {expected});
+  }
+
+  private void checkToken(String id, String input, String expected) throws IOException {
+    checkToken(
+        id, ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY, false, input, expected);
+  }
+
+  private void checkToken(
+      String id, String input, String expected, boolean suppressUnicodeNormExternalization)
+      throws IOException {
+    checkToken(
+        id,
+        ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+        false,
+        input,
+        expected,
+        suppressUnicodeNormExternalization);
+  }
+
+  private void checkToken(
+      String id,
+      int maxRollbackBufferCapacity,
+      boolean failOnRollbackBufferOverflow,
+      String input,
+      String expected)
+      throws IOException {
+    checkToken(
+        getTransliteratingFilter(
+            id, new StringReader(input), maxRollbackBufferCapacity, failOnRollbackBufferOverflow),
+        expected);
+  }
+
+  private void checkToken(
+      String id,
+      int maxRollbackBufferCapacity,
+      boolean failOnRollbackBufferOverflow,
+      String input,
+      String expected,
+      boolean suppressUnicodeNormalizationExternalization)
+      throws IOException {
+    checkToken(
+        getTransliteratingFilter(
+            null,
+            id,
+            new StringReader(input),
+            suppressUnicodeNormalizationExternalization,
+            maxRollbackBufferCapacity,
+            failOnRollbackBufferOverflow),
+        expected);
+  }
+
+  private void checkToken(CharFilter input, String expected) throws IOException {
+    final KeywordTokenizer input1 = new KeywordTokenizer();
+    input1.setReader(input);
+    assertTokenStreamContents(input1, new String[] {expected});
+  }
+
+  public void testRandomStringsLatinToKatakana() throws Exception {
+    // this Transliterator often decreases character length wrt input
+    // we _don't_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Latin-Katakana", 1000, -1);
+  }
+
+  public void testRandomStringsAnyToLatin() throws Exception {
+    // this Transliterator often increases character length wrt input
+    // we _don't_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Any-Latin", 1000, -1);
+  }
+
+  public void testRandomStringsKatakanaToHiragana() throws Exception {
+    // this Transliterator often increases character length wrt input
+    // we _do_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Katakana-Hiragana", -1, 1000);
+  }
+
+  private static Analyzer getAnalyzer(String id, boolean suppressExternalize, boolean[] optimized) {
+    if (optimized != null && !suppressExternalize) {
+      getTransliteratingFilter(optimized, id, new StringReader("dummy"), suppressExternalize);
+    }
+    return new Analyzer() {
+      @Override
+      protected TokenStreamComponents createComponents(String fieldName) {
+        Tokenizer tokenizer = new MockTokenizer(MockTokenizer.WHITESPACE, false);
+        return new TokenStreamComponents(tokenizer, tokenizer);
+      }
+
+      @Override
+      protected Reader initReader(String fieldName, Reader reader) {
+        return super.initReader(
+            fieldName, getTransliteratingFilter(null, id, reader, suppressExternalize));
+      }
+
+      @Override
+      protected Reader initReaderForNormalization(String fieldName, Reader reader) {
+        return super.initReaderForNormalization(
+            fieldName, getTransliteratingFilter(null, id, reader, suppressExternalize));
+      }
+    };
+  }
+
+  /** blast some random strings through the analyzer */
+  private boolean testRandomStrings(final String id, int iterationsDefault, int iterationsOptimized)
+      throws Exception {
+    final boolean firstSuppressExternalize = random().nextBoolean();
+    boolean[] optimized = new boolean[] {false};
+    Analyzer a = getAnalyzer(id, firstSuppressExternalize, optimized);
+    Analyzer b = getAnalyzer(id, !firstSuppressExternalize, optimized);
+    final int iterations;
+    if (optimized[0]) {
+      iterations = iterationsOptimized;
+    } else {
+      iterations = iterationsDefault;
+      // there's no optimization; it doesn't matter which analyzer we use.
+      b.close();
+      b = null;
+    }
+    assertTrue(
+        "implicitly expected optimized=" + optimized[0] + "; requested iterations=" + iterations,
+        iterations >= 0);
+    checkRandomData(random(), a, b, iterations * RANDOM_MULTIPLIER);

Review comment:
       >This class is crazy enough and should not get more complicated!
   
   Yes, thanks for raising this. You're of course correct that these changes would be unnecessary if there were no comparison between the results of the analyzers. But there [are indeed such checks](https://github.com/apache/lucene/pull/15/files#diff-98ca723fc3b08754132e64f882950691fe885b4b8134d8ad14e0608108bbe638R1221-R1229)! even if it's easy to overlook (I made the checks into one-liners because I was trying to keep things concise (or at least avoid taking up any more screen real estate than necessary).
   
   I'm open to considering other approaches, but I guess it doesn't make sense to go too far down the road of considering that prospectively unless you still have objections considering the comparison checks between the two TokenStreams...




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] uschindler commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

uschindler commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r600761769



##########
File path: lucene/analysis/icu/src/test/org/apache/lucene/analysis/icu/TestICUTransformCharFilter.java
##########
@@ -0,0 +1,423 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.icu;
+
+import static org.apache.lucene.analysis.icu.ICUTransformCharFilterFactory.FAIL_ON_ROLLBACK_BUFFER_OVERFLOW_ARGNAME;
+import static org.apache.lucene.analysis.icu.ICUTransformCharFilterFactory.MAX_ROLLBACK_BUFFER_CAPACITY_ARGNAME;
+
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.UnicodeSet;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.StringReader;
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.lucene.analysis.Analyzer;
+import org.apache.lucene.analysis.BaseTokenStreamTestCase;
+import org.apache.lucene.analysis.CharFilter;
+import org.apache.lucene.analysis.MockTokenizer;
+import org.apache.lucene.analysis.Tokenizer;
+import org.apache.lucene.analysis.core.KeywordTokenizer;
+
+/** Test the ICUTransformCharFilter with some basic examples. */
+public class TestICUTransformCharFilter extends BaseTokenStreamTestCase {
+
+  public void testBasicFunctionality() throws Exception {
+    checkToken("Traditional-Simplified", "簡化字", "简化字");
+    checkToken("Katakana-Hiragana", "ヒラガナ", "ひらがな");
+    checkToken("Fullwidth-Halfwidth", "アルアノリウ", "ｱﾙｱﾉﾘｳ");
+    checkToken("Any-Latin", "Αλφαβητικός Κατάλογος", "Alphabētikós Katálogos");
+    checkToken(
+        "NFD; [:Nonspacing Mark:] Remove", "Alphabētikós Katálogos", "Alphabetikos Katalogos");
+    checkToken("Han-Latin", "中国", "zhōng guó");
+  }
+
+  public void testRollbackBuffer() throws Exception {
+    checkToken("Cyrillic-Latin", "яяяяя", "âââââ", false); // final NFC transform applied
+    checkToken(
+        "Cyrillic-Latin",
+        0,
+        false,
+        "яяяяя",
+        "a\u0302a\u0302a\u0302a\u0302a\u0302",
+        true); // final NFC transform never applied
+    checkToken(
+        "Cyrillic-Latin",
+        0,
+        false,
+        "яяяяя",
+        "âââââ",
+        false); // final NFC transform disabled internally, applied externally
+    checkToken("Cyrillic-Latin", 2, false, "яяяяя", "ââa\u0302a\u0302a\u0302", true);
+    checkToken(
+        "Cyrillic-Latin",
+        4,
+        false,
+        "яяяяяяяяяя",
+        "ââa\u0302a\u0302a\u0302a\u0302a\u0302a\u0302a\u0302a\u0302",
+        true);
+    checkToken(
+        "Cyrillic-Latin",
+        8,
+        false,
+        "яяяяяяяяяяяяяяяяяяяя",
+        "ââââââa\u0302ââââa\u0302ââââa\u0302âââ",
+        true);
+    try {
+      checkToken(
+          "Cyrillic-Latin",
+          8,
+          true,
+          "яяяяяяяяяяяяяяяяяяяя",
+          "ââââââa\u0302ââââa\u0302ââââa\u0302âââ",
+          true);
+      fail("with failOnRollbackBufferOverflow=true, we expect to throw a RuntimeException");
+    } catch (RuntimeException ex) {
+      // this is expected.
+    }
+  }
+
+  /**
+   * Sanity check all top-level prepackaged Transliterators to make sure that no trivial errors are
+   * thrown on instantiation. We're not really checking anything in particular here, but under the
+   * hood this will at least make a cursory check for consistency between the "stock"
+   * Transliterator, and any potential "optimized" version with externalized unicode normalization.
+   */
+  @Nightly
+  public void testNormalizationOptimizationOnAvailableIDs() throws Exception {
+    Enumeration<String> ids = Transliterator.getAvailableIDs();
+    List<String> idsWithNestedNormalization = new ArrayList<>();
+    while (ids.hasMoreElements()) {
+      String id = ids.nextElement();
+      @SuppressWarnings("unused")
+      boolean hasNestedUnicodeNormalization =
+          accumulateNestedUnicodeNormalization(id, idsWithNestedNormalization);
+      try {
+        // set `iterationsDefault=0` because in this test we simply want to ignore the non-optimized
+        //  case.
+        // set `iterationsOptimized=1` because `testRandomStrings` can be slow; we want this as a
+        //  sanity check, but as a matter of course we should not need many iterations.
+        @SuppressWarnings("unused")
+        boolean optimized = testRandomStrings(id, 0, 1);
+
+        // We can't really _do_ much with `hasNestedUnicodeNormalization` or `optimized`, in terms
+        // of validation.
+        // They are completely independent (i.e. not mutually exclusive). We leave these checks
+        // stubbed out here because we're initially optimizing normalization that can be easily
+        // detected at the top level, but we want some plumbing in place to be more transparent
+        // about what we're optimizing and what we're not (and to some extent _why_).
+        // TODO: We're aware that this is probably leaving "on the table" a bunch of potential
+        //  optimization of the "_nested_ unicode normalization" case. It may be worth optimizing
+        //  these nested cases (and addressing any additional complexity specific to that case)
+        //  as a separate, follow-up issue.
+      } catch (Exception ex) {
+        // wrap the exception so that we can report the offending `id`
+        throw new RuntimeException("problem for id: " + id, ex);
+      }
+    }
+  }
+
+  /**
+   * It is possible that leading and trailing (or singleton) Transliterators might apply nested
+   * Unicode normalization, thus acting in the capacity of a Normalizer, without qualifying as a
+   * top-level Normalizer as currently defined in {@link
+   * ICUTransformCharFilterFactory#unicodeNormalizationType(String)}. For now, simply detect these
+   * cases (to facilitate more nuanced handling in the future, if necessary).
+   *
+   * @param topLevelId top level Transliterator parent id.
+   * @param ids list to which the topLevelId will be added if nested unicode normalization is
+   *     detected
+   */
+  private static boolean accumulateNestedUnicodeNormalization(String topLevelId, List<String> ids) {
+    Transliterator levelOne = Transliterator.getInstance(topLevelId);
+    Transliterator[] topLevelElements = levelOne.getElements();
+    if (topLevelElements.length == 1 && levelOne == topLevelElements[0]) {
+      // A leaf Transliterator; shortcircuit
+      return false;
+    }
+    ArrayDeque<Transliterator> elements =
+        new ArrayDeque<>(topLevelElements.length << 2); // oversize
+    elements.addAll(Arrays.asList(topLevelElements));
+    do {
+      final Transliterator t = elements.removeFirst();
+      if (ICUTransformCharFilterFactory.unicodeNormalizationType(t.getID()) != null) {
+        ids.add(topLevelId);
+        return true;
+      }
+      Transliterator[] subElements = t.getElements();
+      if (subElements.length > 1 || t != subElements[0]) {
+        for (Transliterator sub : subElements) {
+          elements.addFirst(sub);
+        }
+      }
+    } while (!elements.isEmpty());
+    return false;
+  }
+
+  public void testCustomFunctionality() throws Exception {
+    String rules = "a > b; b > c;"; // convert a's to b's and b's to c's
+    checkToken(
+        Transliterator.createFromRules("test", rules, Transliterator.FORWARD),
+        new StringReader("abacadaba"),
+        "bcbcbdbcb");
+  }
+
+  public void testCustomFunctionality2() throws Exception {
+    String rules = "c { a > b; a > d;"; // convert a's to b's and b's to c's
+    checkToken(
+        Transliterator.createFromRules("test", rules, Transliterator.FORWARD),
+        new StringReader("caa"),
+        "cbd");
+  }
+
+  public void testOptimizer() throws Exception {
+    String rules = "a > b; b > c;"; // convert a's to b's and b's to c's
+    Transliterator custom = Transliterator.createFromRules("test", rules, Transliterator.FORWARD);
+    assertTrue(custom.getFilter() == null);
+    new ICUTransformCharFilter(new StringReader(""), custom);
+    assertTrue(custom.getFilter().equals(new UnicodeSet("[ab]")));
+  }
+
+  public void testOptimizer2() throws Exception {
+    checkToken("Traditional-Simplified; CaseFold", "ABCDE", "abcde");
+  }
+
+  public void testOptimizerSurrogate() throws Exception {
+    String rules = "\\U00020087 > x;"; // convert CJK UNIFIED IDEOGRAPH-20087 to an x
+    Transliterator custom = Transliterator.createFromRules("test", rules, Transliterator.FORWARD);
+    assertTrue(custom.getFilter() == null);
+    new ICUTransformCharFilter(new StringReader(""), custom);
+    assertTrue(custom.getFilter().equals(new UnicodeSet("[\\U00020087]")));
+  }
+
+  private void checkToken(Transliterator transliterator, Reader input, String expected)
+      throws IOException {
+    input =
+        new ICUTransformCharFilter(
+            input,
+            transliterator,
+            ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+            false);
+    final KeywordTokenizer input1 = new KeywordTokenizer();
+    input1.setReader(input);
+    assertTokenStreamContents(input1, new String[] {expected});
+  }
+
+  private void checkToken(String id, String input, String expected) throws IOException {
+    checkToken(
+        id, ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY, false, input, expected);
+  }
+
+  private void checkToken(
+      String id, String input, String expected, boolean suppressUnicodeNormExternalization)
+      throws IOException {
+    checkToken(
+        id,
+        ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+        false,
+        input,
+        expected,
+        suppressUnicodeNormExternalization);
+  }
+
+  private void checkToken(
+      String id,
+      int maxRollbackBufferCapacity,
+      boolean failOnRollbackBufferOverflow,
+      String input,
+      String expected)
+      throws IOException {
+    checkToken(
+        getTransliteratingFilter(
+            id, new StringReader(input), maxRollbackBufferCapacity, failOnRollbackBufferOverflow),
+        expected);
+  }
+
+  private void checkToken(
+      String id,
+      int maxRollbackBufferCapacity,
+      boolean failOnRollbackBufferOverflow,
+      String input,
+      String expected,
+      boolean suppressUnicodeNormalizationExternalization)
+      throws IOException {
+    checkToken(
+        getTransliteratingFilter(
+            null,
+            id,
+            new StringReader(input),
+            suppressUnicodeNormalizationExternalization,
+            maxRollbackBufferCapacity,
+            failOnRollbackBufferOverflow),
+        expected);
+  }
+
+  private void checkToken(CharFilter input, String expected) throws IOException {
+    final KeywordTokenizer input1 = new KeywordTokenizer();
+    input1.setReader(input);
+    assertTokenStreamContents(input1, new String[] {expected});
+  }
+
+  public void testRandomStringsLatinToKatakana() throws Exception {
+    // this Transliterator often decreases character length wrt input
+    // we _don't_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Latin-Katakana", 1000, -1);
+  }
+
+  public void testRandomStringsAnyToLatin() throws Exception {
+    // this Transliterator often increases character length wrt input
+    // we _don't_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Any-Latin", 1000, -1);
+  }
+
+  public void testRandomStringsKatakanaToHiragana() throws Exception {
+    // this Transliterator often increases character length wrt input
+    // we _do_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Katakana-Hiragana", -1, 1000);
+  }
+
+  private static Analyzer getAnalyzer(String id, boolean suppressExternalize, boolean[] optimized) {
+    if (optimized != null && !suppressExternalize) {
+      getTransliteratingFilter(optimized, id, new StringReader("dummy"), suppressExternalize);
+    }
+    return new Analyzer() {
+      @Override
+      protected TokenStreamComponents createComponents(String fieldName) {
+        Tokenizer tokenizer = new MockTokenizer(MockTokenizer.WHITESPACE, false);
+        return new TokenStreamComponents(tokenizer, tokenizer);
+      }
+
+      @Override
+      protected Reader initReader(String fieldName, Reader reader) {
+        return super.initReader(
+            fieldName, getTransliteratingFilter(null, id, reader, suppressExternalize));
+      }
+
+      @Override
+      protected Reader initReaderForNormalization(String fieldName, Reader reader) {
+        return super.initReaderForNormalization(
+            fieldName, getTransliteratingFilter(null, id, reader, suppressExternalize));
+      }
+    };
+  }
+
+  /** blast some random strings through the analyzer */
+  private boolean testRandomStrings(final String id, int iterationsDefault, int iterationsOptimized)
+      throws Exception {
+    final boolean firstSuppressExternalize = random().nextBoolean();
+    boolean[] optimized = new boolean[] {false};
+    Analyzer a = getAnalyzer(id, firstSuppressExternalize, optimized);
+    Analyzer b = getAnalyzer(id, !firstSuppressExternalize, optimized);
+    final int iterations;
+    if (optimized[0]) {
+      iterations = iterationsOptimized;
+    } else {
+      iterations = iterationsDefault;
+      // there's no optimization; it doesn't matter which analyzer we use.
+      b.close();
+      b = null;
+    }
+    assertTrue(
+        "implicitly expected optimized=" + optimized[0] + "; requested iterations=" + iterations,
+        iterations >= 0);
+    checkRandomData(random(), a, b, iterations * RANDOM_MULTIPLIER);

Review comment:
       Please replace this by 2 calls and revert everything in BaseTokenStreamTestCase. I don't want to make the whole thing more complicated. checkRandomData does not need to check both tokenstreams at one, as it sequentially checks, but does not compare them to each other.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-832369359


   Ah, sorry! yeah, now that you mention it I'm afraid I'm not surprised. I'm going to just remove the previous impl (as you suggested would make things clearer). I think that's the right way to go, and new impl tests should definitely be solid.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-832353332


   I got the precommit "working" by just disabling a bunch of build checks with corresponding `TODO` in the source code, reducing visibility of some stuff that didn't need to be public, etc.
   
   I haven't really looked at the code yet, best to start with the automated checks.
   
   Looks to me like removing the old transform impl/tests would really simplify the process too.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-806079907


   > I'm curious what you'll make of [0d8c001](https://github.com/apache/lucene/commit/0d8c001d88bda373fe321550a452c4dd53a3af74) ... the previous state was kind of weird because we were ostensibly "detecting" norm ids that never actually cropped up in practice, but then throwing an `UnsupportedOperationException` if we had ever come to the point of trying to replace them. This worked because of the fact that they never cropped up in practice. I'm pretty sure that the change introduced in [0d8c001](https://github.com/apache/lucene/commit/0d8c001d88bda373fe321550a452c4dd53a3af74) would work fine, but at the moment it's definitely not covered by tests.
   > 
   > Alternatives to [0d8c001](https://github.com/apache/lucene/commit/0d8c001d88bda373fe321550a452c4dd53a3af74) would be:
   > 
   >     1. stop detecting the strings FCC, FCD, and NFKC_CF (i.e. don't recognize them as candidates for replacement/optimization)
   
   either current code or option 1 is fine. Honestly, no one will ever use these.
   
   users use normalization in their rules because they want "one" rule to capture the transformation, e.g:
   ```
   # alef w/ hamza below
   \u0625 -> i;
   ```
   
   * they dont want to write duplicate rules to handle decomposed case (e.g. 0627 + 0655)
   * they dont want explosion of rules to handle diacritics ordering (NFC/NFD enforce an order by combining class)
   
   So in most cases, NFC or NFD is useful. Whether the person picks NFC or NFD depends more on what the standard/rulesystem is supposed to do, and how the writing system works, or in some cases maybe just arbitrary. In the case of Korean, if we use NFD and work on Jamo, it will be tiny amount of rules (work on characters, like an alphabet). But if we use NFC we would need like 11,000 rules, one for each syllable.
   
   For some writing systems, there may be legacy compatibility cases, just designed for round-tripping back to old charsets. In our arabic example here, these exist, and you might see them if you extract from PDF (e.g. FE87, FE88).
   So in these cases, NFKC or NFKD is a better choice.
   
   But nobody will need fast c&d (this is for collation i think?) or nfkc_cf here (usually if there is capitalization, you tend to preserve that in the rules).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r596142606



##########
File path: lucene/analysis/icu/src/java/org/apache/lucene/analysis/icu/ICUTransformCharFilter.java
##########
@@ -0,0 +1,686 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.lucene.analysis.icu;
+
+import com.ibm.icu.impl.Utility;
+import com.ibm.icu.text.ReplaceableString;
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.Transliterator.Position;
+import com.ibm.icu.text.UTF16;
+import java.io.IOException;
+import java.io.Reader;
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Locale;
+import java.util.WeakHashMap;
+import org.apache.lucene.analysis.CharFilter;
+import org.apache.lucene.analysis.charfilter.BaseCharFilter;
+import org.apache.lucene.util.ArrayUtil;
+
+/**
+ * A {@link CharFilter} that transforms text with ICU.
+ *
+ * <p>ICU provides text-transformation functionality via its Transliteration API. Although script
+ * conversion is its most common use, a Transliterator can actually perform a more general class of
+ * tasks. In fact, Transliterator defines a very general API which specifies only that a segment of
+ * the input text is replaced by new text. The particulars of this conversion are determined
+ * entirely by subclasses of Transliterator.
+ *
+ * <p>Some useful transformations for search are built-in:
+ *
+ * <ul>
+ *   <li>Conversion from Traditional to Simplified Chinese characters
+ *   <li>Conversion from Hiragana to Katakana
+ *   <li>Conversion from Fullwidth to Halfwidth forms.
+ *   <li>Script conversions, for example Serbian Cyrillic to Latin
+ * </ul>
+ *
+ * <p>Example usage:
+ *
+ * <blockquote>
+ *
+ * stream = new ICUTransformCharFilter(reader,
+ * Transliterator.getInstance("Traditional-Simplified"));
+ *
+ * </blockquote>
+ *
+ * <br>
+ * For more details, see the <a href="http://userguide.icu-project.org/transforms/general">ICU User
+ * Guide</a>.
+ */
+public final class ICUTransformCharFilter extends BaseCharFilter {
+
+  // Transliterator to transform the text
+  private final Transliterator transform;
+
+  // Reusable position object
+  private final Position position = new Position();
+
+  private static final int READ_BUFFER_SIZE = 1024;
+  private final char[] tmpBuffer = new char[READ_BUFFER_SIZE];
+
+  private static final int INITIAL_TRANSLITERATE_BUFFER_SIZE = 1024;
+  private final StringBuffer buffer = new StringBuffer(INITIAL_TRANSLITERATE_BUFFER_SIZE);
+  private final ReplaceableString replaceable = new ReplaceableString(buffer);
+
+  private static final int BUFFER_PRUNE_THRESHOLD = 1024;
+
+  private int outputCursor = 0;
+  private boolean inputFinished = false;
+  private int charCount = 0;
+  private int offsetDiffAdjust = 0;
+
+  private static final int HARD_MAX_ROLLBACK_BUFFER_CAPACITY =
+      Integer.highestOneBit(Integer.MAX_VALUE);
+  static final int DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY = 8192;
+  private final int maxRollbackBufferCapacity;
+
+  private static final int DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY = 4; // must be power of 2
+  private char[] rollbackBuffer;
+  private int rollbackBufferSize = 0;
+
+  static final boolean DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW = true;
+  private final boolean failOnRollbackBufferOverflow;
+
+  ICUTransformCharFilter(Reader in, Transliterator transform) {
+    this(
+        in,
+        transform,
+        DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+        DEFAULT_FAIL_ON_ROLLBACK_BUFFER_OVERFLOW);
+  }
+
+  /**
+   * Construct new {@link ICUTransformCharFilter} with the specified {@link Transliterator}, backed
+   * by the specified {@link Reader}.
+   *
+   * @param in input source
+   * @param transform used to perform transliteration
+   * @param maxRollbackBufferCapacityHint used to control the maximum size to which this {@link
+   *     ICUTransformCharFilter} will buffer and rollback partial transliteration of input
+   *     sequences. The provided hint will be converted to an enforced limit of "the greatest power
+   *     of 2 (excluding '1') less than or equal to the specified value". It is illegal to specify a
+   *     negative value. There is no power of 2 greater than <code>
+   *     Integer.highestOneBit(Integer.MAX_VALUE))</code>, so to prevent overflow, values in this
+   *     range will resolve to an enforced limit of <code>Integer.highestOneBit(Integer.MAX_VALUE))
+   *     </code>. Specifying "0" (or "1", in practice) disables rollback. Larger values can in some
+   *     cases yield more accurate transliteration, at the cost of performance and
+   *     resolution/accuracy of offset correction. This is intended primarily as a failsafe, with a
+   *     relatively large default value of {@value
+   *     ICUTransformCharFilter#DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY}. See comments "To understand
+   *     the need for rollback" in private method: {@link
+   *     Transliterator#filteredTransliterate(com.ibm.icu.text.Replaceable, Position, boolean,
+   *     boolean)}
+   * @param failOnRollbackBufferOverflow if true, transliteration failing to complete within the
+   *     specified rollbackBuffer window will fail with an exception. If false, transliteration
+   *     forges ahead with possibly-inconsistent results.
+   */
+  ICUTransformCharFilter(
+      Reader in,
+      Transliterator transform,
+      int maxRollbackBufferCapacityHint,
+      boolean failOnRollbackBufferOverflow) {
+    super(in);
+    this.transform = ICUTransformFilter.optimizeForCommonCase(transform);
+    this.failOnRollbackBufferOverflow = failOnRollbackBufferOverflow;
+    if (maxRollbackBufferCapacityHint < 0) {
+      throw new IllegalArgumentException(
+          "It is illegal to request negative rollback buffer max capacity");
+    } else if (maxRollbackBufferCapacityHint >= HARD_MAX_ROLLBACK_BUFFER_CAPACITY) {
+      // arg is positive, so user wants the largest possible buffer capacity limit
+      // we know what that is (static), protecting for overflow.
+      this.maxRollbackBufferCapacity = HARD_MAX_ROLLBACK_BUFFER_CAPACITY;
+      this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+    } else {
+      // greatest power of 2 (excluding "1") less than or equal to the specified hint
+      this.maxRollbackBufferCapacity =
+          Integer.highestOneBit(maxRollbackBufferCapacityHint - 1) << 1;
+      if (this.maxRollbackBufferCapacity == 0) {
+        this.rollbackBuffer = null;
+      } else {
+        this.rollbackBuffer = new char[DEFAULT_INITIAL_ROLLBACK_BUFFER_CAPACITY];
+      }
+    }
+  }
+
+  /**
+   * Reads characters into a portion of an array. This method will block until some input is
+   * available, an I/O error occurs, or the end of the stream is reached.
+   *
+   * @param cbuf Destination buffer
+   * @param off Offset at which to start storing characters
+   * @param len Maximum number of characters to read
+   * @return The number of characters read, or -1 if the end of the stream has been reached
+   * @throws IOException If an I/O error occurs
+   */
+  @Override
+  public int read(char[] cbuf, int off, int len) throws IOException {
+    while (!inputFinished || position.start > outputCursor) {
+      // (expecting more input) || (output remains that has not been flushed)
+      if (position.start > outputCursor) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      int resLen = transliterateBufferContents();
+      if (resLen > 0) {
+        return outputFromResultBuffer(cbuf, off, len);
+      }
+
+      if (!readInputToBuffer()) {
+        final int preStart = position.start;
+        final int preLimit;
+        final int bufferLength = buffer.length();
+        if (preStart < bufferLength) {
+          if (UTF16.isLeadSurrogate(buffer.charAt(bufferLength - 1))) {
+            // if last char is a lead surrogate, transform won't handle it properly
+            // preLimit = bufferLength - 1; // could simply pass the malformed input along
+            throw new RuntimeException(
+                "malformed input; final UTF16 character is a lead surrogate");
+          } else {
+            preLimit = bufferLength;
+          }
+          position.contextLimit = preLimit;
+          position.limit = preLimit;
+          transform.finishTransliteration(replaceable, position);
+        } else if (offsetDiffAdjust == 0) {
+          break;
+        } else {
+          preLimit = bufferLength;
+        }
+        cursorAdvanced(preStart, preLimit);
+      }
+    }
+
+    return -1;
+  }
+
+  private void rollback(int preStart, int preLimit) {
+    buffer.delete(preStart, position.limit); // delete uncommitted chars
+    buffer.insert(preStart, rollbackBuffer, 0, rollbackBufferSize);
+    position.start = preStart;
+    position.contextLimit = preLimit;
+    position.limit = preLimit;
+  }
+
+  /**
+   * Grow rollback buffer if necessary, within constraints of {@link #maxRollbackBufferCapacity}.
+   * This imposes an arbitrary failsafe to prevent the possibility that the rollback buffer could
+   * grow indefinitely.
+   *
+   * @return true if upon return, rollback buffer has sufficient capacity for new input chars,
+   *     otherwise false.
+   */
+  private boolean ensureRollbackBufferCapacity() {
+    // ensure space for at least 2 chars (surrogate pair, max possible space needed)
+    if (rollbackBuffer.length - rollbackBufferSize < 2) {
+      if (rollbackBuffer.length < maxRollbackBufferCapacity) {
+        rollbackBuffer = ArrayUtil.growExact(rollbackBuffer, rollbackBuffer.length << 1);
+      } else {
+        // hit threshold; not going to increase the buffer size
+        if (failOnRollbackBufferOverflow) {
+          throw new RuntimeException(
+              "input could not be transliterated without overflowing maxRollbackBufferCapacity ("
+                  + maxRollbackBufferCapacity
+                  + "); "
+                  + "try increasing maxRollbackBufferCapacity, or setting failOnRollbackBufferOverflow=false");
+        }
+        return false;
+      }
+    }
+    return true;
+  }
+
+  private int pushRollbackBuffer(int idx, int bufferLength) {
+    if (idx >= bufferLength) {
+      return 0;
+    } else {
+      // note: we have already ensured sufficient buffer capacity
+      final char candidate = buffer.charAt(idx);
+      if (!UTF16.isLeadSurrogate(candidate)) {
+        if (rollbackBuffer != null) {
+          rollbackBuffer[rollbackBufferSize++] = candidate;
+        }
+        return 1;
+      } else {
+        if (++idx < bufferLength) {
+          if (rollbackBuffer != null) {
+            rollbackBuffer[rollbackBufferSize++] = candidate;
+            rollbackBuffer[rollbackBufferSize++] = buffer.charAt(idx);
+          }
+          return 2;
+        } else {
+          // we don't yet have the high surrogate
+          if (inputFinished) {
+            // no more input available; proceed with the char we have; rollback not needed
+            return 1;
+          } else {
+            return 0; // wait for more input to be available
+          }
+        }
+      }
+    }
+  }
+
+  /**
+   * Transliterate as much of the contents of {@link #buffer} as possible.
+   *
+   * @return number of output characters transliterated (possibly 0)
+   */
+  private int transliterateBufferContents() {
+    int nextCharLength = pushRollbackBuffer(position.limit, buffer.length());
+    if (nextCharLength == 0) {
+      return 0;
+    }
+    final int preCharCount = charCount;
+    int preStart = position.start;
+    int preLimit;
+    do {
+      position.limit += nextCharLength;
+      preLimit = position.limit;
+      position.contextLimit = preLimit;
+      transform.filteredTransliterate(replaceable, position, true);
+      boolean rollbackSizeWithinBounds = true;
+      if (rollbackBuffer != null
+          && position.start < position.limit
+          && (rollbackSizeWithinBounds = ensureRollbackBufferCapacity())) {
+        // complete pass not transliterated, and not yet at rollback buffer threshold cap.
+        // N.b.: rollback buffer threshold cap is arbitrary, so we check here (somewhat
+        // counterintuitively, *before* rolling back, as opposed to before pushing to
+        // the buffer) so that we can fall through to the else clause, clear the rollback
+        // buffer, and proceed.
+        rollback(preStart, preLimit);
+      } else {
+        rollbackBufferSize = 0;
+        if (position.start > preStart) {
+          // cursor advanced
+          cursorAdvanced(preStart, preLimit);
+
+          // N.b.: advancing contextStart precludes support for quantifiers, but is crucial for
+          // streaming,
+          // so we'll do it anyway.
+          // See comments in Transliterator source code:
+          // TODO
+          // This doesn't work once we add quantifier support.  Need to rewrite
+          // this code to support quantifiers and 'use maximum backup <n>;'.
+          //
+          //         index.contextStart = Math.max(index.start - getMaximumContextLength(),
+          //                                       originalStart);
+          position.contextStart =
+              Math.max(position.start - transform.getMaximumContextLength(), preStart);
+          preStart = position.start;
+          if (!rollbackSizeWithinBounds) {
+            // prepopulate newly cleared rollback buffer with all top-level uncommitted characters
+            rollbackBufferSize = position.limit - preStart;
+            if (rollbackBuffer.length - rollbackBufferSize < 2) {
+              // even after flushing all committed text, there's not enough space in the rollback
+              // buffer.
+              // This is an edge case of an edge case, when the last char32 read into the rollback
+              // buffer
+              // is a surrogate pair (completely filling the rollback buffer), *and* the last
+              // transliteration pass advanced position.start by exactly one char16 (not a surrogate
+              // pair).
+              preStart = forceAdvance(preStart, preLimit);
+              rollbackBufferSize = position.limit - preStart;
+            }
+            buffer.getChars(preStart, position.limit, rollbackBuffer, 0);
+          }
+        } else if (preLimit != position.limit) {
+          // cursor hasn't advanced; incoming characters have probably been deleted
+          offsetDiffAdjust += preLimit - position.limit;
+          // edge case of !rollbackSizeWithinBounds needs no special handling here, since input
+          // characters *are* being
+          // processed (deleted) -- we *are* progressing through the input stream, although the
+          // output stream hasn't changed.
+        } else if (!rollbackSizeWithinBounds) {
+          // cursor hasn't advanced, no incoming characters have been deleted, and the rollback
+          // buffer is full.
+          preStart = forceAdvance(preStart, preLimit);
+        }
+      }
+    } while ((nextCharLength = pushRollbackBuffer(position.limit, buffer.length())) > 0);
+
+    return charCount - preCharCount;
+  }
+
+  private static final int FORCE_THRESHOLD = 2; // conservative; the length of a surrogate pair
+
+  private int forceAdvance(int preStart, int preLimit) {
+    int shift;
+    if (maxRollbackBufferCapacity == 2) {
+      rollbackBufferSize = 0;
+      shift = 2;
+    } else {
+      shift = 0;
+      do {
+        shift += UTF16.isLeadSurrogate(rollbackBuffer[shift]) ? 2 : 1;
+      } while (shift < FORCE_THRESHOLD && shift < maxRollbackBufferCapacity);
+      rollbackBufferSize -= shift;
+      System.arraycopy(rollbackBuffer, shift, rollbackBuffer, 0, rollbackBufferSize);
+    }
+    position.start += shift; // mock transliterator advance
+    cursorAdvanced(preStart, preLimit);
+    position.contextStart =
+        Math.max(position.start - transform.getMaximumContextLength(), preStart);
+    return position.start;
+  }
+
+  private void cursorAdvanced(int preStart, int preLimit) {
+    final int outputLength = position.start - preStart;
+    final int diff = preLimit - position.limit + offsetDiffAdjust;
+    offsetDiffAdjust = 0;
+    if (diff == 0) {
+      // increment charCount; no offset correction necessary
+      charCount += outputLength;
+    } else {
+      // limit change indicates change in length of replacement text; correct offsets accordingly
+      // we correct offsets as frequently as possible, for every incremental change. This will
+      // sometimes be for a range of characters, but that's the best we can do as far as the level
+      // of granularity that's available to us via the Transliterator API.
+      recordOffsetDiff(diff, outputLength);
+    }
+  }
+
+  private void recordOffsetDiff(int diff, int outputLength) {
+    final int cumuDiff = getLastCumulativeDiff();
+    if (diff < 0) {
+      // positive diff indicates an increase in character count wrt input
+      for (int i = 1; i <= -diff; ++i) {
+        addOffCorrectMap(charCount + i, cumuDiff - i);
+      }
+    } else {
+      // positive diff indicates an decrease in character count wrt input
+      addOffCorrectMap(charCount + outputLength, cumuDiff + diff);
+    }
+    charCount += outputLength;
+  }
+
+  private boolean readInputToBuffer() throws IOException {
+    int res = input.read(tmpBuffer, 0, tmpBuffer.length);
+    if (res == -1) {
+      inputFinished = true;
+      return false;
+    } else {
+      buffer.append(tmpBuffer, 0, res);
+      return true;
+    }
+  }
+
+  private int outputFromResultBuffer(char[] cbuf, int begin, int len) {
+    len = Math.min(position.start - outputCursor, len);
+    buffer.getChars(outputCursor, outputCursor + len, cbuf, begin);
+    outputCursor += len;
+    if (outputCursor > BUFFER_PRUNE_THRESHOLD && position.contextStart > BUFFER_PRUNE_THRESHOLD) {
+      final int pruneSize = Math.min(outputCursor, position.contextStart);
+      buffer.delete(0, pruneSize);
+      position.contextStart -= pruneSize;
+      position.start -= pruneSize;
+      position.limit -= pruneSize;
+      position.contextLimit -= pruneSize;
+      outputCursor -= pruneSize;
+    }
+    return len;
+  }
+
+  /**
+   * Strips off Unicode character normalization form rules from a Transliterator. Do this if, as is
+   * typical, your analysis chain already includes another component that does normalization, like
+   * {@link ICUNormalizer2CharFilter}.
+   *
+   * @return a new Transliterator with no normalization, or the original Transliterator if it
+   *     already did no normalization.
+   */
+  public static Transliterator withoutUnicodeNormalization(Transliterator transliterator) {
+    final String modifiedRules = modifyRules(false, transliterator);
+    if (modifiedRules == null) {
+      return transliterator;
+    }
+    String baseId = transliterator.getID();
+    String modId = baseId.concat(baseId.lastIndexOf('/') < 0 ? "/X_NO_NORM_IO" : "_X_NO_NORM_IO");
+    return Transliterator.createFromRules(modId, modifiedRules, Transliterator.FORWARD);
+  }

Review comment:
       Thanks! this all makes sense to me; I hope to push commits accommodating these comments soon.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-799579642


   Thanks for looking at this, @rmuir! And no worries wrt "questions already answered" -- it's been long enough that this all feels fresh to me again, for better or worse :-)
   
   The Traditional->Simplified use case is a good example of where this type of functionality (however accomplished) is really necessary, because of the significance of dictionary-based tokenization for these scripts. It sounds promising to map the transliteration file onto a file for MappingCharFilter. Based on what you say, it looks like that would be viable for Traditional->Simplified. I think that approach would also address some of the weird issues this PR had to work around wrt offset resolution in composite transliterators. I wonder whether the same approach could apply to other scripts that commonly use dictionary-based tokenization?
   
   Perhaps this is what you were suggesting, but if the "MappingCharFilter" approach were used as an optimization, while still preserving generic ICUTransformCharFilter directly backed by ICU Transliterator, it'd be possible to test for consistency between the two approaches, while preserving "vanilla" ICUTransformCharFilter for unanticipated use cases, etc. ...
   
   True, `assumeExternalUnicodeNormalization` gets you into "shoot yourself in the foot" territory for sure, but results in roughly 4x performance improvement (based on some naive but I think representative benchmarks). [This comment](https://github.com/apache/lucene-solr/pull/892#issuecomment-537538822) describes the approach, and the [comment immediately following](https://github.com/apache/lucene-solr/pull/892#issuecomment-537601926) confirms results. Regarding which transformations this applies to (from the first linked comment):
   >NFC, as a trailing transformation step, is both very common and very active -- active in the sense that it will in many common contexts block output waiting for combining diacritics for literally almost every character
   I'm inclined to think the significant performance gain for such a common case is worth it -- as a user I'd certainly not want that type of functionality hidden from me. I wonder if there's a way to "have cake and eat it too" ...
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-808398015


   FYI, this is requiring one last burst of persistence! Everything's coming together, but not quite ready yet ... just checking in to let you know that my silence doesn't indicate I've dropped the ball on this.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-806297526


   Thank you, ICU side looks good! Let's just take care of BaseTokenStreamTestCase and then I think we are ready to merge. I'm sorry this PR has taken so embarrassingly long. Thank you @magibney for your persistence!


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-800749558


   Right; the normalization optimizations are not conceived of as a general-purpose type thing. I think [this comment](https://github.com/apache/lucene-solr/pull/892#issuecomment-563293051) might be applicable? particularly:
   
   > The assumeExternalUnicodeNormalization arg cannot simply be flipped in isolation without affecting behavior. My conception of this arg is as an "expert" option that can significantly improve performance, but requires users to know (and externally compensate for, with specific ICUNormalizer2CharFilters in the top-level analysis chain) the specific type of input and output unicode normalization applied by a particular transliterator.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-842630574


   Quick status update: comments are formatted properly, precommit check passes (apologies again, and thanks to all who weighed in on that).
   
   >Looks to me like removing the old transform impl/tests would really simplify the process too.
   
   I removed the old transform impl. Removal does indeed simplify the process, at the expense of having a useful way to cross-check offset correction against a completely different offset impl. I think this is fine, but we probably can/should compensate by filling in `expectedOffsets` where possible for any tests with non-randomized input. Some of the explicit input is manually constructed and already tests offsets specifically; but many of the (`testBespoke*`) tests are edge cases derived from randomly generated data, and as such "expected" offsets are difficult to determine _a priori_. For these cases I would propose initially bootstrapping expected offsets based on _actual_ offsets, and clearly marking "expected" values so-generated as basically documenting existing behavior so that we have more "canary" coverage to detect unintended changes. This isn't ideal, but I think it's a good tradeoff, given how difficult it would be to determine canonical "correct" offset correction for r
 andomly derived (though static) input.
   
   It's worth reiterating to be clear about the status of this PR: "AnyTransliterator" is not yet supported. Once the already-implemented core functionality for "non-dynamic" (non-Any) transliterators is squared away, it should be straightforward to use that as a base to incorporate support for "AnyTransliterator", but I don't want to do that until further feedback/review of the core functionality as currently implemented.
   
   In summary: this should be ready for initial review, unless there's a preference to first incorporate synthetic "expected" offset values (as proposed above). Please let me know what's preferred wrt synthetic "expected" offset values -- I should be able to turn that around quickly if it's desired.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-806168671


   Ah, I see. Thanks @rmuir for the extra context wrt FCC/FCD/NFKC_CF. I recall when I checked to see what was actually there it was exactly as you said: mostly NFC/NFD, a handful of NFKC/NFKD (and as I mentioned, none of the others).
   
   I went ahead and removed FCC/FCD/NFKC_CF in c8f7109 and left a comment explaining why they're not there. Attempting to detect them does come at some cost (however negligible), and leaving them in could also be confusing to people (myself included) who aren't familiar with the context you explain above.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-799603748


   > I'm inclined to think the significant performance gain for such a common case is worth it -- as a user I'd certainly not want that type of functionality hidden from me. I wonder if there's a way to "have cake and eat it too" ...
   
   Another option is to improve them in CLDR itself. I have reported bugs in the area before: https://unicode-org.atlassian.net/browse/CLDR-2348
   
   I doubt performance was considered much when authors wrote thes rules. I'll pick on one of these I contributed: https://github.com/unicode-org/cldr/blob/master/common/transforms/Maldivian-Latin-BGN.xml 
   
   It converts to NFD but this may be a no-op as I'm not sure anything accepted by the filter really decomposes, and no accent-reordering here. I can't for the life of me remember why i put that NFD there :) 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-832354853


   ugh, and i guess that `spotlessApply` really made some of the code ugly, especially comments. maybe we can manually wrap them in a way that the spotless checker still accepts. sorry, was just trying to get thru the guantlet of all the build checks...


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] magibney commented on a change in pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

magibney commented on a change in pull request #15:
URL: https://github.com/apache/lucene/pull/15#discussion_r600869251



##########
File path: lucene/analysis/icu/src/test/org/apache/lucene/analysis/icu/TestICUTransformCharFilter.java
##########
@@ -0,0 +1,423 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.icu;
+
+import static org.apache.lucene.analysis.icu.ICUTransformCharFilterFactory.FAIL_ON_ROLLBACK_BUFFER_OVERFLOW_ARGNAME;
+import static org.apache.lucene.analysis.icu.ICUTransformCharFilterFactory.MAX_ROLLBACK_BUFFER_CAPACITY_ARGNAME;
+
+import com.ibm.icu.text.Transliterator;
+import com.ibm.icu.text.UnicodeSet;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.StringReader;
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.lucene.analysis.Analyzer;
+import org.apache.lucene.analysis.BaseTokenStreamTestCase;
+import org.apache.lucene.analysis.CharFilter;
+import org.apache.lucene.analysis.MockTokenizer;
+import org.apache.lucene.analysis.Tokenizer;
+import org.apache.lucene.analysis.core.KeywordTokenizer;
+
+/** Test the ICUTransformCharFilter with some basic examples. */
+public class TestICUTransformCharFilter extends BaseTokenStreamTestCase {
+
+  public void testBasicFunctionality() throws Exception {
+    checkToken("Traditional-Simplified", "簡化字", "简化字");
+    checkToken("Katakana-Hiragana", "ヒラガナ", "ひらがな");
+    checkToken("Fullwidth-Halfwidth", "アルアノリウ", "ｱﾙｱﾉﾘｳ");
+    checkToken("Any-Latin", "Αλφαβητικός Κατάλογος", "Alphabētikós Katálogos");
+    checkToken(
+        "NFD; [:Nonspacing Mark:] Remove", "Alphabētikós Katálogos", "Alphabetikos Katalogos");
+    checkToken("Han-Latin", "中国", "zhōng guó");
+  }
+
+  public void testRollbackBuffer() throws Exception {
+    checkToken("Cyrillic-Latin", "яяяяя", "âââââ", false); // final NFC transform applied
+    checkToken(
+        "Cyrillic-Latin",
+        0,
+        false,
+        "яяяяя",
+        "a\u0302a\u0302a\u0302a\u0302a\u0302",
+        true); // final NFC transform never applied
+    checkToken(
+        "Cyrillic-Latin",
+        0,
+        false,
+        "яяяяя",
+        "âââââ",
+        false); // final NFC transform disabled internally, applied externally
+    checkToken("Cyrillic-Latin", 2, false, "яяяяя", "ââa\u0302a\u0302a\u0302", true);
+    checkToken(
+        "Cyrillic-Latin",
+        4,
+        false,
+        "яяяяяяяяяя",
+        "ââa\u0302a\u0302a\u0302a\u0302a\u0302a\u0302a\u0302a\u0302",
+        true);
+    checkToken(
+        "Cyrillic-Latin",
+        8,
+        false,
+        "яяяяяяяяяяяяяяяяяяяя",
+        "ââââââa\u0302ââââa\u0302ââââa\u0302âââ",
+        true);
+    try {
+      checkToken(
+          "Cyrillic-Latin",
+          8,
+          true,
+          "яяяяяяяяяяяяяяяяяяяя",
+          "ââââââa\u0302ââââa\u0302ââââa\u0302âââ",
+          true);
+      fail("with failOnRollbackBufferOverflow=true, we expect to throw a RuntimeException");
+    } catch (RuntimeException ex) {
+      // this is expected.
+    }
+  }
+
+  /**
+   * Sanity check all top-level prepackaged Transliterators to make sure that no trivial errors are
+   * thrown on instantiation. We're not really checking anything in particular here, but under the
+   * hood this will at least make a cursory check for consistency between the "stock"
+   * Transliterator, and any potential "optimized" version with externalized unicode normalization.
+   */
+  @Nightly
+  public void testNormalizationOptimizationOnAvailableIDs() throws Exception {
+    Enumeration<String> ids = Transliterator.getAvailableIDs();
+    List<String> idsWithNestedNormalization = new ArrayList<>();
+    while (ids.hasMoreElements()) {
+      String id = ids.nextElement();
+      @SuppressWarnings("unused")
+      boolean hasNestedUnicodeNormalization =
+          accumulateNestedUnicodeNormalization(id, idsWithNestedNormalization);
+      try {
+        // set `iterationsDefault=0` because in this test we simply want to ignore the non-optimized
+        //  case.
+        // set `iterationsOptimized=1` because `testRandomStrings` can be slow; we want this as a
+        //  sanity check, but as a matter of course we should not need many iterations.
+        @SuppressWarnings("unused")
+        boolean optimized = testRandomStrings(id, 0, 1);
+
+        // We can't really _do_ much with `hasNestedUnicodeNormalization` or `optimized`, in terms
+        // of validation.
+        // They are completely independent (i.e. not mutually exclusive). We leave these checks
+        // stubbed out here because we're initially optimizing normalization that can be easily
+        // detected at the top level, but we want some plumbing in place to be more transparent
+        // about what we're optimizing and what we're not (and to some extent _why_).
+        // TODO: We're aware that this is probably leaving "on the table" a bunch of potential
+        //  optimization of the "_nested_ unicode normalization" case. It may be worth optimizing
+        //  these nested cases (and addressing any additional complexity specific to that case)
+        //  as a separate, follow-up issue.
+      } catch (Exception ex) {
+        // wrap the exception so that we can report the offending `id`
+        throw new RuntimeException("problem for id: " + id, ex);
+      }
+    }
+  }
+
+  /**
+   * It is possible that leading and trailing (or singleton) Transliterators might apply nested
+   * Unicode normalization, thus acting in the capacity of a Normalizer, without qualifying as a
+   * top-level Normalizer as currently defined in {@link
+   * ICUTransformCharFilterFactory#unicodeNormalizationType(String)}. For now, simply detect these
+   * cases (to facilitate more nuanced handling in the future, if necessary).
+   *
+   * @param topLevelId top level Transliterator parent id.
+   * @param ids list to which the topLevelId will be added if nested unicode normalization is
+   *     detected
+   */
+  private static boolean accumulateNestedUnicodeNormalization(String topLevelId, List<String> ids) {
+    Transliterator levelOne = Transliterator.getInstance(topLevelId);
+    Transliterator[] topLevelElements = levelOne.getElements();
+    if (topLevelElements.length == 1 && levelOne == topLevelElements[0]) {
+      // A leaf Transliterator; shortcircuit
+      return false;
+    }
+    ArrayDeque<Transliterator> elements =
+        new ArrayDeque<>(topLevelElements.length << 2); // oversize
+    elements.addAll(Arrays.asList(topLevelElements));
+    do {
+      final Transliterator t = elements.removeFirst();
+      if (ICUTransformCharFilterFactory.unicodeNormalizationType(t.getID()) != null) {
+        ids.add(topLevelId);
+        return true;
+      }
+      Transliterator[] subElements = t.getElements();
+      if (subElements.length > 1 || t != subElements[0]) {
+        for (Transliterator sub : subElements) {
+          elements.addFirst(sub);
+        }
+      }
+    } while (!elements.isEmpty());
+    return false;
+  }
+
+  public void testCustomFunctionality() throws Exception {
+    String rules = "a > b; b > c;"; // convert a's to b's and b's to c's
+    checkToken(
+        Transliterator.createFromRules("test", rules, Transliterator.FORWARD),
+        new StringReader("abacadaba"),
+        "bcbcbdbcb");
+  }
+
+  public void testCustomFunctionality2() throws Exception {
+    String rules = "c { a > b; a > d;"; // convert a's to b's and b's to c's
+    checkToken(
+        Transliterator.createFromRules("test", rules, Transliterator.FORWARD),
+        new StringReader("caa"),
+        "cbd");
+  }
+
+  public void testOptimizer() throws Exception {
+    String rules = "a > b; b > c;"; // convert a's to b's and b's to c's
+    Transliterator custom = Transliterator.createFromRules("test", rules, Transliterator.FORWARD);
+    assertTrue(custom.getFilter() == null);
+    new ICUTransformCharFilter(new StringReader(""), custom);
+    assertTrue(custom.getFilter().equals(new UnicodeSet("[ab]")));
+  }
+
+  public void testOptimizer2() throws Exception {
+    checkToken("Traditional-Simplified; CaseFold", "ABCDE", "abcde");
+  }
+
+  public void testOptimizerSurrogate() throws Exception {
+    String rules = "\\U00020087 > x;"; // convert CJK UNIFIED IDEOGRAPH-20087 to an x
+    Transliterator custom = Transliterator.createFromRules("test", rules, Transliterator.FORWARD);
+    assertTrue(custom.getFilter() == null);
+    new ICUTransformCharFilter(new StringReader(""), custom);
+    assertTrue(custom.getFilter().equals(new UnicodeSet("[\\U00020087]")));
+  }
+
+  private void checkToken(Transliterator transliterator, Reader input, String expected)
+      throws IOException {
+    input =
+        new ICUTransformCharFilter(
+            input,
+            transliterator,
+            ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+            false);
+    final KeywordTokenizer input1 = new KeywordTokenizer();
+    input1.setReader(input);
+    assertTokenStreamContents(input1, new String[] {expected});
+  }
+
+  private void checkToken(String id, String input, String expected) throws IOException {
+    checkToken(
+        id, ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY, false, input, expected);
+  }
+
+  private void checkToken(
+      String id, String input, String expected, boolean suppressUnicodeNormExternalization)
+      throws IOException {
+    checkToken(
+        id,
+        ICUTransformCharFilter.DEFAULT_MAX_ROLLBACK_BUFFER_CAPACITY,
+        false,
+        input,
+        expected,
+        suppressUnicodeNormExternalization);
+  }
+
+  private void checkToken(
+      String id,
+      int maxRollbackBufferCapacity,
+      boolean failOnRollbackBufferOverflow,
+      String input,
+      String expected)
+      throws IOException {
+    checkToken(
+        getTransliteratingFilter(
+            id, new StringReader(input), maxRollbackBufferCapacity, failOnRollbackBufferOverflow),
+        expected);
+  }
+
+  private void checkToken(
+      String id,
+      int maxRollbackBufferCapacity,
+      boolean failOnRollbackBufferOverflow,
+      String input,
+      String expected,
+      boolean suppressUnicodeNormalizationExternalization)
+      throws IOException {
+    checkToken(
+        getTransliteratingFilter(
+            null,
+            id,
+            new StringReader(input),
+            suppressUnicodeNormalizationExternalization,
+            maxRollbackBufferCapacity,
+            failOnRollbackBufferOverflow),
+        expected);
+  }
+
+  private void checkToken(CharFilter input, String expected) throws IOException {
+    final KeywordTokenizer input1 = new KeywordTokenizer();
+    input1.setReader(input);
+    assertTokenStreamContents(input1, new String[] {expected});
+  }
+
+  public void testRandomStringsLatinToKatakana() throws Exception {
+    // this Transliterator often decreases character length wrt input
+    // we _don't_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Latin-Katakana", 1000, -1);
+  }
+
+  public void testRandomStringsAnyToLatin() throws Exception {
+    // this Transliterator often increases character length wrt input
+    // we _don't_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Any-Latin", 1000, -1);
+  }
+
+  public void testRandomStringsKatakanaToHiragana() throws Exception {
+    // this Transliterator often increases character length wrt input
+    // we _do_ expect unicode norm externalization optimization in practice
+    testRandomStrings("Katakana-Hiragana", -1, 1000);
+  }
+
+  private static Analyzer getAnalyzer(String id, boolean suppressExternalize, boolean[] optimized) {
+    if (optimized != null && !suppressExternalize) {
+      getTransliteratingFilter(optimized, id, new StringReader("dummy"), suppressExternalize);
+    }
+    return new Analyzer() {
+      @Override
+      protected TokenStreamComponents createComponents(String fieldName) {
+        Tokenizer tokenizer = new MockTokenizer(MockTokenizer.WHITESPACE, false);
+        return new TokenStreamComponents(tokenizer, tokenizer);
+      }
+
+      @Override
+      protected Reader initReader(String fieldName, Reader reader) {
+        return super.initReader(
+            fieldName, getTransliteratingFilter(null, id, reader, suppressExternalize));
+      }
+
+      @Override
+      protected Reader initReaderForNormalization(String fieldName, Reader reader) {
+        return super.initReaderForNormalization(
+            fieldName, getTransliteratingFilter(null, id, reader, suppressExternalize));
+      }
+    };
+  }
+
+  /** blast some random strings through the analyzer */
+  private boolean testRandomStrings(final String id, int iterationsDefault, int iterationsOptimized)
+      throws Exception {
+    final boolean firstSuppressExternalize = random().nextBoolean();
+    boolean[] optimized = new boolean[] {false};
+    Analyzer a = getAnalyzer(id, firstSuppressExternalize, optimized);
+    Analyzer b = getAnalyzer(id, !firstSuppressExternalize, optimized);
+    final int iterations;
+    if (optimized[0]) {
+      iterations = iterationsOptimized;
+    } else {
+      iterations = iterationsDefault;
+      // there's no optimization; it doesn't matter which analyzer we use.
+      b.close();
+      b = null;
+    }
+    assertTrue(
+        "implicitly expected optimized=" + optimized[0] + "; requested iterations=" + iterations,
+        iterations >= 0);
+    checkRandomData(random(), a, b, iterations * RANDOM_MULTIPLIER);

Review comment:
       Sorry; I do see there's definitely stuff I overlooked here! Thanks for pointing this out, I hope to update shortly.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org

[GitHub] [lucene] rmuir commented on pull request #15: LUCENE-8972: Add ICUTransformCharFilter, to support pre-tokenizer ICU text transformation

[GitBox <gi...@apache.org>]

rmuir commented on pull request #15:
URL: https://github.com/apache/lucene/pull/15#issuecomment-832367527


   the tests are failing for me locally too. Mostly it seemed to be previous implementations test? It does `assertEquals(AnalysisResult a, AnalysisResult b)` but AnalysisResult has no equals()...


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org