-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: fix global index scan, index join, and query filtering reorg partition for global index #41992
executor: fix global index scan, index join, and query filtering reorg partition for global index #41992
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Hi @L-maple. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
e4e4aaa
to
bd5461a
Compare
/assign @mjonss |
bd5461a
to
4bac118
Compare
4bac118
to
fec2992
Compare
/cc @Defined2014 |
/cc @mjonss |
fec2992
to
7de3f70
Compare
Does the PR ready to be reviewed? Maybe you could remove |
@Defined2014 Hi, could you run /ok-to-test? |
a2f055a
to
c56ccc9
Compare
1cecff9
to
0fa36f5
Compare
0fa36f5
to
3bfdcf3
Compare
Current CI failure seems to be:
Basically this. |
3bfdcf3
to
78a5bce
Compare
78a5bce
to
7d28afc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Defined2014 please help me review this PR, thanks~ |
executor/builder.go
Outdated
tmp, ok := b.is.TableByID(ret.table.Meta().ID) | ||
if !ok { | ||
b.err = err | ||
return nil | ||
} | ||
tbl, ok := tmp.(table.PartitionedTable) | ||
if !ok { | ||
b.err = exeerrors.ErrBuildExecutor | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe could remove those codes and take L3786 - L3787 to L3766
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L3766 is actually more clean. But @mjonss told me I should check explicitly before. You can talk about it and I will follow your common advice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I handle the err (ErrTableNotExists) and still check the type transform explicitly here. @Defined2014
executor/builder.go
Outdated
if !ok { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot set err here. By the way, I think no need check ok here, just like L4612.
Also please fix other similar codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes👍🏻,You are right. But I find other places in this method also not set err. When the builder.err should be set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could change it to return nil, infoschema.ErrTableNotExists
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The err without setting maybe always equal to nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
7d28afc
to
bafae2a
Compare
/merge |
This pull request has been accepted and is ready to merge. Commit hash: bafae2a
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #41991
Issue Number: close #42065
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.