Skip to content

planner: fix wrong type when unsigned int value union int value#26545

Merged
ti-chi-bot merged 20 commits intopingcap:masterfrom
sylzd:fix_24953
Aug 11, 2021
Merged

planner: fix wrong type when unsigned int value union int value#26545
ti-chi-bot merged 20 commits intopingcap:masterfrom
sylzd:fix_24953

Conversation

@sylzd
Copy link
Contributor

@sylzd sylzd commented Jul 26, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: close #24953

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

  • fix wrong type when unsigned int value union int value

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 26, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • XuHuaiyu
  • ichn-hu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 26, 2021
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 26, 2021

@sylzd: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jul 26, 2021
@sylzd
Copy link
Contributor Author

sylzd commented Jul 26, 2021 •
edited
Loading

@XuHuaiyu @ichn-hu

It's conflict with the UT TestUnionAutoSignedCast. Could I change the line:

	tk.MustQuery("select id, i, b, d, dd from t2 union select id, i, b, d, dd from t1 order by id").
		Check(testkit.Rows("1 0 0 0 -1", "2 1 1 1.1 1"))

to

	tk.MustQuery("select id, i, b, d, dd from t2 union select id, i, b, d, dd from t1 order by id").
		Check(testkit.Rows("1 -1 -1 -1.1 -1", "2 1 1 1.1 1"))

@tisonkun
Copy link
Contributor

tisonkun commented Jul 26, 2021 •
edited
Loading

Thanks for your contribution @sylzd ! you can use /cc @reviewer to request review.

/cc @XuHuaiyu @ichn-hu

@ti-chi-bot ti-chi-bot requested review from XuHuaiyu and ichn-hu July 26, 2021 15:56
@tisonkun
Copy link
Contributor

tisonkun commented Jul 26, 2021

Also would you setup a link to the correct issue? #24593 is an unrelated PR that closed.

@sylzd
Copy link
Contributor Author

sylzd commented Jul 27, 2021

Also would you setup a link to the correct issue? #24593 is an unrelated PR that closed.

Certainly! I made a mistake, it's #24953!

@ichn-hu ichn-hu mentioned this pull request Jul 27, 2021
Closed
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2021
@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Jul 30, 2021

I've test the failed case in MySQL 5.7 and MySQL 8.0

The failed case seems to be a bug in MySQL 5.7.
But it was fixed in MySQL 8.0

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Jul 30, 2021

I found a similar MySQL bug issue https://bugs.mysql.com/bug.php?id=95148.
I think we can modify the test result of the test cases if we've confirmed in MySQL8.0

@sylzd
Copy link
Contributor Author

sylzd commented Jul 31, 2021

I found a similar MySQL bug issue https://bugs.mysql.com/bug.php?id=95148.
I think we can modify the test result of the test cases if we've confirmed in MySQL8.0

So, you mean I can modify like this?

@XuHuaiyu @ichn-hu

It's conflict with the UT TestUnionAutoSignedCast. Could I change the line:

	tk.MustQuery("select id, i, b, d, dd from t2 union select id, i, b, d, dd from t1 order by id").
		Check(testkit.Rows("1 0 0 0 -1", "2 1 1 1.1 1"))

to

	tk.MustQuery("select id, i, b, d, dd from t2 union select id, i, b, d, dd from t1 order by id").
		Check(testkit.Rows("1 -1 -1 -1.1 -1", "2 1 1 1.1 1"))

@sylzd
Copy link
Contributor Author

sylzd commented Aug 1, 2021

I found a similar MySQL bug issue https://bugs.mysql.com/bug.php?id=95148.
I think we can modify the test result of the test cases if we've confirmed in MySQL8.0

refer to https://bugs.mysql.com/bug.php?id=95148, I changed the line, PTAL~

@sylzd
Copy link
Contributor Author

sylzd commented Aug 1, 2021

/cc @XuHuaiyu @ichn-hu

@github-actions github-actions bot added the sig/execution SIG execution label Aug 1, 2021
@zhouqiang-cl zhouqiang-cl removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 4, 2021
XuHuaiyu
XuHuaiyu approved these changes Aug 4, 2021
View reviewed changes
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.

LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 4, 2021
@XuHuaiyu XuHuaiyu added the type/bugfix This PR fixes a bug. label Aug 4, 2021
@bb7133
Copy link
Member

bb7133 commented Aug 5, 2021

/run-integration-test

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 10, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2021
@ichn-hu
Copy link
Contributor

ichn-hu commented Aug 11, 2021

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 11, 2021

This pull request has been accepted and is ready to merge.

DetailsCommit hash: 4b40b3c

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 11, 2021
@sylzd
Copy link
Contributor Author

sylzd commented Aug 11, 2021

/run-all-tests

1 similar comment
@sylzd
Copy link
Contributor Author

sylzd commented Aug 11, 2021

/run-all-tests

@ti-chi-bot ti-chi-bot merged commit d4208b5 into pingcap:master Aug 11, 2021
@zhouqiang-cl zhouqiang-cl mentioned this pull request Aug 11, 2021
Closed
@XuHuaiyu XuHuaiyu added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

@ichn-hu ichn-hu ichn-hu approved these changes

Assignees

No one assigned

Labels

compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. component/expression sig/execution SIG execution size/S Denotes a PR that changes 10-29 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

executor: select with join return inconsistent result with MySQL

7 participants

@sylzd @ti-chi-bot @tisonkun @XuHuaiyu @bb7133 @ichn-hu @zhouqiang-cl