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

*: fix a bug causes indexed virtual generated column return wrong value and refine admin check table (#18408) #19062

Merged
merged 6 commits into from
Aug 11, 2020

Conversation

wjhuang2016
Copy link
Member

What problem does this PR solve?

Issue Number: close #17989

Problem Summary:

  1. Previously, we try to use column substitution to decode a multi-level virtual generated column, which is buggy.
  2. Since admin check table uses tableReader, and tableReader can output rows with a virtual generated column already, we don't need to compute it again. Besides, the previous implementation also suffered the above problem.
    I’m

What is changed and how it works?

  1. Build virtual expression for virtual generated column in ColumnInfos2ColumnsAndNames.
  2. Remove genExprs in checkIndexValue.
  3. In compareData, don't compute the virtual generated column again.
  4. Export a function rewriteAstExpr, so that we can use rewrite in package expression.
  5. Refine buildPhysicalIndexLookUpReader, make it simple.
  6. Move table/tables/gen_expr_test.go → util/generatedexpr/gen_expr_test.go. to avoid circular reference.

What's Changed:

How it Works:

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

Release note

  • Fix a bug causes indexed virtual generated column return wrong value and refine admin check table

Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>
Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>
@bb7133
Copy link
Member

bb7133 commented Aug 11, 2020

Please fix the CI

1
Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>
1
Signed-off-by: wjhuang2016 <huangwenjun1997@gmail.com>
Copy link
Member

@bb7133 bb7133 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 added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 11, 2020
Copy link
Contributor

@djshow832 djshow832 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 Aug 11, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 11, 2020
@djshow832
Copy link
Contributor

Any conflicts?

@jebter
Copy link

jebter commented Aug 11, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 11, 2020
@wjhuang2016
Copy link
Member Author

Any conflicts?

Cherry-pick from a middle commit. Because the previous commit contains clustered indexes stuffs.

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@wjhuang2016 merge failed.

@jebter
Copy link

jebter commented Aug 11, 2020

/run-unit-test

@jebter jebter merged commit 1c8feb3 into pingcap:release-4.0 Aug 11, 2020
@wjhuang2016 wjhuang2016 deleted the virtual_index_40hotfix branch November 17, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/executor component/expression 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. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants