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
…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)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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:
|
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
FileSourcewhosefile_formatcould not bedetermined (not set explicitly, and the path doesn't end in
.parquet, and it isn't aDelta table) caused
_read_data_source()induckdb.pyto fall through itsif/elifchain and implicitly return
None. ThatNonethen propagated intoibis.py'sread_fv(), which calls.mutate(...)on it, crashing several stack frames away from thereal problem with
AttributeError: 'NoneType' object has no attribute 'mutate', giving theuser no indication of the actual cause.
This PR adds an explicit
elsebranch that raises a clearValueErrorat the point wherethe format could not be resolved, telling the user to either set
file_formatexplicitlyon the
FileSourceor use a path with a recognized extension (e.g..parquet).This does not change behavior for any currently-working
FileSource:feast/data_format.pyonly defines
ParquetFormatandDeltaFormat, both of which are already handled by thepreceding branches. It only replaces a silent
Nonereturn with an actionable error.Which issue(s) this PR fixes:
Fixes #5390
Checks
git commit -s)Testing Strategy
Added
sdk/python/tests/unit/infra/offline_stores/test_duckdb.py, which constructs aFileSourcewith an unresolvable format (a.csvpath, no explicitfile_format) andasserts that
_read_data_sourceraisesValueError. Confirmed the test fails withDID 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 skippeduv run ruff check/uv run ruff format --checkon both changed files — cleanuv run mypy feast/infra/offline_stores/duckdb.py— no issuesMisc
Note: base
masterCI was green at the time of this PR (checked viagh run list).