-
Notifications
You must be signed in to change notification settings - Fork 1.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: Enable users to use Entity Query as CTE during historical retrieval #5202
feat: Enable users to use Entity Query as CTE during historical retrieval #5202
Conversation
Signed-off-by: Blake <[email protected]>
Signed-off-by: Blake <[email protected]>
Signed-off-by: Blake <[email protected]>
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.
Awesome, would you mind updating the documentation for Postgres while you're here?
Signed-off-by: Blake <[email protected]>
@franciscojavierarceo Let me know if the updated docs are what you were looking for. |
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.
this lgtm, as a follow up you may want to have a way to enforce the temp_table to be unique if two people end up having collisions with the desire to operate on different partitions of the data.
I believe in this particular case, the |
Signed-off-by: Blake <[email protected]>
Signed-off-by: Blake <[email protected]>
What this PR does / why we need it:
This PR gives users the option to use CTEs for querying historical features instead of the current approach of creating a table, using the table and then dropping the table.
The option sits behind a flag that's configurable on the Offline Store configuration.
Which issue(s) this PR fixes:
When trying to join more than 1 or 2 entities to a feature view, we started noticing performance issues. During this time, we were able to manipulate the query SQL to use a CTE instead of the TABLE created by default, and saw noticeable improvements in getting results back.
We believe this will have limited if any downsides to the existing implementation.
Misc
There's a note that this SQL template should stay in sync w/ Redshift, but we are now drifting.