mirror of
https://github.com/milvus-io/milvus.git
synced 2026-07-21 10:15:43 +00:00
5def5ced4a66c5ed7c67db938cee361f80b4244c
12
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
5def5ced4a |
enhance: enforce function-field binding principle for function DDL (#51360)
## What Enforce a function–field binding principle for function DDL (add / drop / alter function on an existing collection), so a function and its output field stay coupled and already-stored vectors can never be silently invalidated by DDL. BM25 and MinHash follow the strict binding: add/drop only via the unified `add_function_field` / `drop_function_field`, output field always coupled. **TextEmbedding is a temporary carve-out** — its legacy `AddCollectionFunction` / `DropCollectionFunction` paths are retained, so attach-over-existing-field and detach are still possible for it, because `add_function_field` embedding backfill is not yet implemented. It will be folded into the unified path in a follow-up. So the "always coupled" invariant currently holds for BM25/MinHash, not TextEmbedding. ## Changes - **AlterCollectionSchema invariant**: reject standalone add-function (a function must be added together with its new output field), reject detaching a function without dropping its output field, and reject function cascade (a new function's input being another function's output). - **Legacy RPCs**: `AddCollectionFunction` / `DropCollectionFunction` are rejected for BM25/MinHash (must use `add_function_field` / `drop_function_field`) and retained only for TextEmbedding (see carve-out). `AlterCollectionFunction` is kept and gains the whitelist below. In the alter and legacy-add paths, function field IDs are always re-derived from field names (never trusted from the request), so a request cannot inject an unrelated field ID that a later `drop_function_field` would delete. - **alter_function whitelist**: only connection/runtime params may change (TextEmbedding: `url`, `credential`, `timeout_ms`, `max_client_batch_size`, `region`, `location`, `projectid`, `user`); BM25/MinHash have no alterable params. Function identity (type, name, input/output fields) and output-shaping params (`dim`, `model_name`, `endpoint` — the TEI model identity, `normalize`, `truncate*`, prompts, ...) are immutable. Params are normalized (keys lowercased, duplicate keys rejected) so a crafted duplicate key cannot bypass the diff. - **drop_function_field**: allow any output-producing function (dropping needs no backfill), removing the previous BM25/MinHash-only restriction. ## Not in scope / follow-up - Extending `add_function_field` to TextEmbedding (post-creation add would require backfilling every existing row through the external embedding model) — deferred; folding the TextEmbedding legacy paths into the unified binding follows this. - MinHash input-field analyzer immutability lives on a different DDL path (`AlterCollectionField`); tracked as a separate follow-up. ## Breaking changes - `DropCollectionFunction` on a **MinHash** function (detach — remove the function but keep its output field) is no longer supported; use `drop_function_field` instead (drops the function together with its output field). If a released version supported MinHash detach, migrate any such call to `drop_function_field`. issue: #51348 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: MrPresent-Han <chun.han@gmail.com> Co-authored-by: MrPresent-Han <chun.han@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
e67f265a0c |
test: update python client test expectations (#50642)
Update Python client test expectations for current master behavior. - Update describe_collection expected schema metadata. - Update binary vector dimension mismatch error messages for insert/upsert. - Update drop_collection_function negative cases to assert expected errors. Tests: - Not run per request. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: nico <cheng.yuan@zilliz.com> Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
6c84618986 |
test: remove stale xfail on TextEmbedding drop-function e2e tests (#50647)
## Summary Bug #50424 — *"dropping a TextEmbedding function incorrectly removes the output vector field"* — was fixed and closed by #50471 ("fix: Split function drop semantics by request flag"). The three `drop_collection_function` e2e cases in `test_text_embedding_function_e2e.py` were still marked `@pytest.mark.xfail(strict=True)` referencing #50424. Now that the behavior is correct, the tests **pass**, and with `strict=True` pytest reports them as `XPASS(strict)` → **FAILED**, breaking `ci-v2/e2e-default` on `master` and on every open PR. ``` [XPASS(strict)] issue: .../issues/50424, dropping TextEmbedding function incorrectly removes output vector field = 3 failed, 5857 passed, 339 skipped, 2 xfailed, 1 xpassed, 9 rerun = ``` This is deterministic (not flaky): the `DataNotMatchException: Insert missed an field 'dense'` log lines are the tests' own expected negative-path assertions (after detaching the function, inserting without the vector must fail). ## Changes - Remove the three `@pytest.mark.xfail(...)` decorators from: - `TestTextEmbeddingFunctionCURD::test_drop_collection_function_verify_crud` - `TestTextEmbeddingFunctionCURD::test_drop_collection_function_one_of_multiple` - `TestTextEmbeddingFunctionCURD::test_drop_collection_function_then_add_again` - Remove the now-unused `DROP_TEXT_EMBEDDING_FUNCTION_XFAIL_REASON` constant. The tests now run as normal positive cases validating the fixed drop-function semantics from #50471. issue: #50646 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: xiaofanluan <xf@hjjaq.com> Co-authored-by: xiaofanluan <xf@hjjaq.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
zhuwenxingandGitHub
|
b3fe69d844 |
test: add struct array nullable dynamic field coverage (#50109)
### What this PR does Adds Python client and REST coverage for struct array nullable fields, including dynamic field scenarios and nullable vector config variants. ### Tests Not run; PR creation only. --------- Signed-off-by: zhuwenxing <wenxing.zhu@zilliz.com> |
||
|
yanliang567andGitHub
|
d2bf17e734 |
test: Add external table add field E2E tests (#50420)
Related to #50416 ### What changed - Add MilvusClient E2E coverage for external table `add_collection_field` on scalar and vector fields. - Cover refresh/load/search/query behavior after adding fields, including about 10% NULL values in newly added scalar/vector fields. - Add non-Parquet source coverage for add scalar field on Lance, Iceberg, and Vortex. - Add full supported DataType matrix coverage for external add field, including scalar, array, geometry, and vector fields. - Add negative coverage for missing/duplicate `external_field`, type/dim mismatch, unsupported public data types, add/drop function, and drop field APIs. - Update Python client test dependency pin to `pymilvus==3.1.0rc35`. ### Verification - `python -m py_compile tests/python_client/milvus_client/test_milvus_client_external_table.py tests/python_client/common/external_table_common.py tests/python_client/base/client_v2_base.py tests/python_client/check/func_check.py` - `python -m ruff check tests/python_client/milvus_client/test_milvus_client_external_table.py tests/python_client/common/external_table_common.py tests/python_client/base/client_v2_base.py tests/python_client/check/func_check.py` - `python -m pytest -n 4 milvus_client/test_milvus_client_external_table.py --host 10.104.18.101 --port 19530 --minio_host 10.104.18.27 --minio_bucket yanliang-mas2 --tb=short -q -s` Result: ```text 182 passed, 1 xfailed in 318.94s (0:05:18) ``` The xfailed case is linked to #50416 and validates that old-field search still works before the server returns the current internal QueryNode assert for newly added fields before refresh. --------- Signed-off-by: Yanliang Qiao <yanliang.qiao@zilliz.com> |
||
|
|
e2787d3981 |
enhance: standardize error handling on merr + Sys/Input classification (#50221)
issue: #47420 ## What this PR does Project-wide migration of raw `fmt.Errorf` / `errors.New` in function bodies onto the `merr` framework, plus the Sys-vs-Input error classification and the machinery it drives (retriability, fine-grained metrics, segcore unification), plus the convention docs and a linter that keeps it from regressing. Scope: storage, proxy, coordinators (root/data/query), query node, data node, `pkg/util` & `internal/util`, expression parser, message queue, streaming, and misc packages. Bare raw-error usages went from ~3000 to a ~340 allowlist (package-level sentinels / build-tag / test sites). --- ## How to review this PR It is large but the vast majority is mechanical. Changes fall into three tiers; spend review budget on Part 2 and Part 3. ### Part 1 — Mechanical standardization (low risk, verify by rule) Each converted call follows one of a small fixed set of rules. To review, check that each site obeys the matching rule rather than reading every line: | Pattern | Rule | |---|---| | `fmt.Errorf("...")` originating a new error | → `merr.WrapErrXxxMsg("...")` with a code matching the failure's meaning | | Adding context to an existing typed error | → `merr.Wrap(err, "...")` / `merr.Wrapf(...)` — **preserves** the inner code (never `WrapErr*Err`, which overwrites it) | | Errors inside the streaming subsystem | → `status.New*` factories (StreamingError), **not** merr — this is the component-internal dialect (see `docs/dev/error_handling_guide.md`) | | Low-level / control-flow signal caught by `errors.Is` | → kept as a package-level `errors.New` sentinel (lowercase, same-package) | Conventions are documented in `docs/dev/error_handling_guide.md` (how-to) and `docs/dev/error_sentinel_convention.md` (rules + audit). A `gocritic`/`ruleguard` rule (`rawmerrerror`, in `rules.go`) enforces "no raw `return errors.New/fmt.Errorf`" under `make verifiers`. ### Part 2 — Behavior changes (review these closely) These are the sites where the wire contract or runtime behavior changes, not just the source text. Listed by category; representative locations given, full set in the diff. **A. gRPC wire-code shifts: `UnexpectedError(1)/Code 65535` → typed code.** Where a handler previously returned a raw error (collapsed to `Code=65535` on the wire), it now returns a typed merr, so the client sees a real code. The most common shift is to `IllegalArgument(5)/Code 1100` (ParameterInvalid). Touch points include datanode task handlers (CreateTask/Query/Drop), proxy Upsert, querynode GetMetrics, datacoord CreateIndex, httpserver query-response builder, and typeutil schema validation. One code refinement: an index-param validation moved `1100` → `1101` (ParameterMissing). **Client/SDK assertions and any code that switched on `Code=65535` for these paths must be re-checked** (the go_client e2e assertions were already aligned in this PR). **B. Prometheus `status` label contract change (externally visible).** The proxy metric's coarse `fail` / `rejected` values are split into `fail_input` / `fail_system` and `rejected_user` / `rejected_system` (in `requestutil.ParseMetricLabel`; auth/privilege rejections count as `rejected_user`), so dashboards can attribute a failure to caller vs operator. **Dashboards/alerts querying `status="fail"` must migrate to `status=~"fail_.*"`, and `status="rejected"` to `status=~"rejected_.*"`.** The in-repo Grafana dashboard is already migrated; external dashboards built on the old values silently go empty after upgrade. This is the one change that requires an ops-side migration. **C. Retriability semantics.** - C1: `merr.Status(err)` now forces `Retriable=false` when the error is an `InputError` — a malformed request can never succeed on blind retry, so clients never get the self-contradictory "your input is wrong but you may retry". - C2: `retry.Do` short-circuits an `InputError` (non-retriable) — **but only when the caller did not pass a `RetryErr` predicate**. The check is an `if c.isRetryErr != nil { ... } else if InputError { ... }` *mutually exclusive* branch (`pkg/util/retry/retry.go`): an explicit `RetryErr` takes precedence and bypasses the InputError abort. `retry.Handle` deliberately does **not** apply the InputError abort (its callers signal abort via `shouldRetry=false`). Four flusher startup callsites that must retry through transient "not ready" errors were given explicit `RetryErr` escape hatches. **D. segcore (C++→Go) error classification.** A single shared Go-side table (`pkg/util/merr/segcore.go`) maps each segcore code to a merr sentinel + InputError/signal category, replacing scattered hand-written `if errorCode == ...` switches in the cgo wrappers. **Wire `Code` values change for every segcore pass-through error, not just the remapped ones.** Named sentinels remap (C++ `2003` → merr `2001`, `2033` → `2002`, Folly/Knowhere codes likewise); **all remaining pass-through codes (`2004`–`2043`, previously surfaced to clients as raw C++ enum values) now serialize as `2000`** (`ErrSegcore`), with the original C++ code preserved in the `Reason` text (`segcoreCode=...`); unknown/future codes collapse to `2000` as well (pinned by the `wire_code_projection` test). Transient segcore classes (object storage / file IO / OOM / mmap / FieldNotLoaded — 11 codes) now report `Retriable=true`. **Any client switching on raw segcore codes in the `2004`–`2043` range must be re-checked**; the in-Reason code remains available for diagnostics. Signal codes (PretendFinished / FollyCancel) are recognized centrally. `errors.Is`-based control flow on these (e.g. scheduler skip/retry) is preserved. **E. InputError classification (25 sentinels + dynamic marks).** 25 sentinels in `errors.go` carry `WithErrorType(InputError)` (the Collection / ResourceGroup / Database families, `ErrIndexDuplicate`, `ErrParameterInvalid`, `ErrPrivilegeNotAuthenticated`, `ErrImportFailed`, `ErrQueryPlan`, ...), plus dynamic marks for the 8 segcore input codes (ExprInvalid, DimNotMatch, MetricTypeInvalid, FieldIDInvalid, ...) and `WrapErrAsInputError`. The widest blast radius is `ErrParameterInvalid` (1100): ~2335 `WrapErrParameterInvalid*` callsites now classify as input / non-retriable. Because of C1/C2 this changes retriability for any path that returns these. **The audit to confirm no transient path was mis-marked is the single most important review item** (see Part 3). One reverse correction: storage field-stats parsing moved from `ErrParameterInvalid` (input) to `ErrDataIntegrity` — a corrupted stored stat is data corruption, not user input. ### Part 3 — Known risks & traps (called out proactively) 1. **`merr.Wrap` vs `WrapErr*Err` (code-masking).** `WrapErr*Err` builds a `wrappedMilvusError{sentinel: ErrServiceInternal}` whose `code()` returns the *outer* sentinel — it overwrites the inner typed code and hides the `errors.Is` chain. This is intentional (use it to *deliberately* downgrade), but it was a recurring conversion defect; the rule "add context with `merr.Wrap`, downgrade with `WrapErr*Err`" is enforced by convention and reviewed across the diff. 2. **InputError × `retry.Do` blast radius.** Marking a sentinel `InputError` makes any `retry.Do(...)` without a `RetryErr` predicate stop retrying it. Reviewers should sanity-check that no transient use of the 19 newly-marked sentinels (especially `ErrParameterInvalid`) sits inside a retry loop that needed to keep spinning. The known flusher cases were handled (see C2). 3. **The ~340 raw-error allowlist.** What remains as bare `errors.New` is, by design: package-level sentinels (caught by `errors.Is`), `//go:build test` sites, and out-of-band trees (`cmd/`, `tests/`, codegen, walimpls). The linter only bans the *direct-return* form; assignment-then-return escapes and the full no-exceptions ban are deferred to an AST-based linter (Tier 2, documented). 4. **segcore C++ second step deferred.** This PR unifies classification on the Go side; splitting the dual-semantic C++ codes at the source is a follow-up. --- ## Validation - `make verifiers`: Go side clean (gofmt + static-check across modules, including the new `rawmerrerror` rule with a 0-hit baseline repo-wide). - `make test-go`: passing; the one real regression introduced (a datanode `invalid_task_type` assertion shifting `1` → `5` from a ParameterInvalid conversion) was fixed in-tree. - go_client e2e CreateIndex assertions aligned to the new merr messages. --------- Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
|
1cd722fe86 |
test: update Python client nightly expectations (#49642)
## Summary - Align Python client negative test expectations with updated server error messages for hybrid search, search filter, sparse index validation, query_mode validation, and TEI function alteration. - Avoid the known duplicate-PK growing-segment issue in the dynamic-field insert test while preserving coverage for inserting different dynamic fields. ## Related issues - #49341 - #49190 - #48916 - #48666 - #48725 - #49191 ## Test plan - [x] `test_milvus_client_hybrid_search.py::TestMilvusClientHybridSearch::test_hybrid_search_RRFRanker_k_out_of_range` against `--host 10.104.22.80`: 2 passed - [x] `test_milvus_client_search.py::TestSearchInvalidShared::test_search_param_invalid_expr_type` against `--host 10.104.22.80`: 2 passed - [x] `test_milvus_client_insert.py::TestMilvusClientInsertValid::test_milvus_client_insert_different_fields` against `--host 10.104.22.80`: 1 passed - [x] `test_text_embedding_function_e2e.py::TestTextEmbeddingFunctionCURDNegative::test_alter_collection_function_invalid_new_endpoint` against `--host 10.104.17.21`: 1 passed - [x] `test_index.py::TestIndexInvalid::test_invalid_sparse_inverted_index_algo --collect-only`: 2 tests collected - [x] `test_large_topk.py::TestLargeTopkIndependent::{test_query_mode_value_case_insensitive,test_query_mode_key_case_sensitive} --collect-only`: 6 tests collected 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: nico <cheng.yuan@zilliz.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
|
87b04e8a63 |
fix: remove golangci-lint v2 exclusion rules and fix ~1500 lint violations (#48586)
## Summary Remove all temporary exclusion rules added during the golangci-lint v1→v2 upgrade (PR #48286), fixing ~1500 lint violations: - **QF series (~1080)**: auto-fixed with `--fix` (embedded field selectors, if/else→switch, De Morgan's law) - **ST1005 (148)**: lowercase error strings per Go conventions - **gosec (116)**: fix G118 context cancel leaks, G306 file permissions; nolint for G602/G120/G705 false positives - **revive (41)**: `Json`→`JSON`, `Url`→`URL` naming conventions - **ineffassign (14)**: remove unused assignments - **unconvert (13)**: remove unnecessary type conversions - **depguard (9)**: replace banned `errors` import with `cockroachdb/errors` - **gocritic (14)**: fix ruleguard violations - **Other staticcheck (12)**: S1034, S1008, SA3001 etc. Kept disabled: govet `printf` analyzer (known Go 1.25 + golangci-lint v2 panic bug) issue: #48574 pr: #48286 ## Test plan - [x] `golangci-lint run` passes with 0 issues locally - [ ] CI passes 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Li Liu <li.liu@zilliz.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
zhuwenxingandGitHub
|
fc11816690 |
test: add test cases for alter function with invalid mock (#46995)
## Summary - Add mock TEI server utility (`common/mock_tei_server.py`) for testing text embedding functions - Add test cases for PR #46984 fix (alter function when other function is invalid) ## Test Cases 1. `test_alter_function_when_other_function_is_invalid` - Verify that altering a valid function succeeds even when another function in the collection is invalid 2. `test_alter_invalid_function_to_valid_endpoint` - Verify that users can fix an invalid function by altering it to a valid endpoint ## Related Issues - Issue: https://github.com/milvus-io/milvus/issues/46949 - Fix PR: https://github.com/milvus-io/milvus/pull/46984 ## Test Plan - [x] Verified test fails with unfixed Milvus (`master-20260112-7bcd3b10`) - [x] Verified test passes with fixed Milvus (`master-20260112-b39ecb0`) --------- Signed-off-by: zhuwenxing <wenxing.zhu@zilliz.com> |
||
|
zhuwenxingandGitHub
|
f9ff0e8402 |
test: add testcases for add/alter/drop text embedding function (#46229)
/kind improvement Signed-off-by: zhuwenxing <wenxing.zhu@zilliz.com> |
||
|
junjiejiangjjjandGitHub
|
fe81c7baae |
feat: Add function config (#40534)
#35856 1. Add function-related configuration in milvus.yaml 2. Add null and empty value check to TextEmbeddingFunction Signed-off-by: junjie.jiang <junjie.jiang@zilliz.com> |
||
|
zhuwenxingandGitHub
|
6889b0470e |
test: add text embedding function test (#40668)
/kind improvement --------- Signed-off-by: zhuwenxing <wenxing.zhu@zilliz.com> |