Skip to content

fix: Preserve event-time ordering within Redis online_write_batch - #6656

Merged
ntkathole merged 1 commit into
feast-dev:masterfrom
adarshsm:fix/redis-batch-event-time-ordering
Jul 30, 2026
Merged

fix: Preserve event-time ordering within Redis online_write_batch#6656
ntkathole merged 1 commit into
feast-dev:masterfrom
adarshsm:fix/redis-batch-event-time-ordering

Conversation

@adarshsm

@adarshsm adarshsm commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

online_write_batch performs its staleness check against a snapshot that is read
before any write is queued:

  1. Read phasehmget the stored event timestamp for every row's entity key, then
    pipe.execute() once.
  2. Write phase — loop the rows, skip any row whose timestamp is <= the stored one,
    otherwise queue an hset.

Because the read phase completes before the first write is queued, rows in the same batch
that share an entity key all compare against the same pre-batch value. None of them can
see the others. When nothing is stored yet, prev_event_time is None for all of them,
the guard is skipped entirely, and every row gets an hset queued. Redis applies
pipelined commands in order, so the last row in list order wins — regardless of its
event time.

Pushing one entity key with timestamps in descending order therefore leaves the oldest
value in the store:

test_data = pd.DataFrame({
    "customer_id": [1] * 3,
    "total_purchases": [30, 20, 10],
    "event_timestamp": [t3, t2, t1],   # latest first
})
store.push_features(fg_name, test_data, mode="online")
# get_online_features returns 10, the value belonging to t1

No error and no warning — just a silently incorrect feature value.

The fix

Track the latest event timestamp queued so far per entity key within the batch, and
compare each row against whichever is newer: the stored timestamp, or an earlier row of
the same batch.

latest_seen_nanos = max(prev_total_nanos, batch_latest_nanos.get(redis_key_bin, 0))
if latest_seen_nanos and new_total_nanos <= latest_seen_nanos:
    if progress:
        progress(1)
    continue
batch_latest_nanos[redis_key_bin] = new_total_nanos

redis_key_bin is already computed in the read phase and uniquely identifies
(project, entity_key), so it was free to reuse as the dedup key. The guard had to move
out of the if prev_event_time: block, with prev_total_nanos initialised to 0 so the
max() works when nothing is stored yet — that's the structural part of the diff.

online_write_batch_async had the identical defect and gets the same fix; fixing only the
sync path would have left half the bug in place.

The skip_dedup fast path is deliberately untouched. Its own comment says it is "suitable
for initial loads or append-only pipelines where out-of-order writes are not a concern," so
losing ordering there is the documented tradeoff of enabling it.

One design note worth your input

The issue suggests sorting the batch by timestamp, or reducing to one row per key. Either
would work, but both change behaviour for batches that are already correct — they alter
how many writes get queued in the ascending case and shift the progress() accounting.
I went with the running-max approach because it repairs only the broken case and leaves
every already-working case byte-identical. Happy to switch to sort-or-reduce if you'd
rather have the write-amplification reduction too; it's a small change from here.

Behaviour that is intentionally preserved:

  • Rows older than the value already stored are still skipped.
  • Ties within a batch keep the first row, matching the existing <= "older or same
    instant" semantics.
  • A stored timestamp of epoch 0 still doesn't trigger a skip — the original required a
    truthy prev_total_nanos, and max(0, ...) preserves that.

Note on the labels

The wontfix label on #5163 was applied by stale[bot] as its configured staleLabel,
not by a maintainer declining the report — the timeline shows the bot's comment as the only
activity on the issue. Flagging it so it doesn't read as a prior decision. kind/bug and
priority/p2 are the defaults from bug_report.md. If this is out of scope for other
reasons, I'm glad to hear it.

Which issue(s) this PR fixes:

Fixes #5163

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

Misc

Five tests added to sdk/python/tests/unit/infra/online_store/test_redis.py, reusing the
existing MagicMock pipeline idiom, so no Redis server or Docker is needed:

Test Before fix After fix
keeps_latest_event_time_within_batch[descending] fails passes
keeps_latest_event_time_within_batch[unordered] fails passes
keeps_latest_event_time_within_batch[ascending] passes passes
async_keeps_latest_event_time_within_batch fails passes
still_skips_rows_older_than_stored_value passes passes

The two that pass before the fix are the load-bearing ones — they're there to show the
change repairs the broken orderings without disturbing the ascending case or the existing
staleness guard.

The pre-fix failure is assert b'\x18\n' == b'\x18\x1e', i.e. int32_val=10 where 30 was
expected — the reporter's "returns 10 instead of 30", reproduced deterministically.

test_redis.py goes 18 → 23 passing. ruff check, ruff format --check and
mypy feast/infra/online_stores/redis.py are all clean.

I ran unit tests only, not the integration suite — it needs cloud credentials I don't have.
Happy to add an integration test if you'd like one for this path.

One small correction to the issue for the record: it describes the code as skipping
"records with timestamps older than what's already been processed in the current batch."
Nothing intra-batch is tracked today, which is exactly why the bug exists — but the symptom
as reported is accurate.

@adarshsm
adarshsm requested a review from a team as a code owner July 29, 2026 11:01

@franciscojavierarceo franciscojavierarceo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The per-key running maximum is applied consistently in both sync and async write paths, and the added cases cover descending, ascending, unordered, and pre-existing stored timestamps. I found no blocking issue in the remote diff.

@codecov-commenter

codecov-commenter commented Jul 30, 2026

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 75.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.06%. Comparing base (adb8c1c) to head (edc9781).

Files with missing lines Patch % Lines
sdk/python/feast/infra/online_stores/redis.py 75.00% 2 Missing and 2 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    #6656      +/-   ##
==========================================
+ Coverage   45.98%   46.06%   +0.07%     
==========================================
  Files         414      414              
  Lines       50037    50045       +8     
  Branches     7147     7147              
==========================================
+ Hits        23012    23051      +39     
+ Misses      25413    25380      -33     
- Partials     1612     1614       +2     
Flag Coverage Δ
go-feature-server 30.58% <ø> (+0.05%) ⬆️
python-unit 47.33% <75.00%> (+0.07%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/infra/online_stores/redis.py 57.75% <75.00%> (+8.84%) ⬆️

... and 1 file 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 adb8c1c...edc9781. 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.

online_write_batch reads every existing event timestamp in a single
pipeline before it queues any write, so rows in the same batch that share
an entity key all compared against the same pre-batch snapshot. None of
them could see the others, so every row passed the staleness check and
Redis applied the queued writes in list order — the last row won
regardless of its event time. A batch of [t3, t2, t1] for one entity key
left the value belonging to t1 in the store.

Track the latest event timestamp queued so far for each entity key and
compare each row against whichever is newer: the stored timestamp, or an
earlier row of the same batch. Batches that already arrived in ascending
order keep their existing behaviour, and rows older than the value
already stored are still skipped.

online_write_batch_async had the identical defect and gets the same fix.
The skip_dedup fast path is deliberately left alone, since it documents
that out-of-order writes are not a concern when it is enabled.

Fixes feast-dev#5163

Signed-off-by: adarshsm <24850536+adarshsm@users.noreply.github.com>
@ntkathole
ntkathole force-pushed the fix/redis-batch-event-time-ordering branch from a1ebc75 to edc9781 Compare July 30, 2026 05:05
@ntkathole
ntkathole merged commit 40fb788 into feast-dev:master Jul 30, 2026
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redis online store fails to maintain event-time ordering in batch writes

4 participants