Add Post Aggregators for Tuple Sketches #13819

anshu-makkar · 2023-02-17T08:57:31Z

Description

Added New Post Aggregators for Tuple Sketches

Release note

New: You can now do the following operations with TupleSketches in Post Aggregation Step

Get the Sketch Output as Base64 String
Provide a constant Tuple Sketch in post-aggregation step that can be used in Set Operations
Get the Estimated Value(Sum) of Summary/Metrics Objects associated with Tuple Sketch

Key changed/added classes in this PR

Added

ArrayOfDoublesSketchToEncodedStringPostAggregator.java
ArrayOfDoublesSketchToMetricsEstimatePostAggregator.java
ArrayOfDoublesSketchConstantPostAggregator.java
ArrayOfDoublesSketchToMetricsEstimatePostAggregatorTest.java
ArrayOfDoublesSketchToEncodedStringPostAggregatorTest.java

Modified

AggregatorUtil.java
ArrayOfDoublesSketchModule.java

This PR has:

been self-reviewed.
a release note entry in the PR description.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

...ry/aggregation/datasketches/tuple/ArrayOfDoublesSketchToEncodedStringPostAggregatorTest.java

.../aggregation/datasketches/tuple/ArrayOfDoublesSketchToMetricsEstimatePostAggregatorTest.java

anshu-makkar · 2023-02-17T14:37:10Z

@techdocsmith Hello, if you could help me tag a reviewer for these new features.

techdocsmith · 2023-02-17T17:00:39Z

Thanks for the contribution @anshu-makkar . I'm not sure who should do this review--I focus mostly on markdown :D . @abhishekagarwal87 do you have a recommendation?

abhishekagarwal87 · 2023-02-18T03:17:44Z

@anshu-makkar - Thank you for your contribution. can you please address build failures? You also need to update docs - https://github.com/apache/druid/blob/master/docs/development/extensions-core/datasketches-tuple.md#post-aggregators

anshu-makkar · 2023-02-18T03:23:09Z

@anshu-makkar - Thank you for your contribution. can you please address build failures? You also need to update docs - https://github.com/apache/druid/blob/master/docs/development/extensions-core/datasketches-tuple.md#post-aggregators

@abhishekagarwal87 : Yes I will work on adding the documentation, and address the build failures.

Any other comments if you have I can address those.

...e/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchConstantPostAggregator.java

abhishekagarwal87 · 2023-02-21T15:32:03Z

@anshu-makkar - are you going to update the code later? You have resolved the comments.

anshu-makkar · 2023-02-21T15:33:50Z

@anshu-makkar - are you going to update the code later? You have resolved the comments.

Hi @abhishekagarwal87 I have modified the code and addressed all the comments.

…hu-makkar/druid into feature/add-post-aggs-tuple-sketches

docs/development/extensions-core/datasketches-tuple.md

Co-authored-by: Victoria Lim <[email protected]>

anshu-makkar · 2023-02-23T05:25:35Z

@abhishekagarwal87 : Request to you review if there are more changes required.

...e/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchConstantPostAggregator.java

docs/development/extensions-core/datasketches-tuple.md

abhishekagarwal87 · 2023-02-23T07:11:47Z

docs/development/extensions-core/datasketches-tuple.md

+
+### Estimated metrics values for each column of ArrayOfDoublesSketch
+
+This post aggregator returns a list of estimated values(sum) from a given ArrayOfDoublesSketch. The result is _N_ double values, where _N_ is the number of double values kept in the sketch per key.


Suggested change

This post aggregator returns a list of estimated values(sum) from a given ArrayOfDoublesSketch. The result is _N_ double values, where _N_ is the number of double values kept in the sketch per key.

This post aggregator returns a list of estimated values(sum of distincts) from a given ArrayOfDoublesSketch. The result is _N_ double values, where _N_ is the number of double values kept in the sketch per key.

I think this needs an overall rephrase, basically with each distinct value, there is a array of doubles, with this post-aggregator we want to provide a double array with sum.

For E.g.

Key_1, {1.0, 2.0}
Key_2, {2.0, 5.0}

Output from Post-aggregator is {3.0, 7.0}

