fix: Fix five bugs in milvus online store by korbonits · Pull Request #6275 · feast-dev/feast

korbonits · 2026-04-14T06:53:34Z

What does this PR do?

Fixes five bugs in sdk/python/feast/infra/online_stores/milvus_online_store/milvus.py and adds regression tests for three of them.

1. `update()` overwrites the collection cache (`_collections`) instead of updating it

_get_or_create_collection() already updates self._collections[collection_name] as a side effect and returns that entry. The assignment in update() replaced the entire _collections dict with the raw describe_collection() dict for the last table, corrupting all subsequent cache lookups for any other collection.

Fix: drop the assignment; the side effect in _get_or_create_collection() is sufficient.

2. `plan()` raises `NotImplementedError` instead of returning `[]`

The OnlineStore base class default is return []. Raising NotImplementedError caused feast plan to crash for any project using the Milvus store.

3. `online_read()` has an extra `full_feature_names` parameter not in the base class

The parameter was unused in the method body and violates the OnlineStore interface contract (online_read takes requested_features as the last argument).

4. `retrieve_online_documents_v2()` crashes with `TypeError` when the composite key is absent from a hit

# before — bytes.fromhex(None) raises TypeError
entity_key_bytes = bytes.fromhex(
    hit.get("entity", {}).get(composite_key_name, None)
)

# after
raw_key = hit.get("entity", {}).get(composite_key_name)
entity_key_bytes = bytes.fromhex(raw_key) if raw_key else None

5. `_connect()` uses `print()` instead of `logging`

Connection messages now use logger.info() so they respect the application's logging configuration instead of unconditionally writing to stdout.

Tests

Three regression tests added in tests/unit/online_store/test_online_retrieval.py:

Test	Bug covered
`test_milvus_update_preserves_collection_cache`	`_collections` cache corruption in `update()`
`test_milvus_plan_returns_empty_list`	`plan()` raising `NotImplementedError`
`test_milvus_retrieve_online_documents_v2_missing_entity_key`	`TypeError` from `bytes.fromhex(None)`

Full Milvus unit test results:

tests/unit/online_store/test_online_retrieval.py -k milvus
→ 8 passed, 1 skipped  (7.90s)

ruff check passes with no errors.

🤖 Generated with Claude Code

Add missing RST files for feast.aggregation (including tiling sub-package), feast.dbt, and feast.openlineage. Also add feast.diff.apply_progress which was missing from feast.diff.rst. Update feast.rst toctree to include all three new top-level packages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>

No new warning categories are introduced by adding feast.aggregation, feast.dbt, and feast.openlineage to the Sphinx docs. The 10 new "more than one target found" warnings follow the same pre-existing pattern (8 already present) caused by feast.__init__ re-exporting classes under multiple paths. Fixes applied: - Remove :undoc-members: from dataclass automodules (Aggregation, IRMetadata, OpenLineageConfig) to prevent duplicate attribute docs - Document re-exporting __init__ packages at package level only to avoid duplicate descriptions from submodule entries - Fix markdown-style code fences in openlineage/__init__.py docstring - Fix RST formatting in client.py Example section (Example::) and emitter.py ASCII art diagram (literal block) - Fix repo_config.py field docstring that triggered an ambiguous OpenLineageConfig cross-reference Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>

1. update() overwrote self._collections with the raw describe_collection() dict from the last table instead of updating the keyed cache entry. _get_or_create_collection() already updates self._collections as a side effect, so the assignment is simply dropped. 2. plan() raised NotImplementedError instead of returning [] like the base class default. This caused `feast plan` to crash for Milvus stores. 3. online_read() carried an extra full_feature_names parameter not present in the OnlineStore base class, violating the interface contract. The parameter was unused and is removed. 4. retrieve_online_documents_v2() passed the raw hit.get() result (which can be None when the composite key is absent) directly to bytes.fromhex(), raising TypeError. Guard added: only call bytes.fromhex() when the value is non-None. 5. Replace print() calls in _connect() with logger.info() so connection messages respect standard logging configuration instead of always writing to stdout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>

devin-ai-integration

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional findings.

