Skip to content

fix: Changing Snowflake template code to avoid query not implemented error… #3315

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
amithadiraju1694 wants to merge 0 commits into from
Closed

fix: Changing Snowflake template code to avoid query not implemented error… #3315

amithadiraju1694 wants to merge 0 commits into from

Conversation

@amithadiraju1694
Copy link
Contributor

@amithadiraju1694 amithadiraju1694 commented Oct 27, 2022

Signed-off-by: amithadiraju1694 amith.adiraju@gmail.com

What this PR does / why we need it:

The Snowflake template code has some bugs when implementing sql query to fetch data from snowflake.
store.list_data_sources()[-1] selects a RequestSource object rather than SnowflakeSource object; obviously request source object doesn't implement get_table_query_string() throwing NotImplementedError.

Changing store.list_data_sources()[-1] to store.get_data_source('table_name_of_source') solves the issue and is more reliable way of executing the query on the source.

Which issue(s) this PR fixes:

Fixes #3301

@amithadiraju1694
Copy link
Contributor Author

amithadiraju1694 commented Oct 27, 2022

/assign @woop

@amithadiraju1694 amithadiraju1694 changed the title Changing Snowflake template code to avoid query not implemented error… fix: Changing Snowflake template code to avoid query not implemented error… Oct 27, 2022
@sfc-gh-madkins sfc-gh-madkins self-requested a review October 27, 2022 17:22
@sfc-gh-madkins
Copy link
Collaborator

sfc-gh-madkins commented Oct 27, 2022

/ok-to-test

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2022

Codecov Report

Base: 67.65% // Head: 57.87% // Decreases project coverage by -9.78% ⚠️

Coverage data is based on head (0663e2c) compared to base (0ad0ace).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3315      +/-   ##
==========================================
- Coverage   67.65%   57.87%   -9.79%     
==========================================
  Files         181      215      +34     
  Lines       16645    18092    +1447     
==========================================
- Hits        11262    10471     -791     
- Misses       5383     7621    +2238
Flag Coverage Δ
integrationtests ?
unittests 57.87% <ø> (?)

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

Impacted Files Coverage Δ
...sts/integration/registration/test_universal_cli.py 20.20% <0.00%> (-79.80%) ⬇️
...ts/integration/offline_store/test_offline_write.py 26.08% <0.00%> (-73.92%) ⬇️
...fline_store/test_universal_historical_retrieval.py 28.75% <0.00%> (-71.25%) ⬇️
...dk/python/tests/integration/e2e/test_validation.py 26.71% <0.00%> (-70.23%) ⬇️
...ests/integration/e2e/test_python_feature_server.py 31.34% <0.00%> (-68.66%) ⬇️
...s/integration/registration/test_universal_types.py 32.25% <0.00%> (-67.75%) ⬇️
sdk/python/feast/infra/online_stores/redis.py 28.39% <0.00%> (-66.67%) ⬇️
sdk/python/tests/integration/e2e/test_usage_e2e.py 33.87% <0.00%> (-66.13%) ⬇️
sdk/python/tests/data/data_creator.py 34.78% <0.00%> (-65.22%) ⬇️
...n/tests/integration/registration/test_inference.py 35.71% <0.00%> (-64.29%) ⬇️
... and 167 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@feast-ci-bot
Copy link
Collaborator

feast-ci-bot commented Oct 27, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amithadiraju1694
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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

@sfc-gh-madkins
Copy link
Collaborator

sfc-gh-madkins commented Oct 27, 2022

See #3319

@amithadiraju1694
Copy link
Contributor Author

amithadiraju1694 commented Oct 27, 2022

See #3319

Thanks for update @sfc-gh-madkins new to contributions, want to fix mistakes on this one. Wondering why this PR was closed and you had to create another one referencing this,

is it because I haven't submitted test reports on my PR ? Appreciate the guidance, Thanks again !

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

Reviewers

@sfc-gh-madkins sfc-gh-madkins Awaiting requested review from sfc-gh-madkins

Assignees

@woop woop

Labels

ok-to-test size/XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feast[snowflake] demo workflow fails to get historical features through sql. get_table_query_string not implemented

5 participants

@amithadiraju1694 @sfc-gh-madkins @codecov-commenter @feast-ci-bot @woop