@techdocsmith @vtlim Can you help rephrase this

This explanation works. Though it does need to be called out that result is an estimated sum and not accurate sum.

could This post aggregator returns a list of estimated values(sum) from a given ArrayOfDoublesSketch. be more like This post aggregator returns a list of estimated sum for each metric value from a given ArrayOfDoublesSketch. ?

Sorry, just saw this. The example isn't clear to me; is the sum being added row-wise or column-wise?

For this example,

Key_1, {1.0, 3.0} Key_2, {2.0, 5.0}

Would the result be {3.0, 8.0} or {4.0, 7.0}?

Commented a suggestion in https://github.com/apache/druid/pull/13819/files?file-filters%5B%5D=.md&show-viewed-files=true#r1120990146. It would be great to include this example in the docs as well.

Result would be {3.0, 8.0}

rohangarg

@anshu-makkar : Thanks a lot for the changes! I have left some comments on the core changes, please let me know your thoughts on them.

...e/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchConstantPostAggregator.java

.../query/aggregation/datasketches/tuple/ArrayOfDoublesSketchToEncodedStringPostAggregator.java

...uery/aggregation/datasketches/tuple/ArrayOfDoublesSketchToMetricsEstimatePostAggregator.java

...e/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchConstantPostAggregator.java

rohangarg

Thanks for addressing the review @anshu-makkar! 👍 I have done a second pass over the changes and left some comments. Request you to please take a look at it.

...uery/aggregation/datasketches/tuple/ArrayOfDoublesSketchToMetricsEstimatePostAggregator.java

...n/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchModule.java

...e/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchConstantPostAggregator.java

rohangarg · 2023-02-27T11:34:49Z

docs/development/extensions-core/datasketches-tuple.md

+
+### Estimated metrics values for each column of ArrayOfDoublesSketch
+
+This post aggregator returns a list of estimated values(sum) from a given ArrayOfDoublesSketch. The result is _N_ double values, where _N_ is the number of double values kept in the sketch per key.


could This post aggregator returns a list of estimated values(sum) from a given ArrayOfDoublesSketch. be more like This post aggregator returns a list of estimated sum for each metric value from a given ArrayOfDoublesSketch. ?

processing/src/main/java/org/apache/druid/query/aggregation/AggregatorUtil.java

...uid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchConstantPostAggregatorTest.java

anshu-makkar · 2023-02-28T07:49:09Z

@abhishekagarwal87 @rohangarg Thanks for taking time to review this. I will work on remaining review comment to add a test case to ArrayOfDoublesSketchAggregationTest

Based on Druid native query support changes being added on: apache#13819

docs/development/extensions-core/datasketches-tuple.md

abhishekagarwal87 · 2023-03-01T10:00:39Z

thanks @anshu-makkar. Please let us know once you think it's ready for review. I think we are almost there.

anshu-makkar · 2023-03-01T11:17:50Z

@abhishekagarwal87 : I have added the tests as suggested for native query testing for newly added post-aggs.

Add description of post-aggregator with example Co-authored-by: Victoria Lim <[email protected]>

abhishekagarwal87

some minor comments. LGTM otherwise.

...g/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchAggregationTest.java

...n/java/org/apache/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchModule.java

…oMetricsSumEstimate Co-authored-by: Abhishek Agarwal <[email protected]>

vtlim

LGTM for docs

anshu-makkar · 2023-03-03T03:32:34Z

Request to merge, since we have another development activity on top of this to add SQL support to Tuple Sketches

abhishekagarwal87 · 2023-03-03T04:04:38Z

Merged. Thank you for your contribution @anshu-makkar

Includes tests which exercise the use-case outlined on: apache#13819

You can now do the following operations with TupleSketches in Post Aggregation Step Get the Sketch Output as Base64 String Provide a constant Tuple Sketch in post-aggregation step that can be used in Set Operations Get the Estimated Value(Sum) of Summary/Metrics Objects associated with Tuple Sketch

This PR is a follow-up to #13819 so that the Tuple sketch functionality can be used in SQL for both ingestion using Multi-Stage Queries (MSQ) and also for analytic queries against Tuple sketch columns.

