Skip to content
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

fix: Revert snowflake #2907

Closed
wants to merge 2 commits into from
Closed

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
This reverts commit 0ceb39c.

Signed-off-by: Kevin Zhang <[email protected]>
@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

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

@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

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

[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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants