Skip to content

Conversation

@expani
Copy link
Contributor

@expani expani commented Feb 11, 2025

Description

We override IndexSearcher#search(List<LeafReaderContext> leaves, Weight weight, Collector collector) and it contains the logic to apply time series desc optimisation for scanning segments in reverse for descending sort use-cases.

Existing Lucene 9.12.1 and OS 2.19 call flow

Screenshot 2025-02-11 at 2 40 05 PM

Lucene 10.1.0 and OS 3.0 call flow

Screenshot 2025-02-11 at 2 40 52 PM

With Lucene 10 changes, the new replacement method

search(LeafReaderContextPartition[] partitions, Weight weight, Collector collector)

only gets invoked via IndexSearcher#search(Query, CollecterManager) which is not used by OpenSearch in QueryPhase#searchWithCollector.

So, it was never getting called causing the time series desc optimization to be skipped.

My changes ensure it will be called in the same way that IndexSearcher does for CollectorManager variant.

Related Issues

Resolves #17386

@expani
Copy link
Contributor Author

expani commented Feb 11, 2025

@msfroh @reta Please help in reviewing the same.

I am trying to see if there is an easy to catch this with existing UTs as well.

@reta
Copy link
Contributor

reta commented Feb 11, 2025

I am trying to see if there is an easy to catch this with existing UTs as well.

This is explains the xxx_desc regressions in OSB, thanks @expani !

@expani
Copy link
Contributor Author

expani commented Feb 11, 2025

@reta With this change, OS 2.19 and OS 3.0 are at same for following query types in big5

asc_sort_timestamp
asc_sort_timestamp_can_match_shortcut
asc_sort_timestamp_no_can_match_shortcut

Without it, the latency for OS 3.0 was 10ms more than OS 2.19

@github-actions
Copy link
Contributor

❌ Gradle check result for 21d5903: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

❌ Gradle check result for 0ce530b: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

❌ Gradle check result for 1757058: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

❕ Gradle check result for 1757058: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.snapshots.DedicatedClusterSnapshotRestoreIT.testSnapshotWithStuckNode

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Signed-off-by: expani <[email protected]>
@github-actions
Copy link
Contributor

❌ Gradle check result for ccf11fb: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Contributor

❌ Gradle check result for 4ddd3fa: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@expani expani closed this Mar 25, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in Performance Roadmap Mar 25, 2025
@expani expani reopened this Mar 25, 2025
@github-project-automation github-project-automation bot moved this from Done to In Progress in Performance Roadmap Mar 25, 2025
@github-actions
Copy link
Contributor

✅ Gradle check result for 4ddd3fa: SUCCESS

@codecov
Copy link

codecov bot commented Mar 25, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.47%. Comparing base (6d53f9d) to head (4ddd3fa).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...ensearch/search/internal/ContextIndexSearcher.java 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #17329      +/-   ##
============================================
+ Coverage     72.40%   72.47%   +0.07%     
- Complexity    65828    65887      +59     
============================================
  Files          5316     5322       +6     
  Lines        305294   305477     +183     
  Branches      44289    44310      +21     
============================================
+ Hits         221033   221399     +366     
+ Misses        66187    65919     -268     
- Partials      18074    18159      +85     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[BUG] Time Series Desc Optimisation gets skipped with Lucene 10 Upgrade

5 participants