The version originally added in apache#13819 was missing an annotation for the "value" property. Fixes apache#16539. Line endings for ArrayOfDoublesSketchConstantPostAggregator.java are changed from \r\n to \n. Adds a serde test, and improves various other datasketches post-aggregator serde tests to deserialize into PostAggregator. This verifies that the type information is set up correctly.

* Fix serde for ArrayOfDoublesSketchConstantPostAggregator. The version originally added in #13819 was missing an annotation for the "value" property. Fixes #16539. Line endings for ArrayOfDoublesSketchConstantPostAggregator.java are changed from \r\n to \n. Adds a serde test, and improves various other datasketches post-aggregator serde tests to deserialize into PostAggregator. This verifies that the type information is set up correctly. * Fix excessive imports. * Fix equals, hashCode.

Add Post Aggregators for Tuple Sketches

8209728

github-advanced-security bot found potential problems Feb 17, 2023

View reviewed changes

...ry/aggregation/datasketches/tuple/ArrayOfDoublesSketchToEncodedStringPostAggregatorTest.java Fixed Show resolved Hide resolved

.../aggregation/datasketches/tuple/ArrayOfDoublesSketchToMetricsEstimatePostAggregatorTest.java Fixed Show resolved Hide resolved

Making changes for codeql fixes and removing deprecated code

c15b17f

abhishekagarwal87 added the Area - Querying label Feb 17, 2023

Add documentation for post-aggregators

4027e49

abhishekagarwal87 reviewed Feb 18, 2023

View reviewed changes

anshu-makkar added 2 commits February 21, 2023 21:24

Modified Test Cases for Code Coverage Issues

86b4b70

Merge branch 'feature/add-post-aggs-tuple-sketches' of github.com:ans…

2ba9237

…hu-makkar/druid into feature/add-post-aggs-tuple-sketches

github-actions bot added the Area - Documentation label Feb 21, 2023

Changes for Adding Test Cases for Constant Post Aggregator

6bb4bb9

vtlim reviewed Feb 21, 2023

View reviewed changes

Apply suggestions from code review

89e04bc

Co-authored-by: Victoria Lim <[email protected]>

abhishekagarwal87 reviewed Feb 23, 2023

View reviewed changes

...e/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchConstantPostAggregator.java Outdated Show resolved Hide resolved

docs/development/extensions-core/datasketches-tuple.md Outdated Show resolved Hide resolved

abhishekagarwal87 reviewed Feb 23, 2023

View reviewed changes

Review Comments Incorporated

b659e34

rohangarg reviewed Feb 24, 2023

View reviewed changes

...e/druid/query/aggregation/datasketches/tuple/ArrayOfDoublesSketchConstantPostAggregator.java Show resolved Hide resolved

Changes for Review Comments and minor optimizations

ad9d087

rohangarg reviewed Feb 27, 2023

View reviewed changes

Refining code for review comments and class name changes

60341bd

frankgrimes97 added a commit to frankgrimes97/druid that referenced this pull request Feb 28, 2023

Initial Tuple Sketch SQL support

8187217

Based on Druid native query support changes being added on: apache#13819

frankgrimes97 added a commit to frankgrimes97/druid that referenced this pull request Feb 28, 2023

Initial Tuple Sketch SQL support

316b69d

Based on Druid native query support changes being added on: apache#13819

vtlim reviewed Mar 1, 2023

View reviewed changes

docs/development/extensions-core/datasketches-tuple.md Outdated Show resolved Hide resolved

Added Test for New Post-Aggs to test with Native Query

8a3ac42

Update docs/development/extensions-core/datasketches-tuple.md

9af76f3

Add description of post-aggregator with example Co-authored-by: Victoria Lim <[email protected]>

abhishekagarwal87 approved these changes Mar 2, 2023

View reviewed changes

Rename arrayOfDoublesSketchToMetricsEstimate to arrayOfDoublesSketchT…

b91c8fb

…oMetricsSumEstimate Co-authored-by: Abhishek Agarwal <[email protected]>

vtlim approved these changes Mar 2, 2023

View reviewed changes

abhishekagarwal87 merged commit a10e415 into apache:master Mar 3, 2023

