Enhance DistinctCountThetaSketchAggregationFunction #6004

Jackie-Jiang · 2020-09-11T07:06:01Z

Description

Enhance DistinctCountThetaSketchAggregationFunction, and add the following supports:

Support aggregating on raw values of all data types (both SV and MV)
Support nested filter (E.g. A = 1 AND (B = 2 OR C = 3))
Support filter on all data types (both SV and MV)
Support simple union without filters (E.g. thetaSketch(col))
Support $0 as the default sketch (sketch without filter)

Also change the DistinctCountRawThetaSketchAggregationFunction to have the same usage as the DistinctCountThetaSketchAggregationFunction and returns base64 encoded sketch.
Fix the issue where theta-sketch parameters cannot be correctly handled in BaseBrokerRequestHandler.updateColumnNames()

Release Notes

This aggregation function is still in beta version. This PR involves change on the format of data sent from server to broker, so it works only when both broker and server are upgraded to the new version.

mayankshriv

What's the motivation to add a new aggregation function as opposed to enhancing the existing one? Is there a backward compatibility issue? If not, it adds more confusion on the user side to have multiple variations of the same aggregation functions.

mayankshriv · 2020-09-29T16:12:29Z

What's the motivation to add a new aggregation function as opposed to enhancing the existing one? Is there a backward compatibility issue? If not, it adds more confusion on the user side to have multiple variations of the same aggregation functions.

We discussed offline and did some performance benchmark to ensure there is no regression. Based on the testing/benchmarking results, we decided to move the code under existing function instead of creating a new one.

mayankshriv

Mostly minor comments, except on raw version being feature compatible with non-raw version. Please address comments before merging.

mayankshriv · 2020-09-29T16:15:49Z

pinot-core/src/main/java/org/apache/pinot/core/common/ObjectSerDeUtils.java

+
+      // Directly return the size (0) for empty list
+      if (size == 0) {
+        return new byte[Integer.BYTES];


Nit: Can a static final be used here (to avoid creating new object)?

It is safer to not reuse this as we have no control on not modifying it

mayankshriv · 2020-09-29T16:16:24Z

pinot-core/src/main/java/org/apache/pinot/core/common/ObjectSerDeUtils.java

+        return new byte[Integer.BYTES];
+      }
+
+      // No need to close these 2 streams


Please also add why in the comment.

mayankshriv · 2020-09-29T16:44:31Z

...he/pinot/core/query/aggregation/function/DistinctCountRawThetaSketchAggregationFunction.java

+ * The {@code DistinctCountRawThetaSketchAggregationFunction} collects the values for a given expression (can be
+ * single-valued or multi-valued) into a {@link Sketch} object, and returns the sketch as a base64 encoded string. It
+ * treats BYTES expression as serialized sketches.
+ * <p>The function takes an optional second argument as the parameters for the function. Currently there is only 1


What about predicates, and post-aggregation expression? Are those arguments not supported?

mayankshriv · 2020-09-29T16:48:36Z

