Skip to content

fix(sparse): declare SparseFloatArray dim from the max index, not the last one - #3774

Open
sainikhiljuluri wants to merge 1 commit into
milvus-io:masterfrom
sainikhiljuluri:fix/sparse-dim-max-index
Open

fix(sparse): declare SparseFloatArray dim from the max index, not the last one#3774
sainikhiljuluri wants to merge 1 commit into
milvus-io:masterfrom
sainikhiljuluri:fix/sparse-dim-max-index

Conversation

@sainikhiljuluri

Copy link
Copy Markdown

What & why

sparse_rows_to_proto() builds the SparseFloatArray.dim incorrectly for a
dict/list sparse row whose indices are not in ascending order.

The row's indices are collected in input order (entity_helper.py:154-157),
and the byte payload is sorted before packing (sparse_float_row_to_bytes), so the
stored data is fine. But the dimension was taken from the last collected index:

if len(indices) > 0:
    row_dim = indices[-1] + 1     # last index, not the largest
dim = max(dim, row_dim)

For an unsorted row the last index is not the maximum, so dim is under-declared:

input row max index correct dim old dim
{5: 0.1, 2: 0.3} 5 6 3
[(7, .1), (1, .2), (4, .3)] 7 8 5

Milvus then receives a dim that does not cover every index present in the row.

Fix

Use max(indices) + 1 (still guarded by the existing len(indices) > 0 check) so
the declared dim always covers the largest index regardless of input order. The
scipy code paths already derive dim from shape and are untouched.

Testing

Added test_sparse_rows_to_proto_dim_uses_max_index in
tests/unit/test_client_entity_helper.py with an unsorted dict row and an unsorted
list row. It fails on the previous code (assert 3 == 6) and passes with the fix.
Full tests/unit/test_client_entity_helper.py: 139 passed. ruff check clean.

sparse_rows_to_proto() collects a dict/list sparse row's indices in input
order, then computed the array dimension as indices[-1] + 1. The byte payload
is sorted before packing, but the dim was taken from whatever index happened to
be last, so an unsorted row under-declared dim: e.g. {5: .., 2: ..} yielded
dim 3 instead of 6, and [(7,..),(1,..),(4,..)] yielded 5 instead of 8. Milvus
then sees a dim that does not cover every index in the row.

Use max(indices) + 1 (guarded by the existing len(indices) > 0 check) so the
declared dim always covers the largest index regardless of input order. The
scipy paths already derive dim from shape and are unaffected. Adds a regression
test with unsorted dict and list rows (fails on the previous code: 3 != 6).

Signed-off-by: Sainikhil Juluri <sainikhiljuluri19008@gmail.com>
@sre-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sainikhiljuluri
To complete the pull request process, please assign longjiquan after the PR has been reviewed.
You can assign the PR to them by writing /assign @longjiquan in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mergify

mergify Bot commented Aug 25, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

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