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: support the remained types for max/min #7056

Merged
merged 12 commits into from
Jul 16, 2018

Conversation

XuHuaiyu
Copy link
Contributor

@XuHuaiyu XuHuaiyu commented Jul 16, 2018

What have you changed? (mandatory)

support String/ Time/ Duration/ JSON for max/min

What is the type of the changes? (mandatory)

  • Improvement

How has this PR been tested? (mandatory)

The existing test cases.
randgen-test

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)

@zz-jason
Copy link
Member

LGTM

@zz-jason
Copy link
Member

/run-all-tests

@zz-jason
Copy link
Member

@XuHuaiyu please add some labels

@zz-jason zz-jason added this to the 2.1 milestone Jul 16, 2018
@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 16, 2018
@XuHuaiyu XuHuaiyu added sig/execution SIG execution type/enhancement The issue or PR belongs to an enhancement. labels Jul 16, 2018
@crazycs520
Copy link
Contributor

LGTM

@crazycs520 crazycs520 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 16, 2018
@XuHuaiyu
Copy link
Contributor Author

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

@XuHuaiyu
Copy link
Contributor Author

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

@XuHuaiyu
Copy link
Contributor Author

I tried these ways @zz-jason

func BenchmarkCopyString(b *testing.B) {
	a := "123"
	for i := 0; i < b.N; i ++{
		a = fmt.Sprintf("%s", a)
	}
}

func BenchmarkCopyString2(b *testing.B) {
	a := "123"
	for i := 0; i < b.N; i ++{
		a = a + ""
	}
}

func BenchmarkCopyString3(b *testing.B) {
	a := "123"
	for i := 0; i < b.N; i ++{
		bytes := []byte(a)
		b := make([]byte, len(bytes))
		copy(b,bytes )
		a = string(b)
	}
}

func BenchmarkCopyString4(b *testing.B) {
	a := "123"
	for i := 0; i < b.N; i ++{
		pbytes := (*reflect.SliceHeader)(unsafe.Pointer(&b))
		b := make([]byte, pbytes.Len)
		copy(b, a)
		a = string(b)
	}
}

BenchmarkCopyString-4 10000000 132 ns/op 19 B/op 2 allocs/op
BenchmarkCopyString2-4 100000000 11.4 ns/op 0 B/op 0 allocs/op
BenchmarkCopyString3-4 30000000 45.6 ns/op 6 B/op 2 allocs/op
BenchmarkCopyString4-4 200 10791498 ns/op 52690966 B/op 2 allocs/op

@zz-jason
Copy link
Member

zz-jason commented Jul 16, 2018

@XuHuaiyu BenchmarkCopyString4 is not correct, should not use b directly

zz-jason
zz-jason previously approved these changes Jul 16, 2018
@zz-jason
Copy link
Member

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

@crazycs520
Copy link
Contributor

crazycs520 commented Jul 16, 2018

@XuHuaiyu BenchmarkCopyString2 is not correct, it have not copy

import . "github.com/pingcap/tidb/util/hack"
func TestStringcs(t *testing.T) {
	a := "abc"
	b := a + ""
	bufA := Slice(a)
	bufB := Slice(b)
	fmt.Println(&bufA[0], &bufB[0]) // the adress is same
}

Another question: Why we need to copy string? string cannot be modified after create.

@winoros winoros dismissed zz-jason’s stale review July 16, 2018 06:11

Still need discussing

@@ -290,9 +311,202 @@ func (e *maxMin4Decimal) UpdatePartialResult(sctx sessionctx.Context, rowsInGrou
continue
}
cmp := input.Compare(&p.val)
if e.isMax && cmp == 1 || !e.isMax && cmp == -1 {
if e.isMax && cmp > 0 || !e.isMax && cmp < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only change this line? line#363,411?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

caused by the implementation of different compare function.

winoros
winoros previously approved these changes Jul 16, 2018
}
if p.isNull {
// We need to copy the value of input to avoid the original value be covered.
p.val = "" + input
Copy link
Member

Choose a reason for hiding this comment

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

Better wrap it as a method in util package.

@@ -234,3 +235,8 @@ func DoMatch(str string, patChars, patTypes []byte) bool {
}
return sIdx == len(str)
}

// CopyString deep copies a string.
func CopyString(src string) string {
Copy link
Member

Choose a reason for hiding this comment

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

How about s/CopyString/Copy/? We usually call the function with the package name, "stringutil.Copy()" would be simpler.

@@ -234,3 +235,8 @@ func DoMatch(str string, patChars, patTypes []byte) bool {
}
return sIdx == len(str)
}

// Copy deep copies a string.
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment why we need to copy a string. because the string is hack from byte(chunk)

Copy link
Member

Choose a reason for hiding this comment

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

no need to add it here, should add the comments in the caller side.

@zz-jason
Copy link
Member

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

@winoros winoros merged commit 9c9ddf3 into pingcap:master Jul 16, 2018
@XuHuaiyu XuHuaiyu deleted the max_min branch April 22, 2019 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants