Skip to content

fix: Revert snowflake #2907

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.

Sign up for GitHub

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

Jump to bottom
Closed
kevjumba wants to merge 2 commits into from
Closed

fix: Revert snowflake #2907

kevjumba wants to merge 2 commits into from

Conversation

@kevjumba
Copy link
Collaborator

@kevjumba kevjumba commented Jul 5, 2022

What this PR does / why we need it:
@sfc-gh-madkins I think the snowflake PR was merged in prematurely and we've decided to revert it first. I think there's some other issues that need to be addressed/ just general reviews from team members.

Please reopen the PR and give us a chance to do a more thorough review first!

Which issue(s) this PR fixes:

Fixes #

kevjumba added 2 commits July 5, 2022 14:56
@kevjumba
Revert "feat: Add snowflake online store (feast-dev#2902)"
e559da5 
This reverts commit f758f9e.

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>
@kevjumba
Revert "fix snowflake testing (feast-dev#2903)"
f63f8a3 
This reverts commit 0ceb39c.

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>
@kevjumba kevjumba force-pushed the revert_snowflake branch from 5de2ed7 to f63f8a3 Compare July 5, 2022 21:56
@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2022 •
edited
Loading

Codecov Report

Merging #2907 (f63f8a3) into master (0159f38) will decrease coverage by 21.36%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master    #2907       +/-   ##
===========================================
- Coverage   80.68%   59.31%   -21.37%     
===========================================
  Files         178      177        -1     
  Lines       15837    15679      -158     
===========================================
- Hits        12778     9300     -3478     
- Misses       3059     6379     +3320
Flag Coverage Δ
integrationtests ?
unittests 59.31% <0.00%> (+0.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
sdk/python/feast/infra/utils/snowflake_utils.py 24.13% <0.00%> (-53.64%) ⬇️
...ts/integration/feature_repos/repo_configuration.py 55.74% <ø> (-30.05%) ⬇️
.../integration/online_store/test_online_retrieval.py 16.84% <0.00%> (-83.16%) ⬇️
sdk/python/tests/utils/online_read_write_test.py 19.35% <0.00%> (-80.65%) ⬇️
...fline_store/test_universal_historical_retrieval.py 23.80% <0.00%> (-76.20%) ⬇️
.../integration/online_store/test_universal_online.py 17.82% <0.00%> (-73.42%) ⬇️
...ts/integration/offline_store/test_offline_write.py 27.11% <0.00%> (-72.89%) ⬇️
...dk/python/tests/integration/e2e/test_validation.py 27.77% <0.00%> (-71.43%) ⬇️
...ests/integration/e2e/test_python_feature_server.py 28.78% <0.00%> (-71.22%) ⬇️
sdk/python/feast/wait.py 23.52% <0.00%> (-70.59%) ⬇️
... and 78 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0159f38...f63f8a3. Read the comment docs.

@kevjumba kevjumba requested a review from adchia July 5, 2022 22:55
@sfc-gh-madkins
Copy link
Collaborator

sfc-gh-madkins commented Jul 6, 2022

@kevjumba Yes this was a fat finger merge for #2902

However, #2903 was a code error @adchia and I had found. What is your problem with that one?

@achals
Copy link
Member

achals commented Jul 6, 2022

Agreed #2903 shouldn't be reverted

felixwang9817
felixwang9817 approved these changes Jul 6, 2022
View reviewed changes
Copy link
Collaborator

@felixwang9817 felixwang9817 left a comment

Choose a reason for hiding this comment

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

/lgtm

@feast-ci-bot
Copy link
Collaborator

feast-ci-bot commented Jul 6, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: felixwang9817, kevjumba

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [felixwang9817,kevjumba]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kevjumba kevjumba closed this Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixwang9817 felixwang9817 felixwang9817 approved these changes

@adchia adchia Awaiting requested review from adchia

Assignees

@felixwang9817 felixwang9817

Labels

approved do-not-merge/hold lgtm size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kevjumba @codecov-commenter @sfc-gh-madkins @achals @feast-ci-bot @felixwang9817