From a1c48a34755a3fa9760d1c71dfbf7c16eba8fa56 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Sun, 23 Aug 2026 17:30:50 -0700 Subject: [PATCH 1/2] fix: Make ClickHouse point-in-time join SQL ClickHouse-compatible Motivation: The MULTIPLE_FEATURE_VIEW_POINT_IN_TIME_JOIN Jinja template used by the ClickHouse offline store generated two SQL patterns ClickHouse rejects: 1. `ON TRUE` in a JOIN clause raises `INVALID_JOIN_ON_EXPRESSION` - ClickHouse requires a concrete predicate expression, not a bare boolean literal. This affected every query, single or multi FeatureView. 2. Chaining multiple `USING (...)` clauses across the final LEFT JOINs raises `Code: 48. Multiple USING statements are not supported`, breaking any get_historical_features() call spanning 2+ FeatureViews. Approach: - Replace `ON TRUE` + unconditional `AND` chain with a conditional `ON`/`AND` chain over the featureview's entities. FeatureViews with no entities (non-entity retrieval) fall back to `ON 1 = 1`, an equality expression ClickHouse accepts, instead of emitting a JOIN with no condition at all. - Replace the final `USING ("{{featureview.name}}__entity_row_unique_id")` with an explicit `ON "{{featureview.name}}"."...id" = entity_dataframe."...id"`, so each LEFT JOIN carries its own qualified predicate instead of colliding on a shared USING clause. Both forms are standard SQL, also valid on PostgreSQL, though postgres.py's own template is untouched since PostgreSQL already accepts `ON TRUE` and is not affected by this bug. Validation: - Added TestMultipleFeatureViewPointInTimeJoinQuery to sdk/python/tests/unit/infra/offline_stores/test_clickhouse.py, rendering the real MULTIPLE_FEATURE_VIEW_POINT_IN_TIME_JOIN template via build_point_in_time_query() (no mocking of the template itself) for: two FeatureViews with entities (the reported bug), and a FeatureView with zero entities (the non-entity-retrieval edge case the fix also has to preserve). - Confirmed both new tests FAIL against the pre-fix template (still emit `ON TRUE` / colliding `USING`) and PASS against the post-fix template - a failing-then-passing reproduction of the reported defect, run via `uv run pytest sdk/python/tests/unit/infra/offline_stores/test_clickhouse.py -v`. - Ran `uv run ruff check` and `uv run ruff format --check` on both changed files (pass), and `mypy` on the changed source file (pass, via `uv run bash -c "cd sdk/python && mypy feast/infra/offline_stores/contrib/clickhouse_offline_store/clickhouse.py"`). - Could not run against a live ClickHouse instance or the repo's broader `make test-python-unit` / full mypy sweep in this environment (no Docker/ClickHouse available locally, and several unrelated contrib offline stores require optional extras this environment doesn't have installed); the defect here is a SQL syntax incompatibility visible directly in the rendered template output, so the failing-to-passing unit test is a direct reproduction of it. Report: https://github.com/feast-dev/feast/issues/6141 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code) --- .../clickhouse_offline_store/clickhouse.py | 9 ++- .../infra/offline_stores/test_clickhouse.py | 62 +++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/sdk/python/feast/infra/offline_stores/contrib/clickhouse_offline_store/clickhouse.py b/sdk/python/feast/infra/offline_stores/contrib/clickhouse_offline_store/clickhouse.py index 9e353151746..c84fa3678a4 100644 --- a/sdk/python/feast/infra/offline_stores/contrib/clickhouse_offline_store/clickhouse.py +++ b/sdk/python/feast/infra/offline_stores/contrib/clickhouse_offline_store/clickhouse.py @@ -533,10 +533,13 @@ def _append_alias(field_names: List[str], alias: str) -> List[str]: entity_dataframe."{{featureview.name}}__entity_row_unique_id" FROM "{{ featureview.name }}__subquery" AS subquery INNER JOIN "{{ featureview.name }}__entity_dataframe" AS entity_dataframe - ON TRUE + {% if featureview.entities %} {% for entity in featureview.entities %} - AND subquery."{{ entity }}" = entity_dataframe."{{ entity }}" + {% if loop.first %}ON{% else %}AND{% endif %} subquery."{{ entity }}" = entity_dataframe."{{ entity }}" {% endfor %} + {% else %} + ON 1 = 1 + {% endif %} WHERE TRUE AND subquery.event_timestamp <= entity_dataframe.entity_timestamp @@ -625,6 +628,6 @@ def _append_alias(field_names: List[str], alias: str) -> List[str]: ,"{% if full_feature_names %}{{ featureview.name }}__{{featureview.field_mapping.get(feature, feature)}}{% else %}{{ featureview.field_mapping.get(feature, feature) }}{% endif %}" {% endfor %} FROM "{{ featureview.name }}__cleaned" -) AS "{{featureview.name}}" USING ("{{featureview.name}}__entity_row_unique_id") +) AS "{{featureview.name}}" ON "{{featureview.name}}"."{{featureview.name}}__entity_row_unique_id" = entity_dataframe."{{featureview.name}}__entity_row_unique_id" {% endfor %} """ diff --git a/sdk/python/tests/unit/infra/offline_stores/test_clickhouse.py b/sdk/python/tests/unit/infra/offline_stores/test_clickhouse.py index 7789cde72b3..e37fba1c845 100644 --- a/sdk/python/tests/unit/infra/offline_stores/test_clickhouse.py +++ b/sdk/python/tests/unit/infra/offline_stores/test_clickhouse.py @@ -5,12 +5,74 @@ import pytest +from feast.infra.offline_stores.contrib.clickhouse_offline_store.clickhouse import ( + MULTIPLE_FEATURE_VIEW_POINT_IN_TIME_JOIN, + build_point_in_time_query, +) from feast.infra.utils.clickhouse.clickhouse_config import ClickhouseConfig from feast.infra.utils.clickhouse.connection_utils import get_client, thread_local logger = logging.getLogger(__name__) +def _feature_view_query_context(name, entities): + return { + "name": name, + "ttl": 3600, + "entities": entities, + "features": ["feature1"], + "field_mapping": {}, + "timestamp_field": "event_timestamp", + "created_timestamp_column": None, + "table_subquery": f"{name}_table", + "entity_selections": [f'"{entity}" as "{entity}"' for entity in entities], + "min_event_timestamp": None, + "max_event_timestamp": "2023-01-01", + "date_partition_column": None, + "timestamp_field_type": None, + } + + +class TestMultipleFeatureViewPointInTimeJoinQuery: + """ + ClickHouse rejects `ON TRUE` (INVALID_JOIN_ON_EXPRESSION) and rejects more than + one `USING` clause per query (Code: 48). Both appeared in the multi-feature-view + point-in-time join template, breaking any query that joined 2+ FeatureViews, as + well as every single-FeatureView query via the `ON TRUE` clause. + """ + + def test_no_on_true_or_multiple_using_with_entities(self): + query = build_point_in_time_query( + [ + _feature_view_query_context("fv1", ["driver_id"]), + _feature_view_query_context("fv2", ["driver_id"]), + ], + left_table_query_string="entity_table", + entity_df_event_timestamp_col="event_timestamp", + entity_df_columns=["driver_id", "event_timestamp"], + query_template=MULTIPLE_FEATURE_VIEW_POINT_IN_TIME_JOIN, + ) + + assert "ON TRUE" not in query + assert 'ON "fv1"."fv1__entity_row_unique_id"' in query + assert 'ON "fv2"."fv2__entity_row_unique_id"' in query + assert 'USING ("fv1__entity_row_unique_id")' not in query + assert 'USING ("fv2__entity_row_unique_id")' not in query + + def test_no_on_true_with_no_entities(self): + """Non-entity FeatureViews (entities=[]) must still produce a valid ON clause.""" + query = build_point_in_time_query( + [_feature_view_query_context("fv1", [])], + left_table_query_string="entity_table", + entity_df_event_timestamp_col="event_timestamp", + entity_df_columns=["event_timestamp"], + query_template=MULTIPLE_FEATURE_VIEW_POINT_IN_TIME_JOIN, + ) + + assert "ON TRUE" not in query + assert "ON 1 = 1" in query + + @pytest.fixture def clickhouse_config(): """Create a test ClickHouse configuration.""" From bc786606315d515fe80cc1fab04ef874a9bbdcb9 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Sun, 23 Aug 2026 22:33:00 -0700 Subject: [PATCH 2/2] fix: Update stale line number in secrets baseline for test_clickhouse.py The new test class added earlier in test_clickhouse.py shifted the existing password="password" placeholder down to line 83, which the detect-secrets pre-commit hook flagged as a stale baseline entry. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> --- .secrets.baseline | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index bdeaa2d9d75..8d367153a4e 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -1401,7 +1401,7 @@ "filename": "sdk/python/tests/unit/infra/offline_stores/test_clickhouse.py", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 21 + "line_number": 83 } ], "sdk/python/tests/unit/infra/offline_stores/test_offline_store.py": [ @@ -1564,5 +1564,5 @@ } ] }, - "generated_at": "2026-08-20T15:20:58Z" + "generated_at": "2026-08-24T05:29:25Z" }