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 — feat: DuckDB historical retrieval without entity dataframe by Vperiodt · Pull Request #6108 · feast-dev/feast Conversation
|
| full_feature_names: bool = False, | ||
| **kwargs, | ||
| ) -> RetrievalJob: | ||
| start_date: Optional[datetime] = kwargs.get("start_date", None) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
instead keep it as optional params in get_historical_features
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| DEFAULT_ENTITY_DF_EVENT_TIMESTAMP_COL = "event_timestamp" | ||
|
|
||
|
|
||
| def _build_entity_df_from_sources( |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
add docstring for this
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| ) | ||
|
|
||
|
|
||
| DEFAULT_ENTITY_DF_EVENT_TIMESTAMP_COL = "event_timestamp" |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
import this from from feast.infra.offline_stores.offline_utils import DEFAULT_ENTITY_DF_EVENT_TIMESTAMP_COL
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@Vperiodt There is integration test tests/integration/offline_store/test_non_entity_mode.py, see if duckdb coverage can be included there |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| start_date = end_date - timedelta(seconds=max_ttl_seconds) | ||
| else: | ||
| start_date = end_date - timedelta(days=30) | ||
| start_date = make_tzaware(start_date) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Line 229 - Line 244 is common across the offline store to figure out the start_date & end_date. If its not too much, is it possible to create utility function which can be re-used?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yes, I’ll refactor this into a reusable utility function and apply it to similar date range calculations across the codebase in a follow-up PR
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@Vperiodt The lint checks are failing please check and rebase the PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The approach is clean — making entity_df optional and building a synthetic entity DataFrame from the feature view sources is a good way to enable date-range retrieval for DuckDB. Test coverage is solid (553 lines of unit tests + integration test updates).
However, CI is currently failing (lint-python and unit-test-python 3.10), so this is not merge-ready yet.
lint-python and unit-test-python (3.10, ubuntu-latest) are red on the latest run (Apr 13). Please investigate and fix.repo_configuration.py — breaks all tests when ibis is not installed. See inline comment for the fix._build_entity_df_from_sources (flagged by @ntkathole, still pending).compute_non_entity_date_range.Will ACK once CI is green and the blockers are addressed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| ) | ||
|
|
||
|
|
||
| def _build_entity_df_from_sources( |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Missing docstring for this function. Given the non-trivial logic around join_key_map handling, field_mapping, and timestamp filtering, a brief docstring explaining the purpose ("Build a synthetic entity DataFrame from feature view sources for non-entity retrieval mode") and the return value would help future maintainers.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| make_tzaware(end_date) if end_date else datetime.now(timezone.utc) | ||
| ) | ||
|
|
||
| if start_date is None: | ||
| max_ttl_seconds = 0 | ||
| for fv in feature_views: | ||
| if fv.ttl and isinstance(fv.ttl, timedelta): | ||
| max_ttl_seconds = max( | ||
| max_ttl_seconds, int(fv.ttl.total_seconds()) | ||
| ) | ||
| if max_ttl_seconds > 0: | ||
| start_date = end_date - timedelta(seconds=max_ttl_seconds) | ||
| else: | ||
| start_date = end_date - timedelta(days=30) | ||
| start_date = make_tzaware(start_date) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This start_date/end_date derivation logic (defaulting end_date to now, computing start_date from max TTL, falling back to 30 days) is duplicated across the DuckDB, Postgres, and File offline stores. The Postgres PR (#6057) has the same pattern inlined.
Please extract this into a shared utility (e.g. in offline_utils.py or reuse the existing compute_non_entity_date_range from feast.utils). This would keep the stores consistent and avoid divergence over time.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| "duckdb": ( | ||
| "local", | ||
| importlib.import_module( | ||
| "tests.universal.feature_repos.duckdb_repo_configuration" | ||
| ).DuckDBDataSourceCreator, | ||
| ), |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This importlib.import_module call runs at module load time, which transitively imports ibis (via duckdb_repo_configuration → feast.infra.offline_stores.duckdb). If ibis is not installed, this breaks all tests that import from repo_configuration.py — not just DuckDB-specific tests.
Other non-core offline stores (BigQuery, Redshift, Snowflake at lines 116-137) are added conditionally behind environment variable checks. Please wrap this in a try/except ImportError:
try: from tests.universal.feature_repos.duckdb_repo_configuration import DuckDBDataSourceCreator OFFLINE_STORE_TO_PROVIDER_CONFIG["duckdb"] = ("local", DuckDBDataSourceCreator) except ImportError: pass
Sorry, something went wrong.
There was an error while loading. Please reload this page.
713e92e to
7eaff77
Compare
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
f35adb9 to
da63740
Compare
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
Hi @Vperiodt @ntkathole, is this PR still being actively worked on? Happy to help pick it up |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## master #6108 +/- ## ========================================== + Coverage 46.45% 46.57% +0.11% ========================================== Files 414 414 Lines 50138 50166 +28 Branches 7173 7179 +6 ========================================== + Hits 23294 23366 +72 + Misses 25204 25154 -50 - Partials 1640 1646 +6
*This pull request uses carry forward flags. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@Vperiodt Please rebase and fix linting / tests issues |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Signed-off-by: Vanshika Vanshika <[email protected]> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
DuckDB - historical retrieval without entity dataframe
There was an error while loading. Please reload this page.