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-35398: Improve DDL statement detection for SQLite #10913

Closed
wants to merge 5 commits into from

Conversation

montanalow
Copy link

@montanalow montanalow commented Dec 5, 2018

SQLite driver depends on DDL detection to return an accurate row count for a statement. SQL statements that begin with comments currently break DDL detection, which results in incorrect row counts being returned.

https://bugs.python.org/issue35398

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@montanalow
Copy link
Author

I've signed the CLA with both my github name and BPO name.

@tirkarthi
Copy link
Member

Thanks for the PR. This would require signing a CLA as indicated. Travis fails due to indentation in your code. Please run make patchcheck to fix the whitespace errors. This would also require tests for the new code with single and multi line comments along with a NEWS entry. I would wait for inputs from others on the tracker or PR to proceed further on how this can be approached.

@ghost
Copy link

ghost commented Dec 5, 2018

You can use strncmp() rather than PyOS_strnicmp(), no need case-insensitive comparison here.
-- left by a passerby

@ghost
Copy link

ghost commented Dec 6, 2018

Using strcmp() is wrong, strncmp() is suitable.

You also need add test-cases, I guess you can modify existing test-case with some comments.

And a "News Entry", using blurb to create is very easy, see https://devguide.python.org/committing/#what-s-new-and-news-entries

@ghost
Copy link

ghost commented Dec 6, 2018

strcmp(p, "*/") will always return non-zero value, unless "*/" appears at the end of a string, because strcmp() doing comparision until the implied ending '\0'.

To be honest, I'm not very skilled too.

@montanalow
Copy link
Author

From my reading of strcmp(p, "*/") it depends on the shorter of the strings, so because it is in a loop per char, it will still terminate early, successfully. I have moved to strncmp though to avoid any ambiguity in the code for future reference.

@matrixise
Copy link
Member

@montanalow you have signed the CLA but your github is not associated to your BPO account. Could you update it because without that we can not review your code. Thank you

@brettcannon
Copy link
Member

Thanks for the PR, but closing as the CLA has not been signed within the last month. If you do decide to sign the CLA we can re-open this PR.

@coleifer
Copy link

See also #13216 which solves this much more reliably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants