Skip to content

Add SVS-VAMANA index to info fields telemetry [MOD-10104]#6614

Merged
alonre24 merged 4 commits intomasterfrom
add_vamana_to_info
Aug 18, 2025
Merged

Add SVS-VAMANA index to info fields telemetry [MOD-10104]#6614
alonre24 merged 4 commits intomasterfrom
add_vamana_to_info

Conversation

@alonre24
Copy link
Copy Markdown
Collaborator

@alonre24 alonre24 commented Aug 7, 2025

Describe the changes in the pull request

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

@github-actions github-actions bot added the size:S label Aug 7, 2025
@alonre24 alonre24 changed the title Add SVS-VAMANA index to info fields telemetry Add SVS-VAMANA index to info fields telemetry [MOD-10104] Aug 7, 2025
@alonre24 alonre24 requested a review from meiravgri August 7, 2025 16:07
meiravgri
meiravgri previously approved these changes Aug 7, 2025
env.assertEqual(field_info_to_dict(fieldsInfo['search_fields_numeric']), get_search_field_info('Numeric', 2, NoIndex=1))
env.assertEqual(field_info_to_dict(fieldsInfo['search_fields_geo']), get_search_field_info('Geo', 1))
env.assertEqual(field_info_to_dict(fieldsInfo['search_fields_vector']), get_search_field_info('Vector', 2, Flat=1, HNSW=1))
env.assertEqual(field_info_to_dict(fieldsInfo['search_fields_vector']), get_search_field_info('Vector', 4, Flat=1, HNSW=1, SVS_VAMANA=2, SVS_VAMANA_Compressed=1))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we have a test that also checks we count gc_marked_deleted_vectors and used_memory_vector_index correctly?
I suggest to incrementally add svs changes and making sure they take effect (for example, create an svs index and ensure the used_memory_vector_index increased)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I don't think that we count the marked deleted vector of svs as part of the vector stats today. We should address that and confirm it here after we implement it in vecsim. Regarding the used memory of vector indexes - it should be accounted - I will add a test

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yep, svs should implement Tombstone

template <typename DataType, typename DistType>
VecSimIndexStatsInfo VecSimTieredIndex<DataType, DistType>::statisticInfo() const {
    auto stats = VecSimIndexStatsInfo{
        .memory = this->getAllocationSize(),
        .numberOfMarkedDeleted = 0, // Default value if cast fails
    };

    // If backend implements VecSimIndexTombstone, get number of marked deleted
    if (auto tombstone = dynamic_cast<VecSimIndexTombstone *>(this->backendIndex)) {
        stats.numberOfMarkedDeleted = tombstone->getNumMarkedDeleted();
    }

    return stats;
}

Copy link
Copy Markdown
Collaborator

@meiravgri meiravgri Aug 18, 2025

Choose a reason for hiding this comment

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

or to implement statisticInfo. The number of marked deleted vectors is available via changes_num field of the svs index.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Right
I opened a ticket for that

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 7, 2025

Codecov Report

❌ Patch coverage is 18.18182% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.30%. Comparing base (926d220) to head (0f990b1).
⚠️ Report is 25 commits behind head on master.

Files with missing lines Patch % Lines
src/info/info_redis/info_redis.c 0.00% 6 Missing ⚠️
src/info/global_stats.c 40.00% 3 Missing ⚠️

❌ Your project status has failed because the head coverage (48.88%) is below the adjusted base coverage (86.51%). You can increase the head coverage or adjust the Removed Code Behavior.

❗ There is a different number of reports uploaded between BASE (926d220) and HEAD (0f990b1). Click for more details.

HEAD has 7 uploads less than BASE
Flag BASE (926d220) HEAD (0f990b1)
flow 4 0
unit 4 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #6614       +/-   ##
===========================================
- Coverage   87.57%   49.30%   -38.28%     
===========================================
  Files         278      282        +4     
  Lines       44606    44802      +196     
  Branches     7571     7712      +141     
===========================================
- Hits        39064    22089    -16975     
- Misses       5430    22593    +17163     
- Partials      112      120        +8     
Flag Coverage Δ
flow ?
unit 49.30% <18.18%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@alonre24 alonre24 added this pull request to the merge queue Aug 18, 2025
Merged via the queue into master with commit 6ecbd3e Aug 18, 2025
15 checks passed
@alonre24 alonre24 deleted the add_vamana_to_info branch August 18, 2025 09:38
redisearch-backport-pull-request bot pushed a commit that referenced this pull request Aug 18, 2025
* account for vector index in total memory info

* Add svs-vamana to info telemetry

* revert manually

* add svs vamana info test

(cherry picked from commit 6ecbd3e)
@redisearch-backport-pull-request
Copy link
Copy Markdown
Contributor

Successfully created backport PR for 8.2:

github-merge-queue bot pushed a commit that referenced this pull request Aug 18, 2025
Add SVS-VAMANA index to info fields telemetry [MOD-10104] (#6614)

* account for vector index in total memory info

* Add svs-vamana to info telemetry

* revert manually

* add svs vamana info test

(cherry picked from commit 6ecbd3e)

Co-authored-by: alonre24 <alon.reshef@redis.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.

2 participants