...he/pinot/core/query/aggregation/function/DistinctCountRawThetaSketchAggregationFunction.java

  }

  @Override
  public void aggregate(int length, AggregationResultHolder aggregationResultHolder,
      Map<ExpressionContext, BlockValSet> blockValSetMap) {
-    _thetaSketchAggregationFunction.aggregate(length, aggregationResultHolder, blockValSetMap);
+    BlockValSet blockValSet = blockValSetMap.get(_expression);


Why is the raw version not able to re-use the non-raw version of the code as in the previous implementation?

mayankshriv · 2020-09-29T16:53:05Z

...pache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java

-  private final SetOperationBuilder _setOperationBuilder;
+@SuppressWarnings({"rawtypes", "unchecked"})
+public class DistinctCountThetaSketchAggregationFunction implements AggregationFunction<List<Sketch>, Long> {
+  private static final String SET_UNION = "SET_UNION";


Use enum for set operations?

Using string has these 2 benefits over enum:

Save the extra parsing of the enum

Simplify the handling of invalid operations

mayankshriv · 2020-09-29T17:32:02Z

...pache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java

+      _filterEvaluators = Collections.emptyList();
+      _postAggregationExpression = ExpressionContext.forIdentifier(DEFAULT_SKETCH_IDENTIFIER);
+    } else {
+      // Union with post-aggregation


Just to ensure, we have test for this?

Yes, we have unit test and integration test for this

mayankshriv · 2020-09-29T17:36:14Z

...pache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java

+    // Main expression is always index 0
+    if (valueTypes[0] != DataType.BYTES) {
+      List<UpdateSketch> updateSketches = getUpdateSketches(aggregationResultHolder);
+      if (singleValues[0]) {


Are these if/else blocks re-factorable?

I tried and don't see a better way to organize the code. We need to have slightly different logic for each primitive type

… the new one Make `DistinctCountRawThetaSketchAggregationFunction` the same usage as `DistinctCountThetaSketchAggregationFunction` and returns base64 encoded sketch. Also fix the issue in BaseBrokerRequestHandler.updateColumnNames()

Jackie-Jiang · 2020-09-29T18:51:54Z

@mayankshriv Changed the ``DistinctCountRawThetaSketchAggregationFunctionto have the same usage asDistinctCountThetaSketchAggregationFunction`

codecov-commenter · 2020-09-29T19:30:37Z

Codecov Report

Merging #6004 into master will decrease coverage by 2.39%.
The diff coverage is 37.52%.

@@            Coverage Diff             @@
##           master    #6004      +/-   ##
==========================================
- Coverage   66.44%   64.05%   -2.40%     
==========================================
  Files        1075     1211     +136     
  Lines       54773    56796    +2023     
  Branches     8168     8338     +170     
==========================================
- Hits        36396    36380      -16     
- Misses      15700    17774    +2074     
+ Partials     2677     2642      -35

Flag	Coverage Δ
#unittests	`64.05% <37.52%> (?)`

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

Impacted Files	Coverage Δ
...e/pinot/broker/api/resources/PinotBrokerDebug.java	`0.00% <0.00%> (-79.32%)`	⬇️
...ot/broker/broker/AllowAllAccessControlFactory.java	`71.42% <ø> (-28.58%)`	⬇️
.../helix/BrokerUserDefinedMessageHandlerFactory.java	`33.96% <0.00%> (-32.71%)`	⬇️
...ava/org/apache/pinot/client/AbstractResultSet.java	`53.33% <0.00%> (-3.81%)`	⬇️
.../main/java/org/apache/pinot/client/Connection.java	`35.55% <0.00%> (-13.29%)`	⬇️
...inot/client/JsonAsyncHttpPinotClientTransport.java	`10.90% <0.00%> (-51.10%)`	⬇️
.../org/apache/pinot/client/ResultTableResultSet.java	`0.00% <0.00%> (-34.29%)`	⬇️
...not/common/assignment/InstancePartitionsUtils.java	`73.80% <ø> (+0.63%)`	⬆️
.../apache/pinot/common/exception/QueryException.java	`90.27% <ø> (+5.55%)`	⬆️
...pinot/common/function/AggregationFunctionType.java	`96.49% <ø> (-3.51%)`	⬇️
... and 1015 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2379791...297874d. Read the comment docs.

Jackie-Jiang requested a review from mayankshriv September 11, 2020 07:06

mayankshriv reviewed Sep 11, 2020

View reviewed changes

Add ThetaSketchAggregationFunction

51f718e

Jackie-Jiang force-pushed the theta_sketch branch from adc1d7c to 06155e4 Compare September 29, 2020 05:19

Jackie-Jiang changed the title ~~Add ThetaSketchAggregationFunction~~ Enhance DistinctCountThetaSketchAggregationFunction Sep 29, 2020

Jackie-Jiang added the backward-incompat Referenced by PRs that introduce or fix backward compat issues label Sep 29, 2020

mayankshriv approved these changes Sep 29, 2020

View reviewed changes

Jackie-Jiang force-pushed the theta_sketch branch from 06155e4 to 297874d Compare September 29, 2020 18:49

Jackie-Jiang merged commit e892cb2 into apache:master Sep 29, 2020

Jackie-Jiang deleted the theta_sketch branch September 29, 2020 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance DistinctCountThetaSketchAggregationFunction #6004

Enhance DistinctCountThetaSketchAggregationFunction #6004

Jackie-Jiang commented Sep 11, 2020 •

edited

Loading

mayankshriv left a comment

mayankshriv commented Sep 29, 2020

mayankshriv left a comment

mayankshriv Sep 29, 2020

Jackie-Jiang Sep 29, 2020

mayankshriv Sep 29, 2020

Jackie-Jiang Sep 29, 2020

mayankshriv Sep 29, 2020

mayankshriv Sep 29, 2020

mayankshriv Sep 29, 2020

Jackie-Jiang Sep 29, 2020

mayankshriv Sep 29, 2020

Jackie-Jiang Sep 29, 2020

mayankshriv Sep 29, 2020

Jackie-Jiang Sep 29, 2020

Jackie-Jiang commented Sep 29, 2020

codecov-commenter commented Sep 29, 2020

Enhance DistinctCountThetaSketchAggregationFunction #6004

Enhance DistinctCountThetaSketchAggregationFunction #6004

Conversation

Jackie-Jiang commented Sep 11, 2020 • edited Loading

Description

Release Notes

mayankshriv left a comment

Choose a reason for hiding this comment

mayankshriv commented Sep 29, 2020

mayankshriv left a 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

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

Jackie-Jiang commented Sep 29, 2020

codecov-commenter commented Sep 29, 2020

Codecov Report

Jackie-Jiang commented Sep 11, 2020 •

edited

Loading