feat: Adding saved dataset capabilities for Postgres #3002
Conversation
AlexEijssen
changed the title
There was a problem hiding this comment.
I'm assuming that this fixes at least one test in https://github.com/feast-dev/feast/blob/master/Makefile#L112
Mind removing the excluded tests and running to make sure the new tests pass?
| @@ -214,7 +223,7 @@ def pull_all_from_table_or_query( | |||
|
|
|||
| query = f""" | |||
| SELECT {field_string} | |||
| FROM {from_expression} AS paftoq_alias | |||
There was a problem hiding this comment.
i think someone added this in because it caused a crash without this? #2956
There was a problem hiding this comment.
I had a similar issue. I thought it was caused by the fact data_source.get_table_query_string() always puts "()" around the from_expression. Even when from_expression is just a table name. I decided to adjust get_table_query_string() inspired by redshift_source.py. It's rather unfortunate that the context of #2954 isn't clear. because I had the exact same error. But I'm unsure if his fix works for me and vice versa. I think adding AS paftoq_alias and combining it with my version of get_table_query_string() would be the safest option.
There was a problem hiding this comment.
yeah im not sure. But what you said as the safe option sounds good to me
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AlexEijssen The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #3002 +/- ##
==========================================
- Coverage 77.69% 77.53% -0.17%
==========================================
Files 193 194 +1
Lines 16269 16298 +29
==========================================
- Hits 12641 12637 -4
- Misses 3628 3661 +33
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
I'm not sure if my latest commit is what you meant by: "Mind removing the excluded tests and running to make sure the new tests pass?" |
I sent a PR in your repo to help with it, but had some trouble getting the normal |
|
Are you on the Slack? Might be easier to discuss this syncrhonously in slack.feast.dev |
|
the postgres tests aren't being run continuously (mostly because we don't have anybody who is willing to "own" the component) so you'd actually have to manually run the male command to run the tests |
Signed-off-by: alex.eijssen [email protected]
What this PR does / why we need it:
This PR adds the possibility to save datasets in Postgres and register them in Feast. I also tried to move the Postgres code more in the direction of Redshift and Bigquery.
Which issue(s) this PR fixes:
Fixes #2995
DISCLAIMER:
If you register a dataset the UI breaks. This appears to be independent of Postgres, as issue #2996 describes the same issue I have, but then for Snowflake. I suspect the issue is in feast/ui/src/parsers/feastSavedDataset.ts that is too stringent/limited. And I'm working on a fix (wrapping up a PR) that ensures registering a dataset doesn't break the UI.