Clone enhancement/back to bm25 #10
Conversation
With the recent lucene upgrade we have temporarily adopted the LegacyBM25Similarity which exposes the same scores as BM25Similarity before the k1+1 factor was removed from the numerator of the scoring formula. This commit moves the default Elasticsearch similarity back to the Bm25Similarity and updates the scores that have changed in our docs and tests
This allows users to opt-out of the updated bm25 similarities and use the deprecated legacy bm25 similarity. This helps especially cross-cluster search cases across clusters on multiple versions, otherwise sorting by score would lead to very weird results depending on the version of the data node that scores each doc.
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Summary of ChangesHello @DDShantanuBadmanji, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes Elasticsearch's default BM25 scoring algorithm by adopting the standard Lucene Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request updates numeric scores across documentation, tests, and test data following a shift in the similarity scoring mechanism. It introduces explicit LegacyBM25 support for indices created before version 7.0.0, changes the default BM25 method to return a generic Similarity type, and adds rolling upgrade tests to validate search scoring behavior across cluster upgrade paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Index as Index (Created)
participant Provider as SimilarityProviders
participant Service as SimilarityService
participant Similarity as Similarity Instance
Index->>Provider: createBM25Similarity(settings, version)
alt version >= 7.0.0
Provider->>Similarity: new BM25Similarity()
else version < 7.0.0
Provider->>Similarity: new LegacyBM25Similarity()
end
Similarity-->>Service: return Similarity
Service->>Service: setDefault(similarity)
Note over Service: Legacy indices get LegacyBM25<br/>Newer indices get BM25
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the default BM25 similarity implementation and ensures backward compatibility for indices created with older versions. For indices created before 7.0, it preserves the LegacyBM25Similarity, while newer indices will use the updated BM25Similarity. A new LegacyBM25 similarity is also introduced for explicit use. The changes are well-supported by updates to documentation, core similarity services, and extensive testing, including a new rolling upgrade integration test (SearchIT) to validate scoring consistency during upgrades. The overall changes look solid and improve the scoring mechanism while maintaining backward compatibility.
| for (int i = 0; i < numDocs; i++) { | ||
| Tuple<String, String> document; | ||
| do { | ||
| document = randomFrom(DOCS); | ||
| } while (docExists("multi-old", document.v1()) || docExists("multi-mixed", document.v1())); | ||
| index(index, document); | ||
| } |
There was a problem hiding this comment.
The current implementation of indexNewDocs uses a do-while loop with random document selection, which can be inefficient due to a non-deterministic number of expensive docExists network calls. This can impact test performance and predictability.
A more robust approach is to first collect all available documents and then select from that list. This makes the number of docExists calls deterministic and improves the overall efficiency and maintainability of the test.
List<Tuple<String, String>> availableDocs = new ArrayList<>();
for (Tuple<String, String> doc : DOCS) {
if (docExists("multi-old", doc.v1()) == false && docExists("multi-mixed", doc.v1()) == false) {
availableDocs.add(doc);
}
}
Collections.shuffle(availableDocs, random());
if (availableDocs.size() < numDocs) {
throw new IllegalStateException("Not enough documents available to index. Required: " + numDocs
+ ", available: " + availableDocs.size());
}
for (int i = 0; i < numDocs; i++) {
index(index, availableDocs.get(i));
}
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
x-pack/plugin/sql/qa/src/main/resources/docs.csv-spec (2)
915-918: Score consistency verified across different orderings.The SCORE() values here match those in the
orderByScoretest (lines 901-904), correctly maintaining the same scores for the same documents even when ordered differently (by page_count in this case). This demonstrates proper data integrity.
929-932: Score consistency verified in third ordering scenario.The SCORE() values remain consistent with the previous tests (lines 901-904, 915-918), correctly maintaining document-specific scores when ordered by release year. All three test scenarios demonstrate proper score-to-document mapping.
🧹 Nitpick comments (15)
docs/reference/search/request-body.asciidoc (1)
38-38: Request body example scores aligned.
max_score and first hit _score both 0.6301338; consistent.If these docs target 8.x, consider typeless mappings/APIs in adjacent examples in a future pass; ignore if this page intentionally documents 7.x behavior.
Also applies to: 44-44
docs/reference/mapping/params/normalizer.asciidoc (1)
92-117: LGTM: scores updated consistentlymax_score and both hit scores align. Optional: consider making the doc-test resilient to future scoring drift by normalizing these fields via TESTRESPONSE (e.g., replace hardcoded max_score and each hits[n]._score with $body paths), similar to how "took" is handled.
docs/reference/search/request/highlighting.asciidoc (1)
867-874: LGTM: updated highlight example scores are internally consistentBoth responses use the same max_score/_score values. Optional: add TESTRESPONSE substitutions for max_score and the first hit’s _score in each block to reduce future churn from similarity tweaks.
Also applies to: 926-933
docs/reference/aggregations/metrics/tophits-aggregation.asciidoc (1)
331-351: LGTM: nested top_hits scores matchmax_score equals the single hit’s _score as expected. Optional: normalize these with TESTRESPONSE (e.g., $body.aggregations.by_sale.by_user.buckets.0.by_nested.hits.max_score and hits.hits.0._score) to reduce future maintenance.
docs/reference/how-to/recipes/stemming.asciidoc (1)
86-113: LGTM: stemming examples keep scores consistent across max_score/_scoreNumbers are coherent in both multi-hit and single-hit cases. Optional: consider TESTRESPONSE replacements for max_score and each shown _score to avoid brittle literals over future similarity changes.
Also applies to: 148-165, 206-225
docs/reference/search/explain.asciidoc (1)
36-43: Explain math verified; consider relaxing numeric literal assertionsidf (1.3862944) × tf (0.5555555) ≈ 0.7701635, matching both value fields; removing the boost detail is consistent. Optional: to avoid future drift, use TESTRESPONSE substitutions for explanation.value and the first details value rather than hardcoding the float literals.
qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/SearchIT.java (6)
47-55: Fix minor typos in comments.Nit: “searchging” → “searching”, “wil” → “will”.
- //values are in the expected scoring order when searchging for "quick brown fox dog", docs wil be shuffled + // values are in the expected scoring order when searching for "quick brown fox dog"; docs will be shuffled
59-65: Avoid manual JSON string assembly for docs.Safer to build via XContent to handle escaping if texts change.
- DOCS.add(Tuple.tuple(String.valueOf(i), "{\"text\": \"" + VALUES[i] + "\"}")); + try (var xb = JsonXContent.contentBuilder()) { + xb.startObject().field("text", VALUES[i]).endObject(); + DOCS.add(Tuple.tuple(String.valueOf(i), Strings.toString(xb))); + }
98-121: Harden cluster readiness before cross-index search.Add multi-upgraded to the health wait to reduce flakiness right after creation.
- waitForGreen("multi-old,multi-mixed"); - createIndex("multi-upgraded", Settings.builder().put("index.similarity.default.type", "LegacyBM25").build()); + waitForGreen("multi-old,multi-mixed"); + createIndex("multi-upgraded", + Settings.builder().put("index.similarity.default.type", "LegacyBM25").build()); + waitForGreen("multi-old,multi-mixed,multi-upgraded");
124-132: Guard against duplicate IDs within the target index.If numDocs > 1 in future, the loop could re-pick an ID already indexed into the same target index. Include the target index in the existence check.
- do { - document = randomFrom(DOCS); - } while (docExists("multi-old", document.v1()) || docExists("multi-mixed", document.v1())); + do { + document = randomFrom(DOCS); + } while ( + docExists("multi-old", document.v1()) + || docExists("multi-mixed", document.v1()) + || docExists(index, document.v1()) + );
147-160: Constrain result size to what you assert.Explicitly request 5 hits to reduce payload and make intent obvious.
- SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder().size(5);
162-167: More robust xContent registry for parsing (future-proofing).Parsing works now, but consider using a SearchModule-backed registry to tolerate added sections (aggs/suggest) without surprises.
- return SearchResponse.fromXContent(JsonXContent.jsonXContent.createParser(NamedXContentRegistry.EMPTY, - DeprecationHandler.THROW_UNSUPPORTED_OPERATION, responseString)); + var registry = new NamedXContentRegistry( + new org.elasticsearch.search.module.SearchModule(Settings.EMPTY, List.of()).getNamedXContents() + ); + return SearchResponse.fromXContent( + JsonXContent.jsonXContent.createParser(registry, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, responseString) + );server/src/main/java/org/elasticsearch/index/similarity/SimilarityProvider.java (1)
58-64: Comment grammar nits (optional).Tighten wording and fix a typo (“for similarities a single index” → “within a single index”) for consistency with equals()/hashCode() rationale.
Apply:
- /* - * We check <code>name</code> only because the <code>similarity</code> is + /* + * We check only <code>name</code> because the <code>similarity</code> is * re-created for each new instance and they don't implement equals. * This is not entirely correct though but we only use equality checks - * for similarities inside the same index and names are unique in this case. + * within a single index, and names are unique in this case. **/- /* - * We use <code>name</code> only because the <code>similarity</code> is + /* + * We use only <code>name</code> because the <code>similarity</code> is * re-created for each new instance and they don't implement equals. * This is not entirely correct though but we only use equality checks - * for similarities a single index and names are unique in this case. + * within a single index, and names are unique in this case. **/Also applies to: 69-75
server/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java (1)
160-163: Avoid local variable shadowing of fielddefaultSimilarity.Rename the local to improve readability.
- Supplier<Similarity> defaultSimilarity = providers.get("default"); - this.defaultSimilarity = (defaultSimilarity != null) ? - defaultSimilarity.get() : providers.get(SimilarityService.DEFAULT_SIMILARITY).get(); + Supplier<Similarity> defaultSupplier = providers.get("default"); + this.defaultSimilarity = (defaultSupplier != null) + ? defaultSupplier.get() + : providers.get(SimilarityService.DEFAULT_SIMILARITY).get();server/src/main/java/org/elasticsearch/index/similarity/SimilarityProviders.java (1)
274-291: Verification complete; call sites are compatible with new signature.Both usages correctly expect
Similaritybase type:
- Line 78 explicitly assigns to
Similarity similarity- Line 104 returns from lambda in a context-compatible manner
No callers assume
LegacyBM25Similarityreturn type. The method signature change is safe.The original review's optional suggestion to add a method-level comment clarifying the LegacyBM25 fallback behavior remains valid for documentation purposes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
docs/reference/aggregations/metrics/tophits-aggregation.asciidoc(2 hunks)docs/reference/analysis/charfilters/pattern-replace-charfilter.asciidoc(1 hunks)docs/reference/analysis/tokenizers/edgengram-tokenizer.asciidoc(1 hunks)docs/reference/how-to/recipes/stemming.asciidoc(4 hunks)docs/reference/mapping/params/normalizer.asciidoc(2 hunks)docs/reference/query-dsl/terms-set-query.asciidoc(1 hunks)docs/reference/search/explain.asciidoc(1 hunks)docs/reference/search/request-body.asciidoc(1 hunks)docs/reference/search/request/highlighting.asciidoc(2 hunks)docs/reference/search/request/inner-hits.asciidoc(6 hunks)docs/reference/search/uri-request.asciidoc(1 hunks)qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/IndexingIT.java(2 hunks)qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/SearchIT.java(1 hunks)server/src/main/java/org/elasticsearch/index/similarity/SimilarityProvider.java(2 hunks)server/src/main/java/org/elasticsearch/index/similarity/SimilarityProviders.java(3 hunks)server/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java(3 hunks)server/src/test/java/org/elasticsearch/index/similarity/LegacySimilarityTests.java(2 hunks)server/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java(3 hunks)server/src/test/java/org/elasticsearch/index/similarity/SimilarityTests.java(5 hunks)server/src/test/java/org/elasticsearch/search/basic/TransportTwoNodesSearchIT.java(2 hunks)server/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java(1 hunks)x-pack/plugin/sql/qa/src/main/resources/docs.csv-spec(3 hunks)x-pack/plugin/sql/qa/src/main/resources/fulltext.csv-spec(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
server/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java (2)
server/src/main/java/org/elasticsearch/index/similarity/SimilarityProviders.java (1)
SimilarityProviders(68-361)server/src/main/java/org/elasticsearch/common/settings/Settings.java (1)
Settings(84-1467)
server/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java (1)
server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java (1)
IndexMetaData(82-1556)
qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/SearchIT.java (3)
server/src/main/java/org/elasticsearch/common/settings/Settings.java (1)
Settings(84-1467)server/src/main/java/org/elasticsearch/search/SearchHit.java (1)
SearchHit(81-1006)server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java (1)
SearchSourceBuilder(83-1579)
server/src/test/java/org/elasticsearch/index/similarity/SimilarityTests.java (1)
server/src/main/java/org/elasticsearch/common/settings/Settings.java (1)
Settings(84-1467)
server/src/test/java/org/elasticsearch/index/similarity/LegacySimilarityTests.java (2)
server/src/main/java/org/elasticsearch/common/settings/Settings.java (1)
Settings(84-1467)server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java (1)
IndexMetaData(82-1556)
🔇 Additional comments (29)
server/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java (1)
329-329: LGTM! Test assertion correctly updated for new explanation format.The change makes the test more robust by dynamically using
explanation.getValue()instead of hardcoding a numeric score prefix, while still verifying the explanation message structure. This aligns with the scoring mechanism changes described in the PR summary.server/src/test/java/org/elasticsearch/search/basic/TransportTwoNodesSearchIT.java (2)
151-160: Explanation tree navigation correctly updated for new structure.The changes consistently adjust the test to navigate a shallower explanation tree:
- Line 151: Expects 2 details at the outer level (previously 3)
- Line 152 & subsequent: Accesses
details[0]instead ofdetails[1]at the second levelThe verification logic for term frequency statistics (n and N values) remains intact, just accessed via the new tree structure. This aligns with the scoring mechanism changes described in the PR.
190-199: Consistent explanation tree updates in sorted variant.The changes mirror those in
testDfsQueryThenFetch, maintaining consistency across both DFS_QUERY_THEN_FETCH test variants (with and without sorting). The identical adjustment pattern confirms these are systematic updates to accommodate the new explanation structure.x-pack/plugin/sql/qa/src/main/resources/fulltext.csv-spec (1)
79-79: Score value updates verified as consistent; tests require manual verification.Both SCORE() expected outputs have been updated to
1.8660883for the same underlying queryMATCH(first_name, 'Erez'):
- Line 79: Direct SCORE() function test
- Line 86: SCORE() with alias 's'
The changes are consistent. However, the test suite could not be executed in this environment. Please verify that all SQL full-text query tests pass with these updated expected values.
Also applies to: 86-86
x-pack/plugin/sql/qa/src/main/resources/docs.csv-spec (1)
901-904: LGTM - Score values are internally consistent and align with BM25 enhancement.The updated SCORE() values appear valid for this BM25 similarity scoring change. Scores are properly ordered, values are reasonable for text similarity (0.63–1.04 range), and the same scores are consistently used across all three test scenarios (lines 901–904, 915–918, 929–932). This change is part of the "back_to_bm25" enhancement effort reflected in the git history.
docs/reference/search/uri-request.asciidoc (1)
34-34: URI search example scores updated and consistent.
Matches the request-body page values; looks good.Also applies to: 40-40
docs/reference/query-dsl/terms-set-query.asciidoc (1)
79-79: Terms set example scores updated coherently.
Replacement values are consistent within the response.Also applies to: 85-85
docs/reference/analysis/charfilters/pattern-replace-charfilter.asciidoc (1)
248-248: Char filter highlight example scores adjusted; consistent.
max_score and _score match at 0.13076457; no other changes.Also applies to: 254-254
docs/reference/analysis/tokenizers/edgengram-tokenizer.asciidoc (1)
307-307: Scores updated consistently (max_score == top hit _score). LGTM.
Verification confirmed the update to 0.26152915 at lines 307 and 313 is coherent and complete. No structural changes; example remains valid.docs/reference/search/request/inner-hits.asciidoc (2)
398-427: LGTM: Hierarchical nested query scores updated correctly.The score values for the hierarchical nested match query are internally consistent. The different score (0.3150669 vs 0.47475836 in the earlier example) appropriately reflects the different query and document characteristics under BM25 scoring.
277-302: TESTRESPONSE directives validate response structure only, not numeric score accuracy.The score values (0.47475836, 0.3150669) are hardcoded literals in the JSON examples that are not covered by the TESTRESPONSE substitution patterns (which only replace
_sourceand...placeholders). While these precise decimals suggest they derive from actual test output, there is no evidence in the codebase that numeric scores are explicitly validated or compared with tolerance during doc tests.The changes align with the PR's BM25 scoring objective, but manually verify that these score values are correct by running the integration tests or checking the actual query responses with the updated BM25 algorithm.
qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/IndexingIT.java (2)
22-24: Import cleanup looks good.Removing the duplicate imports improves readability; no behavior change.
148-159: Static bulk helper is fine; no behavior change.bulk(...) has no instance dependencies and client() is callable from static context in these tests. All call sites remain valid.
server/src/test/java/org/elasticsearch/index/similarity/LegacySimilarityTests.java (2)
32-33: Import addition LGTM.Needed for randomized 6.x version selection.
47-48: Good: randomized 6.x creation version.Covers broader 6.x range and avoids brittle hard-coding.
server/src/main/java/org/elasticsearch/index/similarity/SimilarityService.java (2)
78-84: Default suppliers: version-aware BM25 + explicit LegacyBM25.Back-compat handled cleanly via provider indirection. No issues.
105-107: Built-in LegacyBM25 wiring looks correct.Factory delegates to SimilarityProviders; consistent with others.
server/src/main/java/org/elasticsearch/index/similarity/SimilarityProviders.java (2)
26-26: Imports updated appropriately.Required for the split between BM25Similarity and Similarity return types.
Also applies to: 54-54
293-303: Explicit LegacyBM25 factory: clear and consistent.Subset-assertion and parameter handling mirror BM25 path. No issues.
server/src/test/java/org/elasticsearch/index/similarity/SimilarityServiceTests.java (4)
46-47: Default similarity assertion LGTM.Current default resolves to BM25Similarity.
49-55: 6.x default fallback to LegacyBM25 verified.Good coverage across randomized 6.x versions.
74-75: Override default to boolean: OK.Asserts override path without touching built-ins.
77-83: Override default to LegacyBM25: OK.Confirms explicit legacy selection works.
server/src/test/java/org/elasticsearch/index/similarity/SimilarityTests.java (6)
23-23: LGTM! Import additions support the test enhancements.The new imports for
BM25SimilarityandnullValue()are necessary for the enhanced test assertions and follow standard testing practices.Also applies to: 54-54
63-73: LGTM! Test properly validates both BM25 variants.The updates correctly verify that both
BM25SimilarityandLegacyBM25Similarityare available as default similarities, and the change tonullValue()follows Hamcrest best practices.
75-75: LGTM! Unnecessary exception removed from signature.The
IOExceptionwas correctly removed from the throws clause since the method body doesn't throw any checked exceptions.
86-106: LGTM! Enhanced test coverage with parameter validation.The added assertions properly verify that BM25 similarity parameters (k1, b, discount_overlaps) are correctly configured from index settings. The instanceOf check ensures type safety before casting.
108-128: LGTM! Comprehensive test coverage for LegacyBM25 similarity.This new test method properly validates LegacyBM25 configuration with parameter verification. It follows the established test pattern and ensures backward compatibility support for legacy indices.
261-261: LGTM! Unnecessary exception removed from signature.The
IOExceptionwas correctly removed from the throws clause since the method body doesn't throw any checked exceptions.
User description
gradle check?CodeAnt-AI Description
Restore BM25 as default similarity and add LegacyBM25 for backward compatibility
What Changed
Impact
✅ Consistent search ordering during rolling upgrades✅ Same legacy scores for indices created on 6.x✅ Different numeric search scores for newly created indices (docs and examples updated)💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Documentation
New Features
Changes