-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-38185: Fixed case-insensitive string comparison in sqlite3.Row indexing. #16190
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
bpo-38185: Fixed case-insensitive string comparison in sqlite3.Row indexing. #16190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find! The PR looks good to me. I probably wouldn't bother fixing it in 3.7, but it's ultimately up to you :)
if (!PyUnicode_Check(left) || !PyUnicode_Check(right)) { | ||
return 0; | ||
} | ||
if (!PyUnicode_IS_ASCII(left) || !PyUnicode_IS_ASCII(right)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looks like sqlite supports non-ascii characters as identifiers: https://github.com/mackyle/sqlite/blob/a419afd73a544e30df878db55f7faa17790c01bd/ext/misc/normalize.c#L174-L177
And as shown in your example on bpo, should we maybe just simplify this code and do a casefold
comparison?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial version used casefold
. But then I made some experiments and found that Sqlite itself ignores case only for ASCII letters. It was just my guess, and thank you for confirming it with the reference to sources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah okay, I thought the case-insensitivity for the Row
object was for the convenience of the Python API, not trying to mirror any particular part of sqlite itself. Which part of sqlite ignores case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which part of sqlite ignores case?
I guess any part which compare identifiers. For example, the following code passes:
create table test(a text)
select A from test
and the following fails:
create table test(a text, A text)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah gotcha, thanks for taking the time to explain Serhiy.
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8. |
…dexing. (pythonGH-16190) (cherry picked from commit f669581) Co-authored-by: Serhiy Storchaka <[email protected]>
GH-16216 is a backport of this pull request to the 3.8 branch. |
…dexing. (pythonGH-16190) (cherry picked from commit f669581) Co-authored-by: Serhiy Storchaka <[email protected]>
GH-16217 is a backport of this pull request to the 3.7 branch. |
It could be fixed even in 2.7, but I don't bother. The bug is too tiny. |
…dexing. (GH-16190) (cherry picked from commit f669581) Co-authored-by: Serhiy Storchaka <[email protected]>
…dexing. (GH-16190) (cherry picked from commit f669581) Co-authored-by: Serhiy Storchaka <[email protected]>
https://bugs.python.org/issue38185