feat(tdigest): add TDIGEST.MAX/MIN command #2811

wyxxxcat · 2025-02-28T02:33:03Z

This closes #2793.

src/commands/cmd_tdigest.cc

Copilot

PR Overview

This PR adds tests for the new TDIGEST.MAX and TDIGEST.MIN commands to validate their behavior, including error handling and correct value computation.

Introduces the error message constant errValueDoesNotExist for cases where a tdigest exists but contains no data.
Adds subtests for both TDIGEST.MAX and TDIGEST.MIN covering invalid arguments, empty digests, and digests with one or multiple values.

Reviewed Changes

File	Description
tests/gocase/unit/type/tdigest/tdigest_test.go	Added tests to cover TDIGEST.MAX and TDIGEST.MIN functionality

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

tests/gocase/unit/type/tdigest/tdigest_test.go:217

[nitpick] Consider reinitializing the tdigest or using a fresh key when transitioning from testing a single value to multiple values. This ensures that previous state does not affect the aggregation and makes each test case more isolated.

require.NoError(t, rdb.Do(ctx, "TDIGEST.CREATE", key, "compression", "100").Err())

tests/gocase/unit/type/tdigest/tdigest_test.go:212

[nitpick] Consider expanding the invalid argument tests by including cases with extra arguments for both TDIGEST.MAX and TDIGEST.MIN to further improve test coverage.

require.ErrorContains(t, rdb.Do(ctx, "TDIGEST.MAX").Err(), errMsgWrongNumberArg)

Copilot

PR Overview

This PR adds tests for the new TDIGEST.MAX and TDIGEST.MIN commands to ensure their correct behavior with various arguments.

Added a new error constant for missing values.
Introduced test cases for TDIGEST.MAX and TDIGEST.MIN covering invalid arguments, empty digests, and digests with one or multiple values.

Reviewed Changes

File	Description
tests/gocase/unit/type/tdigest/tdigest_test.go	Added tests for TDIGEST.MAX and TDIGEST.MIN commands and a new error constant

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

tests/gocase/unit/type/tdigest/tdigest_test.go:39

[nitpick] Consider renaming 'errValueDoesNotExist' to 'errMsgValueDoesNotExist' for consistency with the other error constants defined in this file.

errValueDoesNotExist   = "no value exists for key"

mapleFU

Code general LGTM, but suddenly I realized that TDigest.Add nan is invalid: https://github.com/RedisBloom/RedisBloom/blob/f68a72e463b84c2298990295c6a3db4719dc5c7a/src/rm_tdigest.c#L187

Would you mind also change that? ( Can be in another pr )

wyxxxcat · 2025-02-28T03:30:16Z

Code general LGTM, but suddenly I realized that TDigest.Add nan is invalid: https://github.com/RedisBloom/RedisBloom/blob/f68a72e463b84c2298990295c6a3db4719dc5c7a/src/rm_tdigest.c#L187

Would you mind also change that? ( Can be in another pr )

I will fix

src/commands/cmd_tdigest.cc

LindaSummer

Hi @wyxxxcat ,

Leave one comment and other parts are generally LGTM. 😊

Best Regards,
Edward

LindaSummer · 2025-03-03T06:13:43Z

src/types/redis_tdigest.cc


  metadata.total_observations += inputs.size();
  metadata.total_weight += inputs.size();
+  metadata.maximum = std::max(metadata.maximum, *std::max_element(inputs.cbegin(), inputs.cend()));


Hi @wyxxxcat ,

The metadata.minimum and metadata.maximum should represent the centroids min and max.
It will be updated in merge action and updated by dumping after merging.

Maybe we could just compute inplace or create a new property in metadata to keep the consistency of metadata and centroids. 😊

If we try to do this in buffer appending, we may need to refactor the merge logic to make sure these max min properties are maintained by just one place.

I don't think this matters since DumpCentroids() is always called after Merge currently?

Hi @mapleFU ,

Thanks for your update! 😊

Currently the min and max are updated after Merge with DumpCentroids().

But Add will not always trigger the Merge action until the buffer is full.

So, if we try to update it in Add maybe we'd better remove the updating of min&max after DumpCentroids().

Since when we try to merge, the buffer will always be locked, it is safe to update the value either in Add or Merge.

Best Regards,
Edward

To change the word here, what's the main purpose to set min max after merge🤔? Since update min max is free when calling Add if any io really happens. And Quantile will always merge before doing any real things?

Without updating min max in add, the tdigest.min/max will traverse the unmerged data buffer

Both way is ok to me here but I just don't know the point or trade-off here

Hi @mapleFU ,

I agree on your view. 😊
Frankly this probably just because I implement the Merge function ahead of Add. 😄

So, I think move it to the Add function is reasonable. But we'd better remove the updating after Merge if we decide to maintain it in Add.

