Skip to content

refactor: consolidate duplicated array element type mapping#3401

Open
pymilvus-bot wants to merge 1 commit intomasterfrom
refactor/consolidate-array-element-type-mapping
Open

refactor: consolidate duplicated array element type mapping#3401
pymilvus-bot wants to merge 1 commit intomasterfrom
refactor/consolidate-array-element-type-mapping

Conversation

@pymilvus-bot
Copy link
Copy Markdown
Collaborator

Summary

The DataType-to-protobuf-attribute mapping for array elements (e.g.
INT64 -> long_data, VARCHAR -> string_data) was copy-pasted across
four separate functions in entity_helper.py and search_result.py.

This PR introduces a single _ARRAY_ELEMENT_TYPE_TO_ATTR dict and
rewrites the helpers to use it:

  • Merge extract_array_row_data_with_validity / _no_validity into one
    extract_array_rows(field_data, rows, count, has_valid) function
  • Simplify extract_array_row_data (single-row variant) to a two-line
    dict lookup
  • Simplify extract_array_row_data in search_result.py to use the
    shared mapping

Net result: -86 lines, one source of truth for the mapping, easier
to maintain when new array element types are added.

Signed-off-by: pymilvus-bot pymilvus@zilliz.com

…le dict

The DataType-to-protobuf-attribute mapping for array elements (e.g.
INT64 -> long_data, VARCHAR -> string_data) was duplicated across four
functions in entity_helper.py and search_result.py.  Introduce a single
_ARRAY_ELEMENT_TYPE_TO_ATTR dict and rewrite the helpers to use it:

- Merge extract_array_row_data_with_validity / _no_validity into one
  extract_array_rows(field_data, rows, count, has_valid) function.
- Simplify extract_array_row_data (single-row) to a two-line lookup.
- Simplify extract_array_row_data in search_result.py to use the shared
  mapping via entity_helper._ARRAY_ELEMENT_TYPE_TO_ATTR.

Net result: -86 lines, one source of truth for the mapping.

Signed-off-by: pymilvus-bot <pymilvus@zilliz.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 8, 2026

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.82%. Comparing base (d4e0816) to head (528c497).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
pymilvus/client/entity_helper.py 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3401      +/-   ##
==========================================
+ Coverage   90.76%   90.82%   +0.05%     
==========================================
  Files          64       64              
  Lines       13887    13837      -50     
==========================================
- Hits        12605    12567      -38     
+ Misses       1282     1270      -12     

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

@mergify mergify Bot added the ci-passed label Apr 8, 2026
XuanYang-cn

This comment was marked as low quality.

@XuanYang-cn XuanYang-cn dismissed their stale review April 17, 2026 04:10

Reposting from bot account (pymilvus-bot) — auto-review must come from bot.

@sre-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pymilvus-bot
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

pymilvus-bot

This comment was marked as low quality.

@XuanYang-cn
Copy link
Copy Markdown
Contributor

Clean refactor — consolidating four copies of the same mapping into one dict is the right call, and replacing the list-comprehension side-effect pattern with for loops is a nice bonus.

A few things worth addressing:

1. Private symbol accessed cross-module
search_result.py references entity_helper._ARRAY_ELEMENT_TYPE_TO_ATTR. Since it's now shared across modules, consider dropping the leading underscore to make it explicitly public.

2. Constant defined below first use
_ARRAY_ELEMENT_TYPE_TO_ATTR is declared around line 1178, after the functions that reference it. It works at runtime, but the convention is to put module-level constants above their first consumer. Easy move.

3. Silent skip for unsupported types in search_result.py
The refactored loop silently produces a shorter list when attr is None and ith_array is not None (the elif just falls through). This was a pre-existing bug, not introduced here, but since we're touching this function anyway it's a good opportunity to add the same raise MilvusException(...) guard that entity_helper.py uses.

4. Docstring
The extract_array_rows docstring restates what the parameter name already communicates. Can be dropped.

None of these are blockers — the core change is correct and a clear improvement.

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.

3 participants