GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Vector length validation is unreachable from the batch path, skipped unless the vector is the first field, and off by default · Issue #6907 · feast-dev/feast Closed Labels DescriptionSummary
All line numbers are against 1. Validation only runs on the online-write path
So:
Embedding workloads are overwhelmingly batch. The path that actually carries vectors at volume is the unvalidated one. 2. The check assumes the vector is the first featureif feature_view.features and feature_view.features[0].vector_index: If the vector field is not at index 0, There is already a helper that does this correctly — 3.
|
| item | what | status |
|---|---|---|
| 1 | validation unreachable from the batch path | #6909 merged (29af8c0ab) |
| 2 | features[0] positional assumption |
#6909 merged |
| 4 | iterrows() did not scale |
#6909 merged (~60x on 200k rows) |
| 5 | Field.__eq__ ignores vector_index / vector_search_metric |
#6934 |
| 3 | vector_length=0 is the default and means "skip" |
#6935 |
Two corrections to what I originally wrote here, both from digging into why the code was the way it was.
Item 5 was deliberate, not an oversight. I guessed as much in the original text; 1035cd4ab from #4855 confirms it:
"Have to remove the equality test for the new fields...for now we're going to ignore them so it is backwards compatible"
The same commit changed from_proto to getattr(field_proto, 'vector_search_metric', ''), which made an unset metric deserialize as "" while the constructor default is None. With the comparison enabled, every field lacking an explicit metric compared unequal to its own deserialized copy, so every feast plan would report spurious changes. #6934 fixes that asymmetry, which removes the reason for the exclusion — rather than just uncommenting and reintroducing the churn.
Item 3 as written is not viable. I proposed requiring vector_length when vector_index=True. There are ~29 call sites that do not set it, including feast init -t rag and -t ray_rag, several examples/, and four online stores that fall back to 512. Requiring it would break the onboarding path.
#6935 takes a non-breaking route to the same goal instead: when vector_length is undeclared, infer the width from the first non-null row and enforce it across the rest of the table, rather than skipping. An ANN index needs a fixed width, so a ragged column is a defect either way, and no existing declaration has to change.
All five items are now merged.
| item | what | PR | commit |
|---|---|---|---|
| 1 | validation unreachable from the batch path | #6909 | 29af8c0ab |
| 2 | features[0] positional assumption |
#6909 | 29af8c0ab |
| 4 | iterrows() did not scale (~60x on 200k rows) |
#6909 | 29af8c0ab |
| 5 | Field.__eq__ ignored vector_index / vector_search_metric |
#6934 | 298c3f6a4 |
| 3 | vector_length=0 meant "skip" |
#6935 | 005656522 |
Two of the five did not land the way this issue originally described them, both because digging into why the code was that way changed the answer:
Item 5 was deliberate, not an oversight — 1035cd4ab from #4855 disabled the comparisons for backwards compatibility, because an unset metric deserialized as "" against a None constructor default, so every field lacking an explicit metric compared unequal to its own deserialized copy. #6934 fixed that asymmetry in Field.__init__ instead, which removed the reason for the exclusion rather than reintroducing the plan churn.
Item 3 as proposed was not viable — requiring vector_length when vector_index=True would have broken ~29 call sites including feast init -t rag and -t ray_rag. #6935 instead infers the width from the first non-null row when it is undeclared, which closes the same hole without requiring any existing declaration to change.
Closing.