Update it in two places may not be a good practice.

Best Regards,
Edward

Hmm so I'm wondering if we should submit a PR to make these changes before this PR is merged?

Hi @PragmaTwice ,

Got it!
I'll create a patch today to refactor the min max updating logic to Add.😊

Best Regards,
Edward

mapleFU · 2025-03-05T10:50:47Z

I would approve and merge after #2811 (comment) is applied

PragmaTwice

Looks good to me. To see if @LindaSummer has any comment.

LindaSummer

Hi @wyxxxcat ,

Thanks for your effort!😊
Generally LGTM.

Left some minor nitpick.
Current code is also good to me.

If you don't mind, you could update it in next commit.

Best Regards,
Edward

src/commands/cmd_tdigest.cc

mapleFU · 2025-03-07T05:58:52Z

Thanks all, merged!

By the way, meaning full commit message and not using same commit is appreciated

sonarqubecloud · 2025-03-07T06:07:29Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
73.8% Coverage on New Code
13.8% Duplication on New Code

See analysis details on SonarQube Cloud

git-hulk requested a review from Copilot February 28, 2025 03:03

This comment was marked as duplicate.

Sign in to view

wyxxxcat force-pushed the min_max branch 2 times, most recently from fa1c04c to d4d2c8f Compare February 28, 2025 03:08

mapleFU reviewed Feb 28, 2025

View reviewed changes

src/commands/cmd_tdigest.cc Outdated Show resolved Hide resolved

src/commands/cmd_tdigest.cc Outdated Show resolved Hide resolved

git-hulk requested a review from Copilot February 28, 2025 03:08

Copilot AI reviewed Feb 28, 2025

View reviewed changes

git-hulk requested a review from Copilot February 28, 2025 03:10

Copilot AI reviewed Feb 28, 2025

View reviewed changes

wyxxxcat force-pushed the min_max branch from d4d2c8f to 9ac6262 Compare February 28, 2025 03:19

mapleFU reviewed Feb 28, 2025

View reviewed changes

mapleFU previously approved these changes Feb 28, 2025

View reviewed changes

wyxxxcat dismissed mapleFU’s stale review via f8306db February 28, 2025 06:33

wyxxxcat force-pushed the min_max branch from 9ac6262 to f8306db Compare February 28, 2025 06:33

mapleFU reviewed Feb 28, 2025

View reviewed changes

src/commands/cmd_tdigest.cc Show resolved Hide resolved

LindaSummer reviewed Mar 3, 2025

View reviewed changes

LindaSummer mentioned this pull request Mar 4, 2025

feat(tdigest): refactor buffer merge's min max updating logic #2814

Merged

PragmaTwice reviewed Mar 6, 2025

View reviewed changes

LindaSummer reviewed Mar 6, 2025

View reviewed changes

src/commands/cmd_tdigest.cc Outdated Show resolved Hide resolved

src/commands/cmd_tdigest.cc Outdated Show resolved Hide resolved

wyxxxcat force-pushed the min_max branch from 52861d2 to ab92fd4 Compare March 6, 2025 07:10

mapleFU reviewed Mar 6, 2025

View reviewed changes

src/commands/cmd_tdigest.cc Outdated Show resolved Hide resolved

1

4d3ae9b

wyxxxcat force-pushed the min_max branch from ab92fd4 to 4d3ae9b Compare March 7, 2025 01:56

Merge branch 'unstable' into min_max

84633df

mapleFU reviewed Mar 7, 2025

View reviewed changes

src/commands/cmd_tdigest.cc Outdated Show resolved Hide resolved

Update src/commands/cmd_tdigest.cc

b8be6da

mapleFU approved these changes Mar 7, 2025

View reviewed changes

mapleFU merged commit 231b093 into apache:unstable Mar 7, 2025
34 checks passed

feat(tdigest): add TDIGEST.MAX/MIN command #2811

feat(tdigest): add TDIGEST.MAX/MIN command #2811

Uh oh!

Conversation

wyxxxcat commented Feb 28, 2025 • edited by git-hulk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

wyxxxcat commented Feb 28, 2025

Uh oh!

Uh oh!

LindaSummer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LindaSummer Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

LindaSummer Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

mapleFU Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

LindaSummer Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

PragmaTwice Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LindaSummer Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Mar 5, 2025

Uh oh!

PragmaTwice left a comment

Choose a reason for hiding this comment

Uh oh!

LindaSummer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mapleFU commented Mar 7, 2025

Uh oh!

sonarqubecloud bot commented Mar 7, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

wyxxxcat commented Feb 28, 2025 •

edited by git-hulk

Loading

LindaSummer left a comment •

edited

Loading

LindaSummer Mar 3, 2025 •

edited

Loading

PragmaTwice Mar 4, 2025 •

edited

Loading

LindaSummer left a comment •

edited

Loading