Skip to content

fix: compare index parameters using their dictionary representation - #3883

Open
JackyHsu-X wants to merge 2 commits into
milvus-io:masterfrom
JackyHsu-X:fix/index-param-equality
Open

JackyHsu-X wants to merge 2 commits into
milvus-io:masterfrom
JackyHsu-X:fix/index-param-equality

Conversation

@JackyHsu-X

@JackyHsu-X JackyHsu-X commented Oct 22, 2026 •

Copy link
Copy Markdown

IndexParam.__eq__ calls dict(self), but IndexParam is not a mapping or an iterable. Comparing two separately constructed index definitions, comparing against a dictionary, or comparing IndexParams lists therefore raises TypeError instead of returning a boolean.

Use the existing to_dict() representation in both supported comparison branches. It includes the field name, index name, index type and flattened index configuration. Comparisons with unrelated types retain their existing false result.

Regression coverage checks equal definitions, differences in every identity/configuration component, dictionaries in either operand order, and independently built IndexParams lists whose nested/flat inputs normalize to the same configuration.

Review follow-up (2026-10-31)

Added a regression for nlist="128" versus nlist=128, with string/string and int/int equality controls. Assertions cover object comparison in both orders, inequality, and dictionary comparisons in both directions. Production code is unchanged.

  • The three new cases on the original 98429b1 source: 3 failed (TypeError).
  • Current index-parameter and MilvusClient suites: 322 passed, one existing deprecation warning.
  • index.py has full statement coverage and 99% branch-inclusive coverage in this targeted run (one pre-existing branch unvisited).
  • Repository-wide Black (203 files), Ruff and git diff --check passed.

Full unit/server/platform tests were not rerun for this test-only follow-up. Codex prepared and ran the checks. The full-unit results below are retained historical evidence from the original submission, not a new full run.

Original submission validation

Validation on Windows / Python 3.12.8 using the frozen dev environment:

  • The final index-parameter tests on the original source: 9 failed, 11 passed; with the fix: 20 passed.
  • Index-parameter and MilvusClient tests: 319 passed, one existing deprecation warning.
  • Complete tests/unit --cov=pymilvus: 5,481 passed, 2 skipped, 4 failed, 95% statement coverage overall and 100% for milvus_client/index.py.
  • Three failures require Windows symlink privileges (WinError 1314); all three reproduce with the unchanged source. The fourth, test_upload_progress_idle_timeout_retries, is timing-sensitive: it fails with this patch and also failed on attempt 3 of isolated runs with the unchanged source. An unchanged-source run of its entire module passed all 55 tests. No unrelated tests were modified or suppressed.
  • Repository-wide Black (203 files), Ruff and git diff --check pass.

The complete supported Python/OS matrix and live Milvus/Milvus Lite integration were not run locally. No dependency, lockfile or CI changes.

AI assistance: OpenAI Codex assisted with investigation, implementation, tests and this description. The account owner authorized submission; no independent human review is claimed.

Signed-off-by: JackyHsu-X <xv1070733106@gmail.com>
@sre-ci-robot

Copy link
Copy Markdown

Welcome @JackyHsu-X! It looks like this is your first PR to milvus-io/pymilvus 🎉

@mergify

mergify Bot commented Oct 22, 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

@chrikrah chrikrah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@JackyHsu-X this looks ready to me. __eq__ has raised since #2718 removed __iter__ (v2.6.0); to_dict() is the right comparison.

$ python -m pytest tests/unit -q -n 4   # export of 1289050, CPython 3.12
1 failed, 5484 passed, 2 skipped   # test_get_commit, fails the same with index.py from 98429b1 (no .git in the export)

# pymilvus/milvus_client/index.py from 98429b1, tests kept
$ python -m pytest tests/unit/test_index_params.py -q
9 failed, 11 passed   # each: TypeError: 'IndexParam' object is not iterable

# probe at 1289050 (98429b1 raises TypeError on all four)
# params={"M": 16} vs M=16                   True
# nlist="128" vs nlist=128                   False
# index_type="" vs a dict without the key   False
# one side after get_index_configs()         True
# not run: integration tests against a server

Worth pinning the nlist string case in a test?

Signed-off-by: JackyHsu-X <xv1070733106@gmail.com>

Copy link
Copy Markdown
Author

Added a parameterized regression for nlist="128" versus nlist=128. It asserts inequality in both object operand orders and in dictionary comparisons in both directions. Same-type string/string and int/int cases are equality controls. Production code is unchanged.

The three new cases raise TypeError with the original 98429b1 implementation; with the existing fix, the index-parameter and MilvusClient suites pass 322 tests, with one existing deprecation warning. Repository-wide Black (203 files), Ruff and git diff --check pass. The full unit suite and live Milvus/Milvus Lite integrations were not rerun. Codex prepared and validated this follow-up.

@chrikrah chrikrah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@JackyHsu-X the nlist cases fail with the old index.py and pass here. Nothing blocks a merge from my side.

# git archive fc80a95, CPython 3.12, uv venv with -e . pytest pytest-asyncio; pymilvus resolves into the export
$ python -m pytest tests/unit/test_index_params.py -q -p no:cacheprovider
23 passed                    # head fc80a95
3 failed, 20 deselected      # -k nlist, milvus_client/index.py from 98429b1
# not run: the rest of tests/unit, a Milvus server

@sre-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chrikrah, JackyHsu-X
To complete the pull request process, please assign tedxu after the PR has been reviewed.
You can assign the PR to them by writing /assign @tedxu 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

Copy link
Copy Markdown
Author

/assign @tedxu

This branch has not been deployed

No deployments
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