mirror of
https://github.com/milvus-io/milvus.git
synced 2026-07-21 18:25:44 +00:00
master
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> |
||
|
|
b16c821b36 |
enhance: carry TEXT LOB refs through schema-bump full-rewrite compaction (#51125)
### What & why `bump_schema_version`'s full-rewrite path (`runFullSchemaRewrite`, reached only for a **drop-field** schema bump) builds a from-scratch output manifest via `NewBinlogRecordWriter` and did **not** carry the source segment's TEXT LOB references — leaving dangling refs for an out-of-line (`>=64KB`) TEXT column. This was the `TODO(#50021)`. ### What this does Wire **REUSE_ALL** LOB handling into `runFullSchemaRewrite`, mirroring sort compaction. Both are `1->1` with a single output manifest, so REUSE_ALL is always correct: a schema bump never changes the existing TEXT LOB data — only its references are carried. - `internal/compaction/lob_compaction.go`: `GetForcedStrategy` forces REUSE_ALL for `BumpSchemaVersionCompaction`. - `internal/datanode/compactor/bump_schema_version_compactor.go`: collect the source LOB files into a `LOBCompactionContext`, pass `storage.WithTextRefsAsBinary()`, update per-LOB-file `valid_rows` (`SetSegmentRowStats`), and merge the LOB references into the output manifest (`applyLOBCompaction`) **before** building the text-match index — `createTextIndex` reads the TEXT column through this manifest, so the carried LOB files must already be referenced (matches sort/mix ordering). Removed the `TODO(#50021)`. - Unit tests: forced strategy, init/apply LOB wiring, the applyLOBCompaction-before-createTextIndex ordering (`TestFullRewriteMergesLOBRefsBeforeBuildingTextIndex`), and a real add-nullable-TEXT + drop-field full-rewrite regression driving the actual packed writer (`TestFullRewriteFillsMissingNullableTextAsBinary`). ### Also: guard add_function_field behind StorageV3 (#51167) Adding a function to an existing collection must backfill the new output field into **pre-existing** sealed segments; that backfill runs through `bump_schema_version` compaction, which only works on a StorageV3 segment. A pre-existing V2 segment is only backfilled after the storage-version upgrade compaction rewrites it to V3, and that upgrade runs only when **both** `common.storage.useLoonFFI` and `dataCoord.compaction.storageVersion.enabled` are on. The proxy now rejects add-function unless both are enabled (`validateAddFunctionRequiresStorageV3`, wired into `addCollectionFunctionTask` and `alterCollectionSchemaTask`). `create_collection` with a function is unaffected. Full reasoning in #51167. ### Notes - Reuses the LOB machinery from **#50784**. - The `GenerateEmptyArrayFromSchema` binary-TEXT production fix landed independently on master via **#51124**; this PR keeps the regression test guarding the bump path. issue: #50021, #51167 🤖 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> |
||
|
Zhen YeandGitHub
|
b07c62d53c |
enhance: replace log package with context-aware mlog (#50094)
issue: #35917 - mlog package: move logger initialization, zap core, async buffered writes, field helpers, and scoped logger binding into pkg/mlog while removing pkg/log. - logging callsites: migrate Milvus logging usage to context-aware mlog APIs and simplify redundant With chains across components, utilities, tests, and tools. - trace propagation: replace logutil trace interceptors with mlog/tracer integration and add client_request_id fallback propagation for server stats handlers. --------- Signed-off-by: chyezh <chyezh@outlook.com> |
||
|
sijie-ni-0214andGitHub
|
ea76e2a7ec |
fix: support alter schema add request validation (#50118)
## Summary - Allow AlterCollectionSchema AddRequest to carry a field-only add request while keeping the added field as a non-function output. - Support function-only add requests against existing output fields, and validate the merged function schema through the shared function validator. - Keep the BM25 field-plus-function path explicit, reject invalid BM25 add forms, and normalize TIMESTAMPTZ default values before broadcasting field-only schema changes. issue: #50119 ## Test Plan - [x] source scripts/setenv.sh && go test -v -count=1 -tags dynamic,test -gcflags="all=-N -l" -ldflags="-r ${MILVUS_WORK_DIR}/cmake_build/lib -r ${MILVUS_WORK_DIR}/internal/core/output/lib" ./internal/proxy -run 'TestAlterCollectionSchemaTask($|_)' -timeout 300s - [x] gofumpt -l internal/proxy/function_task.go internal/proxy/function_task_test.go internal/proxy/task.go internal/proxy/task_test.go internal/proxy/util.go internal/proxy/util_test.go internal/rootcoord/ddl_callbacks_alter_collection_schema.go internal/rootcoord/ddl_callbacks_alter_collection_schema_test.go internal/util/function/validator/validator.go - [x] git diff --check upstream/master..HEAD - [ ] source scripts/setenv.sh && go test -v -count=1 -tags dynamic,test -gcflags="all=-N -l" -ldflags="-r ${MILVUS_WORK_DIR}/cmake_build/lib -r ${MILVUS_WORK_DIR}/internal/core/output/lib" ./internal/rootcoord -run 'TestDDLCallbacksBroadcastAlterCollectionSchema|TestDDLCallbacksAlterCollectionSchemaAddSkipsSchemaDropReady' -timeout 300s (blocked locally at link time by stale C++/Loon symbols such as _FreeCFieldMemSizeList and _loon_segment_writer_* in local core libraries) Signed-off-by: sijie-ni-0214 <sijie.ni@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> |
||
|
wei liuandGitHub
|
7bdd40d692 |
enhance: [ExternalTable Part10] enable function output fields on external collections (#49307)
issue: #45881 ## Summary Part 10 of the External Table series. Enables BM25 / MinHash / TextEmbedding function output fields on external collections, and makes external-table text_match work with persisted text indexes. Related: [#45881](https://github.com/milvus-io/milvus/issues/45881) (External Collection Lakehouse Integration tracking). ### What's in this PR **Schema & segcore** - Allow `Function` declarations on external schemas; function-output fields skip `external_field_mapping` validation - `Schema` tracks function-output field IDs; `ChunkedSegmentSealedImpl` and `ManifestGroupTranslator` resolve function-output columns by field name - Fast paths for retrieve / search load function-output columns by field name; external columns continue using `external_field_mapping` - `Util.cpp::GetFieldDatasFromManifest` resolves function-output columns by field name so indexbuilder reads the correct packed column **Refresh pipeline (format-agnostic via loon FFI)** 1. `CreateSegmentManifestWithBasePath` creates the input manifest that references external original files as CG0 2. `FFIPackedReader(v1)` streams input columns into `InsertData` via the shared `ArrowRecordToInsertData` helper 3. `embedding.RunAll` populates function-output fields in-place 4. `FFIPackedWriter` writes output fields as a new column group on top of v1 5. `AddStatsToManifest` registers BM25 stats into the final manifest 6. Memory peak stays around one Arrow batch, defaulting to 64 MiB **RootPath isolation** - Function-output input manifests, output manifests, BM25 stats, and text index artifacts now use the same `RootPath/insert_log/<collection>/<partition>/<segment>` layout as normal external table StorageV3 manifests - This avoids writing function-output artifacts under bucket-root `external/...` paths when multiple clusters share the same bucket **Text match** - External text fields with `enable_match` persist text index artifacts during refresh and load them through the regular external segment path - English and Chinese analyzer coverage are both included in the E2E test **Cross-bucket** - `NewPackedFFIReaderWithManifest` accepts `ExternalReaderContext` and injects per-collection `extfs.{collID}.*` aliases when `external_source` is set. Existing callers pass `{}` and are unaffected **VirtualPK** - `VirtualPKChunkedColumn` implements `GetChunk` / `GetAllChunks` so proxy requery filter-by-PK works for external collections with virtual primary keys **Shared embedding runner** - New `internal/util/function/embedding/runner.go` provides canonical `RunAll(ctx, schema, data, opts)` shared by import and external-table refresh paths - Supports BM25 output vector types including Float, BFloat16, Float16, Binary, and Sparse ### Test Plan - [x] Go unit tests for `internal/datanode/external` with 99.8% package coverage - [x] Go unit tests for changed import, packed, embedding, and schema helpers from the existing branch validation - [x] C++ rebuild + `milvus_storage` / `milvus_core` lib install from the existing branch validation - [x] E2E function-output tests in `tests/go_client/testcases/external_table_function_test.go` - `TestExternalTableBM25Function` - `TestExternalTableMinHashFunction` - `TestExternalTableTextEmbeddingFunction` - [x] `TestExternalTableTextMatch` with 10 files x 5000 rows = 50000 rows, English and Chinese text fields, persisted text index object checks in MinIO, load readiness check, and query correctness checks Signed-off-by: Wei Liu <wei.liu@zilliz.com> |
||
|
congqixiaandGitHub
|
7311d450a0 |
enhance: bump Go dependencies to v3 modules (#49485)
Related to #49398 Bump Go module references from pkg/v2 and milvus-proto/go-api/v2 to pkg/v3 and milvus-proto/go-api/v3 so the client tracks the Milvus 3.x release line. This prepares the repository for the upcoming 3.x.y release by aligning imports, module dependencies, and proto API references with the new major-version module paths. --------- Signed-off-by: Congqi Xia <congqi.xia@zilliz.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> |
||
|
junjiejiangjjjandGitHub
|
7b1efc7f63 |
fix: Fix the issue where alterFunction cannot be invoked when multiple functions become invalid (#46984)
https://github.com/milvus-io/milvus/issues/46949 Signed-off-by: junjie.jiang <junjie.jiang@zilliz.com> |
||
|
junjiejiangjjjandGitHub
|
f4e459cbf7 |
fix: Fix function edit interface bug (#46781)
https://github.com/milvus-io/milvus/issues/46780 Signed-off-by: junjie.jiang <junjie.jiang@zilliz.com> |
||
|
junjiejiangjjjandGitHub
|
d3164e8030 |
feat: add configurable batch factor and runtime check bypass for embedding functions (#45592)
https://github.com/milvus-io/milvus/issues/45544 - Add batch_factor configuration parameter (default: 5) to control embedding provider batch sizes - Add disable_func_runtime_check property to bypass function validation during collection creation - Add database interceptor support for AddCollectionFunction, AlterCollectionFunction, and DropCollectionFunction requests Signed-off-by: junjie.jiang <junjie.jiang@zilliz.com> |
||
|
junjiejiangjjjandGitHub
|
102481e53f |
feat: Support add_function/alter_function/drop_function (#44895)
https://github.com/milvus-io/milvus/issues/44053 Signed-off-by: junjie.jiang <junjie.jiang@zilliz.com> |