Skip to content
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

Merged
merged 7 commits into from Jan 30, 2024

Conversation

NazerkeBS
Copy link
Contributor

@NazerkeBS NazerkeBS commented Jan 4, 2024

*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:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@epugh
Copy link
Contributor

epugh commented Jan 4, 2024

It's great that we are working to stay up to date with Lucene! Looks like two of the tests are failing?

@NazerkeBS
Copy link
Contributor Author

It's great that we are working to stay up to date with Lucene! Looks like two of the tests are failing?

TestSolrQueryParser#testSyntax - re-run it a few times locally , surprisingly it passed then failed, passed, failed; somewhat flaky.

TestScoreJoinQPNoScore#testRandomJoin - took a look into it; it seems the problem relates to ScoreMode ScoreJoinQParserPlugin. Locally in my machineorg.apache.lucene.search.ScoreMode class is not found and this surprised me. Looked into jars and seeing ScoreMode.class instead of ScoreMode.

Comment on lines 501 to 502
FieldComparator<?> comparator =
sortField.getComparator(1, Pruning.GREATER_THAN_OR_EQUAL_TO);
Copy link
Contributor

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);
Copy link
Contributor

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());
Copy link
Contributor

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

@cpoerschke
Copy link
Contributor

TestSolrQueryParser#testSyntax - re-run it a few times locally , surprisingly it passed then failed, passed, failed; somewhat flaky.

Was able to reproduce the failure locally with ./gradlew test --tests TestSolrQueryParser.testSyntax -Dtests.seed=9EEE6CC2A6BE4FEA from the CI run. Added a commit to account for the apache/lucene@690462c change and now it passes locally.

Comment on lines -250 to +259
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 + ")";
Copy link
Contributor

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());
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@cpoerschke
Copy link
Contributor

cross-referencing w.r.t. the TestScoreJoinQPNoScore#testRandomJoin failures:

@cpoerschke
Copy link
Contributor

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 TestScoreJoinQPNoScore#testRandomJoin failures.

./gradlew test --tests TestScoreJoinQPNoScore.testRandomJoin -Plucene.dev.version=9.9.2-SNAPSHOT

@NazerkeBS
Copy link
Contributor Author

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 TestScoreJoinQPNoScore#testRandomJoin failures.

./gradlew test --tests TestScoreJoinQPNoScore.testRandomJoin -Plucene.dev.version=9.9.2-SNAPSHOT

Thanks Christine for quickly testing it. I will wait for the v.9.9.2 official release and update this PR.

@NazerkeBS NazerkeBS changed the title SOLR-17097: Upgrade Solr to use Lucene 9.9.1 SOLR-17097: Upgrade Solr to use Lucene 9.9.2 Jan 29, 2024
@NazerkeBS
Copy link
Contributor Author

all tests passed locally

@gerlowskija
Copy link
Contributor

This looks awesome, thanks to all! I'm going to test this out locally and then plan to merge later today!

@gerlowskija gerlowskija merged commit 4d22f0c into apache:main Jan 30, 2024
2 of 3 checks passed
gerlowskija pushed a commit that referenced this pull request Jan 30, 2024
Co-authored-by: Nazerke Seidan <nseidan@Nazerkes-MacBook-Pro.local>
Co-authored-by: Christine Poerschke <cpoerschke@apache.org>
@cpoerschke
Copy link
Contributor

... I'm going to test this out locally ...

Ran tests locally too, they passed.

gerlowskija pushed a commit that referenced this pull request Jan 30, 2024
Co-authored-by: Nazerke Seidan <nseidan@Nazerkes-MacBook-Pro.local>
Co-authored-by: Christine Poerschke <cpoerschke@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants