Skip to content

fix: Make ClickHouse point-in-time join SQL ClickHouse-compatible - #6775

Open
pujitha24 wants to merge 2 commits into
feast-dev:masterfrom
pujitha24:auto/issue-6141
Open

fix: Make ClickHouse point-in-time join SQL ClickHouse-compatible#6775
pujitha24 wants to merge 2 commits into
feast-dev:masterfrom
pujitha24:auto/issue-6141

Conversation

@pujitha24

Copy link
Copy Markdown

What this PR does / why we need it:

The ClickHouse offline store's MULTIPLE_FEATURE_VIEW_POINT_IN_TIME_JOIN Jinja SQL template generated two SQL patterns that ClickHouse's parser rejects:

  1. ON TRUE in a JOIN clause raises INVALID_JOIN_ON_EXPRESSION. ClickHouse requires a concrete predicate expression in an ON clause, not a bare boolean literal — this broke every get_historical_features() call against the ClickHouse offline store, 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 query spanning 2+ FeatureViews.

This PR rewrites both to standard SQL forms ClickHouse (and PostgreSQL) accept:

  • The ON TRUE + unconditional AND chain becomes a conditional ON/AND chain over the featureview's entities. FeatureViews with no entities (non-entity retrieval, added in feat: Add non-entity retrieval support for ClickHouse offline store #6066) fall back to ON 1 = 1 — an equality expression ClickHouse accepts — instead of emitting a JOIN with no condition at all.
  • The final USING ("{{featureview.name}}__entity_row_unique_id") becomes an explicit qualified ON "{{featureview.name}}"."...id" = entity_dataframe."...id", so each LEFT JOIN carries its own predicate instead of colliding on a shared USING clause.

postgres.py's own template is untouched — PostgreSQL already accepts ON TRUE and isn't affected by this bug.

Which issue(s) this PR fixes:

Fixes #6141

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

Added TestMultipleFeatureViewPointInTimeJoinQuery to sdk/python/tests/unit/infra/offline_stores/test_clickhouse.py, which renders 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 zero-entity FeatureView (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.

Ran ruff check / ruff format --check on both changed files and mypy on the changed source file — all pass.

I did not have Docker/a live ClickHouse instance available in this environment to run an end-to-end query against a real ClickHouse server, so this hasn't been verified against a live cluster. The defect is a SQL syntax incompatibility visible directly in the rendered template output though, so the failing-to-passing unit test is a direct reproduction of it rather than a proxy. Happy to address anything a live run surfaces.

Note: CI on this repo requires a maintainer to add ok-to-test for PRs from non-collaborators — happy to address anything it surfaces.

Misc

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: feast-dev#6141
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
@pujitha24
pujitha24 requested a review from a team as a code owner August 24, 2026 00:31
@codecov-commenter

codecov-commenter commented Aug 24, 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 47.08%. Comparing base (f0bc070) to head (a1c48a3).
❗ 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    #6775      +/-   ##
==========================================
- Coverage   47.09%   47.08%   -0.01%     
==========================================
  Files         419      419              
  Lines       51878    51878              
  Branches     7525     7525              
==========================================
- Hits        24430    24429       -1     
  Misses      25700    25700              
- Partials     1748     1749       +1     
Flag Coverage Δ
go-feature-server 30.58% <ø> (ø)
python-unit 48.39% <ø> (-0.01%) ⬇️
Files with missing lines Coverage Δ
...res/contrib/clickhouse_offline_store/clickhouse.py 32.55% <ø> (ø)

... 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 f0bc070...a1c48a3. 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 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>
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.

ClickHouse offline store: ON TRUE and Multiple USING in JOIN not supported

3 participants