-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: Use ASOF JOIN in Snowflake offline store query #4850
feat: Use ASOF JOIN in Snowflake offline store query #4850
Conversation
Signed-off-by: hkuepers <[email protected]>
e47a253
to
da7b74a
Compare
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.
Nice, thank you for this!
Looks like integration tests are failing. |
Signed-off-by: hkuepers <[email protected]>
I did not realize that there are integation tests for Snowflake, that is great. |
Signed-off-by: hkuepers <[email protected]>
Signed-off-by: hkuepers <[email protected]>
Signed-off-by: hkuepers <[email protected]>
710d1de
to
6b6c48e
Compare
Hmm. Looks like it's still failing fyi. |
Signed-off-by: hkuepers <[email protected]>
Yeah, I did not consider setting up the Snowflake integration tests locally and debugging from the CI logs for the template query is a little painful. I have done some improvements today and am still positive that it is close to being there 🙏 . If not I will set up the Snowflake environment in the next days to see the errors in Snowsight. |
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.
Thank you for this!
What this PR does / why we need it:
The query template for the Snowflake Offline Store is updated to use the recently added ASOF JOIN (https://www.snowflake.com/en/blog/time-series-analytics-asof-join-generally-available/). We have seen a very substantial improvement in feature queries using the new template. Especially, for large feature tables that have many historical entries for certain entity values the performance is even better than using the
ttl
parameter in the current query template.Which issue(s) this PR fixes:
Fixes #4732
Misc
@bimtauer and @miianiemela have shown interest in the feature, are you able to take a look and test it in your setup?