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

expression: avoid Order By constant expression error when plan cache is enabled #16261

Merged
merged 3 commits into from
Apr 21, 2020

Conversation

eurekaka
Copy link
Contributor

@eurekaka eurekaka commented Apr 9, 2020

What problem does this PR solve?

Issue Number: close #16203

Problem Summary:

Error is reported for order by rand() when plan cache is enabled.

What is changed and how it works?

What's Changed:

  • remove Rand / Sysdate / UUID from DeferredFunctions, because they should not be folded into constant according to unFoldableFunctions, otherwise, they would be converted to a Constant with DeferredExpr in funcCallToExpression. This change makes the behavior regarding these functions consistent when plan cache is enabled / disabled.

  • check if the order by items can be treated as constants during execution using a new function, instead of IsMutableEffectsExpr, which is specifically designed for de-duplicating expressions.

How it Works:

Related previous PRs for this change:

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

N/A

Release note

@eurekaka eurekaka requested review from a team as code owners April 9, 2020 15:42
@ghost ghost requested review from lzmhhh123 and wshwsh12 and removed request for a team April 9, 2020 15:43
@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #16261 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #16261   +/-   ##
===========================================
  Coverage   80.6437%   80.6437%           
===========================================
  Files           506        506           
  Lines        138441     138441           
===========================================
  Hits         111644     111644           
  Misses        18193      18193           
  Partials       8604       8604           

@zz-jason
Copy link
Member

zz-jason commented Apr 9, 2020

should we also cherry-pick this bugfix to release-2.1, release-3.0 and release-3.1?

@eurekaka
Copy link
Contributor Author

should we also cherry-pick this bugfix to release-2.1, release-3.0 and release-3.1?

@zz-jason plan cache is not GA for those branches, so we don't need cherry-picks for them?

@zz-jason
Copy link
Member

zz-jason commented Apr 10, 2020

Users may use plan cache in these old releases. If they met this bug and not willing to upgrade the cluster to release-4.0, maybe they can update there cluster to the latest version in the corresponding release branch.

@danmay319
Copy link
Contributor

LGTM

Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123 lzmhhh123 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Apr 21, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 21, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Apr 21, 2020

@eurekaka merge failed.

@eurekaka
Copy link
Contributor Author

/run-unit-test

2 similar comments
@eurekaka
Copy link
Contributor Author

/run-unit-test

@eurekaka
Copy link
Contributor Author

/run-unit-test

@zz-jason zz-jason merged commit 692e009 into pingcap:master Apr 21, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 21, 2020

cherry pick to release-3.0 in PR #16673

@sre-bot
Copy link
Contributor

sre-bot commented Apr 21, 2020

cherry pick to release-3.1 in PR #16674

@sre-bot
Copy link
Contributor

sre-bot commented Apr 21, 2020

cherry pick to release-4.0 in PR #16675

@SunRunAway
Copy link
Contributor

Should LogicalTopN also be modified?

eurekaka added a commit to sre-bot/tidb that referenced this pull request Apr 23, 2020
eurekaka added a commit to sre-bot/tidb that referenced this pull request Apr 23, 2020
eurekaka added a commit that referenced this pull request Apr 28, 2020
Signed-off-by: sre-bot <[email protected]>

Co-authored-by: Kenan Yao <[email protected]>
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/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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plan cache: expression.Expression is *expression.Constant, not *expression.Column
6 participants