executor: support group_concat under new aggregation evaluation framework #7032

XuHuaiyu · 2018-07-12T04:30:13Z

What have you changed? (mandatory)

implement
groupConcat4DistinctString
groupConcat4String

What is the type of the changes? (mandatory)

Improvement

New feature (non-breaking change which adds functionality)
Improvement (non-breaking change which is an improvement to an existing feature)
Bug fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this PR been tested? (mandatory)

The existing test cases.

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

No

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

No

Refer to a related PR or issue link (optional)

#6952

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

…work

zz-jason · 2018-07-12T04:57:24Z

executor/aggfuncs/func_group_concat.go

+// limitations under the License.
+package aggfuncs
+
+import (


reorganize the import packages

zz-jason · 2018-07-12T04:58:31Z

executor/aggfuncs/func_group_concat.go

+	baseAggFunc
+}
+
+func (e *baseGroupConcat4String) AppendFinalResult2Chunk(sctx sessionctx.Context, pr PartialResult, chk *chunk.Chunk) error {


put functions belonging to baseGroupConcat4String together

baseGroupConcat4String only has one function.

zz-jason · 2018-07-12T05:03:20Z

executor/aggfuncs/func_group_concat.go

+			p.buffer.WriteString(s)
+		}
+	}
+	// TODO: if total length is greater than global var group_concat_max_len, truncate it.


make this as an issue and put the issue number into this comment.

zz-jason · 2018-07-12T05:09:21Z

executor/aggfuncs/func_group_concat.go

+
+func (e *groupConcat4String) ResetPartialResult(pr PartialResult) {
+	p := (*partialResult4ConcatString)(pr)
+	p.sep, p.sepInited, p.buffer = "", false, nil


p.sep should be a field of groupConcat4String, not the partial result. The separator can only be a constant and should be consider as a property of the group_concat function.

XuHuaiyu · 2018-07-12T05:10:31Z

/run-all-tests

XuHuaiyu · 2018-07-12T05:36:31Z

PTAL @zz-jason @winoros

zz-jason · 2018-07-12T05:41:22Z

executor/aggfuncs/func_group_concat.go

+
+func (e *groupConcat4String) UpdatePartialResult(sctx sessionctx.Context, rowsInGroup []chunk.Row, pr PartialResult) (err error) {
+	p := (*partialResult4ConcatString)(pr)
+	if !e.sepInited {


e.sep can be initialized when we create the groupConcat4String object?

zz-jason · 2018-07-12T05:59:30Z

executor/aggfuncs/builder.go

+		}
+	}
+	base := baseAggFunc{
+		args:    aggFuncDesc.Args,


how about args: aggFuncDesc.Args[:len(aggFuncDesc.Args)-1]

executor/aggfuncs/func_group_concat.go

+			}
+			if isNull {
+				continue
+			}


XuHuaiyu · 2018-07-13T07:47:57Z

PTAL @zz-jason
The previous implementation may be more readable?

zz-jason · 2018-07-13T08:38:19Z

@XuHuaiyu We need some benchmarks to decide whether it is worthy to make that change.

jackysp · 2018-07-13T08:44:10Z

/run-all-tests tidb-test=pr/573

XuHuaiyu · 2018-07-16T09:10:37Z

@zz-jason
Benchmark is interesting:

func BenchmarkString(b *testing.B) {
	buffer := &bytes.Buffer{}
	array := []string{"abc", "bcd", "efg", "hij"}
	b.ResetTimer()
	valBuf := make([]string, len(array))
	for i := 0; i < b.N; i ++{
		for j := range array {
			valBuf[j] = array[j]
		}
		for j := range valBuf{
			buffer.WriteString(valBuf[j])
		}
	}
}

func BenchmarkString2(b *testing.B) {
	buffer := &bytes.Buffer{}
	array := []string{"abc", "bcd", "efg", "hij"}
	b.ResetTimer()
	for i := 0; i < b.N; i ++{
		for j := range array {
			buffer.WriteString(array[j])
		}
	}
}

BenchmarkString-4 20000000 70.1 ns/op 28 B/op 0 allocs/op
BenchmarkString2-4 30000000 59.8 ns/op 37 B/op 0 allocs/op

XuHuaiyu · 2018-07-16T09:10:50Z

@zz-jason
Benchmark is interesting:

func BenchmarkString(b *testing.B) {
	buffer := &bytes.Buffer{}
	array := []string{"abc", "bcd", "efg", "hij"}
	b.ResetTimer()
	valBuf := make([]string, len(array))
	for i := 0; i < b.N; i ++{
		for j := range array {
			valBuf[j] = array[j]
		}
		for j := range valBuf{
			buffer.WriteString(valBuf[j])
		}
	}
}

func BenchmarkString2(b *testing.B) {
	buffer := &bytes.Buffer{}
	array := []string{"abc", "bcd", "efg", "hij"}
	b.ResetTimer()
	for i := 0; i < b.N; i ++{
		for j := range array {
			buffer.WriteString(array[j])
		}
	}
}

BenchmarkString-4 20000000 70.1 ns/op 28 B/op 0 allocs/op
BenchmarkString2-4 30000000 59.8 ns/op 37 B/op 0 allocs/op

zz-jason · 2018-07-16T09:14:56Z

(70.1-59.8)/59.8, about 17%, I think it's worth to refine.

zz-jason · 2018-07-16T09:17:31Z

we need to reset buffer before for j := range array {?

XuHuaiyu · 2018-07-16T09:47:47Z

it's ok to not reset buffer,
group_concat would not reset buffer if there is no group-by.

zz-jason · 2018-07-16T11:12:53Z

executor/aggfuncs/func_group_concat.go

+			p.buffer.WriteString(e.sep)
+		}
+	}
+	p.buffer.Truncate(p.buffer.Len() - 1)


