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

fix Fingerprint fi overflow cause index out of range panic #30

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

martianzhang
Copy link

@martianzhang martianzhang commented Mar 29, 2018

sql: insert into tb values(1)
Fingerprint(sql) got panic: index out of range

image

    sql: insert into tb values (1)
    Fingerprint(sql) got panic: index out of range
@CLAassistant
Copy link

CLAassistant commented Mar 29, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Mar 29, 2018

Codecov Report

Merging #30 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #30     +/-   ##
=========================================
- Coverage   63.91%   63.82%   -0.1%     
=========================================
  Files           8        8             
  Lines        1416     1418      +2     
=========================================
  Hits          905      905             
- Misses        400      401      +1     
- Partials      111      112      +1
Impacted Files Coverage Δ
query/query.go 65.5% <0%> (-0.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 847a20d...9e53ee8. Read the comment docs.

@codecov
Copy link

codecov bot commented Nov 8, 2018

Codecov Report

Merging #30 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   67.02%   67.07%   +0.04%     
==========================================
  Files           7        7              
  Lines        1389     1391       +2     
==========================================
+ Hits          931      933       +2     
  Misses        348      348              
  Partials      110      110
Impacted Files Coverage Δ
query/query.go 65.91% <100%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5cfaf6...0ec5ea7. Read the comment docs.

@rnovikovP
Copy link
Contributor

Thanks for contributing @martianzhang! Let us check this and get back to you soon

Copy link
Contributor

@percona-csalguero percona-csalguero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the code change, the tests are passing anyways.
I don't think we need to merge this.
In order to accept this change, I would like to see a failing case.

@martianzhang
Copy link
Author

@percona-csalguero I update the test case, no space between values and the left bracket will raise a panic.

@askomorokhov askomorokhov removed their request for review January 11, 2021 11:48
@askomorokhov askomorokhov removed their assignment Jan 11, 2021
TarmoKople pushed a commit to TarmoKople/go-mysql that referenced this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants