feat: support search aggregation API(#3437)#3438
feat: support search aggregation API(#3437)#3438MrPresent-Han wants to merge 1 commit intomilvus-io:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MrPresent-Han 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 |
|
/hold |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3438 +/- ##
==========================================
+ Coverage 90.81% 90.92% +0.11%
==========================================
Files 64 65 +1
Lines 13969 14194 +225
==========================================
+ Hits 12686 12906 +220
- Misses 1283 1288 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MrPresent-Han
force-pushed
the
support-search-agg-master
branch
from
61cc355 to
3d997b7
Compare
MrPresent-Han
force-pushed
the
support-search-agg-master
branch
from
3d997b7 to
12ff621
Compare
|
/unhold |
MrPresent-Han
force-pushed
the
support-search-agg-master
branch
from
12ff621 to
47a6394
Compare
support search aggregation on pymilvus issue: milvus-io#3437 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: MrPresent-Han <chun.han@gmail.com>
MrPresent-Han
force-pushed
the
support-search-agg-master
branch
from
47a6394 to
7ae73d0
Compare
XuanYang-cn
left a comment
XuanYang-cn
left a comment
There was a problem hiding this comment.
Phase-1 search-aggregation API is well-scoped, mirrors issue #3437, and has strong unit coverage for request-side validation and response-side parsing. Public API changes are default-safe. Three blockers before a clean LGTM — see inline comments.
Blocking
schema_pb2.pyi— silent rename ofSearchResultData.order_by_field_values→group_by_field_valuesfrom the proto submodule bump. Undocumented breaking surface change. Call out in PR body or split proto bump into its own PR so downstream consumers aren't surprised.prepare.py—aggregation_safe_factorhas two ingestion paths (kwargs vs inside thesearch_paramsdict) that send different wire encodings and only the kwargs path is validated. The example itself uses the un-validatedsearch_paramsdict path (examples/search_aggregation.py:240). Invalid values (floats/bools/negatives) ship silently. Unify on one canonical form or validate both.search_result.py:_split_agg_buckets— never checkssum(topks) == len(parsed)and collapses multi-query responses (nq > 1) into an nq=1 shape whenagg_topksis absent. Add at least a warning + nq-aware fallback so bucket drops or shape collapses don't go silent.
Nits (inline): JSON-path rejection catches [...] only (not dotted meta.region); _MAX_NESTING_DEPTH = 4 magic; unused _oneof_value helper; bucket-key vs hit-field missing-name fallback inconsistency; docstring claims limit is ignored with no in-client warn; no end-to-end test through MilvusClient.search / AsyncMilvusClient.search / Collection.search — only Prepare is exercised.
| HIGHLIGHT_RESULTS_FIELD_NUMBER: _ClassVar[int] | ||
| ELEMENT_INDICES_FIELD_NUMBER: _ClassVar[int] | ||
| ORDER_BY_FIELD_VALUES_FIELD_NUMBER: _ClassVar[int] | ||
| GROUP_BY_FIELD_VALUES_FIELD_NUMBER: _ClassVar[int] |
There was a problem hiding this comment.
Blocking — silent breaking change. The proto submodule bump renames SearchResultData.order_by_field_values → group_by_field_values. Any downstream consumer that handcrafted against this .pyi breaks with no warning. Either call this rename out explicitly in the PR description, or split the proto bump into its own PR so it lands with its own changelog entry.
| ) | ||
| ) | ||
| search_params[AGGREGATION_SAFE_FACTOR] = aggregation_safe_factor | ||
|
|
There was a problem hiding this comment.
Blocking — dual ingestion path, one validated. aggregation_safe_factor passed as a kwarg hits this validator and becomes a top-level search_params KV. But users can also put it inside search_params["aggregation_safe_factor"] — which is what examples/search_aggregation.py:240 does — and it then flows through get_params (utils.py:278-293) into the serialized params blob, unvalidated, with a different wire shape. Invalid values (floats, negatives, bools, strings) ship silently via that path. Pick one canonical form and either reject the other or validate both.
| # If the server didn't set agg_topks (older responses or nq==1), fall | ||
| # back to putting everything under a single query. | ||
| topks = list(res.agg_topks) if len(res.agg_topks) > 0 else [len(parsed)] | ||
| out: List[List[AggregationBucket]] = [] |
There was a problem hiding this comment.
Blocking — silent bucket drops / shape collapse.
- No check that
sum(topks) == len(parsed). A buggy/future server that sendsagg_topks=[2,1]with 4 buckets silently loses the trailing bucket. - The
else [len(parsed)]fallback triggers wheneveragg_topksis empty — including annq > 1query where middleware drops the field. A 3-vector query then gets a one-element outer list shaped like nq=1.
Add at minimum a logger.warning + assert offset == len(parsed) after the loop, and an nq-aware fallback (e.g. use res.num_queries to emit the right number of empty inner lists).
|
|
||
| _VALID_METRIC_OPS = {"avg", "sum", "count", "min", "max"} | ||
| _VALID_DIRECTIONS = {"asc", "desc"} | ||
| _SPECIAL_ORDER_KEYS = {"_count", "_key"} |
There was a problem hiding this comment.
Nit — _MAX_NESTING_DEPTH = 4 is a client-side magic number. Does it mirror a server cap, a proto constraint, or is it arbitrary? Either source it from proto/config or add a one-line comment with the rationale so it's not brittle when the server limit changes.
| message=( | ||
| f"SearchAggregation.fields does not yet support JSON path " | ||
| f"expressions, got {f!r}" | ||
| ) |
There was a problem hiding this comment.
Nit — bracket-only JSON-path rejection. This catches meta['region'] and a[0] but lets meta.region through silently. Either widen the regex to cover dotted JSON paths, or narrow the error message to say "bracketed JSON path expressions not yet supported" so the rejection surface is explicit.
| ) | ||
| if self._sub_aggregation is not None: | ||
| self._sub_aggregation._check_depth(current + 1) | ||
|
|
There was a problem hiding this comment.
Nit — _oneof_value is dead code. Neither AggregationHit nor AggregationBucket calls it; each inlines its own WhichOneof loop. Either use it for consistency across the two call sites, or drop it.
| """Resolve the populated oneof branch on a proto message; returns None if none set.""" | ||
| which = msg.WhichOneof("value") if msg.DESCRIPTOR.oneofs_by_name.get("value") else None | ||
| return getattr(msg, which) if which else None | ||
|
|
There was a problem hiding this comment.
Nit — inconsistent missing-name fallback. AggregationHit falls back to str(field_id) when the proto has no field_name, but AggregationBucket.key falls back to empty string (verified in tests/test_search_aggregation.py:394 vs :450-454). Pick one behavior — the asymmetry will trip users who iterate both sides of the response.
| ids (Optional[Union[List[int], List[str], str, int]]): The ids to use for the search. | ||
| Defaults to None. | ||
| search_aggregation (SearchAggregation, optional): Hierarchical bucket aggregation spec. | ||
| Mutually exclusive with group_by_field. When set, `limit` is ignored and the root |
There was a problem hiding this comment.
Nit — docstring says "limit is ignored" when search_aggregation is set, but nothing in the client enforces or warns. A user passing both will silently have limit dropped at the server. Consider at least logger.warning when a non-default limit collides with search_aggregation, so the silent drop isn't a debugging black hole.
3 participants
support search aggregation on pymilvus
issue: #3437