can be removed?

executor/aggfuncs/func_group_concat.go

+			}
+			p.buffer.WriteString(v)
+		}
+		if isWriteSep {


zz-jason · 2018-07-16T11:16:31Z

executor/aggfuncs/func_group_concat.go

+			if isNull {
+				continue
+			}
+			valsBuf[i] = v


valsBuf can be optimized out.

This valsBuf should be maintained,
cause we need to get all the values here for checking distinct later.

Can we make valsBuf be an object of bytes.Buffer? Maybe it's faster than strings.Join?

XuHuaiyu · 2018-07-16T13:52:39Z

/run-all-tests

XuHuaiyu · 2018-07-17T02:42:57Z

PTAL @zz-jason @winoros

zz-jason · 2018-07-17T02:46:11Z

executor/aggfuncs/builder.go

+		sep, _, err := c.EvalString(nil, nil)
+		// This err will never happen, we check it here for passing the errcheck.
+		if err != nil {
+			log.Warning("Error happened when buildGroupConcat:", errors.Trace(err).Error())


I think we should return nil or panic here to avoid further execution.

…oncat :q!

XuHuaiyu · 2018-07-17T02:53:01Z

PTAL @zz-jason

XuHuaiyu · 2018-07-17T02:55:07Z

/run-all-tests

zz-jason · 2018-07-17T02:48:00Z

executor/aggfuncs/aggfuncs.go

@@ -38,6 +38,9 @@ var (
 	// All the AggFunc implementations for "MAX" are listed here.
 	// All the AggFunc implementations for "MIN" are listed here.
 	// All the AggFunc implementations for "GROUP_CONCAT" are listed here.
+	_ AggFunc = (*groupConcat4DistinctString)(nil)
+	_ AggFunc = (*groupConcat4String)(nil)


group_concat always executes on the string inputs, I think we can remove the 4String suffix. How about groupConcat and groupConcatDistinct?

zz-jason · 2018-07-17T02:50:05Z

executor/aggfuncs/func_group_concat.go

+	buffer *bytes.Buffer
+}
+
+type partialResult4ConcatString struct {


how about partialResult4GroupConcat?

zz-jason · 2018-07-17T02:51:50Z

executor/aggfuncs/func_group_concat.go

+	v, isNull := "", false
+	for _, row := range rowsInGroup {
+		isWriteSep := false
+		for i, l := 0, len(e.args); i < l; i++ {


for _, arg := range e.args could be simpler.

zz-jason · 2018-07-17T02:56:39Z

executor/aggfuncs/func_group_concat.go

+	p := (*partialResult4ConcatString)(pr)
+	v, isNull := "", false
+	for _, row := range rowsInGroup {
+		isWriteSep := false


how about s/isWriteSep/writeSep/?

I prefer the current name.😄

zz-jason · 2018-07-17T02:57:09Z

executor/aggfuncs/func_group_concat.go

+	return nil
+}
+
+type partialResult4ConcatDistinctString struct {


how about partialResult4GroupConcatDistinct?

zz-jason · 2018-07-17T02:58:05Z

executor/aggfuncs/func_group_concat.go

+	p := (*partialResult4ConcatDistinctString)(pr)
+	v, isNull, valsBuf, joinedVals := "", false, make([]string, len(e.args)), ""
+	for _, row := range rowsInGroup {
+		for i, l := 0, len(e.args); i < l; i++ {


zz-jason · 2018-07-17T02:59:37Z

executor/aggfuncs/func_group_concat.go

+			if isNull {
+				continue
+			}
+			valsBuf[i] = v


Can we make valsBuf be an object of bytes.Buffer? Maybe it's faster than strings.Join?

XuHuaiyu · 2018-07-17T03:39:01Z

@AndreMouche is working on the failed case.

XuHuaiyu · 2018-07-17T03:57:14Z

comments addressed, PTAL @zz-jason

zz-jason

LGTM

crazycs520 · 2018-07-17T04:42:37Z

executor/aggfuncs/builder.go

+	base := baseAggFunc{
+		args:    aggFuncDesc.Args[:len(aggFuncDesc.Args)-1],
+		ordinal: ordinal,
+	}


remove base(line#201~204) to below default(line#209) ?

crazycs520 · 2018-07-17T05:28:20Z

executor/aggfuncs/func_group_concat.go

+	p := (*partialResult4GroupConcat)(pr)
+	v, isNull := "", false
+	for _, row := range rowsInGroup {
+		isWriteSep := false


How about define isWriteSep out of for loop?

…oncat :q!

crazycs520

LGTM

zz-jason · 2018-07-17T10:47:06Z

/run-all-tests

XuHuaiyu added 2 commits July 12, 2018 12:26

execuotr: support group_concat under new aggregation evaluation frame…

701eb4c

…work

tiny change

6b3c066

zz-jason added type/enhancement The issue or PR belongs to an enhancement. sig/execution SIG execution labels Jul 12, 2018

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

zz-jason reviewed Jul 12, 2018

View reviewed changes

fix golint

437a9e5

address comment

d08fd92

zz-jason reviewed Jul 12, 2018

View reviewed changes

address comment

91557ef

zz-jason reviewed Jul 12, 2018

View reviewed changes

lysu changed the title ~~execuotr: support group_concat under new aggregation evaluation framework~~ executor: support group_concat under new aggregation evaluation framework Jul 12, 2018

XuHuaiyu added 2 commits July 12, 2018 15:32

address comment

3ad8478

tiny change

892709f

zz-jason reviewed Jul 12, 2018

View reviewed changes

executor/aggfuncs/func_group_concat.go

}

if isNull {

continue

}

This comment was marked as resolved.

Sign in to view

address comment

4665a59

zz-jason reviewed Jul 16, 2018

View reviewed changes

address comment

68aad31

zz-jason reviewed Jul 17, 2018

View reviewed changes

XuHuaiyu added 2 commits July 17, 2018 10:52

address comment

ae7b9b8

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

6a1ba50

…oncat :q!

XuHuaiyu added status/all tests passed and removed status/all tests passed labels Jul 17, 2018

remove useless import

df72247

zz-jason reviewed Jul 17, 2018

View reviewed changes

address comment

04c23d7

zz-jason reviewed Jul 17, 2018

View reviewed changes

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 17, 2018

crazycs520 reviewed Jul 17, 2018

View reviewed changes

address comment

bf314b2

crazycs520 reviewed Jul 17, 2018

View reviewed changes

XuHuaiyu added 2 commits July 17, 2018 14:10

address comment

4c809f8

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

1129a42

…oncat :q!

crazycs520 reviewed Jul 17, 2018

View reviewed changes

Merge branch 'master' into group_concat

9690a2c

zz-jason approved these changes Jul 17, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 17, 2018

Merge branch 'master' into group_concat

94600fb

zz-jason merged commit b29d52b into pingcap:master Jul 17, 2018

executor: support group_concat under new aggregation evaluation framework #7032

executor: support group_concat under new aggregation evaluation framework #7032

Conversation

XuHuaiyu commented Jul 12, 2018

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

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

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

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

XuHuaiyu commented Jul 12, 2018

XuHuaiyu commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

XuHuaiyu commented Jul 13, 2018

zz-jason commented Jul 13, 2018

jackysp commented Jul 13, 2018

XuHuaiyu commented Jul 16, 2018

XuHuaiyu commented Jul 16, 2018

zz-jason commented Jul 16, 2018 • edited Loading

zz-jason commented Jul 16, 2018

XuHuaiyu commented Jul 16, 2018

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Jul 16, 2018

XuHuaiyu commented Jul 17, 2018

Choose a reason for hiding this comment

XuHuaiyu commented Jul 17, 2018

XuHuaiyu commented Jul 17, 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

Choose a reason for hiding this comment

XuHuaiyu commented Jul 17, 2018

XuHuaiyu commented Jul 17, 2018

zz-jason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 left a comment

Choose a reason for hiding this comment

zz-jason commented Jul 17, 2018

zz-jason commented Jul 16, 2018 •

edited

Loading