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: fix LEAD and LAG's default value can not adapt to field type #20747

Merged
merged 7 commits into from
Dec 11, 2020

Conversation

blueseason
Copy link
Contributor

@blueseason blueseason commented Nov 1, 2020

What problem does this PR solve?

Issue Number: close #11755

Problem Summary: fix LEAD and LAG's default value can not adapt to field type

What is changed and how it works?

convert LEAD/LAG default value to field type

Check List

Tests

  • Integration test

Release note

  • fix LEAD and LAG's default value can not adapt to field type

@blueseason blueseason requested a review from a team as a code owner November 1, 2020 06:24
@blueseason blueseason requested review from wshwsh12 and removed request for a team November 1, 2020 06:24
@sre-bot
Copy link
Contributor

sre-bot commented Nov 1, 2020

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@blueseason blueseason changed the title fix LEAD and LAG's default value can not adapt to field type (#11755) executor: fix LEAD and LAG's default value can not adapt to field type (#11755) Nov 1, 2020
@ichn-hu ichn-hu mentioned this pull request Nov 3, 2020
Copy link
Contributor

@wshwsh12 wshwsh12 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 Nov 16, 2020
@wshwsh12 wshwsh12 changed the title executor: fix LEAD and LAG's default value can not adapt to field type (#11755) executor: fix LEAD and LAG's default value can not adapt to field type Nov 16, 2020
@wshwsh12 wshwsh12 requested a review from qw4990 November 16, 2020 08:40
Comment on lines +682 to +684
if err1 == nil {
defaultExpr = &expression.Constant{Value: res, RetType: aggFuncDesc.RetTp}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what if err1 is not nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if err1 not null, the type conversion is failed, just do nothing and go to the normal process

@wjhuang2016
Copy link
Member

@ichn-hu PTAL

ichn-hu
ichn-hu previously approved these changes Dec 8, 2020
Copy link
Contributor

@ichn-hu ichn-hu 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 Dec 8, 2020
ti-srebot
ti-srebot previously approved these changes Dec 8, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 8, 2020
@wjhuang2016
Copy link
Member

@blueseason Please resolve the conflict.

@XuHuaiyu XuHuaiyu added the type/bugfix This PR fixes a bug. label Dec 9, 2020
@SunRunAway SunRunAway dismissed stale reviews from ti-srebot and ichn-hu via bc92025 December 10, 2020 08:05
@ti-srebot ti-srebot removed the status/LGT2 Indicates that a PR has LGTM 2. label Dec 10, 2020
@ti-srebot ti-srebot added the status/LGT3 The PR has already had 3 LGTM. label Dec 10, 2020
@SunRunAway
Copy link
Contributor

/merge

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

/run-all-tests

@ti-srebot
Copy link
Contributor

@blueseason merge failed.

@SunRunAway
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@blueseason merge failed.

@SunRunAway
Copy link
Contributor

/run-all-tests

@SunRunAway
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21660

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 6e1c2ac into pingcap:master Dec 11, 2020
@SunRunAway
Copy link
Contributor

/run-cherry-picker

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

cherry pick to release-4.0 in PR #21665

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression 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.

Window function LEAD and LAG's default value can not adapt to field type automatically
8 participants