statistics: using standard lib slices instead of exp slices #45996

fatelei · 2023-08-11T05:52:43Z

What problem does this PR solve?

using builtin slices lib instead of exp slices

Issue Number: ref #45933

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

sre-bot · 2023-08-11T05:52:50Z

All committers have signed the CLA.

ti-chi-bot · 2023-08-11T05:52:52Z

Welcome @fatelei!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to pingcap/tidb. 😃

ti-chi-bot · 2023-08-11T05:52:53Z

Hi @fatelei. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tiprow · 2023-08-11T05:53:10Z

Hi @fatelei. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hawkingrei · 2023-08-11T06:06:02Z

/ok-to-test

hawkingrei · 2023-08-11T06:09:23Z

@fatelei Thank you.

TiDB use bazel to speed up build/test/linter. so you need to run make bazel_prepare to update the bazel config.

hawkingrei · 2023-08-11T06:10:29Z

@fatelei Thank you.

TiDB use bazel to speed up build/test/linter. so you need to run make bazel_prepare to update the bazel config.

CI will tell you where to change. you can update config by it.

codecov · 2023-08-11T06:19:04Z

Codecov Report

Merging #45996 (0c095a4) into master (66033d5) will decrease coverage by 1.6643%.
Report is 9 commits behind head on master.
The diff coverage is 56.6666%.

❗ Current head 0c095a4 differs from pull request most recent head 5b4cab0. Consider uploading reports for the commit 5b4cab0 to get more accurate results

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #45996        +/-   ##
================================================
- Coverage   74.7898%   73.1256%   -1.6643%     
================================================
  Files          1293       1300         +7     
  Lines        398010     399704      +1694     
================================================
- Hits         297671     292286      -5385     
- Misses        80960      88828      +7868     
+ Partials      19379      18590       -789

