Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6832 +/- ##
==========================================
+ Coverage 47.18% 47.27% +0.09%
==========================================
Files 419 419
Lines 51964 52036 +72
Branches 7548 7558 +10
==========================================
+ Hits 24521 24602 +81
+ Misses 25689 25679 -10
- Partials 1754 1755 +1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
DKilkenny
force-pushed
the
feat/postgres-batched-online-read
branch
from
September 14, 2026 15:24
eba56b7 to
a5f767c
Compare
Author
|
Rebased to pick up the CI |
ntkathole
force-pushed
the
feat/postgres-batched-online-read
branch
from
September 15, 2026 12:22
a5f767c to
f13e46a
Compare
The generic OnlineStore._read_features_per_fv loop issues one online_read per feature view, so a request spanning N feature views costs N round trips. Redis already overrides the seam to batch; Postgres now does too. Each feature view has its own table, so the per-view reads are combined with UNION ALL and split apart again by a constant tag column. The tag is what keeps them separable: the table name is not in the result set, and two feature views can legitimately return the same entity key AND the same feature name. Batching only pays where round trips dominate. Measured against Postgres 16 with a 2ms round trip, a 10-view request is ~5x faster at 1 entity and ~3x at 10, but a wash at 5000 entities, where holding every view's rows at once costs tens of MB per in-flight request. Requests above MAX_BATCHED_READ_KEYS entity keys therefore fall back to the generic per-view path, which processes and releases one view at a time. The threshold is checked from the request shape before any keys are serialized, so choosing not to batch costs nothing. Both the sync and async paths are overridden. The generic async path already runs the per-view queries concurrently via asyncio.gather, so the async win is N concurrent queries becoming one rather than a change from serial. Note that Postgres does not advertise async_supported today, so the async path is not yet reachable from the feature server. Also corrects four doc references that described Redis and Postgres as overriding get_online_features; both override _read_features_per_fv. Refs feast-dev#3259 Signed-off-by: DKilkenny <danny.kilkenny18@gmail.com>
ntkathole
force-pushed
the
feat/postgres-batched-online-read
branch
from
September 16, 2026 14:45
f13e46a to
4b02e22
Compare
Signed-off-by: DKilkenny <danny.kilkenny18@gmail.com>
Signed-off-by: DKilkenny <danny.kilkenny18@gmail.com>
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
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:
The generic
OnlineStore._read_features_per_fvloop issues oneonline_readper feature view, so a request spanning N feature views costs N round trips. Redis already overrides that seam to batch; Postgres now does too, which is the follow-up described in #3259.Postgres keeps each feature view in its own table, so the per-view reads can go out as one
UNION ALL. Each branch carries a constant tag column and the combined result is split back apart by it. The tag is doing real work here: the table name is not in the result set, and two feature views can return both the same entity key and the same feature name. Both the sync and async seams are overridden.When this helps, and when it does not. Batching trades round trips for holding more rows at once, so it only pays where round trips dominate. Measured against Postgres 16 with a 2 ms delay injected per execute to stand in for a remote database:
At 5000 entities there is no saving left, and holding every view's rows before emitting any of them took peak memory to 85 MB against a flat 9.6 MB for the generic path. That is the wrong trade for a concurrent feature server, so requests above
MAX_BATCHED_READ_KEYSentity keys fall through to the generic path, which processes and releases one view at a time. Memory at that size is now identical to before. The check reads the request shape before any keys are serialized, so declining to batch costs nothing.That 2048 threshold is my number from the measurements above, taken on a laptop against a container. Happy to move it, make it configurable, or drop it entirely if you would rather this always batch.
Two things worth flagging. Postgres does not advertise
async_supported, so_read_features_per_fv_asyncis not reachable from the feature server today; it is implemented and verified against a real database so that it behaves whenever that changes. And the docs diff is slightly wider than the feature, because four lines described Redis and Postgres as overridingget_online_featureswhen both override_read_features_per_fv. Three of those were already there, and correcting them seemed better than adding a fifth alongside.No changes to
online_store.py, so the base class contract is untouched.Which issue(s) this PR fixes:
Refs #3259
Checks
git commit -s)Testing Strategy
10 unit tests and 6 integration tests. The integration tests run the same request through the batched path and through
OnlineStore._read_features_per_fvand assert the protobuf responses are identical, including the case where an entity exists in one view but not another.I mutation tested these rather than trusting a green run. Ignoring the tag, executing the query twice, cross wiring one view's entity index mapping onto another, and disabling the size guard each fail exactly the tests meant to catch them. Two of those slipped past an earlier version of the tests, which is why the demux tests give both feature views the same feature name: with different names a crossed bucket still resolves the right value and the test proves nothing.
The
tests/unit/infrasuite is unchanged at 41 failed / 279 passed before and 41 failed / 289 passed after, same pre-existing failures plus the new tests. Those failures and the 27 collection errors are missing optional extras in my environment, not related to this change. ruff and mypy are clean.The numbers in the table above come from a manual benchmark against a Postgres 16 testcontainer, counting executes and
tracemallocpeak across view and entity counts with and without an injected per-execute delay. It is a throwaway script rather than part of the suite; happy to clean it up intosdk/python/tests/benchmarks/if that is useful to keep.One caveat on coverage: I ran all of this on Python 3.11. There is no py3.13 requirements file and the pinned
pytest-timeoutdoes not import on 3.12 or later, so 3.10 and 3.12 are unverified on my side and CI will be the first run against them.Misc
Release note: PostgreSQL online store now reads all feature views in a single query, reducing round trips for requests that span multiple feature views.