FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: Use UTC end bound and correct docs for disable_event_timestamp by patelchaitany · Pull Request #6956 · feast-dev/feast · GitHub

  1. feast-dev
  2. feast

Repository navigation

fix: Use UTC end bound and correct docs for disable_event_timestamp - #6956

Merged
ntkathole merged 2 commits into
feast-dev:masterfrom
patelchaitany:fix/disable-event-timestamp-docs-and-utc
Oct 9, 2026
ConversationCommits (2)ChecksFiles changed
Merged

fix: Use UTC end bound and correct docs for disable_event_timestamp#6956
ntkathole merged 2 commits into
feast-dev:masterfrom
patelchaitany:fix/disable-event-timestamp-docs-and-utc

Conversation

patelchaitany commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

disable_event_timestamp (CLI --disable-event-timestamp, FeatureStore.materialize, feature server /materialize) was documented as materializing all data "using current datetime as event timestamp". No materialization engine reads the flag, so rows always keep their source event timestamps (see #6936).

This PR:

  1. Docs: Corrects the docstrings (feature_store.py, infra/provider.py), the CLI help, README.md, the README template, and the pages under docs/. The flag now only promises full-window materialization (1970-01-01 → now), and the docs say rows keep their source event timestamps. It also drops the "useful when source data lacks event timestamps" wording, because the offline pull still requires timestamp_field.
  2. Timezone fix: The CLI and the feature server computed the end bound with naive datetime.now(). make_tzaware() then treats that value as UTC, so on hosts west of UTC the window ended hours early and the newest rows were silently skipped. Both now use datetime(1970, 1, 1, tzinfo=timezone.utc) → _utc_now().

Implementing "overwrite with current time" is intentionally out of scope. It would need changes in every engine, it could hide stale data behind a fresh timestamp, and it still would not help sources that have no timestamp column.

Which issue(s) this PR fixes:

Fixes #6936

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 test_parse_materialize_timestamps_disable_event_timestamp_uses_utc, which checks that both bounds are tz-aware UTC and that the end bound matches the real UTC now. I ran it together with the existing materialize CLI/server tests under TZ=America/Los_Angeles: 4 passed.

🤖 Generated with Claude Code

patelchaitany requested a review from a team as a code owner October 6, 2026 07:48

codecov-commenter commented Oct 6, 2026 •
edited
Loading

Copy link
Copy Markdown

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.92%. Comparing base (af20fe4) to head (16e8c4a).
⚠️ Report is 8 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 

@@           Coverage Diff           @@
##           master    #6956   +/-   ##
=======================================
  Coverage   49.92%   49.92%           
=======================================
  Files         443      443           
  Lines       55511    55504    -7     
  Branches     8096     8096           
=======================================
- Hits        27714    27712    -2     
+ Misses      25874    25869    -5     
  Partials     1923     1923
Flag Coverage Δ *Carryforward flag
go-feature-server 30.58% <ø> (ø)
python-unit 51.35% <ø> (+<0.01%) ⬆️ Carriedforward from af20fe4

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

Files with missing lines Coverage Δ
sdk/python/feast/cli/cli.py 56.43% <ø> (+0.46%) ⬆️
sdk/python/feast/feature_server.py 64.20% <ø> (+0.12%) ⬆️
sdk/python/feast/feature_store.py 46.46% <ø> (ø)
sdk/python/feast/infra/provider.py 92.30% <ø> (ø)

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 ac47f30...16e8c4a. 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.

haoxu0 approved these changes Oct 7, 2026
View reviewed changes

haoxu0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

haoxu0 commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

can we explicitly say it is utc now?

patelchaitany force-pushed the fix/disable-event-timestamp-docs-and-utc branch from 39a5451 to cbae98f Compare October 7, 2026 05:30

patelchaitany commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

can we explicitly say it is utc now?

Done, the docstrings and CLI help now say "from 1970-01-01 up to the current UTC time"

patelchaitany force-pushed the fix/disable-event-timestamp-docs-and-utc branch from cbae98f to 60a9ab4 Compare October 7, 2026 06:50
ntkathole force-pushed the fix/disable-event-timestamp-docs-and-utc branch from 60a9ab4 to dcff28c Compare October 7, 2026 11:20
patelchaitany force-pushed the fix/disable-event-timestamp-docs-and-utc branch 2 times, most recently from d1a4c2f to df9c47d Compare October 8, 2026 07:23
fix: Use UTC end bound and correct docs for disable_event_timestamp
9617d6e 
disable_event_timestamp was documented as stamping rows with the
current datetime, but no materialization engine reads the flag; rows
keep their source event timestamps. Update docstrings, CLI help and
docs so the flag only promises full-window materialization.

Also compute the full-window bounds in UTC. The CLI and feature server
used naive datetime.now(), which make_tzaware() treats as UTC, so hosts
west of UTC skipped the newest rows.

Fixes feast-dev#6936

Signed-off-by: Chaitany Patel <patelchaitany93@gmail.com>
docs: State the UTC upper bound for disable_event_timestamp
16e8c4a 
Signed-off-by: Chaitany Patel <patelchaitany93@gmail.com>
ntkathole force-pushed the fix/disable-event-timestamp-docs-and-utc branch from df9c47d to 16e8c4a Compare October 9, 2026 07:44
ntkathole merged commit cc07c82 into feast-dev:master Oct 9, 2026
21 of 30 checks passed
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ntkathole ntkathole approved these changes
haoxu0 haoxu0 approved these changes

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

disable_event_timestamp is plumbed into MaterializationTask but no materialization engine reads it

4 participants

Back | FazBrowse Home | New Git URL