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

bpo-38185: Fixed case-insensitive string comparison in sqlite3.Row indexing. #16190

Conversation

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Sep 16, 2019

Copy link
Member

berkerpeksag left a comment

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)) {

This comment has been minimized.

Copy link
@ammaraskar

ammaraskar Sep 16, 2019

Member

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?

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Sep 17, 2019

Author Member

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.

This comment has been minimized.

Copy link
@ammaraskar

ammaraskar Sep 17, 2019

Member

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?

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Sep 17, 2019

Author Member

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)

This comment has been minimized.

Copy link
@ammaraskar

ammaraskar Sep 17, 2019

Member

Aah gotcha, thanks for taking the time to explain Serhiy.

@serhiy-storchaka serhiy-storchaka merged commit f669581 into python:master Sep 17, 2019
4 checks passed
4 checks passed
Azure Pipelines PR #20190916.24 succeeded
Details
bedevere/issue-number Issue number 38185 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@serhiy-storchaka serhiy-storchaka deleted the serhiy-storchaka:sqlite-row-index-case-insensitive branch Sep 17, 2019
@miss-islington

This comment has been minimized.

Copy link

miss-islington commented Sep 17, 2019

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒🤖

miss-islington added a commit to miss-islington/cpython that referenced this pull request Sep 17, 2019
…dexing. (pythonGH-16190)

(cherry picked from commit f669581)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Sep 17, 2019

GH-16216 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit to miss-islington/cpython that referenced this pull request Sep 17, 2019
…dexing. (pythonGH-16190)

(cherry picked from commit f669581)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Sep 17, 2019

GH-16217 is a backport of this pull request to the 3.7 branch.

@serhiy-storchaka

This comment has been minimized.

Copy link
Member Author

serhiy-storchaka commented Sep 17, 2019

It could be fixed even in 2.7, but I don't bother. The bug is too tiny.

miss-islington added a commit that referenced this pull request Sep 17, 2019
…dexing. (GH-16190)

(cherry picked from commit f669581)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
miss-islington added a commit that referenced this pull request Sep 17, 2019
…dexing. (GH-16190)

(cherry picked from commit f669581)

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.