GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat: DuckDB historical retrieval without entity dataframe by Vperiodt · Pull Request #6108 · feast-dev/feast

Conversation

@Vperiodt

@Vperiodt Vperiodt commented Mar 14, 2026 •
edited by devin-ai-integration Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Adds date-range historical retrieval for the DuckDB offline store when entity_df is omitted.

Which issue(s) this PR fixes:

fixes #5832 related to #1611

Misc


Open with Devin

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Vperiodt
Vperiodt marked this pull request as ready for review March 15, 2026 18:17
@Vperiodt
Vperiodt requested a review from a team as a code owner March 15, 2026 18:17
devin-ai-integration[bot]

This comment was marked as resolved.

Sign in to view

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

devin-ai-integration[bot]

This comment was marked as resolved.

Sign in to view

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ntkathole
ntkathole reviewed Mar 18, 2026
View reviewed changes
Comment thread sdk/python/feast/infra/offline_stores/duckdb.py Outdated
full_feature_names: bool = False,
**kwargs,
) -> RetrievalJob:
start_date: Optional[datetime] = kwargs.get("start_date", None)

@ntkathole ntkathole Mar 18, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

Vperiodt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
ntkathole
ntkathole reviewed Mar 18, 2026
View reviewed changes
Comment thread sdk/python/feast/infra/offline_stores/duckdb.py Outdated
DEFAULT_ENTITY_DF_EVENT_TIMESTAMP_COL = "event_timestamp"


def _build_entity_df_from_sources(

@ntkathole ntkathole Mar 18, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

add docstring for this

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Vperiodt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
ntkathole
ntkathole reviewed Mar 18, 2026
View reviewed changes
Comment thread sdk/python/feast/infra/offline_stores/duckdb.py Outdated
)


DEFAULT_ENTITY_DF_EVENT_TIMESTAMP_COL = "event_timestamp"

@ntkathole ntkathole Mar 18, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

Vperiodt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@ntkathole

ntkathole commented Mar 18, 2026

Copy link
Copy Markdown
Member

@Vperiodt There is integration test tests/integration/offline_store/test_non_entity_mode.py, see if duckdb coverage can be included there

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aniketpalu
aniketpalu reviewed Mar 19, 2026
View reviewed changes
Comment thread sdk/python/feast/infra/offline_stores/duckdb.py Outdated
start_date = end_date - timedelta(seconds=max_ttl_seconds)
else:
start_date = end_date - timedelta(days=30)
start_date = make_tzaware(start_date)

@aniketpalu aniketpalu Mar 19, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

Vperiodt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@Vperiodt Vperiodt Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
devin-ai-integration[bot]

This comment was marked as resolved.

Sign in to view

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jyejare

jyejare commented Apr 7, 2026

Copy link
Copy Markdown
Collaborator

@Vperiodt The lint checks are failing please check and rebase the PR.

Vperiodt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jyejare
jyejare reviewed Apr 16, 2026
View reviewed changes

@jyejare jyejare left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Review Summary

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.

Blockers

  1. CI failures — lint-python and unit-test-python (3.10, ubuntu-latest) are red on the latest run (Apr 13). Please investigate and fix.
  2. Unconditional ibis import in repo_configuration.py — breaks all tests when ibis is not installed. See inline comment for the fix.

Minor Items

  1. Missing docstring on _build_entity_df_from_sources (flagged by @ntkathole, still pending).
  2. Duplicate date-range logic — the start_date/end_date defaulting is copy-pasted across offline stores. Please extract to a shared utility or reuse compute_non_entity_date_range.

Will ACK once CI is green and the blockers are addressed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread sdk/python/feast/infra/offline_stores/duckdb.py Outdated
)


def _build_entity_df_from_sources(

@jyejare jyejare Apr 16, 2026

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread sdk/python/feast/infra/offline_stores/duckdb.py Outdated
Comment on lines +229 to +243
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)

@jyejare jyejare Apr 16, 2026

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread sdk/python/tests/universal/feature_repos/repo_configuration.py Outdated
Comment on lines +94 to +99
"duckdb": (
"local",
importlib.import_module(
"tests.universal.feature_repos.duckdb_repo_configuration"
).DuckDBDataSourceCreator,
),

@jyejare jyejare Apr 16, 2026

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Vperiodt
Vperiodt force-pushed the patch-dataframe branch from 713e92e to 7eaff77 Compare April 16, 2026 14:24
Vperiodt added 11 commits April 17, 2026 17:12
@Vperiodt
support historical retrieval without entity_df
a8aea3f
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
test cases
b2bec2e
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
fix-lint
14d79e8
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
fix issue
10b7f70
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
fix rename
2d8252b
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
addressed reviews
c2a6449
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
fix-lint
04d72fd
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
fix-lint
ecb84b8
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
address-reviews
81df5a2
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
fix
ff5471d
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
fix
da63740
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
Vperiodt force-pushed the patch-dataframe branch from f35adb9 to da63740 Compare April 17, 2026 11:43
@Vperiodt
fix
9d6ddec
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@hankniel

hankniel commented Jul 4, 2026

Copy link
Copy Markdown

Hi @Vperiodt @ntkathole, is this PR still being actively worked on? Happy to help pick it up

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Vperiodt
Merge branch 'master' into patch-dataframe
585f11d
@codecov-commenter

codecov-commenter commented Jul 17, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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 78.57143% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.57%. Comparing base (c8628eb) to head (376f972).
⚠️ Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
sdk/python/feast/infra/offline_stores/duckdb.py 78.57% 3 Missing and 3 partials ⚠️
❗ 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    #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     
Flag Coverage Δ *Carryforward flag
go-feature-server 30.58% <ø> (ø) Carriedforward from b856595
python-unit 47.89% <78.57%> (+0.12%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
sdk/python/feast/infra/offline_stores/duckdb.py 42.72% <78.57%> (+3.65%) ⬆️

... 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 c8628eb...376f972. 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Vperiodt
Merge branch 'master' into patch-dataframe
9edb04c
@jyejare

jyejare commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

@Vperiodt Please rebase and fix linting / tests issues

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Vperiodt and others added 6 commits July 30, 2026 17:46
@Vperiodt
Merge branch 'master' into patch-dataframe
fca02d9
@Vperiodt
Merge branch 'master' into patch-dataframe
f8de582
@Vperiodt
Merge branch 'feast-dev:master' into patch-dataframe
6b2cc3c
@Vperiodt
fix CI
647baf9
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@Vperiodt
trigger CI
b856595
Signed-off-by: Vanshika Vanshika <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@jyejare
Merge branch 'master' into patch-dataframe
376f972

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

@jyejare jyejare jyejare left review comments
@ntkathole ntkathole ntkathole left review comments
@devin-ai-integration devin-ai-integration[bot] devin-ai-integration[bot] left review comments
+1 more reviewer
@aniketpalu aniketpalu aniketpalu left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DuckDB - historical retrieval without entity dataframe

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@Vperiodt @ntkathole @jyejare @hankniel @codecov-commenter @aniketpalu
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.