[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Berker Peksag]

Berker Peksag added the comment:

Thanks for the patch, Alex.

--
nosy: +berker.peksag
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Roundup Robot]

Roundup Robot added the comment:

New changeset 2126e8cbc12f by Berker Peksag in branch 'default':
Issue #16864: Cursor.lastrowid now supports REPLACE statement
https://hg.python.org/cpython/rev/2126e8cbc12f

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Gerhard Häring]

Changes by Gerhard Häring :


--
assignee:  -> ghaering

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Alex Lord]

Alex Lord added the comment:

Adding a patch for 3.6 since 3.5 is in beta.

--
versions: +Python 3.6 -Python 3.5
Added file: http://bugs.python.org/file39677/replace_lastrowid_3_6.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Alex Lord]

Alex Lord added the comment:

Oh, alright. That makes a lot of sense. Sorry for being dense. I should have 
read the docs on subtest.

All lines are under 80 characters and I modified the unit test to use subtest.

Thanks for taking the time to walk me through this.

--
Added file: http://bugs.python.org/file39425/sqlite_review_2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[R. David Murray]

R. David Murray added the comment:

All lines need to be wrapped to <80 columns.

The idea behind the loop is this:

sql = "{} INTO test(id, unique_test) VALUES (?, ?)"
self.cu.execute(sql.format('INSERT OR REPLACE, (1, "foo")
for statement in ["INSERT OR REPLACE", "REPLACE"]:
with self.subTest(statement=statement):
self.cu.execute(sql.format(statement), (1, "foo"))
self.assertEqual(self.cu.lastrowid, 1) 

What this does is run *both* tests, even if one fails, and reports the results 
separately (labeled with 'statement='INSERT OR REPLACE', ect).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Alex Lord]

Alex Lord added the comment:

There was a bunch of things wrong with that patch.

In addition to the issues you brought up in the review I was mixing up what the 
actual problem is

REPLACE INTO is an alias for INSERT OR REPLACE. INSERT OR REPLACE was correctly 
setting the lastrowid values but REPLACE INTO was not setting the last rowid 
value. I changed the documentation modifications to reflect this.

In addition I cleaned up the unit tests. The unit tests were kind of a mess 
because I was trying to figure out what the problem was and never refactored 
after getting a reproduction.

 I at first went down the path of making the tests use a for loop like you 
suggested

for statement in ["INSERT OR REPLACE", "REPLACE"]:
sql = "{} INTO test(id, unique_test) VALUES (?, ?)".format(
statement)
self.cu.execute(sql, (1, "foo"))
self.assertEqual(self.cu.lastrowid, 1)
self.cu.execute(sql, (1, "foo"))$
self.assertEqual(self.cu.lastrowid, 1) 

Which I don't think is as nice as a cleaned up unrolled version

self.cu.execute("INSERT OR REPLACE INTO test(id, unique_test) VALUES (?, 
?)", (1, "bar",))
self.assertEqual(self.cu.lastrowid, 1)
self.cu.execute("INSERT OR REPLACE INTO test(id, unique_test) VALUES (?, 
?)", (1, "bar",))
self.assertEqual(self.cu.lastrowid, 1)
self.cu.execute("REPLACE INTO test(id, unique_test) VALUES (?, ?)", (1, 
"bar",))
self.assertEqual(self.cu.lastrowid, 1)

I've created a patch that fixes all of the issues brought up in the code review 
and is just generally much cleaner.

--
Added file: http://bugs.python.org/file39411/sqlite_after_review_1.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[R. David Murray]

R. David Murray added the comment:

Added review comments.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Alex Lord]

Changes by Alex Lord :


Added file: http://bugs.python.org/file39367/sqlite_lastrowid_35_updated_2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Alex Lord]

Changes by Alex Lord :


Added file: http://bugs.python.org/file39366/sqlite_lastrowid_35_updated.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Ned Deily]

Changes by Ned Deily :


--
nosy: +ghaering
stage: needs patch -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Alex Lord]

Alex Lord added the comment:

Went back to test to see if the other statements are covered already. Unit 
tests show that lastrowid is set properly no matter what form of sqlite insert 
statement is used.

sqlite_lastrowid_35_v2.patch contains the doc changes, unit test changes, and 
code change.

I believe that the latest version of this patch is ready for a code review by a 
core dev.

--
Added file: http://bugs.python.org/file39339/sqlite_lastrowid_35_v2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Alex Lord]

Alex Lord added the comment:

Thanks for Alex_gayner and lifeless. They pointed out the sqlite3_last_row_id 
is part of the sqlite3 module itself (not cpython).

https://www.sqlite.org/c3ref/last_insert_rowid.html

According the documentation we can expect that if a constraint stops an 
insertion then the lostrowid is not modified. As such the changes required to 
add full INSERT OR CONDITION support for last row id is unit tests for each 
statement and changing the conditional to recognize all insert cases.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Alex Lord]

Alex Lord added the comment:

Updated the patch to have a versionchanged for lastrowid in 
Doc/Library/sqlite3.rst and Doc/whatsnew/3.5.rst

One thing of note is that I wasn't able to get the indentation to be on the 
same level for sqlite3.rst (it would either intent the description text or the 
versionchange text).