1. test_milvus_update_preserves_collection_cache — verifies that applying two FeatureViews leaves self._collections as a proper dict keyed by collection name, with a "fields" entry for each view. Previously, update() overwrote _collections with the raw describe_collection() dict from the last table. 2. test_milvus_plan_returns_empty_list — verifies that plan() returns [] instead of raising NotImplementedError, matching the OnlineStore base class default and allowing `feast plan` to work. 3. test_milvus_retrieve_online_documents_v2_missing_entity_key — verifies that a search hit missing the composite primary key produces a None entity_key_proto instead of a TypeError from bytes.fromhex(None). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>

korbonits · 2026-04-15T05:38:16Z

Hmm, looks like the unit test error for python 3.12 ubuntu was:

________________ tests/unit/local_feast_tests/test_e2e_local.py ________________
[gw6] linux -- Python 3.12.13 /home/runner/work/feast/feast/.venv/bin/python3
worker 'gw6' crashed while running 'tests/unit/local_feast_tests/test_e2e_local.py::test_e2e_local'
=============================== warnings summary ==============================

Maybe re-running would resolve?

devin-ai-integration

Devin Review found 1 new potential issue.

View 6 additional findings in Devin Review.

devin-ai-integration · 2026-04-15T20:14:28Z

🟡 Falsy check on distance drops valid 0.0 distance values

At line 746, if distance is used to decide whether to wrap the distance in a ValueProto(float_val=...). When distance is 0.0 (a valid distance meaning an exact match for L2 or cosine metrics), Python evaluates 0.0 as falsy, so the code returns an empty ValueProto() instead of ValueProto(float_val=0.0). Because float_val is inside a oneof val in the proto definition (protos/feast/types/Value.proto:78), ValueProto() leaves the oneof unset (WhichOneof("val") → None), while ValueProto(float_val=0.0) correctly sets it to "float_val". This is the same bug pattern the PR fixes for raw_key at line 694. The postgres online store handles this correctly with if distance is not None: (sdk/python/feast/infra/online_stores/postgres_online_store/postgres.py:777).

(Refers to line 746)

Was this helpful? React with 👍 or 👎 to provide feedback.

korbonits and others added 3 commits April 13, 2026 15:57

korbonits requested a review from a team as a code owner April 14, 2026 06:53

devin-ai-integration bot reviewed Apr 14, 2026

View reviewed changes

korbonits changed the title ~~fix: fix five bugs in MilvusOnlineStore~~ fix: Fix five bugs in milvus online store Apr 14, 2026

fix: remove unused variable and import in milvus regression test

b104a2a

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>

franciscojavierarceo approved these changes Apr 15, 2026

View reviewed changes

Merge branch 'master' into fix/milvus-online-store-bugs

e84fe3c

ntkathole added the ok-to-test label Apr 15, 2026

franciscojavierarceo enabled auto-merge (squash) April 15, 2026 14:23

Merge branch 'master' into fix/milvus-online-store-bugs

90613b0

franciscojavierarceo merged commit 212504b into feast-dev:master Apr 15, 2026
1 of 2 checks passed

devin-ai-integration bot reviewed Apr 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix five bugs in milvus online store#6275

fix: Fix five bugs in milvus online store#6275
franciscojavierarceo merged 7 commits intofeast-dev:masterfrom
korbonits:fix/milvus-online-store-bugs

korbonits commented Apr 14, 2026 •

edited

Loading

Uh oh!

devin-ai-integration bot left a comment

Uh oh!

korbonits commented Apr 15, 2026

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Uh oh!

devin-ai-integration bot Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

korbonits commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

1. update() overwrites the collection cache (_collections) instead of updating it

2. plan() raises NotImplementedError instead of returning []

3. online_read() has an extra full_feature_names parameter not in the base class

4. retrieve_online_documents_v2() crashes with TypeError when the composite key is absent from a hit

5. _connect() uses print() instead of logging

Tests

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

✅ Devin Review: No Issues Found

Uh oh!

korbonits commented Apr 15, 2026

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

korbonits commented Apr 14, 2026 •

edited

Loading

1. `update()` overwrites the collection cache (`_collections`) instead of updating it

2. `plan()` raises `NotImplementedError` instead of returning `[]`

3. `online_read()` has an extra `full_feature_names` parameter not in the base class

4. `retrieve_online_documents_v2()` crashes with `TypeError` when the composite key is absent from a hit

5. `_connect()` uses `print()` instead of `logging`