fix: Raise a clear error for unresolvable FileSource file_format in DuckDB offline store - #6776
Open
pujitha24 wants to merge 1 commit into
Open
fix: Raise a clear error for unresolvable FileSource file_format in DuckDB offline store#6776pujitha24 wants to merge 1 commit into
pujitha24 wants to merge 1 commit into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
…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 <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code)
Copy link
Copy Markdown
|
⚠️ Please install the Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #6776 +/- ##
=======================================
Coverage 47.09% 47.09%
=======================================
Files 419 419
Lines 51878 51879 +1
Branches 7525 7525
=======================================
+ Hits 24430 24434 +4
+ Misses 25700 25694 -6
- Partials 1748 1751 +3
Continue to review full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
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
3 participants
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:
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
Testing Strategy
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:
Misc
Note: base master CI was green at the time of this PR (checked via gh run list).