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

SNOW-799216 Allow nextset without fetching result for async multistat… #1526

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

sfc-gh-sfan
Copy link
Contributor

…ement

Description

Prior to this change we are not able to call nextset before fetching result if the cursor executes an async multistatement query. The main reason is that we need to first wait the result by invoking _prefetch_hook, but in the async case, we will first call reset(), which clears the _prefetch_hook. This way _prefetch_hook will never be invoked and this causes failure in the connector.

This fix adds a _prefetch_hook invoke before calling reset()

Testing

integ test

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-799216

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    Prior to this change we are not able to call nextset before fetching result if the cursor executes an async multistatement query. The main reason is that we need to first wait the result by invoking _prefetch_hook, but in the async case, we will first call reset(), which clears the _prefetch_hook. This way _prefetch_hook will never be invoked and this causes failure in the connector.

This fix adds a _prefetch_hook invoke before calling reset()

@sfc-gh-sfan sfc-gh-sfan requested review from sfc-gh-jdu, a team and sfc-gh-mkeller and removed request for a team April 21, 2023 22:00
@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

Merging #1526 (a9b47c8) into main (e937591) will increase coverage by 0.06%.
The diff coverage is 93.10%.

@@            Coverage Diff             @@
##             main    #1526      +/-   ##
==========================================
+ Coverage   82.10%   82.17%   +0.06%     
==========================================
  Files          61       61              
  Lines        8622     8638      +16     
  Branches     1274     1279       +5     
==========================================
+ Hits         7079     7098      +19     
+ Misses       1207     1206       -1     
+ Partials      336      334       -2     
Impacted Files Coverage Δ
src/snowflake/connector/options.py 92.45% <ø> (ø)
src/snowflake/connector/cursor.py 94.40% <84.61%> (-0.27%) ⬇️
src/snowflake/connector/file_transfer_agent.py 86.48% <100.00%> (+1.22%) ⬆️
src/snowflake/connector/pandas_tools.py 84.11% <100.00%> (+0.45%) ⬆️
src/snowflake/connector/version.py 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

else:
assert cur.fetchall() == check
savedIds.append(cur.sfqid)
if not skip_to_last_set or index == len(checks) - 1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that we should be skipping the last check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not skipping the last check. We are doing the check if we are testing all sets, or if it is the last set.

test/integ/test_multi_statement.py Outdated Show resolved Hide resolved
test/integ/test_multi_statement.py Outdated Show resolved Hide resolved
…ement

Description

Prior to this change we are not able to call nextset before fetching
result if the cursor executes an async multistatement query. The
main reason is that we need to first wait the result by invoking
_prefetch_hook, but in the async case, we will first call reset(),
which clears the _prefetch_hook. This way _prefetch_hook will never
be invoked and this causes failure in the connector.

This fix adds a _prefetch_hook invoke before calling reset()

Testing

integ test
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller left a comment

Choose a reason for hiding this comment

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

🚢 🚢

@sfc-gh-sfan sfc-gh-sfan merged commit 6592163 into main Apr 26, 2023
@sfc-gh-sfan sfc-gh-sfan deleted the shixuan_no_fetch branch April 26, 2023 19:58
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants