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

server: make long-data type no longer check nullBitmap #7573

Merged
merged 6 commits into from
Sep 5, 2018
Merged

server: make long-data type no longer check nullBitmap #7573

merged 6 commits into from
Sep 5, 2018

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Aug 31, 2018

What problem does this PR solve?

fixes #7572

mariadb send none zero value in nullBitmap, tidb follow protocol docs to check nullBitmap and got nul value

but after experiment and check code, nullBitmap is nouse for LONG_DATA_VALUE(and it's NULL value seem be magic), so we need skip them, and make mariadb client user work.

What is changed and how it works?

if param is long_data_value then skip nullBitmap check.

Check List

use mariadb client can reproduce it, after this PR fixed.

Code changes

  • impl

Side effects

  • no

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

This change is Reviewable

@lysu lysu changed the title server: make long data type no longer check nullBitmap server: make long-data type no longer check nullBitmap Aug 31, 2018
@lysu
Copy link
Contributor Author

lysu commented Sep 3, 2018

/run-all-tests

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp added status/all tests passed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 3, 2018
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

Can we construct any test cases for this?

@lysu
Copy link
Contributor Author

lysu commented Sep 3, 2018

@XuHuaiyu yes, I will do it.

@lysu
Copy link
Contributor Author

lysu commented Sep 3, 2018

/run-all-tests tidb-test=pr/615

Copy link
Contributor

@zhexuany zhexuany left a comment

Choose a reason for hiding this comment

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

We need a unit test.

if boundParams[i] != nil {
args[i] = boundParams[i]
continue
}

if nullBitmap[i>>3]&(1<<(uint(i)%8)) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment for the modification.

@lysu
Copy link
Contributor Author

lysu commented Sep 4, 2018

@zhexuany https://github.com/pingcap/tidb-test/pull/615 integration test is enough, fake byte array ut is meanless and hard maintain.

@zhexuany
Copy link
Contributor

zhexuany commented Sep 4, 2018

Gotcha.

@shenli
Copy link
Member

shenli commented Sep 5, 2018

LGTM

@shenli shenli added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 5, 2018
@lysu lysu merged commit 4538bcd into pingcap:master Sep 5, 2018
@lysu lysu deleted the fix-mariadb-c-api-insert branch September 5, 2018 09:31
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.

prepare insert/update long-data type cols got null using mariadb connector-c
5 participants