xiangfu0 commented on code in PR #19894:
URL: https://github.com/apache/pinot/pull/19894#discussion_r4288764532
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/fst/RegexpMatcher.java:
##########
@@ -43,9 +44,11 @@ public class RegexpMatcher {
private final FST<Long> _fst;
private final Automaton _automaton;
+ @SuppressWarnings("deprecation") // Preserve the complement syntax supported
by existing FST queries.
public RegexpMatcher(String regexQuery, FST<Long> fst) {
_fst = fst;
- _automaton = (new RegExp(regexQuery)).toAutomaton();
+ _automaton = Operations.determinize(new RegExp(regexQuery, RegExp.ALL |
RegExp.DEPRECATED_COMPLEMENT).toAutomaton(),
Review Comment:
Nice catch adding the explicit `Operations.determinize`. In Lucene 10
`RegExp.toAutomaton()` can return a non-deterministic automaton, and
`regexMatchOnFST` below relies on DFA semantics (per-state transition walk), so
without this the FST traversal could return wrong matches — not just `match()`.
The new `testOverlappingRegexAndComplement` with `(a|aa)*` covers exactly this
case, which is great.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/vector/lucene99/HnswCodec.java:
##########
@@ -69,10 +69,11 @@ public KnnVectorsFormat getKnnVectorsFormatForField(String
field) {
/// Instantiates a new codec, specifying the stored fields compression mode
to use.
///
/// @param mode stored fields compression mode to use for newly
flushed/merged segments.
- public HnswCodec(Lucene912Codec.Mode mode, KnnVectorsFormat
defaultKnnVectorsFormat) {
- super("Lucene99", new Lucene912Codec(mode));
+ public HnswCodec(Lucene100Codec.Mode mode, KnnVectorsFormat
defaultKnnVectorsFormat) {
+ // Legacy indexes retain the Lucene99 name and are read by
lucene-backward-codecs.
+ super("Lucene100", new Lucene100Codec(mode));
Review Comment:
Worth confirming: new HNSW indexes now persist the codec name "Lucene100",
which resolves to the built-in `Lucene100Codec` on read, while legacy indexes
keep "Lucene99" (resolved via lucene-backward-codecs). The
`testReadLucene912HnswIndex` test covers the legacy read path; the existing
round-trip test covers the new write+read path, so both directions look
covered. Since the class and package are still named `lucene99`, a short
comment here noting the new indexes are Lucene100-format would help future
readers.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/text/LuceneTextIndexReader.java:
##########
@@ -80,12 +84,66 @@ public class LuceneTextIndexReader implements
TextIndexReader {
private Constructor<QueryParserBase> _queryParserClassConstructor;
private boolean _enablePrefixSuffixMatchingInPhraseQueries = false;
+ /// Retains support for immutable Lucene 8 text indexes through Lucene's
backward codecs.
+ /// The default reader only accepts indexes created with the current or
previous major version.
+ static DirectoryReader openIndexReader(Directory directory)
Review Comment:
The fallback path (catch `IndexFormatTooOldException` then reopen with
`minSupportedMajorVersion=8`) only triggers for Lucene 8 indexes, since Lucene
9.12 is the previous major and opens directly. I verified
lucene-backward-codecs 10.0.0 ships the Lucene 8.x codecs, so this works. One
thought: the `anonymous IndexCommit` is fairly verbose — if Lucene exposes a
simpler way to pass the min-version on the first open, it might reduce this
boilerplate, but it's fine as-is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
