GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-35398: Improve DDL statement detection for SQLite by montanalow · Pull Request #10913 · python/cpython

Conversation

@montanalow

@montanalow montanalow commented Dec 5, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@montanalow
improve ddl statement detection against leading commentss
c517ef2
@the-knights-who-say-ni

the-knights-who-say-ni commented Dec 5, 2018

Copy link
Copy Markdown

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Dec 5, 2018
@bedevere-bot bedevere-bot added the awaiting review label Dec 5, 2018
@montanalow

montanalow commented Dec 5, 2018

Copy link
Copy Markdown
Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

montanalow added 2 commits December 4, 2018 20:03
@montanalow
convert tabs to spaces
39c742f
@montanalow
one more tab
57d466f
@tirkarthi

tirkarthi commented Dec 5, 2018

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost

ghost commented Dec 5, 2018

Copy link
Copy Markdown

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

montanalow reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@montanalow
Use strcmp instead of unnecessary PyOS_strnicmp
b112167
@ghost

ghost commented Dec 6, 2018

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@montanalow
use strncmp over strcmp
1e8fd4c
@ghost

ghost commented Dec 6, 2018

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@montanalow

montanalow commented Dec 6, 2018

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@matrixise

matrixise commented Feb 14, 2019

Copy link
Copy Markdown
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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brettcannon brettcannon removed the CLA not signed label Mar 29, 2019
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Mar 29, 2019
@brettcannon

brettcannon commented Mar 29, 2019

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brettcannon brettcannon closed this Mar 29, 2019
@coleifer

coleifer commented May 11, 2019

Copy link
Copy Markdown

See also #13216 which solves this much more reliably.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@coleifer coleifer mentioned this pull request May 11, 2019
bpo-36859: Use sqlite3_stmt_readonly API when possible to determine if statement is DML.K #13216
Closed
@montanalow montanalow mannequin mentioned this pull request May 16, 2022
SQLite incorrect row count for UPDATE #79579
Closed
@ghost ghost mentioned this pull request Jun 6, 2022
gh-79579: Update sqlite3.Cursor.rowcount for all data-modifying queries #93532
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@montanalow @the-knights-who-say-ni @tirkarthi @matrixise @brettcannon @coleifer @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.