frankgrimes97 added a commit to frankgrimes97/druid that referenced this pull request Mar 3, 2023

Add Tuple Sketch SQL support

da52f99

Includes tests which exercise the use-case outlined on: apache#13819

frankgrimes97 added a commit to frankgrimes97/druid that referenced this pull request Mar 6, 2023

Add Tuple Sketch SQL support

2f26015

Includes tests which exercise the use-case outlined on: apache#13819

frankgrimes97 added a commit to frankgrimes97/druid that referenced this pull request Mar 6, 2023

Add Tuple Sketch SQL support

8d44e5d

Includes tests which exercise the use-case outlined on: apache#13819

frankgrimes97 added a commit to frankgrimes97/druid that referenced this pull request Mar 6, 2023

Add Tuple Sketch SQL support

1ca0a96

Includes tests which exercise the use-case outlined on: apache#13819

frankgrimes97 mentioned this pull request Mar 6, 2023

Tuple sketch SQL support #13887

Merged

4 tasks

clintropolis added this to the 26.0 milestone Apr 10, 2023

techdocsmith mentioned this pull request Apr 12, 2023

[DRAFT] 26.0.0 release notes #14064

Closed

gianm mentioned this pull request Jun 5, 2024

Fix serde for ArrayOfDoublesSketchConstantPostAggregator. #16550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Post Aggregators for Tuple Sketches #13819

Add Post Aggregators for Tuple Sketches #13819

anshu-makkar commented Feb 17, 2023 •

edited

Loading

anshu-makkar commented Feb 17, 2023

techdocsmith commented Feb 17, 2023

abhishekagarwal87 commented Feb 18, 2023

anshu-makkar commented Feb 18, 2023

abhishekagarwal87 commented Feb 21, 2023

anshu-makkar commented Feb 21, 2023 •

edited

Loading

anshu-makkar commented Feb 23, 2023

abhishekagarwal87 Feb 23, 2023

anshu-makkar Feb 23, 2023

anshu-makkar Feb 23, 2023

abhishekagarwal87 Feb 24, 2023

rohangarg Feb 27, 2023

vtlim Mar 1, 2023

anshu-makkar Mar 1, 2023

rohangarg left a comment

rohangarg left a comment

rohangarg Feb 27, 2023

anshu-makkar commented Feb 28, 2023

abhishekagarwal87 commented Mar 1, 2023

anshu-makkar commented Mar 1, 2023

abhishekagarwal87 left a comment

vtlim left a comment

anshu-makkar commented Mar 3, 2023

abhishekagarwal87 commented Mar 3, 2023


		### Estimated metrics values for each column of ArrayOfDoublesSketch

		This post aggregator returns a list of estimated values(sum) from a given ArrayOfDoublesSketch. The result is _N_ double values, where _N_ is the number of double values kept in the sketch per key.

Add Post Aggregators for Tuple Sketches #13819

Add Post Aggregators for Tuple Sketches #13819

Conversation

anshu-makkar commented Feb 17, 2023 • edited Loading

Description

Added New Post Aggregators for Tuple Sketches

Release note

New: You can now do the following operations with TupleSketches in Post Aggregation Step

Key changed/added classes in this PR

Added

Modified

anshu-makkar commented Feb 17, 2023

techdocsmith commented Feb 17, 2023

abhishekagarwal87 commented Feb 18, 2023

anshu-makkar commented Feb 18, 2023

abhishekagarwal87 commented Feb 21, 2023

anshu-makkar commented Feb 21, 2023 • edited Loading

anshu-makkar commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohangarg left a comment

Choose a reason for hiding this comment

rohangarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshu-makkar commented Feb 28, 2023

abhishekagarwal87 commented Mar 1, 2023

anshu-makkar commented Mar 1, 2023

abhishekagarwal87 left a comment

Choose a reason for hiding this comment

vtlim left a comment

Choose a reason for hiding this comment

anshu-makkar commented Mar 3, 2023

abhishekagarwal87 commented Mar 3, 2023

anshu-makkar commented Feb 17, 2023 •

edited

Loading

anshu-makkar commented Feb 21, 2023 •

edited

Loading