-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Add missing pull_all_from_table_or_query to Clickhouse offline store #5667
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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. |
|
Failing unit tests:
Seem to be outside of my changes? 🤔 I see same issues in other latest MRs. |
|
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. |
…terialization logic (calling it) Signed-off-by: lukas.valatka <lukas.valatka@cast.ai>
ntkathole
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
Thanks for fast responses and reviews! |
|
Let's merge? Not sure who can (I don't seem to have the option available). |
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_querymissing 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