Add initial SQL support for non-expression sketch postaggs #8487

jon-wei · 2019-09-07T02:50:42Z

This PR adds support for using PostAggregators on Datasketches aggregators in SQL, by adding new SQL operators that accept quantile, HLL, and theta sketches.

To support this, the SQL system has been updated to allow operators to be converted into non-expression-based PostAggregators. This is accomplished by adding a new PostAggregatorVisitor class and adding new variants of the existing RexNode conversion methods that accept a post agg visitor.

This PR does not add support for all current sketch postaggregators (such as DoublesSketchToCDFPostAggregator) and the tuple sketch is currently not supported, I plan to handle this in a follow on PR.

This PR has:

been self-reviewed.
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.

jon-wei · 2019-09-07T02:51:09Z

Marking WIP temporarily, need to update the docs

jon-wei · 2019-09-12T01:55:43Z

Updated docs, removing WIP tag

ccaominh · 2019-09-18T15:34:17Z

docs/querying/sql.md

+|--------|-----|
+|`THETA_SKETCH_ESTIMATE(expr)`|Returns the distinct count estimate from a theta sketch. `expr` must return a theta sketch.|
+|`THETA_SKETCH_ESTIMATE_WITH_ERROR_BOUNDS(expr, errorBoundsStdDev)`|Returns the distinct count estimate and error bounds from a theta sketch. `expr` must return a theta sketch.|
+|`THETA_SKETCH_UNION([size], expr0, expr1, ...)`|Returns a union of theta sketches, where each input expression must return an theta sketch. The `size` can be optionally specified as the first parameter.|


Typo: an theta sketch -> a theta sketch

(same for following 2 lines)

FYI, it's possible that the spellcheck in travis will have some false positives on the function names. You can add suppress false positives for this file by adding entries after https://github.com/apache/incubator-druid/blob/master/website/.spelling#L1307

thanks, fixed, looks like there were no spellcheck errors

jihoonson · 2019-09-25T05:57:02Z

Added Design Review since this PR adds new SQL functions.

jihoonson

@jon-wei thank you for the very useful feature! Still reviewing, left some comments.

jihoonson · 2019-09-25T03:44:10Z

docs/querying/sql.md

+
+|Function|Notes|
+|--------|-----|
+|`HLL_SKETCH_ESTIMATE(expr)`|Returns the distinct count estimate from an HLL sketch. `expr` must return an HLL sketch.|


Wondering if it's possible to use the same function name for this and APPROX_COUNT_DISTINCT_DS_HLL. It looks like it's possible to call a proper conversion depending on the context in DruidQuery. I think we should use the same function name if possible even if it needs a big refactoring because it makes the life of Druid users easier.

Hmm, i actually like the new functions added in this PR more than APPROX_COUNT_DISTINCT_DS_HLL, I think they express a clearer boundary between the aggregated sketch object and the operations that can be performed on said object. The new functions names also have a 1-to-1 correspondence with the underlying native aggs/postaggs which I think is nice.

If not for backwards compatibility, I would actually consider dropping APPROX_COUNT_DISTINCT_DS_HLL, it was introduced as a way to allow people to use the datasketches implementations before we supported postaggs in SQL (with this patch).

After my second look, the new ones do look better to me too. Thanks.

jihoonson · 2019-09-25T03:44:34Z

docs/querying/sql.md

+
+|Function|Notes|
+|--------|-----|
+|`THETA_SKETCH_ESTIMATE(expr)`|Returns the distinct count estimate from a theta sketch. `expr` must return a theta sketch.|


Similarly, can we use the same function name for this and APPROX_COUNT_DISTINCT_DS_THETA?

My reasoning here is the same as in this comment: https://github.com/apache/incubator-druid/pull/8487/files#r335716107

jihoonson · 2019-09-25T03:48:03Z

docs/querying/sql.md

+|Function|Notes|
+|--------|-----|
+|`HLL_SKETCH_ESTIMATE(expr)`|Returns the distinct count estimate from an HLL sketch. `expr` must return an HLL sketch.|
+|`HLL_SKETCH_ESTIMATE_WITH_ERROR_BOUNDS(expr, [numStdDev])`|Returns the distinct count estimate and error bounds from an HLL sketch. `expr` must return an HLL sketch. An optional `numStdDev` argument can be provided.|


