◐ Shell
reader mode source ↗
Skip to content

feat: Add missing pull_all_from_table_or_query to Clickhouse offline store#5667

Merged
ntkathole merged 1 commit into
feast-dev:masterfrom
astronautas:master
Oct 15, 2025
Merged

feat: Add missing pull_all_from_table_or_query to Clickhouse offline store#5667
ntkathole merged 1 commit into
feast-dev:masterfrom
astronautas:master

Conversation

@astronautas

@astronautas astronautas commented Oct 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

As discussed in #5493, from 0.50.0 materialize does not work against Clickhouse offline store, with the reason being pull_all_from_table_or_query missing in the CH store implementation.

The PR adjusts this by adding the method. Also, makes sure some typing tests are not excluded anymore, apparently they pass just fine now.

Which issue(s) this PR fixes:

#5493,

Misc

@astronautas astronautas requested a review from a team as a code owner October 15, 2025 11:57
@franciscojavierarceo franciscojavierarceo changed the title Add missing pull_all_from_table_or_query to Clickhouse offline store Oct 15, 2025
@franciscojavierarceo franciscojavierarceo changed the title fat: Add missing pull_all_from_table_or_query to Clickhouse offline store Oct 15, 2025
@astronautas

astronautas commented Oct 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I ran "make test-python-universal-clickhouse-offline", to make sure CH integration tests pass, they actually failed before adding the change, worked after adding the change, hopefully the same suite runs on CI as well :D.

@astronautas

astronautas commented Oct 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Failing unit tests:

  • ERROR sdk/python/tests/unit/permissions/auth/server/test_auth_registry_server.py::test_registry_apis[\nauth:\n type: oidc\n client_id: feast-integration-client\n client_secret: feast-integration-client-secret\n username: reader_writer\n password: password\n auth_discovery_url: KEYCLOAK_URL_PLACE_HOLDER/realms/master/.well-known/openid-configuration\n-applied_permissions2-tls_mode0] - AssertionError

  • ERROR sdk/python/tests/unit/test_rag_retriever.py - ValueError: libcublas.so.*[0-9] not found in the system path

  • FAILED sdk/python/tests/unit/online_store/test_online_retrieval.py::test_get_online_features - ImportError: Torch is not available or failed to import.
    Original error:
    libcublas.so.*[0-9] not found in the system path ['/home/runner/work/feast/feast/sdk/python', '/home/runner/work/feast/feast', '/opt/hostedtoolcache/Python/3.12.11/x64/lib/python312.zip', '/opt/hostedtoolcache/Python/3.12.11/x64/lib/python3.12', '/opt/hostedtoolcache/Python/3.12.11/x64/lib/python3.12/lib-dynload', '/opt/hostedtoolcache/Python/3.12.11/x64/lib/python3.12/site-packages', '/home/runner/work/feast/feast/sdk/python', '/opt/hostedtoolcache/Python/3.12.11/x64/lib/python3.12/site-packages/setuptools/_vendor']

Seem to be outside of my changes? 🤔 I see same issues in other latest MRs.

@astronautas

astronautas commented Oct 15, 2025

Copy link
Copy Markdown
Contributor Author

@franciscojavierarceo

@ntkathole

ntkathole commented Oct 15, 2025

Copy link
Copy Markdown
Member

Those failures are due to pytorch releasing wheels early than pypi release. I have a fix #5668. Once merged, you can rebase and pipeline issue should fix.

@astronautas @ntkathole
add pull_all_from_table_or_query for clickhouse, to align with new ma… …
1752a4f 
…terialization logic (calling it)

Signed-off-by: lukas.valatka <lukas.valatka@cast.ai>
ntkathole
ntkathole approved these changes Oct 15, 2025
View reviewed changes

@ntkathole ntkathole left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

lgtm

@astronautas

astronautas commented Oct 15, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for fast responses and reviews!

@astronautas

astronautas commented Oct 15, 2025

Copy link
Copy Markdown
Contributor Author

Let's merge? Not sure who can (I don't seem to have the option available).

Hide details View details @ntkathole ntkathole merged commit eeef56e into feast-dev:master Oct 15, 2025
17 checks passed
@astronautas astronautas mentioned this pull request Oct 28, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ntkathole ntkathole ntkathole approved these changes

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@astronautas @ntkathole @franciscojavierarceo