Visitar URL original
fix: Infer vector width when vector_length is undeclared by haoxu0 · Pull Request #6935 · feast-dev/feast · GitHub
Skip to content

fix: Infer vector width when vector_length is undeclared - #6935

Merged
ntkathole merged 1 commit into
feast-dev:masterfrom
haoxu0:fix/vector-length-consistency
Oct 4, 2026
Merged

ntkathole merged 1 commit into
feast-dev:masterfrom
haoxu0:fix/vector-length-consistency

Conversation

@haoxu0

@haoxu0 haoxu0 commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator

Addresses item 3 of #6907 — but not the way the issue proposed, because that way does not work.

Why "require vector_length" is off the table

The issue suggested requiring vector_length when vector_index=True. I checked the blast radius first: ~29 call sites declare vector_index=True without vector_length, including

  • Feast's own templates: feast/templates/rag/feature_repo/example_repo.py and feast/templates/ray_rag/feature_repo/feature_definitions.py — i.e. feast init -t rag would stop working
  • examples/rag/, examples/rag-docling/, examples/agent_feature_store/, the Milvus and pgvector tutorials
  • four online stores that deliberately fall back to 512: sqlite, qdrant, elasticsearch, mongodb

Making it mandatory would break the onboarding path. Not worth it.

What this does instead

The underlying complaint is still valid: vector_length defaults to 0, and 0 meant skip, so the common case was unvalidated — a correctness check that is off by default.

So rather than skipping when it is undeclared, the width of the first non-null row becomes the contract for the rest of the table. An ANN index requires a fixed width, so a ragged vector column is a defect whether or not anyone declared a length. Inferring is strictly better than skipping, and it requires nobody to change a single declaration.

case before after
vector_length declared enforced enforced (unchanged)
undeclared, uniform widths skipped passes
undeclared, ragged widths skipped rejected
undeclared, fixed_size_list skipped no check needed — uniform by construction
undeclared, all-null column skipped no-op, nothing to infer from
vector_index=False skipped skipped

The gate also moves from vector_length to vector_index, since vector_index is the attribute that actually says "this field is a vector".

Error messages distinguish the two sources, so an inferred failure is not mistaken for a declared one:

Row 2: Vector length 7 does not match expected 4, inferred from the first row
       for feature 'embedding' in feature view 'fv'.

Testing

11 new tests in test_vector_length_validation.py (32 total), covering both the Arrow and DataFrame paths: ragged rejected, uniform passes, declared still wins over inference, fixed_size_list needs no check, nulls neither infer nor compare, all-null is a no-op, non-sequence still caught, and vector_index=False untouched.

Three of them fail against master and pass here.

The important check for this PR is that inference does not break the 29 undeclared sites. I diffed failure names across the full unit suite rather than comparing counts, since the suite has flaky members:

with this change : 41 failures
master           : 41 failures
only with change : (none)

That is also the evidence that none of those sites produce ragged columns in practice.

ruff check / ruff format --check clean. mypy unchanged — same 2 pre-existing errors at feature_store.py:2123.

Relationship to the other PRs

With this, #6907 can be closed.

@codecov-commenter

codecov-commenter commented Oct 3, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.70%. Comparing base (298c3f6) to head (bb0c5a1).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
sdk/python/feast/feature_store.py 81.81% 1 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6935      +/-   ##
==========================================
+ Coverage   48.64%   48.70%   +0.05%     
==========================================
  Files         427      427              
  Lines       53864    53924      +60     
  Branches     7849     7863      +14     
==========================================
+ Hits        26203    26262      +59     
+ Misses      25792    25789       -3     
- Partials     1869     1873       +4     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 50.07% <92.30%> (+0.05%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/utils.py 81.69% <100.00%> (+0.19%) ⬆️
sdk/python/feast/feature_store.py 44.62% <81.81%> (+0.17%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1196e22...bb0c5a1. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@shuchu shuchu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Addresses item 3 of feast-dev#6907, which asked for vector_length to be required when
vector_index=True. That specific change is not viable, so this takes a
non-breaking route to the same goal.

Requiring it would break roughly 29 call sites that declare vector_index=True
without vector_length, including the bundled feast init -t rag and -t ray_rag
templates, several entries under examples/, and four online stores that
deliberately fall back to 512. The onboarding path would stop working.

The underlying problem is still real: vector_length defaults to 0 and 0 meant
skip, so the common case was unvalidated. Instead of skipping, the width of the
first non-null row now becomes the contract for the rest of the table. An
approximate nearest neighbour index requires a fixed width, so a ragged vector
column is a defect whether or not anyone declared a length, and inferring is
strictly better than skipping.

A declared vector_length still wins over inference, so nothing about the
declared case changes. Fixed-size list columns are uniform by construction, so
an undeclared one needs no check at all. Null rows carry no width and are
neither used for inference nor compared.

The gate moves from vector_length to vector_index, since vector_index is what
says the field is a vector.

Verified no regressions by diffing failure names across the full unit suite
before and after: 41 pre-existing failures on both sides, none introduced,
which also confirms none of the undeclared sites produce ragged columns.

Signed-off-by: hao-xu5 <hxu44@apple.com>
@ntkathole
ntkathole force-pushed the fix/vector-length-consistency branch from 63b2d5b to bb0c5a1 Compare October 4, 2026 13:06
@ntkathole
ntkathole merged commit 0056565 into feast-dev:master Oct 4, 2026
19 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants