镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

fix: Keep entity rows apart in the ibis point-in-time join - #6884

Merged
ntkathole merged 1 commit into
feast-dev:masterfrom
MohammadHijjawi97:fix/ibis-entity-row-id
Sep 29, 2026
Merged

ntkathole merged 1 commit into
feast-dev:masterfrom
MohammadHijjawi97:fix/ibis-entity-row-id

Conversation

@MohammadHijjawi97

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

The ibis point-in-time join (used by the DuckDB, MSSQL and Oracle offline stores) identifies each entity row by concatenating the string values of its join keys and event timestamp into entity_row_id, then deduplicates and joins feature rows back on that id. Two problems with plain concatenation:

  1. Different entity rows can get the same id, e.g. driver_id="1", customer_id="11" and driver_id="11", customer_id="1" at the same timestamp both become "_111...", so one row silently receives the other row's feature values (whether it happens depends on the column order, which comes from a set).
  2. A null key makes the whole id null, so the row loses the features of every feature view, not just the one keyed on the null column.

Repro with point_in_time_join (drivers FV keyed on driver_id, customers FV keyed on customer_id):

Before:
    driver_id customer_id  driver_rating  customer_score
            1          11            1.0           110.0
           11           1            1.0           110.0   <- driver 1's values
            7        None            NaN             NaN   <- driver_rating lost
After:
            1          11            1.0           110.0
           11           1           11.0            10.0
            7        None            7.0             NaN

This length-prefixes each value and gives nulls their own marker, so the id is only shared by rows with equal values. Both places that build the id now use the same helper.

Which issue(s) this PR fixes:

No existing issue.

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

Added test_point_in_time_join_keeps_entity_rows_apart to sdk/python/tests/unit/infra/offline_stores/test_ibis.py; it fails on master (deterministically, through the null-key case) and passes with this change. tests/unit/infra/offline_stores/test_filter_by_created_timestamp.py (the other caller of point_in_time_join) gives the same results as on master. ruff and mypy on ibis.py are clean. The id format is internal and dropped before results are returned.

@MohammadHijjawi97
MohammadHijjawi97 requested a review from a team as a code owner September 28, 2026 13:28
@codecov-commenter

codecov-commenter commented Sep 28, 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 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 47.84%. Comparing base (ce37029) to head (881cc39).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
sdk/python/feast/infra/offline_stores/ibis.py 88.88% 1 Missing ⚠️
❗ 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    #6884   +/-   ##
=======================================
  Coverage   47.83%   47.84%           
=======================================
  Files         422      422           
  Lines       52727    52727           
  Branches     7663     7662    -1     
=======================================
+ Hits        25223    25225    +2     
+ Misses      25683    25680    -3     
- Partials     1821     1822    +1     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 49.18% <88.88%> (+<0.01%) ⬆️
Files with missing lines Coverage Δ
sdk/python/feast/infra/offline_stores/ibis.py 41.62% <88.88%> (+1.43%) ⬆️

... 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 ce37029...881cc39. 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.

The ibis point-in-time join (used by the DuckDB, MSSQL and Oracle offline
stores) identifies each entity row by concatenating the string values of
its join keys and event timestamp. Different rows could produce the same
id, e.g. driver_id="1", customer_id="11" and driver_id="11",
customer_id="1", so one row got the other's feature values. A null key
also made the whole id null, so the row lost the features of every other
feature view, not just the one keyed on the null column.

Length-prefix each value and give nulls their own marker so the id is only
shared by rows with equal values.

Signed-off-by: Mohammad Hijjawi <mohammad.hijjawi1997@gmail.com>
@ntkathole
ntkathole force-pushed the fix/ibis-entity-row-id branch from 412adf3 to 881cc39 Compare September 29, 2026 05:56
@ntkathole
ntkathole merged commit a87c070 into feast-dev:master Sep 29, 2026
21 of 23 checks passed
ntkathole pushed a commit to sohailshk/feast that referenced this pull request Nov 13, 2026
The point-in-time join templates of the Redshift, Snowflake, BigQuery,
Postgres, Athena, Spark, Trino, ClickHouse and Couchbase offline stores
identify each entity row by concatenating the string values of its join
keys and event timestamp. Different rows could produce the same id, e.g.
driver_id=1, customer_id=12 and driver_id=11, customer_id=2 at the same
timestamp. Such rows got the other row's feature value, or, when both
feature rows had the same event timestamp, each came back twice, once
with each value.

Length-prefix each join key value in the id, as the ibis join does
since feast-dev#6884, so the id is only shared by rows with equal values.

Signed-off-by: sohailshk <122166523+sohailshk@users.noreply.github.com>
ntkathole pushed a commit that referenced this pull request Nov 13, 2026
* fix: Keep entity rows apart in the SQL point-in-time joins

The point-in-time join templates of the Redshift, Snowflake, BigQuery,
Postgres, Athena, Spark, Trino, ClickHouse and Couchbase offline stores
identify each entity row by concatenating the string values of its join
keys and event timestamp. Different rows could produce the same id, e.g.
driver_id=1, customer_id=12 and driver_id=11, customer_id=2 at the same
timestamp. Such rows got the other row's feature value, or, when both
feature rows had the same event timestamp, each came back twice, once
with each value.

Length-prefix each join key value in the id, as the ibis join does
since #6884, so the id is only shared by rows with equal values.

Signed-off-by: sohailshk <122166523+sohailshk@users.noreply.github.com>

* test: pass a subquery as the feature source so DuckDB 1.5 parses the Trino template

Signed-off-by: sohailshk <122166523+sohailshk@users.noreply.github.com>

---------

Signed-off-by: sohailshk <122166523+sohailshk@users.noreply.github.com>
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.

3 participants