executor: support window func for aggregate without frame clause #8899

alivxxx · 2019-01-02T05:31:37Z

What problem does this PR solve?

Support the execution of aggregations without frame clause for window functions.

What is changed and how it works?

Add a new executor and evaluation framework for window functions.

Check List

Tests

Unit test

Code changes

Has exported function/method change

Side effects

None

Related changes

None

This change is

executor/window.go

expression/column.go

executor/aggregate.go

executor/builder.go

planner/core/resolve_indices.go

planner/core/logical_plan_builder.go

executor/windowfuncs/window_funcs.go

executor/window.go

codecov-io · 2019-01-07T09:26:58Z

Codecov Report

Merging #8899 into master will increase coverage by 0.01%.
The diff coverage is 63.73%.

@@            Coverage Diff             @@
##           master    #8899      +/-   ##
==========================================
+ Coverage   67.14%   67.16%   +0.01%     
==========================================
  Files         371      372       +1     
  Lines       76515    76642     +127     
==========================================
+ Hits        51379    51477      +98     
- Misses      20546    20558      +12     
- Partials     4590     4607      +17

Impacted Files	Coverage Δ
planner/core/logical_plans.go	`73.72% <ø> (ø)`	⬆️
planner/core/physical_plans.go	`60.86% <ø> (ø)`	⬆️
expression/aggregation/base_func.go	`40.9% <0%> (-9.1%)`	⬇️
executor/aggregate.go	`75.89% <100%> (+0.2%)`	⬆️
planner/core/exhaust_physical_plans.go	`92.16% <100%> (+0.06%)`	⬆️
util/chunk/chunk.go	`82.62% <100%> (+0.12%)`	⬆️
planner/core/rule_eliminate_projection.go	`95.83% <100%> (+0.07%)`	⬆️
planner/core/rule_column_pruning.go	`91.35% <100%> (+0.09%)`	⬆️
planner/core/rule_build_proj_below_agg.go	`93.75% <100%> (+9.65%)`	⬆️
planner/core/resolve_indices.go	`54.08% <60%> (+0.5%)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f18e034...85f4efe. Read the comment docs.

winoros

No mem tracker currently?

alivxxx · 2019-01-08T03:29:36Z

@winoros Yes, we can add it latter.

alivxxx · 2019-01-09T05:07:08Z

PTAL @zz-jason @eurekaka @winoros

executor/window.go

zz-jason · 2019-01-11T07:46:31Z

executor/window.go

+		return errors.Trace(err)
+	}
+
+	childResult := e.children[0].newFirstChunk()


should we allocate a Chunk with tidb_max_chunk_size capacity?

tidb_max_chunk_size may result in unnecessary results.

Yeah, to avoid unnecessary results, we should use a smaller chunk size for the first Chunk used to fetch child result. But in the next time, we should use a bigger chunk size. This function always use newFirstChunk() to fetch the child result, it's not efficient for big tables.

I tried the way mentioned in the comment, currently, we need to call chunk.Renew to create a larger chunk, and it requires to remember the last chunk... Maybe we can refine it after we can pass down the chunk size to the child.

Could you file an issue to trace this problem?

Done, #9045

executor/window.go

zz-jason · 2019-01-13T07:11:27Z

executor/window.go

+		return errors.Trace(err)
+	}
+
+	childResult := e.children[0].newFirstChunk()


Yeah, to avoid unnecessary results, we should use a smaller chunk size for the first Chunk used to fetch child result. But in the next time, we should use a bigger chunk size. This function always use newFirstChunk() to fetch the child result, it's not efficient for big tables.

util/chunk/chunk.go

executor/builder.go

zz-jason

LGTM

zz-jason · 2019-01-14T09:16:33Z

/run-all-tests

zz-jason · 2019-01-14T09:17:24Z

@XuHuaiyu @eurekaka PTAL

alivxxx · 2019-01-14T09:39:41Z

/run-all-tests

eurekaka

LGTM

alivxxx · 2019-01-15T07:19:52Z

PTAL @eurekaka, I resolved the conflicts.

alivxxx added type/enhancement The issue or PR belongs to an enhancement. status/DNM sig/execution SIG execution labels Jan 2, 2019

winoros reviewed Jan 3, 2019

View reviewed changes

executor/window.go Show resolved Hide resolved

expression/column.go Outdated Show resolved Hide resolved

alivxxx force-pushed the windowexec branch from 67add75 to 2ea0d43 Compare January 3, 2019 11:16

executor: support window func for aggregate without frame clause

d7f739b

alivxxx force-pushed the windowexec branch from 2ea0d43 to d7f739b Compare January 3, 2019 11:20

alivxxx added 3 commits January 3, 2019 19:21

update license year

9736ba3

fix error check

3eca104

refine code comments

1dbee2d

alivxxx removed the status/DNM label Jan 4, 2019

alivxxx requested review from winoros, zz-jason, XuHuaiyu and eurekaka January 4, 2019 05:27

zz-jason reviewed Jan 6, 2019

View reviewed changes

eurekaka reviewed Jan 7, 2019

View reviewed changes

executor/windowfuncs/window_funcs.go Outdated Show resolved Hide resolved

executor/window.go Outdated Show resolved Hide resolved

executor/window.go Outdated Show resolved Hide resolved

alivxxx added 3 commits January 7, 2019 17:00

address comments

b4e7336

Merge branch 'master' of github.com:pingcap/tidb into windowexec

4697196

fix ci

59c86e3

alivxxx added 2 commits January 7, 2019 18:33

address comments

313de1f

fix bug

d29cc68

winoros reviewed Jan 8, 2019

View reviewed changes

alivxxx requested review from zz-jason, eurekaka and winoros January 8, 2019 07:41

zz-jason reviewed Jan 9, 2019

View reviewed changes

executor/window.go Outdated Show resolved Hide resolved

executor/window.go Outdated Show resolved Hide resolved

executor/window.go Show resolved Hide resolved

executor/window.go Outdated Show resolved Hide resolved

let window exec only handles agg without frame now.

8a1d0ff

zz-jason reviewed Jan 11, 2019

View reviewed changes

address comments

c655de1

zz-jason reviewed Jan 13, 2019

View reviewed changes

executor/builder.go Show resolved Hide resolved

rename and add tests

7db14b1

zz-jason reviewed Jan 14, 2019

View reviewed changes

Merge branch 'master' into windowexec

b6673b9

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 14, 2019

alivxxx added 3 commits January 14, 2019 17:27

Merge branch 'master' of github.com:pingcap/tidb into windowexec

528c29d

fix merge errors

db3289a

Merge branch 'windowexec' of github.com:lamxTyler/tidb into windowexec

3f1c831

zz-jason added the status/all tests passed label Jan 14, 2019

eurekaka previously approved these changes Jan 15, 2019

View reviewed changes

eurekaka added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 15, 2019

Merge branch 'master' of github.com:pingcap/tidb into windowexec

85f4efe

alivxxx dismissed eurekaka’s stale review via 85f4efe January 15, 2019 07:13

eurekaka approved these changes Jan 15, 2019

View reviewed changes

eurekaka merged commit 3835bef into pingcap:master Jan 15, 2019

alivxxx deleted the windowexec branch January 15, 2019 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: support window func for aggregate without frame clause #8899

executor: support window func for aggregate without frame clause #8899

alivxxx commented Jan 2, 2019 •

edited

Loading

codecov-io commented Jan 7, 2019 •

edited

Loading

winoros left a comment

alivxxx commented Jan 8, 2019

alivxxx commented Jan 9, 2019

zz-jason Jan 11, 2019

alivxxx Jan 11, 2019

zz-jason Jan 13, 2019

alivxxx Jan 14, 2019

zz-jason Jan 14, 2019

zz-jason Jan 14, 2019

alivxxx Jan 14, 2019

zz-jason Jan 13, 2019

zz-jason left a comment

zz-jason commented Jan 14, 2019

zz-jason commented Jan 14, 2019

alivxxx commented Jan 14, 2019

eurekaka left a comment

alivxxx commented Jan 15, 2019

executor: support window func for aggregate without frame clause #8899

executor: support window func for aggregate without frame clause #8899

Conversation

alivxxx commented Jan 2, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov-io commented Jan 7, 2019 • edited Loading

Codecov Report

winoros left a comment

Choose a reason for hiding this comment

alivxxx commented Jan 8, 2019

alivxxx commented Jan 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Jan 14, 2019

zz-jason commented Jan 14, 2019

alivxxx commented Jan 14, 2019

eurekaka left a comment

Choose a reason for hiding this comment

alivxxx commented Jan 15, 2019

alivxxx commented Jan 2, 2019 •

edited

Loading

codecov-io commented Jan 7, 2019 •

edited

Loading