fix: check_invalid_binary_vector should not raise on non-sized values - #3769
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lokeshramchand-ctrl The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @lokeshramchand-ctrl! It looks like this is your first PR to milvus-io/pymilvus 🎉 |
|
/assign @XuanYang-cn -- Please look into this and let me know if any changes |
check_invalid_binary_vector called len(values) before isinstance(values,
bytes), so a BINARY_VECTOR row containing a non-bytes, non-sized element
(e.g. None) raised a raw TypeError instead of returning False. Callers in
grpc_handler.py and async_grpc_handler.py rely on False to raise a clean
ParamError("Invalid binary vector data exists"), so this crashed cleanly
malformed inserts with a confusing TypeError instead.
Swap the isinstance check ahead of the len() check. Fixes milvus-io#3768.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: lokeshramchand-ctrl <lokeshramchand@gmail.com>
yhmo pointed out that check_invalid_binary_vector still raised TypeError when the *first* element of values was non-bytes, since dim was computed from entity["values"][0] before the per-element isinstance check ran. Guard the first element before computing dim, and add regression tests for a non-sized first element and an all-non-sized list. Signed-off-by: lokeshramchand-ctrl <lokeshramchand@gmail.com>
132ee01 to
8900f65
Compare
|
Tick the box to add this pull request to the merge queue (same as
|
|
Review this and let me know if any edits required - @tedxu @XuanYang-cn |
Problem
check_invalid_binary_vectorraises an unhandledTypeErrorinstead of returningFalsewhen aBINARY_VECTORentity'svalueslist contains an element with no__len__(e.g.None, anint):The per-row loop calls
len(values)before checkingisinstance(values, bytes):Impact
This function's whole job is to return
Falseso the caller can raise a clean error.grpc_handler.py:1094(and:1228, plus the async handler) does:So instead of
ParamError("Invalid binary vector data exists"), the user gets a rawTypeErrorout of the validation helper. Reachable from a malformed binary-vector insert, e.g.Collection.insert(data=[[b"\x00\x01", None]]).This is a sibling gap to the one fixed in #3694 (
and→orfor the empty-list case), in the same function, that survived that fix. The existingtest_non_bytes_valuestest only exercises nested lists as the invalid case, which still have__len__, so it never caught a genuinely non-sized element.Filed as #3768.
Fix
Swap the two checks: verify
isinstance(values, bytes)before callinglen(values).Testing
Added
test_non_sized_valuetoTestCheckInvalidBinaryVectorintests/unit/test_utils.py, alongside the existingtest_inconsistent_dimensions/test_non_bytes_values/test_empty_valuescases.uv run pytest tests/unit/test_utils.py -q→ 71 passeduv run pytest tests/unit/ -q→ 4719 passed, 3 skipped, 1 failed (tests/unit/test_check.py::TestGetCommit::test_get_commit, a pre-existing unrelated failure — same one called out in Reject an empty binary vector list instead of raising IndexError #3694's own PR description — verified to fail identically onmasterbefore this change)TypeErrorbefore the fix and returnsFalseafterThis change was AI-assisted (Claude). I found the bug while doing a broader audit for unclaimed defects, reproduced the
TypeErrordirectly, traced the call sites ingrpc_handler.py/async_grpc_handler.pyto confirm the intended contract, and ran the full unit suite plus the baseline comparison above myself.