Hmm, estimate sounds unclear what it estimates to me. How about HLL_SKETCH_COUNT_DISTINCT (or APPROX_COUNT_DISTINCT_DS_HLL if it's same)? Also, does it make sense to use the same name for this and the above function? It sounds like HLL_SKETCH_ESTIMATE(expr) should use a default error bound.

Within the context of this sketch and its documentation (https://druid.apache.org/docs/latest/development/extensions-core/datasketches-hll.html), I think the meaning of estimate is clear

Ok, thanks.

jihoonson · 2019-09-25T03:48:17Z

docs/querying/sql.md

+|Function|Notes|
+|--------|-----|
+|`THETA_SKETCH_ESTIMATE(expr)`|Returns the distinct count estimate from a theta sketch. `expr` must return a theta sketch.|
+|`THETA_SKETCH_ESTIMATE_WITH_ERROR_BOUNDS(expr, errorBoundsStdDev)`|Returns the distinct count estimate and error bounds from a theta sketch. `expr` must return a theta sketch.|


Same question for the naming here.

I think given the context and other docs (https://druid.apache.org/docs/latest/development/extensions-core/datasketches-theta.html), estimate is clear here

jihoonson · 2019-09-25T03:54:32Z

...a/org/apache/druid/query/aggregation/datasketches/hll/HllSketchToEstimatePostAggregator.java

+  public byte[] getCacheKey()
+  {
+    return new CacheKeyBuilder(PostAggregatorIds.HLL_SKETCH_TO_ESTIMATE_CACHE_TYPE_ID)
+        .appendString(name)


I think name shouldn't be in the cache key?

Removed, thanks

jihoonson · 2019-09-25T06:39:33Z

...che/druid/query/aggregation/datasketches/theta/sql/ThetaSketchSetBaseOperatorConversion.java

+            factory -> Calcites.createSqlType(factory, SqlTypeName.OTHER)
+        ),
+        null,
+        OperandTypes.VARIADIC,


Same here. variadic(SqlOperandCountRanges.from(2))?

Changed to suggested

jihoonson · 2019-09-25T07:00:26Z

...apache/druid/query/aggregation/datasketches/hll/sql/HllSketchSetUnionOperatorConversion.java

+      if (convertedPostAgg == null) {
+        if (operandCounter == 0) {
+          try {
+            lgK = RexLiteral.intValue(operand);


Should we check the kind of operand is LITERAL here before we call intValue()? Otherwise, this line can throw an AssertionError which is not caught by the below catch.

Added sqlkind checks here and in similar places

jihoonson · 2019-09-25T07:02:16Z

...apache/druid/query/aggregation/datasketches/hll/sql/HllSketchSetUnionOperatorConversion.java

+          try {
+            lgK = RexLiteral.intValue(operand);
+          }
+          catch (RuntimeException re) {


It should be better to narrow the exception here, so that we won't catch some exceptions unintentionally. Looks like ClassCastException is the only possible RuntimeException.

Narrowed this to ClassCastException here and elsewhere

jihoonson · 2019-09-25T07:22:30Z

...che/druid/query/aggregation/datasketches/theta/sql/ThetaSketchSetBaseOperatorConversion.java

+      if (convertedPostAgg == null) {
+        if (operandCounter == 0) {
+          try {
+            size = RexLiteral.intValue(operand);


Same here for literal check and exception handling.

jihoonson · 2019-09-25T07:23:46Z

...che/druid/query/aggregation/datasketches/theta/sql/ThetaSketchSetBaseOperatorConversion.java

+
+  public String getSetOperationName()
+  {
+    throw new UnsupportedOperationException("getSetOperationName() is not implemented.");


Suggest public abstract String getSetOperationName().

Changed this to abstract method

jihoonson

Thanks. Left a couple of trivial comments.

jihoonson · 2019-10-17T23:39:45Z

...apache/druid/query/aggregation/datasketches/hll/sql/HllSketchSetUnionOperatorConversion.java

+      SqlFunctionCategory.USER_DEFINED_FUNCTION
+  );
+
+  public HllSketchSetUnionOperatorConversion()


Seems unnecessary.

jihoonson · 2019-10-17T23:58:38Z

sql/src/main/java/org/apache/druid/sql/calcite/expression/PostAggregatorVisitor.java

+    return oldVal;
+  }
+
+  public int getCounter()


Please remove unused method.

jihoonson · 2019-10-17T23:59:01Z

sql/src/main/java/org/apache/druid/sql/calcite/expression/PostAggregatorVisitor.java

+  {
+    int oldVal = counter;
+    counter++;
+    return oldVal;


nit: can be simplified to return counter++;

Changed to suggested

…LL param

jon-wei · 2019-10-18T01:12:35Z

@jihoonson Addressed latest comments, also added a "round" parameter to the HLL sketch estimate postagg, and fixed the numStdDev param for HLL_SKETCH_ESTIMATE_WITH_ERROR_BOUNDS to actually be optional (it's optional in the docs and the underlying postagg).

jihoonson

LGTM. Thanks!

Add initial SQL support for non-expression sketch postaggs

6ea389c

jon-wei added Feature Area - Querying Area - SQL WIP labels Sep 7, 2019

jon-wei added 4 commits September 9, 2019 13:55

Checkstyle, spotbugs

bd0cfbc

checkstyle

b4e6a33

imports

192bffc

Update SQL docs

e0b45e7

jon-wei removed the WIP label Sep 12, 2019

jon-wei added 2 commits September 12, 2019 16:07

Merge remote-tracking branch 'upstream/master' into sketch_sql_postagg3

2ebaef7

Checkstyle

d377d77

ccaominh reviewed Sep 18, 2019

View reviewed changes

Fix theta sketch operator docs

16a04f6

jihoonson added the Design Review label Sep 25, 2019

jihoonson reviewed Sep 25, 2019

View reviewed changes

jon-wei added 4 commits October 15, 2019 13:28

Merge remote-tracking branch 'upstream/master' into sketch_sql_postagg3

bb12e8b

PR comments

bfaeda5

Checkstyle fixes

be0bdfe

Add missing entries for HLL sketch module

a0596b0

jihoonson reviewed Oct 18, 2019

View reviewed changes

PR comments, add round param to HLL estimate operator, fix optional H…

65c530b

…LL param

jihoonson approved these changes Oct 18, 2019

View reviewed changes

jon-wei merged commit d880752 into apache:master Oct 18, 2019

gianm added this to the 0.17.0 milestone Oct 19, 2019

jon-wei mentioned this pull request Nov 7, 2019

Add more datasketches doubles sketch SQL functions #8843

Merged

3 tasks

Add initial SQL support for non-expression sketch postaggs #8487

Add initial SQL support for non-expression sketch postaggs #8487

Conversation

jon-wei commented Sep 7, 2019

jon-wei commented Sep 7, 2019

jon-wei commented Sep 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonson commented Sep 25, 2019

jihoonson 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

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

jihoonson 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

jon-wei commented Oct 18, 2019

jihoonson left a comment

Choose a reason for hiding this comment