Repository navigation
Conversation
With the default configuration the Milvus online store could not return most non-numeric Feast types as written. A Bool failed to read, a UnixTimestamp got no column so every write of it failed, an Array(String) failed to read, a numeric array without vector_index was declared as a vector of embedding_dim and could not be written, Bytes came back as base64 text and Json as the base64 of its protobuf. A missing value was stored as the column's default, which failed to read for a numeric text column and came back as 0 or false for a native one. Search hits decoded only primitive types, and entity columns were written as the text of their Python value. The write path now stores every scalar natively or as text, bytes as base64 and every other value as the base64 of its ValueProto, for features and entity columns alike, and one helper rebuilds the ValueProto for reads and searches from every declared field type. UnixTimestamp gets a column, only a field with vector_index=True becomes a vector, with the dimension from Field.vector_length before embedding_dim, and an empty text in a numeric or timestamp column reads as null. Array(Bool) with vector_index is rejected, because Milvus binary vectors were written unpacked and indexed with a float metric and could not work. The new milvus.nullable field tag makes a column nullable, so a missing value is stored and read as null. Signed-off-by: jiwidi <[email protected]>
5 of 7 tasks
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7119 +/- ##
==========================================
+ Coverage 51.32% 51.38% +0.06%
==========================================
Files 458 458
Lines 57441 57434 -7
Branches 8421 8414 -7
==========================================
+ Hits 29482 29513 +31
+ Misses 25950 25924 -26
+ Partials 2009 1997 -12
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Contributor
Author
|
Closing. This landed with #7120, which included this commit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Every other online store returns a feature value as it was written, whatever its type, because
Redis, Postgres, SQLite, DynamoDB, Elasticsearch, Qdrant and the rest store the serialized
ValueProto. The Milvus online store keeps one column per feature, and with the defaultconfiguration it could not return most non-numeric types. On upstream master with Milvus Lite:
BoolTypeErrorUnixTimestampArray(String)AttributeErrorvector_indexembedding_dim, so every write failsBytesJson"",0orfalse; the read of a numeric text column raisesValueErrorArray,MaporStructin a search hitBytesorUnixTimestampentity columnb'...'or a datetimeThe write path now stores every scalar natively or as text, bytes as base64, and every other
value as the base64 of its
ValueProto, for features and entity columns alike, and one helperrebuilds the
ValueProtofor reads and searches from every declared field type.UnixTimestampgets a column. Only a field withvector_index=Truebecomes a vector,with the dimension from
Field.vector_lengthbeforeembedding_dim; the Elasticsearch, Postgres,Qdrant and MongoDB stores already pick the vector field this way. An empty text in a non-string
column reads as null.
Array(Bool)withvector_index=Truenow raises at collection creation:it was mapped to a binary vector but written unpacked and indexed with a float metric, so it could
not work.
A missing value is still written as the column's default when the column cannot hold
NULL. Thenew
milvus.nullable: "true"field tag makes a column nullable, so a missing value is stored andread as null, as every other store does:
Nothing here needs a flag. Every change fixes a call that failed or returned a wrong value, the
read fixes apply to existing collections at once, and the new columns and the tag only apply when
Feast creates a collection, like the partition key.
Which issue(s) this PR fixes:
N/A
Checks
git commit -s)Testing Strategy
Unit tests
Integration tests
Manual tests
Testing is not required for this change
Unit (Milvus Lite): thirteen Feast values, empty bytes included, round-trip through
online_write_batchandonline_read, with and without native numeric types; a missing valuein a text column reads as null; a
milvus.nullablecolumn stores null and a plain column keepsits default; search hits decode every type the same way as reads;
BytesandUnixTimestampentity columns round-trip.
Unit (mocked client): a numeric array without
vector_indexis aVARCHARcolumn, avector_indexfield takes itsvector_length,UnixTimestampgets a column; the tag isrejected on entity, vector and partition key fields and for an invalid value;
Array(Bool)withvector_indexis rejected.Server: the same round-trip and the nullable column against a real server, with and without
native numeric types.
Unit tests run on Milvus Lite 3.2.1 (pymilvus 3.0.2). The server tests in
sdk/python/tests/integration/online_store/test_milvus_remote.pypass against a local Milvus3.0.2 server. The existing Milvus unit and server tests pass unchanged.
Misc
Follows #7082, #7083 and #7084 and Simon Hearne's series #6882, #6895, #6917, #6921 and #6940.
The next PR builds on this one: native ARRAY and JSON columns and scalar index tags behind
enable_openai_compatible_store.