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

aggfuncs: implement avg(distinct) functions #7015

Merged
merged 7 commits into from
Jul 11, 2018

Conversation

zz-jason
Copy link
Member

@zz-jason zz-jason commented Jul 9, 2018

What have you changed? (mandatory)

Implement AVG functions which handles the distinct property:

  • use map[types.MyDecimal]bool to check whether a decimal values already exists
  • use map[float64]bool for float64 values.

What are the type of the changes (mandatory)?

  • Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested (mandatory)?

-unit test
-explain test

Refer to a related PR or issue link (optional)

to #6952

@zz-jason zz-jason added type/enhancement The issue or PR belongs to an enhancement. sig/execution SIG execution labels Jul 9, 2018
@zz-jason zz-jason added this to the 2.1 milestone Jul 9, 2018
@zz-jason
Copy link
Member Author

zz-jason commented Jul 9, 2018

/run-all-tests

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

Please fix CI.

@zz-jason
Copy link
Member Author

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

1 similar comment
@zz-jason
Copy link
Member Author

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

@zz-jason
Copy link
Member Author

@XuHuaiyu @jackysp PTAL

@@ -121,6 +121,68 @@ func (e *avgPartial4Decimal) UpdatePartialResult(sctx sessionctx.Context, rowsIn
return nil
}

type partialResult4AvgDistinctDecimal struct {
partialResult4AvgDecimal
exists map[types.MyDecimal]bool
Copy link
Member

Choose a reason for hiding this comment

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

map[types.MyDecimal]struct{}?


type partialResult4AvgDistinctFloat64 struct {
partialResult4AvgFloat64
exists map[float64]bool
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

map[types.MyDecimal]bool is much convenient to use and will not introduce much memory overhead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is an experiment:

  • map[int]bool with 10M elements: 315M
  • map[int]struct{} with 10M elements: 286M

1. memory usage of map[int]bool

package main

import (
        "fmt"
        "runtime"
)

func main() {
        numElements := 10000000
        var m1, m2 runtime.MemStats

        runtime.ReadMemStats(&m1)
        a := make(map[int]bool)
        for i := 0; i < numElements; i++ {
                a[i] = true
        }
        runtime.ReadMemStats(&m2)
        fmt.Println("Bytes allocated for A:", m2.Alloc-m1.Alloc)
}

The result is:

[jianzhang.zj:/tmp]
➜ go build a.go

[jianzhang.zj:/tmp]
➜ ./a
Bytes allocated for A: 315457888

2. memory usage of map[int]struct{}

package main

import (
        "fmt"
        "runtime"
)

func main() {
        numElements := 10000000
        var m1, m2 runtime.MemStats

        runtime.ReadMemStats(&m1)
        b := make(map[int]struct{})
        for i := 0; i < numElements; i++ {
                b[i] = struct{}{}
        }
        runtime.ReadMemStats(&m2)
        fmt.Println("Bytes allocated for B:", m2.Alloc-m1.Alloc)
}

the result is:

[jianzhang.zj:/tmp]
➜ go build b.go

[jianzhang.zj:/tmp]
➜ ./b
Bytes allocated for B: 286228080

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@winoros winoros added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 11, 2018
XuHuaiyu
XuHuaiyu previously approved these changes Jul 11, 2018
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@ngaut ngaut dismissed XuHuaiyu’s stale review July 11, 2018 06:31

Using structure{} is the official practice in go

@zz-jason
Copy link
Member Author

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

@ngaut ngaut merged commit 7de2b6a into pingcap:master Jul 11, 2018
@zz-jason zz-jason deleted the dev/avg-distinct branch July 11, 2018 16:20
@zz-jason 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 15, 2018
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.

5 participants