Skip to content

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

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Sep 16, 2019

Copy link
Member

@berkerpeksag berkerpeksag left a 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)) {
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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)

Copy link
Member

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.

@serhiy-storchaka serhiy-storchaka merged commit f669581 into python:master Sep 17, 2019
@serhiy-storchaka serhiy-storchaka deleted the sqlite-row-index-case-insensitive branch September 17, 2019 06:20
@miss-islington
Copy link
Contributor

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

miss-islington pushed 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 <[email protected]>
@bedevere-bot
Copy link

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

miss-islington pushed 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 <[email protected]>
@bedevere-bot
Copy link

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

@serhiy-storchaka
Copy link
Member Author

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 <[email protected]>
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants