Repository navigation
fix: Avoid ON TRUE and multiple USING joins in the ClickHouse point-in-time join - #7152
ShivamSharma43 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7152 +/- ##
=======================================
Coverage 52.39% 52.40%
=======================================
Files 459 459
Lines 58156 58160 +4
Branches 8572 8574 +2
=======================================
+ Hits 30470 30476 +6
+ Misses 25606 25604 -2
Partials 2080 2080
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…n-time join
ClickHouse's old analyzer (the default before 24.3, and still used with
enable_analyzer = 0) rejects two joins in the point-in-time join template:
- `ON TRUE AND subquery.k = entity_dataframe.k` fails with
INVALID_JOIN_ON_EXPRESSION ("Illegal expression 'true' in JOIN ON
section"), for every query.
- The final SELECT joins each feature view back with USING, and with two
or more feature views it fails with "Code: 48. Multiple USING
statements are not supported".
Join the entity keys with `ON k1 = k1 AND k2 = k2`, use CROSS JOIN for a
feature view without entities (it only had `ON TRUE`), and join the
feature views back with `ON "<fv>"."<fv>__entity_row_unique_id" =
entity_dataframe."<fv>__entity_row_unique_id"`.
Fixes feast-dev#6141
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_012XPuJeHi7A5CeHSKHjHmwx
Signed-off-by: Shivam Sharma <[email protected]>
7c03889 to
54a5d33
Compare
What this PR does / why we need it:
The ClickHouse offline store's point-in-time join template has two joins that ClickHouse's old query analyzer rejects, so
get_historical_featuresfails on ClickHouse servers that use it: versions before 24.3, and newer versions withenable_analyzer = 0(allow_experimental_analyzer = 0).ON TRUE AND subquery."driver_id" = entity_dataframe."driver_id". The old analyzer fails on the constantTRUE:Code: 403. Illegal expression 'true' in JOIN ON section (INVALID_JOIN_ON_EXPRESSION). This happens for every query.SELECTjoins each feature view back withUSING ("<feature_view>__entity_row_unique_id"). With two or more feature views there are severalUSINGjoins in thatSELECT, and the old analyzer fails withCode: 48. Multiple USING statements are not supported.Changes:
ON subquery."k1" = entity_dataframe."k1" AND subquery."k2" = ..., without theTRUE. A feature view with no entities had onlyON TRUE(a cross join), so it now usesCROSS JOIN.SELECTjoins each feature view withON "<fv>"."<fv>__entity_row_unique_id" = entity_dataframe."<fv>__entity_row_unique_id"instead ofUSING. Only the entity columns and feature columns are selected, so the extra id column on the right side changes nothing in the output.I rendered the template on master and on this branch for two feature views (one with a
created_timestamp_columnand a TTL) and, separately, with an extra feature view that has no entities, with the tables inlined asvalues()subqueries. I ran the queries on ClickHouse through fiddle.clickhouse.com and play.clickhouse.com:INVALID_JOIN_ON_EXPRESSION; with only theON TRUEfixed:Multiple USING statements are not supportedallow_experimental_analyzer = 0INVALID_JOIN_ON_EXPRESSIONThe new analyzer runs both versions, and the results are the same.
Which issue(s) this PR fixes:
Fixes #6141
Checks
git commit -s)Testing Strategy
Added
TestPointInTimeJoinTemplatetosdk/python/tests/unit/infra/offline_stores/test_clickhouse.py. It renders the template and checks that there is noON TRUE, that the entity keys are joined withON ... AND ..., that the finalSELECTjoins every feature view withONand has noUSING, and that a feature view without entities gets aCROSS JOIN. All three tests fail on master. The existing tests in that file still pass.I did not run the ClickHouse integration suite (
make test-python-universal-clickhouse-offline): it needs a ClickHouse container, and Docker isn't available on my machine. The manual runs above are what I have instead.Misc
This change was written with help from an AI assistant (Claude Code). The ClickHouse runs above were done against real servers, not inferred.
🤖 Generated with Claude Code
https://claude.ai/code/session_012XPuJeHi7A5CeHSKHjHmwx