Flag	Coverage Δ
integration	`26.1058% <56.6666%> (-20.4640%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`85.0494% <ø> (ø)`
br	`51.1288% <ø> (-13.6343%)`	⬇️

Rustin170506 · 2023-08-11T07:49:31Z

/cc

statistics/cmsketch.go

statistics/cmsketch_test.go

fatelei · 2023-08-11T10:38:20Z

bazel_prepare

done

hawkingrei · 2023-08-11T12:15:17Z

@fatelei Please fix the linter.

 statistics/analyze.go:1:1: 
--- statistics/cmsketch_test.go
+++ statistics/cmsketch_test.go
@@ -21,14 +21,13 @@
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
-
 	"github.com/pingcap/errors"
 	"github.com/pingcap/tidb/parser/mysql"
 	"github.com/pingcap/tidb/sessionctx/stmtctx"
 	"github.com/pingcap/tidb/types"
 	"github.com/pingcap/tidb/util/chunk"
 	"github.com/pingcap/tidb/util/codec"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )

fatelei · 2023-08-11T12:39:28Z

@fatelei Please fix the linter.

 statistics/analyze.go:1:1: 
--- statistics/cmsketch_test.go
+++ statistics/cmsketch_test.go
@@ -21,14 +21,13 @@
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
-
 	"github.com/pingcap/errors"
 	"github.com/pingcap/tidb/parser/mysql"
 	"github.com/pingcap/tidb/sessionctx/stmtctx"
 	"github.com/pingcap/tidb/types"
 	"github.com/pingcap/tidb/util/chunk"
 	"github.com/pingcap/tidb/util/codec"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )

done

hawkingrei · 2023-08-12T05:28:24Z

/test all

hawkingrei · 2023-08-13T14:02:06Z

@fatelei Some tests have failed, please check them.

statistics/cmsketch.go

statistics/cmsketch_test.go

hawkingrei · 2023-08-13T15:36:20Z

@fatelei Please run make bazel_preapre to update bazel config.

index 5c2461120..bd200e961 100644

--- a/statistics/BUILD.bazel

+++ b/statistics/BUILD.bazel

@@ -120,7 +120,6 @@ go_test(

         "@com_github_pingcap_errors//:errors",

         "@com_github_pingcap_failpoint//:failpoint",

         "@com_github_pingcap_log//:log",

-        "@com_github_stretchr_testify//assert",

         "@com_github_stretchr_testify//require",

         "@org_uber_go_goleak//:goleak",

         "@org_uber_go_zap//:zap",

fatelei · 2023-08-14T01:17:26Z

@fatelei Please run make bazel_preapre to update bazel config.

index 5c2461120..bd200e961 100644

--- a/statistics/BUILD.bazel

+++ b/statistics/BUILD.bazel

@@ -120,7 +120,6 @@ go_test(

         "@com_github_pingcap_errors//:errors",

         "@com_github_pingcap_failpoint//:failpoint",

         "@com_github_pingcap_log//:log",

-        "@com_github_stretchr_testify//assert",

         "@com_github_stretchr_testify//require",

         "@org_uber_go_goleak//:goleak",

         "@org_uber_go_zap//:zap",

done

Rustin170506

Thanks!

Rustin170506 · 2023-08-14T05:56:47Z

statistics/cmsketch.go

 		if i.Count != j.Count {
-			return i.Count > j.Count
+			return cmp.Compare(j.Count, i.Count)


Could please add a note here?

Rustin170506 · 2023-08-14T05:57:08Z

statistics/cmsketch_test.go

@@ -339,6 +339,18 @@ func TestMergePartTopN2GlobalTopNWithoutHists(t *testing.T) {
 	require.Len(t, leftTopN, 1, "should have 1 left topN")
 }

+func TestSortTopnMeta(t *testing.T) {


ti-chi-bot · 2023-08-14T05:57:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawkingrei, hi-rustin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hawkingrei,hi-rustin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2023-08-14T05:57:40Z

[LGTM Timeline notifier]

Timeline:

2023-08-11 07:48:14.672496953 +0000 UTC m=+271659.221512935: ☑️ agreed by hawkingrei.
2023-08-11 08:18:59.087099189 +0000 UTC m=+273503.636115176: ✖️🔁 reset by hi-rustin.
2023-08-14 03:01:09.240362214 +0000 UTC m=+513633.789378201: ☑️ agreed by hawkingrei.
2023-08-14 05:57:39.751774294 +0000 UTC m=+524224.300790280: ☑️ agreed by hi-rustin.

ti-chi-bot bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 11, 2023

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Aug 11, 2023

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Aug 11, 2023

hawkingrei changed the title ~~feat: pkg statistics using standard lib slices instead of exp slices~~ statistics: using standard lib slices instead of exp slices Aug 11, 2023

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 11, 2023

hawkingrei approved these changes Aug 11, 2023

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 11, 2023

ti-chi-bot bot requested a review from Rustin170506 August 11, 2023 07:49

Rustin170506 requested changes Aug 11, 2023

View reviewed changes

statistics/cmsketch.go Outdated Show resolved Hide resolved

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 11, 2023

fatelei force-pushed the go_121 branch from 0c095a4 to c68bcdd Compare August 11, 2023 08:44

hawkingrei reviewed Aug 11, 2023

View reviewed changes

statistics/cmsketch_test.go Outdated Show resolved Hide resolved

fatelei force-pushed the go_121 branch 2 times, most recently from cb78ffb to 2c1c8a7 Compare August 11, 2023 10:38

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 11, 2023

hawkingrei self-requested a review August 11, 2023 12:16

fatelei force-pushed the go_121 branch from 2c1c8a7 to 54dfe30 Compare August 11, 2023 12:39

hawkingrei reviewed Aug 13, 2023

View reviewed changes

statistics/cmsketch.go Outdated Show resolved Hide resolved

hawkingrei reviewed Aug 13, 2023

View reviewed changes

statistics/cmsketch_test.go Outdated Show resolved Hide resolved

fatelei force-pushed the go_121 branch from 54dfe30 to a0ef266 Compare August 13, 2023 15:06

feat: using standard lib slices instead of exp slices

5b4cab0

fatelei force-pushed the go_121 branch from a0ef266 to 5b4cab0 Compare August 14, 2023 01:17

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 14, 2023

hawkingrei approved these changes Aug 14, 2023

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 14, 2023

Rustin170506 approved these changes Aug 14, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 14, 2023

ti-chi-bot bot merged commit 60588b9 into pingcap:master Aug 14, 2023

D3Hunter mentioned this pull request Jun 4, 2024

*: upgrade x/exp/slices version #53785

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: using standard lib slices instead of exp slices #45996

statistics: using standard lib slices instead of exp slices #45996

fatelei commented Aug 11, 2023 •

edited by hawkingrei

Loading

sre-bot commented Aug 11, 2023 •

edited

Loading

ti-chi-bot bot commented Aug 11, 2023

ti-chi-bot bot commented Aug 11, 2023

tiprow bot commented Aug 11, 2023

hawkingrei commented Aug 11, 2023

hawkingrei commented Aug 11, 2023

hawkingrei commented Aug 11, 2023

codecov bot commented Aug 11, 2023 •

edited

Loading

Rustin170506 commented Aug 11, 2023

fatelei commented Aug 11, 2023

hawkingrei commented Aug 11, 2023

fatelei commented Aug 11, 2023

hawkingrei commented Aug 12, 2023

hawkingrei commented Aug 13, 2023

hawkingrei commented Aug 13, 2023

fatelei commented Aug 14, 2023

Rustin170506 left a comment

Rustin170506 Aug 14, 2023

Rustin170506 Aug 14, 2023

ti-chi-bot bot commented Aug 14, 2023

ti-chi-bot bot commented Aug 14, 2023

statistics: using standard lib slices instead of exp slices #45996

statistics: using standard lib slices instead of exp slices #45996

Conversation

fatelei commented Aug 11, 2023 • edited by hawkingrei Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

sre-bot commented Aug 11, 2023 • edited Loading

ti-chi-bot bot commented Aug 11, 2023

ti-chi-bot bot commented Aug 11, 2023

tiprow bot commented Aug 11, 2023

hawkingrei commented Aug 11, 2023

hawkingrei commented Aug 11, 2023

hawkingrei commented Aug 11, 2023

codecov bot commented Aug 11, 2023 • edited Loading

Codecov Report

Rustin170506 commented Aug 11, 2023

fatelei commented Aug 11, 2023

hawkingrei commented Aug 11, 2023

fatelei commented Aug 11, 2023

hawkingrei commented Aug 12, 2023

hawkingrei commented Aug 13, 2023

hawkingrei commented Aug 13, 2023

fatelei commented Aug 14, 2023

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 Aug 14, 2023

Choose a reason for hiding this comment

Rustin170506 Aug 14, 2023

Choose a reason for hiding this comment

ti-chi-bot bot commented Aug 14, 2023

ti-chi-bot bot commented Aug 14, 2023

[LGTM Timeline notifier]

fatelei commented Aug 11, 2023 •

edited by hawkingrei

Loading

sre-bot commented Aug 11, 2023 •

edited

Loading

codecov bot commented Aug 11, 2023 •

edited

Loading