-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial upgrade to lucene v9.7.0 #585
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a few questions and suggestions.
I am not too sure about those count
methods. I think it would be great, if someone else can take a look at them as well.
payloadField, | ||
contextField); | ||
} | ||
} catch (IOException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be more debug friendly if you catch the exception for each if/else
block separately? I'm not too sure if it's obvious from the exception message which block is throwing it.
@@ -142,20 +140,6 @@ public Directory open(Path path, IndexPreloadConfig preloadConfig) throws IOExce | |||
return new NIOFSDirectory(path); | |||
} | |||
}; | |||
} else if (dirImpl.equals("SimpleFSDirectory")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think javadoc for this method needs to be updated.
return new SimpleFSDirectory(path); | ||
} | ||
}; | ||
} else if (dirImpl.equals("RAMDirectory")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems ByteBuffersDirectory is the replacement for RAMDirectory
. Is it straightforward to add it here, or it's outside the scope of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is beyond the scope of this branch. I don't think the ram directory even worked, and there was no test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's okay to remove RAMDirectory. If someone wants a temporary in-memory index they can just skip calling commit.
// The value is stored in a BINARY field, but it is always a String | ||
BinaryDocValues binaryDocValues = DocValues.getBinary(context.reader(), getName()); | ||
if (docValuesType == DocValuesType.SORTED) { | ||
SortedDocValues binaryDocValues = DocValues.getSorted(context.reader(), getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: r/binaryDocValues/sortedDocValues
|
||
/** Implements per-index {@link Codec}. */ | ||
public class ServerCodec extends Lucene84Codec { | ||
public class ServerCodec extends Lucene95Codec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to maintain compatibility with Lucene 8 indices? https://github.com/apache/lucene/blob/main/lucene/backward-codecs/src/java/org/apache/lucene/backward_codecs/lucene84/Lucene84Codec.java
Can also add later if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that will require some testing to see what is needed to fully support a v8 index. This can be evaluated later as needed.
First pass at upgrading to lucene v9.7.0. Mostly focused on fixing compilation errors and getting the tests passing.