Now that I'm actually starting to understand the dbapi and sqlite3 I've come to 
the conclusion that the lastrowid method should update the lastrowid for the 
INSERT OR ROLLBACK, INSERT OR ABORT, INSERT OR FAIL, INSERT OR IGNORE 
statements as well as the INSERT and INSERT OR REPLACE statements. 

I'm unsure how hard or simple supporting those statements will be

The code in question is

 704 Py_DECREF(self->lastrowid);$
 705 if (!multiple && statement_type == STATEMENT_INSERT) {$
 706 sqlite_int64 lastrowid;$
 707 Py_BEGIN_ALLOW_THREADS$
 708 lastrowid = sqlite3_last_insert_rowid(self->connection->db);$
 709 Py_END_ALLOW_THREADS$
 710 self->lastrowid = _pysqlite_long_from_int64(lastrowid);

And the difficulty will be if sqlite3_last_insert_rowid (line 708) does or 
doesn't return a row id if the OR STATEMENT portion of those inserts are 
triggered.

The Problem I'm having is that when I tried to find sqlite3_last_insert_rowid 
in the Modules/_sqlite directory I got nothing

$ grep -r "sqlite3_last_insert_rowid" Modules/_sqlite/
Modules/_sqlite//cursor.c:lastrowid = 
sqlite3_last_insert_rowid(self->connection->db);

When I pulled the grep out to the entire cpython repository 

$ grep -r "sqlite3_last_insert_rowid" .
Binary file 
./build/lib.macosx-10.10-x86_64-3.5-pydebug/_sqlite3.cpython-35dm-darwin.so 
matches
Binary file ./build/lib.macosx-10.10-x86_64-3.5-pydebug/_sqlite3.so matches
Binary file 
./build/temp.macosx-10.10-x86_64-3.5-pydebug/Users/alexlord/mercurial/cpython/Modules/_sqlite/cursor.o
 matches
./Modules/_sqlite/cursor.c:lastrowid = 
sqlite3_last_insert_rowid(self->connection->db);

I still didn't find anything and I'm not sure where to go from here.

--
Added file: http://bugs.python.org/file39336/sqlite_lastrowid_35.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[R. David Murray]

R. David Murray added the comment:

Made some review comments.  There also needs to be a documentation change since 
the docs currently say it is set for insert only.  There should be a .. 
versionchanged directive.  This may be small enough not to pass the what's new 
threshold, but I'd rather add a note and let the person who does the final edit 
on that document decide, so please add that as well.

(You will note that I've changed this to enhancement...it is documented as 
*only* working for INSERT, so I believe it must be treated as an enhancement to 
make it work for REPLACE.)

--
type: behavior -> enhancement
versions:  -Python 2.7, Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Alex Lord]

Alex Lord added the comment:

Patch that fixes Issue16864. Follows Jim Minters suggestion.

Unit test will reproduce the issue without the c modifications. C modifications 
fix the issue.

--
keywords: +patch
Added file: http://bugs.python.org/file34953/Issue16864_py35.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Antoine Pitrou]

Changes by Antoine Pitrou :


--
stage:  -> needs patch
type: enhancement -> behavior
versions: +Python 3.4, Python 3.5 -Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Alex Lord]

Alex Lord added the comment:

Have a unit test that replicates this bug. Working on the C code to fix it 
right now.

--
nosy: +Alex.Lord

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[R. David Murray]

R. David Murray added the comment:

Considering that sqlite's 'replace' is a synonym for 'insert or replace', I 
think the logic error is actually in the detect_statement_type function.  Since 
actions are conditionally taken on the REPLACE statement type in the code, 
including at least one that adjusts the lastrowid, I don't think the fix for 
lastrowid is as simple as just always setting it.  But I'm not that familiar 
with sqlite internals, so perhaps someone with more knowledge will weigh in.

--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue16864] sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement

[Jim Minter]

New submission from Jim Minter:

sqlite3 doesn't populate the lastrowid member of the Cursor object when a SQL 
REPLACE statement is executed.

The following snippet doesn't work as I would expect:
cursor = db.execute("REPLACE INTO table(column) VALUES ('datum')")
print cursor.lastrowid  # prints None

The following snippet, with SQL which is in effect identical to SQLite, does 
work as expected:
cursor = db.execute("INSERT OR REPLACE INTO table(column) VALUES ('datum')")
print cursor.lastrowid  # prints some rowid

Looking at Modules/_sqlite/cursor.c, in _pysqlite_query_execute(), the 
following snippet is found:

if (!multiple && statement_type == STATEMENT_INSERT) {
Py_BEGIN_ALLOW_THREADS
lastrowid = sqlite3_last_insert_rowid(self->connection->db);
Py_END_ALLOW_THREADS
self->lastrowid = PyLong_FromLong((long)lastrowid);
} else {
Py_INCREF(Py_None);
self->lastrowid = Py_None;
}

I suggest this should read something like:
if (!multiple && (statement_type == STATEMENT_INSERT || statement_type == 
STATEMENT_REPLACE)) {
instead of:
if (!multiple && statement_type == STATEMENT_INSERT) {

Thanks,

Jim

--
components: Library (Lib)
messages: 179049
nosy: jim_minter
priority: normal
severity: normal
status: open
title: sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE 
statement
type: enhancement
versions: Python 2.7, Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com