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

executor: resolve errors caused by in null in point/batch ... #18848

Merged
merged 4 commits into from
Jul 29, 2020

Conversation

qw4990
Copy link
Contributor

@qw4990 qw4990 commented Jul 29, 2020

What problem does this PR solve?

Issue Number: close #18839

Problem Summary: resolve errors caused by in null in point/batch get operators

What is changed and how it works?

Skip null values when casting.

Check List

Tests

  • Unit test

Release note

  • executor: resolve errors caused by in null in point/batch get operators

@qw4990 qw4990 added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Jul 29, 2020
@qw4990
Copy link
Contributor Author

qw4990 commented Jul 29, 2020

/run-all-tests

@tiancaiamao
Copy link
Contributor

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 29, 2020
@qw4990
Copy link
Contributor Author

qw4990 commented Jul 29, 2020

/rebuild

zz-jason
zz-jason previously approved these changes Jul 29, 2020
Copy link
Member

@zz-jason zz-jason 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-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 29, 2020
ti-srebot
ti-srebot previously approved these changes Jul 29, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 29, 2020
@@ -302,6 +302,9 @@ func (e *PointGetExecutor) get(ctx context.Context, key kv.Key) ([]byte, error)
func encodeIndexKey(e *baseExecutor, tblInfo *model.TableInfo, idxInfo *model.IndexInfo, idxVals []types.Datum, tID int64) (_ []byte, err error) {
sc := e.ctx.GetSessionVars().StmtCtx
for i := range idxVals {
if idxVals[i].IsNull() {
Copy link
Member

Choose a reason for hiding this comment

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

If we skip the null value, we built a non exists index key, I think it is not the right way to fix the bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why does the index key not exist? This loop is used to cast the idxVals to target types, which is unnecessary for null type values.
After casting, all null values will also be encoded into the key.

Copy link
Member

Choose a reason for hiding this comment

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

But if the index has a null value, it is not encoded as unique index, the rowid is appended to the key.

Copy link
Member

Choose a reason for hiding this comment

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

I mean we should let the caller know the index value has a null value, we can not point get this key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. PTAL @coocood

@imtbkcat imtbkcat modified the milestones: 4.0.4, v4.0.4 Jul 29, 2020
@imtbkcat imtbkcat added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Jul 29, 2020
@qw4990 qw4990 dismissed stale reviews from ti-srebot and zz-jason via 67a0c89 July 29, 2020 05:56
@@ -313,19 +320,19 @@ func encodeIndexKey(e *baseExecutor, tblInfo *model.TableInfo, idxInfo *model.In
} else {
idxVals[i], err = table.CastValue(e.ctx, idxVals[i], colInfo, true, false)
if types.ErrOverflow.Equal(err) {
return nil, kv.ErrNotExist
return nil, false, kv.ErrNotExist
Copy link
Member

Choose a reason for hiding this comment

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

We could return nil, true, nil, then outer do not need to check kv.ErrNotExist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing with @tiancaiamao , I change the argument nonExist to hasNull. PTAL

@coocood
Copy link
Member

coocood commented Jul 29, 2020

LGTM

@ti-srebot ti-srebot removed the status/LGT2 Indicates that a PR has LGTM 2. label Jul 29, 2020
@ti-srebot ti-srebot added the status/LGT3 The PR has already had 3 LGTM. label Jul 29, 2020
@coocood
Copy link
Member

coocood commented Jul 29, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 29, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@qw4990 merge failed.

@qw4990
Copy link
Contributor Author

qw4990 commented Jul 29, 2020

/rebuild

@qw4990
Copy link
Contributor Author

qw4990 commented Jul 29, 2020

/run-unit-test

@zz-jason zz-jason changed the title executor: resolve errors caused by in null in point/batch get operators executor: resolve errors caused by in null in point/batch ... Jul 29, 2020
@zz-jason zz-jason merged commit bc97abd into pingcap:release-4.0 Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/release-blocker This issue blocks a release. Please solve it ASAP. sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants