fix: Make ClickHouse point-in-time join SQL ClickHouse-compatible - #6775
Open
pujitha24 wants to merge 2 commits into
Open
fix: Make ClickHouse point-in-time join SQL ClickHouse-compatible#6775pujitha24 wants to merge 2 commits into
pujitha24 wants to merge 2 commits into
Conversation
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)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
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>
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 ClickHouse offline store's
MULTIPLE_FEATURE_VIEW_POINT_IN_TIME_JOINJinja SQL template generated two SQL patterns that ClickHouse's parser rejects:ON TRUEin a JOIN clause raisesINVALID_JOIN_ON_EXPRESSION. ClickHouse requires a concrete predicate expression in anONclause, not a bare boolean literal — this broke everyget_historical_features()call against the ClickHouse offline store, single or multi FeatureView.USING (...)clauses across the finalLEFT JOINs raisesCode: 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:
ON TRUE+ unconditionalANDchain becomes a conditionalON/ANDchain 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 toON 1 = 1— an equality expression ClickHouse accepts — instead of emitting a JOIN with no condition at all.USING ("{{featureview.name}}__entity_row_unique_id")becomes an explicit qualifiedON "{{featureview.name}}"."...id" = entity_dataframe."...id", so eachLEFT JOINcarries its own predicate instead of colliding on a sharedUSINGclause.postgres.py's own template is untouched — PostgreSQL already acceptsON TRUEand isn't affected by this bug.Which issue(s) this PR fixes:
Fixes #6141
Checks
git commit -s)Testing Strategy
Added
TestMultipleFeatureViewPointInTimeJoinQuerytosdk/python/tests/unit/infra/offline_stores/test_clickhouse.py, which renders the realMULTIPLE_FEATURE_VIEW_POINT_IN_TIME_JOINtemplate viabuild_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 emitON TRUE/ collidingUSING) and pass against the post-fix template.Ran
ruff check/ruff format --checkon both changed files andmypyon 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-testfor PRs from non-collaborators — happy to address anything it surfaces.Misc