Skip to content

Conversation

@iverase
Copy link
Contributor

@iverase iverase commented Jul 7, 2025

This commit removes an unnecessary loop when computing neighbours. In addition it removes the usage of lists and replace them with arrays as they are fixed length lists and some other clean up.

@iverase iverase requested review from Copilot and john-wagster July 7, 2025 08:52
@iverase iverase requested review from benwtrent and removed request for Copilot July 7, 2025 08:53
@elasticsearchmachine elasticsearchmachine added the Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch label Jul 7, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-relevance (Team:Search Relevance)

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Refactors the local KMeans implementation by removing an unnecessary neighbor loop, switching from List to array for fixed-size collections, and tightening test assertions.

  • Replaced List<NeighborHood> with NeighborHood[] and updated related methods.
  • Streamlined computeNeighborhoods to return an array and changed assignSpilled to operate via side effects.
  • Strengthened HierarchicalKMeansTests with range checks and improved assertion methods.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
server/src/test/java/org/elasticsearch/index/codec/vectors/cluster/HierarchicalKMeansTests.java Added explicit range checks on assignments and replaced bare assert with assertTrue/assertNotEquals.
server/src/main/java/org/elasticsearch/index/codec/vectors/cluster/KMeansLocal.java Converted List<NeighborHood> to NeighborHood[], refactored computeNeighborhoods, updated assignSpilled signature, and removed old loop over centroids.
Comments suppressed due to low confidence (3)

server/src/test/java/org/elasticsearch/index/codec/vectors/cluster/HierarchicalKMeansTests.java:43

  • [nitpick] This assertion would benefit from a descriptive message (e.g., "assignment out of range: " + assignment) to make test failures easier to diagnose.
            assertTrue(assignment >= 0 && assignment < centroids.length);

server/src/main/java/org/elasticsearch/index/codec/vectors/cluster/KMeansLocal.java:188

  • [nitpick] The method signature was changed from returning int[] to void, but its name and any JavaDoc may not reflect that it now writes via side effects. Consider updating docs or renaming to clarify its behavior.
    private void assignSpilled(

server/src/main/java/org/elasticsearch/index/codec/vectors/cluster/KMeansLocal.java:311

  • This assertion assumes soarAssignments starts at length 0, but its initial state may differ. It's safer to explicitly clear or reassign the array rather than relying on an assert about its prior size.
            assert kMeansIntermediate.soarAssignments().length == 0;

@iverase iverase changed the title [IVF] Remove unnecessary loop over centroids and some clean up [IVF] Remove unnecessary loop and some clean up Jul 7, 2025
Copy link
Contributor

@john-wagster john-wagster left a comment

Choose a reason for hiding this comment

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

lgtm +1 to remove any List where possible

@iverase iverase merged commit 168ba07 into elastic:main Jul 7, 2025
32 checks passed
@iverase iverase deleted the removeLoop branch July 7, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>non-issue :Search Relevance/Vectors Vector search Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants