executor: support MAX/MIN in new evaluation framework partially #6971

XuHuaiyu · 2018-07-03T11:26:21Z

What have you changed? (mandatory)

This PR implements max/min partially with new aggregation framework:
max/min( int )
max/min( float32 )
max/min( float64 )
max/min( decimal )

What are the type of the changes (mandatory)?

improvement

How has this PR been tested (mandatory)?

the existing test cases

Does this PR affect documentation (docs/docs-cn) update? (optional)

no

Refer to a related PR or issue link (optional)

#6952 #6852

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

…actor/max_min Conflicts: executor/aggfuncs/aggfuncs.go executor/aggfuncs/builder.go

XuHuaiyu · 2018-07-03T11:27:21Z

PTAL @zz-jason

zz-jason · 2018-07-03T12:42:27Z

executor/aggfuncs/func_max_min.go

+		if isNull {
+			continue
+		}
+		if !e.isUnsigned {


put this if branch outside the for loop

zz-jason · 2018-07-03T12:44:58Z

executor/aggfuncs/func_max_min.go

+}
+
+func (e *maxMin4Int) UpdatePartialResult(sctx sessionctx.Context, rowsInGroup []chunk.Row, pr PartialResult) error {
+	tmp := (*partialResult4MaxMinInt)(pr)


to keep consistent with avg implementations: s/tmp/p/

zz-jason · 2018-07-03T12:45:26Z

executor/aggfuncs/func_max_min.go

+			continue
+		}
+		if !e.isUnsigned {
+			if f0, f1 := int64(*tmp), int64(input); e.isMax && f1 > f0 || !e.isMax && f1 < f0 {


no need to wrap a int64

zz-jason · 2018-07-04T00:28:00Z

@XuHuaiyu When there is no input from child operator, we should return a NULL. Maybe we can do it in the stream aggregate operator.

crazycs520 · 2018-07-05T14:58:53Z

executor/aggfuncs/func_max_min.go

+	"github.com/pingcap/tidb/util/chunk"
+)
+
+type partialResult4MaxMinInt int64


How aboult use type partialResult4MaxMinInt = int64,
then we can use *(*partialResult4MaxMinInt)(pr) instead of int64(*(*partialResult4MaxMinInt)(pr))

below partialResult4MaxMinDecimal, partialResult4MaxMinFloat32, partialResult4MaxMinFloat64 is same too.

here is a benchmark

type num1 = int type num2 int func BenchmarkT1(b *testing.B) { var a1 num1 a1 = 1 for i := 0; i < b.N; { i += a1 } } func BenchmarkT2(b *testing.B) { var a2 num2 a2 = 1 for i := 0; i < b.N; { i += int(a2) } }

▶ go test -bench=. goos: darwin goarch: amd64 pkg: test2 BenchmarkT1-8 2000000000 0.27 ns/op BenchmarkT2-8 2000000000 0.54 ns/op PASS ok test2 1.757s

XuHuaiyu · 2018-07-06T04:12:52Z

Wait for #7002 ,
please review it first.

crazycs520 · 2018-07-06T05:24:06Z

executor/aggfuncs/func_max_min.go

+		}
+		if f0, f1 := *p, input; e.isMax && f1 > f0 || !e.isMax && f1 < f0 {
+			*p = f1
+		}


if e.isMax && input > *p || !e.isMax && input < *p { *p = input }

crazycs520 · 2018-07-06T05:39:04Z

executor/aggfuncs/func_max_min.go

+}
+
+func (e *maxMin4Float32) AppendFinalResult2Chunk(sctx sessionctx.Context, pr PartialResult, chk *chunk.Chunk) error {
+	chk.AppendFloat32(e.ordinal, float32(*(*partialResult4MaxMinFloat32)(pr)))


chk.AppendFloat32(e.ordinal, *(*partialResult4MaxMinFloat32)(pr))

crazycs520 · 2018-07-06T05:39:57Z

executor/aggfuncs/func_max_min.go

+		}
+		if f0, f1 := float32(*p), float32(input); e.isMax && f1 > f0 || !e.isMax && f1 < f0 {
+			*p = partialResult4MaxMinFloat32(f1)
+		}


float32Input := float32(input) if !e.executed { *p = float32Input e.executed = true continue } if e.isMax && float32Input > *p || !e.isMax && float32Input < *p { *p = float32Input }

crazycs520 · 2018-07-06T05:42:38Z

executor/aggfuncs/func_max_min.go

+		}
+		if f0, f1 := *p, uint64(input); e.isMax && f1 > f0 || !e.isMax && f1 < f0 {
+			*p = partialResult4MaxMinUint(f1)
+		}


uint64Input := uint64(input) if !e.executed { *p = uint64Input e.executed = true continue } if e.isMax && uint64Input > *p || !e.isMax && uint64Input < *p { *p = uint64Input }

crazycs520 · 2018-07-06T05:43:41Z

executor/aggfuncs/func_max_min.go

+}
+
+func (e *maxMin4Float64) AppendFinalResult2Chunk(sctx sessionctx.Context, pr PartialResult, chk *chunk.Chunk) error {
+	chk.AppendFloat64(e.ordinal, float64(*(*partialResult4MaxMinFloat64)(pr)))


chk.AppendFloat64(e.ordinal, *(*partialResult4MaxMinFloat64)(pr))

crazycs520 · 2018-07-06T05:45:17Z

executor/aggfuncs/func_max_min.go

+		}
+		if f0, f1 := *p, input; e.isMax && f1 > f0 || !e.isMax && f1 < f0 {
+			*p = partialResult4MaxMinFloat64(f1)
+		}


if e.isMax && input > *p || !e.isMax && input < *p { *p = input }

crazycs520 · 2018-07-06T05:53:26Z

executor/aggfuncs/func_max_min.go

+		cmp := f1.Compare(f0)
+		if e.isMax && cmp == 1 || !e.isMax && cmp == -1 {
+			*p = partialResult4MaxMinDecimal(f1)
+		}


cmp := input.Compare(*p) if e.isMax && cmp == 1 || !e.isMax && cmp == -1 { *p = input }

crazycs520 · 2018-07-06T05:53:46Z

executor/aggfuncs/func_max_min.go

+}
+
+func (e *maxMin4Decimal) AppendFinalResult2Chunk(sctx sessionctx.Context, pr PartialResult, chk *chunk.Chunk) error {
+	chk.AppendMyDecimal(e.ordinal, (*types.MyDecimal)(*(*partialResult4MaxMinDecimal)(pr)))


chk.AppendMyDecimal(e.ordinal, *(*partialResult4MaxMinDecimal)(pr))

…actor/max_min Conflicts: executor/aggfuncs/aggfuncs.go

XuHuaiyu · 2018-07-11T07:37:00Z

/run-all-tests

zz-jason · 2018-07-11T07:38:28Z

executor/aggfuncs/func_max_min.go

+	// executed is used to indicates:
+	// 1. whether the partial result is the initialization value which should not be compared during evaluation;
+	// 2. whether all the values of arg are all null, if so, we should return null as the default value for MAX/MIN.
+	executed bool


this should be in the field of partial results, and should be reset for every group

… into dev/refactor/max_min :q!

…actor/max_min :q!

zz-jason · 2018-07-11T08:56:36Z

executor/aggfuncs/func_max_min.go

+	// executed is used to indicates:
+	// 1. whether the partial result is the initialization value which should not be compared during evaluation;
+	// 2. whether all the values of arg are all null, if so, we should return null as the default value for MAX/MIN.
+	executed bool


Can we use isNull instead? If isNull is true, means there hasn't been any input values, and we should append a NULL for this group in the Chunk.

executed is confusion, because even it is false, there can also exists a group with all NULL values, and actually this partial result is executed.

yes, isNull is better than executed

zz-jason · 2018-07-11T13:32:13Z

executor/aggfuncs/func_max_min.go

+		if isNull {
+			continue
+		}
+		i := uint64(input)


s/i/uintVal/?

zz-jason · 2018-07-11T13:37:13Z

executor/aggfuncs/func_max_min.go

+}
+
+type partialResult4MaxMinDecimal struct {
+	val    *types.MyDecimal


The input value may be changed when child operator reuses its input Chunk, we should store the copied value to avoid this issue, change val *types.MyDecimal to val types.MyDecimal.

…actor/max_min �ý,�ý.�ý`:q!

zz-jason · 2018-07-11T14:42:37Z

executor/aggfuncs/func_max_min.go

@@ -255,13 +255,14 @@ type maxMin4Decimal struct {

 func (e *maxMin4Decimal) AllocPartialResult() PartialResult {
 	p := new(partialResult4MaxMinDecimal)
+	p.val = *new(types.MyDecimal)


This operation can be removed? since we only set it once the first non-NULL value comes.

zz-jason · 2018-07-11T14:42:43Z

executor/aggfuncs/func_max_min.go

 	p.isNull = true
 	return PartialResult(p)
 }

 func (e *maxMin4Decimal) ResetPartialResult(pr PartialResult) {
 	p := (*partialResult4MaxMinDecimal)(pr)
-	p.val = new(types.MyDecimal)
+	p.val = *new(types.MyDecimal)


zz-jason · 2018-07-11T15:08:25Z

LGTM

XuHuaiyu · 2018-07-11T15:10:10Z

/run-all-tests

XuHuaiyu · 2018-07-11T15:10:21Z

PTAL @crazycs520

winoros · 2018-07-12T05:50:40Z

Seems that there's no unit test in expression/aggfuncs.

winoros

LGTM, but we need to find some way to add the unit tests.

XuHuaiyu added 3 commits July 3, 2018 19:16

executor: support MAX/MIN in new evaluation framework partially

0904a62

Merge branch 'master' of https://github.com/pingcap/tidb into dev/ref…

c0bef2d

…actor/max_min Conflicts: executor/aggfuncs/aggfuncs.go executor/aggfuncs/builder.go

tiny change

740c63c

zz-jason reviewed Jul 3, 2018

View reviewed changes

executor/aggfuncs/func_max_min.go Outdated

if isNull {

continue

}

if !e.isUnsigned {

Copy link

Member

zz-jason Jul 3, 2018

Choose a reason for hiding this comment

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

put this if branch outside the for loop

zz-jason reviewed Jul 3, 2018

View reviewed changes

crazycs520 reviewed Jul 5, 2018

View reviewed changes

zz-jason added this to the 2.1 milestone Jul 6, 2018

crazycs520 reviewed Jul 6, 2018

View reviewed changes

XuHuaiyu added 4 commits July 11, 2018 10:34

address comment

661dcd8

address comment

ed6cb9d

Merge branch 'master' of https://github.com/pingcap/tidb into dev/ref…

208e283

…actor/max_min Conflicts: executor/aggfuncs/aggfuncs.go

address comment

4f2e0a4

XuHuaiyu force-pushed the dev/refactor/max_min branch from 796993e to 4f2e0a4 Compare July 11, 2018 07:03

Merge branch 'master' into dev/refactor/max_min

93bbe1d

zz-jason reviewed Jul 11, 2018

View reviewed changes

XuHuaiyu added 3 commits July 11, 2018 16:45

address comment

f714f16

Merge branch 'dev/refactor/max_min' of https://github.com/XuHuaiyu/tidb…

b14296c

… into dev/refactor/max_min :q!

Merge branch 'master' of https://github.com/pingcap/tidb into dev/ref…

85a7cda

…actor/max_min :q!

zz-jason reviewed Jul 11, 2018

View reviewed changes

fix ci

ab1895a

zz-jason reviewed Jul 11, 2018

View reviewed changes

XuHuaiyu added 2 commits July 11, 2018 22:36

address comment

ec9fa07

Merge branch 'master' of https://github.com/pingcap/tidb into dev/ref…

6db67e7

…actor/max_min �ý,�ý.�ý`:q!

zz-jason reviewed Jul 11, 2018

View reviewed changes

address comment

cdd3980

zz-jason added type/enhancement The issue or PR belongs to an enhancement. status/LGT1 Indicates that a PR has LGTM 1. sig/execution SIG execution labels Jul 11, 2018

zz-jason added the status/all tests passed label Jul 12, 2018

XuHuaiyu added the priority/P2 The issue has P2 priority. label Jul 12, 2018

winoros reviewed Jul 12, 2018

View reviewed changes

Merge branch 'master' into dev/refactor/max_min

70e5c45

zz-jason approved these changes Jul 12, 2018

View reviewed changes

Merge branch 'master' into dev/refactor/max_min

b65776e

XuHuaiyu merged commit 0ef52ac into pingcap:master Jul 12, 2018

XuHuaiyu deleted the dev/refactor/max_min branch July 12, 2018 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: support MAX/MIN in new evaluation framework partially #6971

executor: support MAX/MIN in new evaluation framework partially #6971

XuHuaiyu commented Jul 3, 2018 •

edited

Loading

XuHuaiyu commented Jul 3, 2018

zz-jason Jul 3, 2018

zz-jason Jul 3, 2018

zz-jason Jul 3, 2018

zz-jason commented Jul 4, 2018

crazycs520 Jul 5, 2018 •

edited

Loading

XuHuaiyu commented Jul 6, 2018

crazycs520 Jul 6, 2018

crazycs520 Jul 6, 2018

crazycs520 Jul 6, 2018

crazycs520 Jul 6, 2018

crazycs520 Jul 6, 2018

crazycs520 Jul 6, 2018

crazycs520 Jul 6, 2018 •

edited

Loading

crazycs520 Jul 6, 2018

XuHuaiyu commented Jul 11, 2018

zz-jason Jul 11, 2018

zz-jason Jul 11, 2018

XuHuaiyu Jul 11, 2018

zz-jason Jul 11, 2018

zz-jason Jul 11, 2018

zz-jason Jul 11, 2018

zz-jason Jul 11, 2018

zz-jason commented Jul 11, 2018

XuHuaiyu commented Jul 11, 2018

XuHuaiyu commented Jul 11, 2018

winoros commented Jul 12, 2018

winoros left a comment

executor: support MAX/MIN in new evaluation framework partially #6971

executor: support MAX/MIN in new evaluation framework partially #6971

Conversation

XuHuaiyu commented Jul 3, 2018 • edited Loading

What have you changed? (mandatory)

What are the type of the changes (mandatory)?

How has this PR been tested (mandatory)?

Does this PR affect documentation (docs/docs-cn) update? (optional)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

XuHuaiyu commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Jul 4, 2018

crazycs520 Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

XuHuaiyu commented Jul 6, 2018

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

crazycs520 Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Jul 11, 2018

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 commented Jul 11, 2018

XuHuaiyu commented Jul 11, 2018

XuHuaiyu commented Jul 11, 2018

winoros commented Jul 12, 2018

winoros left a comment

Choose a reason for hiding this comment

XuHuaiyu commented Jul 3, 2018 •

edited

Loading

crazycs520 Jul 5, 2018 •

edited

Loading

crazycs520 Jul 6, 2018 •

edited

Loading