Skip to content

SNOW-629080: investigate and implement lastrowid#1201

Merged
sfc-gh-aling merged 4 commits intomainfrom
SNOW-629080-investigate-and-implement-lastrowid-support-in-connector-if-doable
Jul 22, 2022
Merged

SNOW-629080: investigate and implement lastrowid#1201
sfc-gh-aling merged 4 commits intomainfrom
SNOW-629080-investigate-and-implement-lastrowid-support-in-connector-if-doable

Conversation

@sfc-gh-aling
Copy link
Collaborator

@sfc-gh-aling sfc-gh-aling commented Jul 21, 2022 •
edited by sfc-gh-mkeller
Loading

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-629080

  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 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.

    Snowflake doesn't support lastrowid, according to PEP249, we should return None for this attribute.

    Cursor.lastrowid
This read-only attribute provides the rowid of the last modified row 
(most databases return a rowid only when a single INSERT operation is performed).
If the operation does not set a rowid or if the database does not support rowids,
this attribute should be set to None.
The semantics of .lastrowid are undefined in case the last executed
statement modified more than one row, e.g. when using INSERT with .executemany().

@sfc-gh-mkeller sfc-gh-mkeller changed the title SNOW-629080: investigate and implement lastrowid support in connector if doable SNOW-629080: investigate and implement lastrowid Jul 22, 2022
sfc-gh-mkeller
sfc-gh-mkeller requested changes Jul 22, 2022
View reviewed changes
@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2022

Codecov Report

Merging #1201 (ee55cb5) into main (c6ffe2d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1201      +/-   ##
==========================================
+ Coverage   84.96%   84.97%   +0.01%     
==========================================
  Files          58       58              
  Lines        7959     7961       +2     
  Branches     1282     1282              
==========================================
+ Hits         6762     6765       +3     
+ Misses        898      897       -1     
  Partials      299      299
Impacted Files Coverage Δ
src/snowflake/connector/cursor.py 95.09% <100.00%> (+0.36%) ⬆️
src/snowflake/connector/errors.py 89.89% <0.00%> (-0.54%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@sfc-gh-aling @sfc-gh-mkeller
Apply suggestions from code review
7b9f583 
Co-authored-by: Mark Keller <mark.keller@snowflake.com>
@sfc-gh-aling sfc-gh-aling enabled auto-merge (squash) July 22, 2022 23:11
@sfc-gh-aling sfc-gh-aling merged commit d16d7c8 into main Jul 22, 2022
@sfc-gh-aling sfc-gh-aling deleted the SNOW-629080-investigate-and-implement-lastrowid-support-in-connector-if-doable branch July 22, 2022 23:13
@github-actions github-actions bot locked and limited conversation to collaborators Jul 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sfc-gh-mkeller sfc-gh-mkeller sfc-gh-mkeller approved these changes

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sfc-gh-aling @codecov-commenter @sfc-gh-mkeller