Skip to content

fix: Raise a clear error for unresolvable FileSource file_format in DuckDB offline store - #6776

Open
pujitha24 wants to merge 1 commit into
feast-dev:masterfrom
pujitha24:auto/issue-5390
Open

fix: Raise a clear error for unresolvable FileSource file_format in DuckDB offline store#6776
pujitha24 wants to merge 1 commit into
feast-dev:masterfrom
pujitha24:auto/issue-5390

Conversation

@pujitha24

@pujitha24 pujitha24 commented Aug 24, 2026

Copy link
Copy Markdown

What this PR does / why we need it:

When using the DuckDB/ibis offline store, a FileSource whose file_format could not be
determined (not set explicitly, and the path doesn't end in .parquet, and it isn't a
Delta table) caused _read_data_source() in duckdb.py to fall through its if/elif
chain and implicitly return None. That None then propagated into ibis.py's
read_fv(), which calls .mutate(...) on it, crashing several stack frames away from the
real problem with AttributeError: 'NoneType' object has no attribute 'mutate', giving the
user no indication of the actual cause.

This PR adds an explicit else branch that raises a clear ValueError at the point where
the format could not be resolved, telling the user to either set file_format explicitly
on the FileSource or use a path with a recognized extension (e.g. .parquet).

This does not change behavior for any currently-working FileSource: feast/data_format.py
only defines ParquetFormat and DeltaFormat, both of which are already handled by the
preceding branches. It only replaces a silent None return with an actionable error.

Which issue(s) this PR fixes:

Fixes #5390

Checks

  • I've made sure the tests are passing.
  • My commits are signed off (git commit -s)
  • My PR title follows conventional commits format

Testing Strategy

  • Unit tests
  • Integration tests
  • Manual tests
  • Testing is not required for this change

Added sdk/python/tests/unit/infra/offline_stores/test_duckdb.py, which constructs a
FileSource with an unresolvable format (a .csv path, no explicit file_format) and
asserts that _read_data_source raises ValueError. Confirmed the test fails with
DID NOT RAISE <class 'ValueError'> against the code before this change, and passes after.

Ran:

  • uv run python -m pytest sdk/python/tests/unit/infra/offline_stores/ — 227 passed, 20 skipped
  • uv run ruff check / uv run ruff format --check on both changed files — clean
  • uv run mypy feast/infra/offline_stores/duckdb.py — no issues

Misc

Note: base master CI was green at the time of this PR (checked via gh run list).

fix: Raise a clear error for unresolvable FileSource file_format in DuckDB offline store

@pujitha24
pujitha24 requested a review from a team as a code owner August 24, 2026 02:36
@codecov-commenter

codecov-commenter commented Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown

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

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.62%. Comparing base (d9ea120) to head (5de9310).
❗ 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    #6776   +/-   ##
=======================================
  Coverage   47.62%   47.62%           
=======================================
  Files         422      422           
  Lines       52352    52353    +1     
  Branches     7596     7596           
=======================================
+ Hits        24931    24935    +4     
+ Misses      25646    25640    -6     
- Partials     1775     1778    +3
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 48.96% <100.00%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/infra/offline_stores/duckdb.py 38.31% <100.00%> (+1.13%) ⬆️

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 d9ea120...5de9310. 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.

@pujitha24

pujitha24 commented Sep 1, 2026

Copy link
Copy Markdown
Author

This is rebased on master and green (aside from an unrelated flaky macOS test) — happy to make any changes if something would help move review along.

@ntkathole

ntkathole commented Sep 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

@pujitha24 It would be good to validate that file_format of a data source object always has a valid value ? The fix could to validate file_format at FileSource construction time (fail-fast at definition time, not query time).
However, PR is also a reasonable improvement.

@pujitha24

pujitha24 commented Oct 1, 2026

Copy link
Copy Markdown
Author

@ntkathole A construction-time check would reject sources that work today. Outside the DuckDB store, file_format=None already means Parquet whatever the path looks like: FileSource.get_table_column_names_and_types reads it with ParquetDataset, and the Dask store's _read_datasource calls dd.read_parquet on the path. So FileSource(path="data/driver_stats/"), a directory or a path without a .parquet suffix and no file_format, is valid with those stores and would fail at definition time. Only DuckDB's _read_data_source infers Parquet from the .parquet suffix, so this PR raises the error at that point, where the format actually can't be resolved.

@pujitha24
fix: Raise a clear error for unresolvable FileSource file_format in D…
050fd19 
…uckDB offline store

Motivation:
When using the DuckDB/ibis offline store, a FileSource whose file_format
could not be determined (file_format not set and the path does not end
in ".parquet", and it is not a Delta table) caused `_read_data_source()`
in duckdb.py to fall through its if/elif chain and implicitly return
None. That None then propagated to callers that called `.mutate(...)`
on it, crashing several stack frames away from the real problem with
`AttributeError: 'NoneType' object has no attribute 'mutate'`, with no
indication of the actual cause.

Approach:
Add an explicit `else` branch to `_read_data_source()` that raises a
`ValueError` explaining that the file format could not be determined,
and suggesting the user either set `file_format` explicitly on the
FileSource (e.g. ParquetFormat()) or use a path with a recognized
extension (e.g. ".parquet"). This replaces a silent None return with
an actionable error at the point where the format could not be
resolved.

Validation:
Added sdk/python/tests/unit/infra/offline_stores/test_duckdb.py, which
constructs a FileSource with an unresolvable format (a ".csv" path,
no explicit file_format) and asserts that `_read_data_source` raises
ValueError.

Report: feast-dev#5390
Signed-off-by: Pujitha Paladugu <[email protected]>
Assisted-by: claude-sonnet-5 (via Claude Code)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error when trying to run feast with local provider and duckdb as offline store

4 participants

@pujitha24 @codecov-commenter @ntkathole @haoxu0