Skip to content

fix: Keep entity rows apart in the SQL point-in-time joins - #7124

Merged
ntkathole merged 2 commits into
feast-dev:masterfrom
sohailshk:fix/sql-pit-entity-row-id
Nov 13, 2026
Merged

ntkathole merged 2 commits into
feast-dev:masterfrom
sohailshk:fix/sql-pit-entity-row-id

Conversation

@sohailshk

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

The point-in-time join templates of the Redshift, Snowflake, BigQuery, Postgres, Athena, Spark, Trino, ClickHouse and Couchbase offline stores build <feature_view>__entity_row_unique_id by concatenating the join key values and the entity timestamp with no separator, and join the feature rows back to the entity rows on that id. Two entity rows can get the same id: driver_id=1, customer_id=12 and driver_id=11, customer_id=2 at the same timestamp both become 112<timestamp>.

One of these rows then gets the other's feature value, or, if both feature rows have the same event timestamp, each entity row comes back twice, once with each value. get_historical_features with the Postgres offline store on a local PostgreSQL 18, feature view keyed on driver_id and customer_id:

Before:
 driver_id  customer_id event_timestamp  trips
         1           12      2025-01-02    100
         1           12      2025-01-02    200
        11            2      2025-01-02    100
        11            2      2025-01-02    200
After:
 driver_id  customer_id event_timestamp  trips
         1           12      2025-01-02    100
        11            2      2025-01-02    200

Each join key value is now length-prefixed in the id (<length>:<value>), as #6884 did for the ibis point-in-time join, so the id is only shared by rows with equal key values.

Adds test_sql_entity_row_unique_id.py. It runs the rendered Redshift, Athena, Postgres, Trino and ClickHouse queries on DuckDB with two such entity rows and checks that each keeps its own feature value (4 rows instead of 2 on master). The BigQuery, Snowflake, Spark and Couchbase templates use SQL that DuckDB can't run, so for those the test only checks that each join key is length-prefixed in the id.

Which issue(s) this PR fixes:

N/A (no existing issue; #6884 fixed the same problem in the ibis join)

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

pytest tests/unit/infra/offline_stores: 309 passed, 20 skipped; the 2 test_ibis.py::test_point_in_time_join* failures (sqlglot TypeError) also occur on unmodified master. The Snowflake, BigQuery, Spark, Couchbase, Redshift, Athena, Trino and ClickHouse queries were not run against those engines.

@sohailshk
sohailshk requested review from a team and sudohainguyen as code owners November 10, 2026 14:02
@codecov-commenter

codecov-commenter commented Nov 10, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.45%. Comparing base (ce02644) to head (8e53dfe).
⚠️ Report is 1 commits behind head on master.
❗ 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    #7124   +/-   ##
=======================================
  Coverage   52.45%   52.45%           
=======================================
  Files         459      459           
  Lines       58172    58172           
  Branches     8577     8577           
=======================================
  Hits        30515    30515           
  Misses      25573    25573           
  Partials     2084     2084           
Flag Coverage Δ
go-feature-server 30.96% <ø> (ø)
python-unit 53.96% <ø> (ø)
Files with missing lines Coverage Δ
sdk/python/feast/infra/offline_stores/bigquery.py 49.44% <ø> (ø)
...line_stores/contrib/athena_offline_store/athena.py 48.14% <ø> (ø)
...res/contrib/clickhouse_offline_store/clickhouse.py 32.55% <ø> (ø)
...tores/contrib/couchbase_offline_store/couchbase.py 30.76% <ø> (ø)
..._stores/contrib/postgres_offline_store/postgres.py 46.15% <ø> (ø)
...ffline_stores/contrib/spark_offline_store/spark.py 35.42% <ø> (ø)
...ffline_stores/contrib/trino_offline_store/trino.py 38.34% <ø> (ø)
sdk/python/feast/infra/offline_stores/redshift.py 39.00% <ø> (ø)
sdk/python/feast/infra/offline_stores/snowflake.py 37.52% <ø> (ø)

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 ce02644...8e53dfe. 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.

@sohailshk

Copy link
Copy Markdown
Contributor Author

The only red job is unit-test-python (3.11, macos-15): 3886 passed and 2 errors, both in the setup of tests/unit/test_feature_server.py (AssertionError: stdout: b'', the feature server subprocess printed nothing). This PR doesn't touch the feature server, and the same tests passed on 3.12 macOS and on both Ubuntu jobs. Could someone re-run that job?

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>
…Trino template

Signed-off-by: sohailshk <122166523+sohailshk@users.noreply.github.com>
@ntkathole
ntkathole force-pushed the fix/sql-pit-entity-row-id branch from 6cc9ef4 to 8e53dfe Compare November 13, 2026 15:02
@ntkathole
ntkathole merged commit cc17aad into feast-dev:master Nov 13, 2026
19 of 23 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.

3 participants