-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Support older postings formats #85303
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Pinging @elastic/es-search (Team:Search) |
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 left some minor comments. This looks great. Thanks Yannick!
libs/core/src/main/java/org/elasticsearch/core/internal/io/IOUtils.java
Outdated
Show resolved
Hide resolved
.../repository-old-versions/src/test/java/org/elasticsearch/oldrepos/OldRepositoryAccessIT.java
Outdated
Show resolved
Hide resolved
if (suffix == null) { | ||
throw new IllegalStateException("missing attribute: " + PER_FIELD_SUFFIX_KEY + " for field: " + fieldName); | ||
} | ||
PostingsFormat format = legacyAdaptingPerFieldPostingsFormat.getPostingsFormat(formatName); |
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 we introduced this adapter because of line 125? I tried to remove this class by converting the PerFieldPostingsFormat.format
attribute from Lucene50
to BWCLucene50
in FieldInfos, but it didn't work because of line 126. I think it's fine to have this class.
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.
Another motivation for this class is that it keeps the information in these older indices as much "as-is" as possible, not requiring a conversion step before reading. Furthermore, future versions won't have to be aware that that conversion step happened in some prior versions of archive, if we were to remove it e.g. in the future. The nice bit is that this conversion can go away in ES 9 as we won't have a conflicting Lucene50PostingsFormat shipped anymore as part of bwc jars.
...versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/codecs/lucene62/Lucene62Codec.java
Outdated
Show resolved
Hide resolved
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
Adds support for the Lucene 5 postings format (used by Lucene 6 and 7).
Relates #81210