-
Notifications
You must be signed in to change notification settings - Fork 690
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
SOLR-17097: Upgrade Solr to use Lucene 9.9.2 #2176
Conversation
It's great that we are working to stay up to date with Lucene! Looks like two of the tests are failing? |
|
FieldComparator<?> comparator = | ||
sortField.getComparator(1, Pruning.GREATER_THAN_OR_EQUAL_TO); |
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.
@@ -97,7 +97,7 @@ public static LeafReader wrap(IndexReader reader) throws IOException { | |||
in = reader; | |||
in.registerParentReader(this); | |||
if (reader.leaves().isEmpty()) { | |||
metaData = new LeafMetaData(Version.LATEST.major, Version.LATEST, null); | |||
metaData = new LeafMetaData(Version.LATEST.major, Version.LATEST, null, false); |
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.
createdVersionMajor, | ||
minVersion, | ||
null, | ||
reader.leaves().get(0).reader().getMetaData().hasBlocks()); |
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.
minor/subjective: reader.leaves().get(0).reader().getMetaData()
assignment to a local variable so that both the createdVersionMajor
compute above and the hasBlock
determination here can use it
Was able to reproduce the failure locally with |
expected = "(foo_dt:[1378857600000 TO 1378857600000])"; | ||
expected = "foo_dt:[1378857600000 TO 1378857600000]"; | ||
if (foo_dt.hasDocValues() && foo_dt.indexed()) { | ||
expected = "IndexOrDocValuesQuery" + expected; | ||
expected = | ||
"IndexOrDocValuesQuery(IndexOrDocValuesQuery(indexQuery=" | ||
+ expected | ||
+ ", dvQuery=" | ||
+ expected | ||
+ "))"; | ||
} else { | ||
expected = "(" + expected + ")"; |
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.
LeafMetaData leafMetaData = reader.leaves().get(0).reader().getMetaData(); | ||
metaData = | ||
new LeafMetaData( | ||
leafMetaData.getCreatedVersionMajor(), minVersion, null, leafMetaData.hasBlocks()); |
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.
Out of curiosity, why don't we also specify the sort
parameter here (3rd parameter)? It could be retrieved from existing leaf metadata (this is not new with this change).
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.
good point, we could specify leafMetaData.getSort()
if docs are in no particular order, it would be null.
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.
Out of curiosity, why don't we also specify the
sort
parameter here (3rd parameter)? It could be retrieved from existing leaf metadata (this is not new with this change).
I wonder if this might be because we don't yet have 'full' index sorting directly configurable in Solr -- https://issues.apache.org/jira/browse/SOLR-13681 exists for that -- i.e. commonly the sort
would indeed be null
except when a SortingMergePolicy
is configured in which case we'd have 'partial' index sorting i.e. some segments could be sorted and some could be unsorted and thus it would be tricky to confidently set the sort
value to anything other than the null
value.
... (this is not new with this change).
My preference would be to not change the parameter setting as part of the Lucene upgrade but to perhaps leave a note on SOLR-13681 or have a separate issue for that change.
cross-referencing w.r.t. the
|
FYI I've tried out the Solr-with-local-Lucene documentation steps -- #2223 tweaks are a side effect -- to confirm and have confirmed that apache/lucene#13014 solves the
|
Thanks Christine for quickly testing it. I will wait for the v.9.9.2 official release and update this PR. |
all tests passed locally |
This looks awesome, thanks to all! I'm going to test this out locally and then plan to merge later today! |
Co-authored-by: Nazerke Seidan <[email protected]> Co-authored-by: Christine Poerschke <[email protected]>
Ran tests locally too, they passed. |
Co-authored-by: Nazerke Seidan <[email protected]> Co-authored-by: Christine Poerschke <[email protected]>
*https://issues.apache.org/jira/browse/SOLR-17097
Description
Upgrade Solr to use Lucene 9.9.2
Solution
Please provide a short description of the approach taken to implement your solution.
Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.