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

planner: reimplement DEFAULT function to make the behavior consistent with MySQL when looking up the corresponding column #19709

Merged
merged 22 commits into from
Sep 18, 2020

Conversation

rebelice
Copy link
Contributor

@rebelice rebelice commented Sep 2, 2020

What problem does this PR solve?

Issue Number: close #17807

Problem Summary:

The previous Default function parsed to generate ast.ColumnName, and then further parsed the corresponding column. But in fact, the Default function does not need to obtain the value of the corresponding column from the storage, so it should not be further parsed by ast.ColumnName.

The previous implementation produced many incompatibility issues with MySQL, such as:

Have to merge pingcap/parser#1027 first.

What is changed and how it works?

Proposal: xxx

What's Changed:

  • Modify the parser so that DEFAULT function is not generated ast.ColumnName.
  • Add new field allNames for PlanBuilder and expressionRewriter to record the set of columns that can be used to find in evalDefaultExpr.
  • Find the corresponding column in evalDefaultExpr. And frist, it will look for allNames. If it is not found, it will search in expressionRewrite.names.

Check List

Tests

  • Unit test

Side effects

Release note

  • reimplement DEFAULT function to make the behavior consistent with MySQL when looking up the corresponding column

@rebelice
Copy link
Contributor Author

rebelice commented Sep 2, 2020

/label component/planner

@ti-srebot
Copy link
Contributor

These labels are not found component/planner.

@rebelice
Copy link
Contributor Author

rebelice commented Sep 2, 2020

/label sig/planner

@ti-srebot ti-srebot added the sig/planner SIG: Planner label Sep 2, 2020
@qw4990 qw4990 self-requested a review September 2, 2020 08:21
@rebelice
Copy link
Contributor Author

rebelice commented Sep 2, 2020

/run-all-tests

@ichn-hu
Copy link
Contributor

ichn-hu commented Sep 2, 2020

/run-all-tests parser=pr/1007

go.mod Outdated Show resolved Hide resolved
@rebelice
Copy link
Contributor Author

rebelice commented Sep 2, 2020

/run-all-tests parser=pr/1007

@rebelice
Copy link
Contributor Author

rebelice commented Sep 2, 2020

/run-unit-test parser=pr/1007

1 similar comment
@rebelice
Copy link
Contributor Author

rebelice commented Sep 3, 2020

/run-unit-test parser=pr/1007

@qw4990
Copy link
Contributor

qw4990 commented Sep 8, 2020

Please fix conflicts @rebelice .

@rebelice rebelice dismissed stale reviews from qw4990 and ichn-hu via 3fd0e5f September 18, 2020 09:47
@rebelice
Copy link
Contributor Author

/run-all-tests

@rebelice
Copy link
Contributor Author

/run-all-tests

@rebelice
Copy link
Contributor Author

/run-sqllogic-test-2

@rebelice
Copy link
Contributor Author

/run-common-test

@ichn-hu
Copy link
Contributor

ichn-hu commented Sep 18, 2020

/merge

@ti-srebot
Copy link
Contributor

@ichn-hu Oops! auto merge is restricted to Committers of the SIG.See the corresponding SIG page for more information. Related SIGs: execution(slack),planner(slack).

@ti-srebot ti-srebot removed the status/LGT2 Indicates that a PR has LGTM 2. label Sep 18, 2020
@ti-srebot ti-srebot added the status/LGT3 The PR has already had 3 LGTM. label Sep 18, 2020
@lzmhhh123
Copy link
Contributor

/run-all-tests

@lzmhhh123
Copy link
Contributor

/run-check_dev

@rebelice
Copy link
Contributor Author

/label needs-cherry-pick-4.0

@rebelice
Copy link
Contributor Author

/run-cherry-picker

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Nov 26, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #21316

qw4990 pushed a commit that referenced this pull request Nov 30, 2020
… with MySQL when looking up the corresponding column (#19709) (#21316)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression sig/planner SIG: Planner status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P1-[4.0 bug hunting]-[SQL Plan Management]-Using the DEFAULT() function results in an ambiguous column error
6 participants