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.

Sign up for GitHub

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

Jump to bottom

util,executor: use MutableString as key for DecimalSet #9913

Merged
merged 9 commits into from
Apr 1, 2019
Merged

util,executor: use MutableString as key for DecimalSet #9913

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dfd4591
util,executor: use MutableString as key for DecimalSet
XuHuaiyu Mar 27, 2019
0807f14
Merge branch 'master' of https://github.com/pingcap/tidb into fix_sum…
XuHuaiyu Mar 27, 2019
8766901
tiny change
XuHuaiyu Mar 27, 2019
2398f74
make ci happy
XuHuaiyu Mar 27, 2019
ba8dbc5
make golint happy
XuHuaiyu Mar 27, 2019
47695d4
Merge branch 'master' of https://github.com/pingcap/tidb into fix_sum…
XuHuaiyu Mar 28, 2019
ce0198d
address comment
XuHuaiyu Mar 29, 2019
debe01f
Merge branch 'master' of https://github.com/pingcap/tidb into fix_sum…
XuHuaiyu Mar 29, 2019
f8ce329
Merge branch 'master' into fix_sum_distinct
zz-jason Apr 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 14 additions & 6 deletions executor/aggfuncs/func_avg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/pingcap/tidb/sessionctx"
"github.com/pingcap/tidb/types"
"github.com/pingcap/tidb/util/chunk"
"github.com/pingcap/tidb/util/hack"
"github.com/pingcap/tidb/util/set"
)

Expand Down Expand Up @@ -153,7 +154,7 @@ func (e *avgPartial4Decimal) MergePartialResult(sctx sessionctx.Context, src Par

type partialResult4AvgDistinctDecimal struct {
partialResult4AvgDecimal
valSet set.DecimalSet
valSet set.StringSet
}

type avgOriginal4DistinctDecimal struct {
Expand All @@ -162,7 +163,7 @@ type avgOriginal4DistinctDecimal struct {

func (e *avgOriginal4DistinctDecimal) AllocPartialResult() PartialResult {
p := &partialResult4AvgDistinctDecimal{
valSet: set.NewDecimalSet(),
valSet: set.NewStringSet(),
}
return PartialResult(p)
}
Expand All @@ -171,7 +172,7 @@ func (e *avgOriginal4DistinctDecimal) ResetPartialResult(pr PartialResult) {
p := (*partialResult4AvgDistinctDecimal)(pr)
p.sum = *types.NewDecFromInt(0)
p.count = int64(0)
p.valSet = set.NewDecimalSet()
p.valSet = set.NewStringSet()
}

func (e *avgOriginal4DistinctDecimal) UpdatePartialResult(sctx sessionctx.Context, rowsInGroup []chunk.Row, pr PartialResult) error {
Expand All @@ -181,18 +182,25 @@ func (e *avgOriginal4DistinctDecimal) UpdatePartialResult(sctx sessionctx.Contex
if err != nil {
return errors.Trace(err)
}
if isNull || p.valSet.Exist(input) {
if isNull {
continue
}

hash, err := input.ToHashKey()
if err != nil {
return err
}
decStr := string(hack.String(hash))
if p.valSet.Exist(decStr) {
continue
}
p.valSet.Insert(decStr)
newSum := new(types.MyDecimal)
err = types.DecimalAdd(&p.sum, input, newSum)
if err != nil {
return errors.Trace(err)
}
p.sum = *newSum
p.count++
p.valSet.Insert(input)
}
return nil
}
Expand Down
19 changes: 14 additions & 5 deletions executor/aggfuncs/func_sum.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/pingcap/tidb/sessionctx"
"github.com/pingcap/tidb/types"
"github.com/pingcap/tidb/util/chunk"
"github.com/pingcap/tidb/util/hack"
"github.com/pingcap/tidb/util/set"
)

Expand All @@ -38,7 +39,7 @@ type partialResult4SumDistinctFloat64 struct {

type partialResult4SumDistinctDecimal struct {
partialResult4SumDecimal
valSet set.DecimalSet
Copy link
Contributor

Choose a reason for hiding this comment

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

How about keep DecimalSet and use MyDecimal.ToHashKey to make DecimalSet.Exist correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we keep DecimalSet here, we'll call ToHashKey twice if DecimalSet.Exist() returns false. One in DecimalSet.Exist(), another in DecimalSet.Insert(). @qw4990

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can add one method like InsertIfNotExists and return whether it's a value to be inserted.

One question is that, is the behavior of the current DecimalSet right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One question is that, is the behavior of the current DecimalSet right?

Do you find anything wrong? @winoros

Copy link
Member

Choose a reason for hiding this comment

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

No, since the DecimalSet only be in used in the places you changed in this pr.

I just wonder that whether there'll be some cases in the future that will need the original DecimalSet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DecimalSet will not be used by other places now. It was introduced when we were refactoring the agg. @winoros

valSet set.StringSet
}

type baseSumAggFunc struct {
Expand Down Expand Up @@ -222,14 +223,14 @@ type sum4DistinctDecimal struct {
func (e *sum4DistinctDecimal) AllocPartialResult() PartialResult {
p := new(partialResult4SumDistinctDecimal)
p.isNull = true
p.valSet = set.NewDecimalSet()
p.valSet = set.NewStringSet()
return PartialResult(p)
}

func (e *sum4DistinctDecimal) ResetPartialResult(pr PartialResult) {
p := (*partialResult4SumDistinctDecimal)(pr)
p.isNull = true
p.valSet = set.NewDecimalSet()
p.valSet = set.NewStringSet()
}

func (e *sum4DistinctDecimal) UpdatePartialResult(sctx sessionctx.Context, rowsInGroup []chunk.Row, pr PartialResult) error {
Expand All @@ -239,10 +240,18 @@ func (e *sum4DistinctDecimal) UpdatePartialResult(sctx sessionctx.Context, rowsI
if err != nil {
return errors.Trace(err)
}
if isNull || p.valSet.Exist(input) {
if isNull {
continue
}
p.valSet.Insert(input)
hash, err := input.ToHashKey()
if err != nil {
return err
}
decStr := string(hack.String(hash))
if p.valSet.Exist(decStr) {
continue
}
p.valSet.Insert(decStr)
if p.isNull {
p.val = *input
p.isNull = false
Expand Down
2 changes: 1 addition & 1 deletion executor/executor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1139,7 +1139,7 @@ func (s *testSuite) TestUnion(c *C) {
tk.MustExec("drop table if exists t")
tk.MustExec("create table t(a int, b decimal(6, 3))")
tk.MustExec("insert into t values(1, 1.000)")
tk.MustQuery("select count(distinct a) from (select a from t union select b from t) tmp;").Check(testkit.Rows("1"))
tk.MustQuery("select count(distinct a), sum(distinct a), avg(distinct a) from (select a from t union all select b from t) tmp;").Check(testkit.Rows("1 1.000 1.0000000"))
}

func (s *testSuite) TestNeighbouringProj(c *C) {
Expand Down
37 changes: 0 additions & 37 deletions util/set/decimal_set.go
View file
Open in desktop

This file was deleted.