planner: fix compatibility bug for window function #11488

lzmhhh123 · 2019-07-29T03:43:21Z

What problem does this PR solve?

What is changed and how it works?

The error judging should not be involved in the build function. It caused the wrong frame like the default frame to check. That's a fault.

Check List

Tests

Unit test
Integration test

Code changes

None

Related changes

Need to cherry-pick to the release branch
Need to change common test

codecov · 2019-07-29T03:51:46Z

Codecov Report

Merging #11488 into master will decrease coverage by 0.4649%.
The diff coverage is 0%.

@@               Coverage Diff               @@
##             master     #11488       +/-   ##
===============================================
- Coverage   81.8068%   81.3418%   -0.465%     
===============================================
  Files           424        424               
  Lines         92738      90920     -1818     
===============================================
- Hits          75866      73956     -1910     
- Misses        11538      11639      +101     
+ Partials       5334       5325        -9

codecov · 2019-07-29T03:51:46Z

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11488   +/-   ##
===========================================
  Coverage   81.3703%   81.3703%           
===========================================
  Files           425        425           
  Lines         91526      91526           
===========================================
  Hits          74475      74475           
  Misses        11717      11717           
  Partials       5334       5334

planner/core/logical_plan_builder.go

winoros · 2019-07-29T08:44:01Z

I think the codes are not special for DENSE_RANK. It seems that it would influence all window functions?

lzmhhh123 · 2019-07-29T08:45:02Z

@winoros Yes. It is better to change the description.

winoros

lgtm

SunRunAway · 2019-07-29T08:51:04Z

planner/core/logical_plan_builder.go

+			if end.Type == ast.Following && !end.UnBounded {
+				return spec, false, ErrWindowRangeFrameOrderType.GenWithStackByArgs(getWindowName(spec.Name.O))
+			}
+		}


Your code is for frame validation, is that suitable to be put in handleDefaultFrame?

Besides, the ErrWindowRangeFrameOrderType error is needFrame irrelevant, considering this case

SELECT FIRST_VALUE(fieldA) OVER w1 AS 'dense_rank', fieldA, fieldB FROM ( SELECT `col_time` AS fieldA, `col_time` AS fieldB FROM `table20_int_autoinc` ) as t WINDOW w1 AS ( PARTITION BY fieldB ORDER BY fieldB DESC, fieldA DESC RANGE BETWEEN CURRENT ROW AND 1 FOLLOWING);

Actually, the handleDefaultFrame is a function to remove the invalidated frames. So it's reasonable to put it here.

And there are much more frame validations in buildWindowFunctionFrame.

The reasonable order for building a window function is

build from ast

validate

handleDefaultFrame

May also fix #11001 #11002, and make #11011 easy to fix.

Now, the frame validations are after groupWindowFuncs. We have to handleDefaultFrame in groupWindowFuncs. It hard to move this validation to buildWindowFunctionFrame...

lzmhhh123 · 2019-07-29T09:01:02Z

/rebuild

lzmhhh123 · 2019-07-29T09:01:14Z

/run-all-tests

lzmhhh123 · 2019-07-30T06:59:43Z

/run-all-tests

…o dev/fix_11010

alivxxx

LGTM

SunRunAway

LGTM

sre-bot · 2019-07-30T08:53:21Z

/run-all-tests

sre-bot · 2019-07-30T08:55:27Z

@lzmhhh123 merge failed.

lzmhhh123 · 2019-07-31T02:47:53Z

/run-all-tests

lzmhhh123 · 2019-07-31T03:00:25Z

/run-common-test tidb-test=pr/869

lzmhhh123 · 2019-07-31T03:11:54Z

/run-common-test tidb-test=pr/869

lzmhhh123 · 2019-07-31T03:19:41Z

/run-common-test tidb-test=pr/869
/run-integration-common-test tidb-test=pr/869

lzmhhh123 · 2019-07-31T03:32:01Z

/run-common-test tidb-test=pr/869
/run-integration-common-test tidb-test=pr/869

lzmhhh123 · 2019-07-31T03:42:47Z

/run-all-tests

sre-bot · 2019-07-31T03:45:08Z

/run-all-tests

sre-bot · 2019-07-31T03:54:37Z

cherry pick to release-3.0 failed

lzmhhh123 requested review from SunRunAway and alivxxx July 29, 2019 03:49

lzmhhh123 added sig/planner SIG: Planner needs-cherry-pick-3.0 type/bugfix This PR fixes a bug. type/compatibility labels Jul 29, 2019

alivxxx reviewed Jul 29, 2019

View reviewed changes

planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved

lzmhhh123 added the status/WIP label Jul 29, 2019

lzmhhh123 removed the request for review from SunRunAway July 29, 2019 05:19

lzmhhh123 force-pushed the dev/fix_11010 branch from e627647 to 4e02558 Compare July 29, 2019 08:36

lzmhhh123 removed the status/WIP label Jul 29, 2019

lzmhhh123 requested review from alivxxx, SunRunAway and winoros July 29, 2019 08:38

lzmhhh123 changed the title ~~planner: fix compatibility bug for DENSE_RANK window function~~ planner: fix compatibility bug for window function Jul 29, 2019

winoros reviewed Jul 29, 2019

