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

gh-93421: Update sqlite3 cursor.rowcount after SQLITE_DONE #93526

Merged
merged 9 commits into from Jun 8, 2022

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jun 6, 2022

Alternative to #93520
Resolves #93421

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 6, 2022

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit bb4062a 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots label Jun 6, 2022
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 6, 2022

FYI, the failing buildbot are also failing on main; they are unrelated to this PR.

@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots label Jun 6, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 6, 2022

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 731dfc5 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots label Jun 6, 2022
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 7, 2022

I'm going to merge this in a day or two.

The buildbot failures are unrelated to this PR.

@animalize
Copy link
Contributor

@animalize animalize commented Jun 8, 2022

rowcount's type should also be changed.
Otherwise on 32-bit big-endian platform, it will always be 0.

-{"rowcount", T_LONG,     offsetof(pysqlite_Cursor, rowcount), READONLY},
+{"rowcount", T_LONGLONG, offsetof(pysqlite_Cursor, rowcount), READONLY},

lastrowid's type is T_OBJECT, an int object is generated even if the user doesn't access it. we can keep it as integer type. But if access it multiple times, it will be slower.
Of course, this is another issue.

@animalize

This comment was marked as off-topic.

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 8, 2022

rowcount's type should also be changed. Otherwise on 32-bit big-endian platform, it will always be 0.

Good catch! long long is guaranteed to be at least 64 bits.

How about only apply 64-bit function to 3.12 branch? It's more or less a new feature.

I'm fine with backporting that, but I'll remove it from this PR nonetheless. It makes the PR cleaner.

These logical branches can be refactored for 3.12 branch.

Yes, we could do a lot of refactoring in the whole sqlite3 module. I prefer to keep refactorings in separate PRs.

Thanks for reviewing!

@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots label Jun 8, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 8, 2022

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 0ebf8a8 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots label Jun 8, 2022
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 8, 2022

FTR, buildbot failures are unrelated to this PR.

@erlend-aasland erlend-aasland merged commit 875de61 into python:main Jun 8, 2022
73 of 79 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jun 8, 2022

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒🤖

@erlend-aasland erlend-aasland deleted the sqlite-changes-alt branch Jun 8, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 8, 2022

GH-93598 is a backport of this pull request to the 3.11 branch.

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jun 8, 2022

Sorry, @erlend-aasland, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 875de61c296604f3a3a51e9d76355e0f1a24c6af 3.10

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 8, 2022
…pythonGH-93526)

(cherry picked from commit 875de61)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jun 8, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 8, 2022

GH-93599 is a backport of this pull request to the 3.10 branch.

miss-islington added a commit that referenced this issue Jun 8, 2022
…3526)

(cherry picked from commit 875de61)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants