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

Add short circuit if no live docs are in segments #2059

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

VijayanB
Copy link
Member

@VijayanB VijayanB commented Sep 6, 2024

Description

  • Add short circuit if no live docs are in segments
  • Fixed stop watch bug

Related Issues

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@VijayanB VijayanB self-assigned this Sep 6, 2024
@VijayanB VijayanB marked this pull request as draft September 6, 2024 01:52
@VijayanB VijayanB force-pushed the return-live-docs branch 2 times, most recently from 90d1f44 to f06dc38 Compare September 6, 2024 02:11
@VijayanB VijayanB marked this pull request as ready for review September 6, 2024 03:00
// Count the docIds
int totalLiveDocs = getLiveDocs(vectorValuesRetriever.apply(vectorDataType, fieldInfo, VectorProcessingContext));
if (quantizationParams != null && totalLiveDocs > 0) {
if (totalLiveDocs == 0) {
log.debug("No live docs for field " + fieldInfo.name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm - this wont lead to an issue on search if an empty file is expected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmazanec15 Can you give more context when do we expect empty file? I see this is already added inside index method

private void buildAndWriteIndex(final KNNVectorValues<?> knnVectorValues, int totalLiveDocs) throws IOException {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For search and warmup, we should make sure that we arent reading this file if the size is empy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline and confirmed that if file is empty, we are not reading the file since we check whether file exists or not before reading it.

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
@VijayanB
Copy link
Member Author

CI failure is not related to this PR. It is failing since we are in the midst of release.

@VijayanB VijayanB merged commit 7aedefd into opensearch-project:main Sep 12, 2024
28 of 29 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 12, 2024
* Add short circuit if no live docs are in segments

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
---------

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
(cherry picked from commit 7aedefd)
VijayanB added a commit that referenced this pull request Sep 13, 2024
* Add short circuit if no live docs are in segments

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
---------

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
(cherry picked from commit 7aedefd)

Co-authored-by: Vijayan Balasubramanian <balasvij@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants