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

lightning: fix panic due to row's column exceeds definition #33767

Merged
merged 13 commits into from
Apr 13, 2022
Merged

lightning: fix panic due to row's column exceeds definition #33767

merged 13 commits into from
Apr 13, 2022

Conversation

buchuitoudegou
Copy link
Contributor

@buchuitoudegou buchuitoudegou commented Apr 7, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: close #33396

Problem Summary:

What is changed and how it works?

Check if the number of columns exceeds that of the permutation, which should equal to the table definition, before populating the insert statement.

Check List

Tests

  • Unit test

Side effects

Documentation

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Apr 7, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • D3Hunter
  • gozssky

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 release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 7, 2022
@buchuitoudegou
Copy link
Contributor Author

/cc @D3Hunter
/cc @gozssky

@sre-bot
Copy link
Contributor

sre-bot commented Apr 7, 2022 •
edited
Loading

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/1e8534a6e7381bb9d185c863c3877cf97c75d65e

sleepymole
sleepymole reviewed Apr 7, 2022
View reviewed changes
Copy link
Contributor

@sleepymole sleepymole left a comment

Choose a reason for hiding this comment

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

rest LGTM

br/pkg/lightning/backend/tidb/tidb_test.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 7, 2022
ngaut
ngaut reviewed Apr 7, 2022
View reviewed changes
br/pkg/lightning/backend/tidb/tidb_test.go
@@ -594,6 +594,14 @@ func encodeRowsTiDB(t *testing.T, b backend.Backend, tbl table.Table) kv.Rows {
require.NoError(t, err)

row.ClassifyAndAppend(&dataRows, &dataChecksum, &indexRows, &indexChecksum)

rawRow := make([]types.Datum, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Make slice cap with 15.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix it.

ngaut
ngaut reviewed Apr 7, 2022
View reviewed changes
br/pkg/lightning/backend/tidb/tidb_test.go
@@ -594,6 +594,14 @@ func encodeRowsTiDB(t *testing.T, b backend.Backend, tbl table.Table) kv.Rows {
require.NoError(t, err)

row.ClassifyAndAppend(&dataRows, &dataChecksum, &indexRows, &indexChecksum)

rawRow := make([]types.Datum, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Make slice cap with 15.

D3Hunter
D3Hunter approved these changes Apr 11, 2022
View reviewed changes
Copy link
Contributor

@D3Hunter D3Hunter left a comment

Choose a reason for hiding this comment

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

lgtm

br/pkg/lightning/backend/tidb/tidb_test.go

rawRow := make([]types.Datum, 14)
for i := 0; i < 15; i++ {
rawRow = append(rawRow, types.NewIntDatum(0))
Copy link
Contributor

Choose a reason for hiding this comment

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

rawRow will have 29 items, but it's ok for test.

@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 Apr 11, 2022
@buchuitoudegou
Copy link
Contributor Author

/run-integration-br-test

sleepymole
sleepymole reviewed Apr 13, 2022
View reviewed changes
br/pkg/lightning/backend/tidb/tidb_test.go Outdated
@@ -594,6 +594,14 @@ func encodeRowsTiDB(t *testing.T, b backend.Backend, tbl table.Table) kv.Rows {
require.NoError(t, err)

row.ClassifyAndAppend(&dataRows, &dataChecksum, &indexRows, &indexChecksum)

rawRow := make([]types.Datum, 14)
Copy link
Contributor

Choose a reason for hiding this comment

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

rawRow := make([]types.Datum, 15) ? 🤔

@buchuitoudegou
Copy link
Contributor Author

/run-integration-br-test

@buchuitoudegou
Copy link
Contributor Author

/run-integration-br-test

@Ehco1996
Copy link
Contributor

/run-integration-br-test

@sleepymole sleepymole mentioned this pull request Apr 13, 2022
Closed
@Ehco1996
Copy link
Contributor

/run-integration-br-test

@Ehco1996
Copy link
Contributor

/run-integration-br-test

@Ehco1996
Copy link
Contributor

/run-integration-br-test

1 similar comment
@purelind
Copy link
Contributor

/run-integration-br-test

@Ehco1996
Copy link
Contributor

/run-integration-br-test

1 similar comment
@Ehco1996
Copy link
Contributor

/run-integration-br-test

@Ehco1996
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 65e6f68

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 13, 2022
@Ehco1996
Copy link
Contributor

/hold

wait br it test passed

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2022
@Ehco1996
Copy link
Contributor

Ehco1996 commented Apr 13, 2022 •
edited
Loading

/unhlod breaked CI passed
https://ci.pingcap.net/blue/organizations/jenkins/br_ghpr_unit_and_integration_test/detail/br_ghpr_unit_and_integration_test/11185/pipeline/122

@Ehco1996
Copy link
Contributor

/unhold

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2022
@purelind
Copy link
Contributor

/run-mysql-test

@purelind
Copy link
Contributor

/run-mysql-test

@Ehco1996
Copy link
Contributor

/run-unit-test

[FAIL] planner/core TestHashPartitionPruning

@ti-chi-bot ti-chi-bot merged commit 7ca8106 into pingcap:master Apr 13, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Apr 13, 2022

TiDB MergeCI notify

🔴 Bad News! New failing [1] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci-tidb/integration-copr-test 🟥 failed 1, success 0, total 1 6 min 48 sec New failing
idc-jenkins-ci-tidb/integration-br-test ✅ all 29 tests passed 33 min Fixed
idc-jenkins-ci-tidb/common-test ✅ all 12 tests passed 7 min 22 sec Fixed
idc-jenkins-ci/integration-cdc-test 🟢 all 34 tests passed 44 min Existing passed
idc-jenkins-ci-tidb/code-coverage 🟢 Lines coverage: 63.48% 15 min Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 14 min Existing passed
idc-jenkins-ci-tidb/integration-common-test 🟢 all 11 tests passed 14 min Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 8 min 13 sec Existing passed
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 7 min 30 sec Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test 🟢 all 1 tests passed 6 min 36 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 6 min 2 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 3 min 0 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sleepymole sleepymole sleepymole approved these changes

@ngaut ngaut ngaut left review comments

@D3Hunter D3Hunter D3Hunter approved these changes

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

lightning panic due to column count mismatch
8 participants
@buchuitoudegou @ti-chi-bot @sre-bot @Ehco1996 @purelind @ngaut @D3Hunter @sleepymole