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.

Sign up for GitHub

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

Jump to bottom

ddl: fix create partition table error under NO_UNSIGNED_SUBTRACTION #25435

Merged
merged 13 commits into from
Jun 18, 2021
Merged

ddl: fix create partition table error under NO_UNSIGNED_SUBTRACTION #25435

merged 13 commits into from
Jun 18, 2021

Conversation

zhuo-zhi
Copy link
Contributor

@zhuo-zhi zhuo-zhi commented Jun 15, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: close #24880, related to #24150

Problem Summary:

  • create partition table error while using 'NO_UNSIGNED_SUBTRACTION…

What is changed and how it works?

What's Changed:

  • Change some logic in some check functions when creating partition tables.

How it Works:

  • Consider session's SQLMode during correctness check when creating partition tables.
  • But the current insert behavior is not the same as MySQL. This needs to be fixed afterwards.

Check List

Tests

  • Unit test

Side effects

  • N/A

Release note

  • ddl: fix create partition table error under NO_UNSIGNED_SUBTRACTION

@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 15, 2021
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jun 15, 2021
@zhuo-zhi zhuo-zhi mentioned this pull request Jun 15, 2021
Closed
xiongjiwei
xiongjiwei reviewed Jun 16, 2021
View reviewed changes
table/tables/partition_test.go Show resolved Hide resolved
table/tables/partition_test.go Outdated Show resolved Hide resolved
table/tables/partition_test.go Outdated Show resolved Hide resolved
qw4990
qw4990 approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM, please address Jiwei's comments.

@ti-chi-bot
Copy link
Member

@qw4990: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM, please address Jiwei's comments.

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 ti-community-infra/tichi repository.

@qw4990
Copy link
Contributor

qw4990 commented Jun 16, 2021

oh... I have no privilege to approve it, PTAL @wjhuang2016

@qw4990 qw4990 requested a review from wjhuang2016 June 16, 2021 03:45
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 16, 2021
wjhuang2016
wjhuang2016 reviewed Jun 16, 2021
View reviewed changes
table/tables/partition_test.go
@@ -447,6 +447,31 @@ func (ts *testSuite) TestCreatePartitionTableNotSupport(c *C) {
c.Assert(ddl.ErrPartitionFunctionIsNotAllowed.Equal(err), IsTrue)
}

// issue 24880
func (ts *testSuite) TestRangePartitionUnderNoUnsignedSub(c *C) {
tk := testkit.NewTestKitWithInit(c, ts.store)
Copy link
Member

Choose a reason for hiding this comment

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

We‘d better check create table like too.

wjhuang2016
wjhuang2016 reviewed Jun 16, 2021
View reviewed changes
table/tables/partition_test.go
c.Assert(err, NotNil)

// test `create table like`
tk.MustExec(`CREATE TABLE tu2 like tu;`)
Copy link
Member

Choose a reason for hiding this comment

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

MySQL:

mysql> SET @@sql_mode='NO_UNSIGNED_SUBTRACTION';
Query OK, 0 rows affected (0.00 sec)

mysql> CREATE TABLE tu2 like tu;
Query OK, 0 rows affected (0.02 sec)

mysql> SET @@sql_mode='';
Query OK, 0 rows affected (0.00 sec)

mysql> CREATE TABLE tu2 like tu;
ERROR 1563 (HY000): Partition constant is out of partition function domain

Sorry, I don't think you get my point. I want to ask you to test if NO_UNSIGNED_SUBTRACTION affects create table like. To test it, you also need to set the sql_mode back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TiDB's create table like will bypass partition table checks, and can successfully create the above table no matter which SQLMode it is in. This behaves different with MySQL. Should this be fixed in this pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also update this test in the new commit.

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 17, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jun 17, 2021

PTAL @wjhuang2016

@zhuo-zhi
Copy link
Contributor Author

/run-check_dev_2

1 similar comment
@zhuo-zhi
Copy link
Contributor Author

/run-check_dev_2

@qw4990 qw4990 added the type/bugfix This PR fixes a bug. label Jun 18, 2021
@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • wjhuang2016
  • xiongjiwei

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.

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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 18, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jun 18, 2021

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Jun 18, 2021

/merge

@ti-chi-bot
Copy link
Member

@qw4990: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

In response to this:

/merge

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 ti-community-infra/tichi repository.

@qw4990 qw4990 added the sig/planner SIG: Planner label Jun 18, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jun 18, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 43874d2

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 18, 2021
@ti-chi-bot ti-chi-bot merged commit 9c947df into pingcap:master Jun 18, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jun 18, 2021

/run-cherry-picker

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 18, 2021
@zhuo-zhi @ti-srebot
cherry pick pingcap#25435 to release-5.1
9e0809b 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Jun 18, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.1 in PR #25556

ti-chi-bot pushed a commit that referenced this pull request Jun 18, 2021
@ti-srebot
ddl: fix create partition table error under NO_UNSIGNED_SUBTRACTION (#…
47b470d 
…25435) (#25556)
@mjonss mjonss mentioned this pull request Aug 4, 2021
Closed
xiongjiwei added a commit that referenced this pull request Aug 5, 2021
@xiongjiwei
Revert "ddl: fix create partition table error under NO_UNSIGNED_SUBTR…
e73ab7b 
…ACTION (#25435)"

This reverts commit 9c947df.
@xiongjiwei xiongjiwei mentioned this pull request Aug 5, 2021
Merged
12 tasks
This was referenced Aug 10, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiongjiwei xiongjiwei xiongjiwei approved these changes

@wjhuang2016 wjhuang2016 wjhuang2016 approved these changes

@qw4990 qw4990 qw4990 approved these changes

Assignees
No one assigned
Labels
needs-cherry-pick-release-5.1 sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra size/M Denotes a PR that changes 30-99 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.

create partition table error while using 'NO_UNSIGNED_SUBTRACTION sql mode
6 participants
@zhuo-zhi @ti-chi-bot @qw4990 @ti-srebot @wjhuang2016 @xiongjiwei