View reviewed changes

SunRunAway reviewed Jul 29, 2019

View reviewed changes

lzmhhh123 added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 29, 2019

extract error judge from build and make judgment to origin spec

dd1a6ad

lzmhhh123 force-pushed the dev/fix_11010 branch from 4e02558 to dd1a6ad Compare July 30, 2019 06:52

lzmhhh123 removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 30, 2019

lzmhhh123 requested review from winoros and SunRunAway July 30, 2019 06:54

remove unnecessary judgment

3bebdd3

Merge branch 'master' into dev/fix_11010

320eb11

lzmhhh123 added 2 commits July 30, 2019 15:04

refine code

adbde5e

Merge branch 'dev/fix_11010' of https://github.com/lzmhhh123/tidb int…

f1ae089

…o dev/fix_11010

alivxxx reviewed Jul 30, 2019

View reviewed changes

SunRunAway approved these changes Jul 30, 2019

View reviewed changes

SunRunAway added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Jul 30, 2019

lzmhhh123 added 2 commits July 30, 2019 17:33

Merge branch 'master' into dev/fix_11010

ae2cd1a

Merge branch 'master' into dev/fix_11010

48c3e01

lzmhhh123 added status/all tests passed status/can-merge Indicates a PR has been approved by a committer. and removed status/can-merge Indicates a PR has been approved by a committer. labels Jul 31, 2019

Merge branch 'master' into dev/fix_11010

3ed10e0

sre-bot merged commit 13778fe into pingcap:master Jul 31, 2019

lzmhhh123 added a commit to lzmhhh123/tidb that referenced this pull request Jul 31, 2019

planner: fix compatibility bug for window function (pingcap#11488)

86a02ad

lzmhhh123 mentioned this pull request Jul 31, 2019

planner: fix compatibility bug for window function (#11488) #11543

Merged

zz-jason pushed a commit that referenced this pull request Jul 31, 2019

planner: fix compatibility bug for window function (#11488) (#11543)

16a81b6

lzmhhh123 deleted the dev/fix_11010 branch August 14, 2019 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: fix compatibility bug for window function #11488

planner: fix compatibility bug for window function #11488

lzmhhh123 commented Jul 29, 2019 •

edited

Loading

codecov bot commented Jul 29, 2019

codecov bot commented Jul 29, 2019 •

edited

Loading

winoros commented Jul 29, 2019 •

edited

Loading

lzmhhh123 commented Jul 29, 2019

winoros left a comment

SunRunAway Jul 29, 2019

SunRunAway Jul 29, 2019

lzmhhh123 Jul 29, 2019

alivxxx Jul 29, 2019

SunRunAway Jul 29, 2019

lzmhhh123 Jul 29, 2019

lzmhhh123 commented Jul 29, 2019

lzmhhh123 commented Jul 29, 2019

lzmhhh123 commented Jul 30, 2019

alivxxx left a comment

SunRunAway left a comment

sre-bot commented Jul 30, 2019

sre-bot commented Jul 30, 2019

lzmhhh123 commented Jul 31, 2019

lzmhhh123 commented Jul 31, 2019

lzmhhh123 commented Jul 31, 2019

lzmhhh123 commented Jul 31, 2019

lzmhhh123 commented Jul 31, 2019

lzmhhh123 commented Jul 31, 2019

sre-bot commented Jul 31, 2019

sre-bot commented Jul 31, 2019

planner: fix compatibility bug for window function #11488

planner: fix compatibility bug for window function #11488

Conversation

lzmhhh123 commented Jul 29, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Jul 29, 2019

Codecov Report

codecov bot commented Jul 29, 2019 • edited Loading

Codecov Report

winoros commented Jul 29, 2019 • edited Loading

lzmhhh123 commented Jul 29, 2019

winoros left a comment

Choose a reason for hiding this comment

SunRunAway Jul 29, 2019

Choose a reason for hiding this comment

SunRunAway Jul 29, 2019

Choose a reason for hiding this comment

lzmhhh123 Jul 29, 2019

Choose a reason for hiding this comment

alivxxx Jul 29, 2019

Choose a reason for hiding this comment

SunRunAway Jul 29, 2019

Choose a reason for hiding this comment

lzmhhh123 Jul 29, 2019

Choose a reason for hiding this comment

lzmhhh123 commented Jul 29, 2019

lzmhhh123 commented Jul 29, 2019

lzmhhh123 commented Jul 30, 2019

alivxxx left a comment

Choose a reason for hiding this comment

SunRunAway left a comment

Choose a reason for hiding this comment

sre-bot commented Jul 30, 2019

sre-bot commented Jul 30, 2019

lzmhhh123 commented Jul 31, 2019

lzmhhh123 commented Jul 31, 2019

lzmhhh123 commented Jul 31, 2019

lzmhhh123 commented Jul 31, 2019

lzmhhh123 commented Jul 31, 2019

lzmhhh123 commented Jul 31, 2019

sre-bot commented Jul 31, 2019

sre-bot commented Jul 31, 2019

lzmhhh123 commented Jul 29, 2019 •

edited

Loading

codecov bot commented Jul 29, 2019 •

edited

Loading

winoros commented Jul 29, 2019 •

edited

Loading