KLL sketch #12498

AlexanderSaydakov · 2022-05-02T23:08:49Z

This is a module to support new KllFloatsSketch and KllDoublesSketch from Apache DataSketches

gianm · 2022-05-09T16:01:39Z

Has this code been used in a production environment? Any gotches we should know about?

There appears to be a legitimate issue in KllDoublesSketchAggregatorTest.buildingSketchesAtQueryTime when SQL-compatible null handling is enabled: https://app.travis-ci.com/github/apache/druid/jobs/569163704

[ERROR] org.apache.druid.query.aggregation.datasketches.kll.KllFloatsSketchAggregatorTest.buildingSketchesAtQueryTime[groupByConfig = v2, vectorize = true]
[ERROR]   Run 1: KllFloatsSketchAggregatorTest.buildingSketchesAtQueryTime:367 expected:<377> but was:<355>
[ERROR]   Run 2: KllFloatsSketchAggregatorTest.buildingSketchesAtQueryTime:367 expected:<377> but was:<355>
[ERROR]   Run 3: KllFloatsSketchAggregatorTest.buildingSketchesAtQueryTime:367 expected:<377> but was:<355>
[ERROR]   Run 4: KllFloatsSketchAggregatorTest.buildingSketchesAtQueryTime:367 expected:<377> but was:<355>

leerho · 2022-05-09T17:59:02Z

This code is included in our datasketches-java 3.2.0 release dated 27 Apr 2022. There are no gotches we know about, yet :)

AlexanderSaydakov · 2022-05-12T21:05:52Z

@gianm

Has this code been used in a production environment? Any gotches we should know about?

No, this code has not been used yet. I am not aware of any gotchas.
I believe I fixed that error in the test you pointed out.

gianm · 2022-05-26T05:14:22Z

FYI I haven't had a chance to take a look at this but it is in my queue. If anyone gets to review it before I do then thanks!

gianm

Thanks for your patience @AlexanderSaydakov. I'm nearly prepared to merge this one. Other than the comments on docs and merging factory, my biggest question is whether the array return from postaggs is going to play well with usage in expressions. (For example: expression virtual columns in an outer query, or an expression post-aggregator in this same query.) @clintropolis would you be able to take a look at this specific part of it?

gianm · 2022-08-09T16:27:56Z

core/src/main/java/org/apache/druid/segment/column/ColumnType.java

  public static final ColumnType STRING_ARRAY = new ColumnType(ValueType.ARRAY, null, STRING);
  public static final ColumnType LONG_ARRAY = new ColumnType(ValueType.ARRAY, null, LONG);
  public static final ColumnType DOUBLE_ARRAY = new ColumnType(ValueType.ARRAY, null, DOUBLE);
+  public static final ColumnType FLOAT_ARRAY = new ColumnType(ValueType.ARRAY, null, FLOAT);


@clintropolis what are your thoughts on the implications of adding float array? Do we need to update any other code?

Float arrays are allowed to exist in the type system, we just didn't have anything producing them so I didn't make a constant for it. The type system even allows nested arrays if druid.expressions.allowNestedArrays is true (but it is off by default), so there should be no problem with it. I will ensure that float[] is handled correctly in the expression system when I make the other adjustments.

gianm · 2022-08-09T16:50:21Z

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

+|type|This String should be "KllFloatsSketch" or "KllDoublesSketch"|yes|
+|name|A String for the output (result) name of the calculation.|yes|
+|fieldName|A String for the name of the input field (can contain sketches or raw numeric values).|yes|
+|k|Parameter that determines the accuracy and size of the sketch. Higher k means higher accuracy but more space to store sketches. Must be from 8 to 65535.|no, defaults to 200|


Is there a table available of k -> space required and expected accuracy? Would be great to provide some additional guidance to users about this. It's the most common question I get about using sketches in Druid.

I am working on adding such a table on our web site, and I will refer to it here

gianm · 2022-08-09T16:52:02Z

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

+|name|A String for the output (result) name of the calculation.|yes|
+|fieldName|A String for the name of the input field (can contain sketches or raw numeric values).|yes|
+|k|Parameter that determines the accuracy and size of the sketch. Higher k means higher accuracy but more space to store sketches. Must be from 8 to 65535.|no, defaults to 200|
+|maxStreamLength|This parameter defines the number of items presented to each sketch before it might, in the context of a BufferAggregator, grow larger than a preallocated memory region and need to move on heap. Ideally just a few sketches should grow that large.|no, defaults to 1000000000|


Users won't necessarily know what BufferAggregators are, since they're an implementation detail. Suggested alternate wording:

This parameter defines the number of items that can be presented to each sketch before it may need to move from off-heap to on-heap memory. This is relevant to query types that use off-heap memory, including [TopN](../../querying/topnquery.md) and [GroupBy](../../querying/groupbyquery.md). Ideally, should be set high enough such that most sketches can stay off-heap.

This raises the question in users' minds: why not set it to some huge value like Long.MAX_VALUE? I guess that the downside of setting maxStreamLength higher is increased off-heap memory requirements. Is there a table or formula available about the relationship between maxStreamLength and required off-heap memory?

OK, I like your suggestion to reword. And again we need a table of sketch sizes, which I will provide.

gianm · 2022-08-09T17:05:08Z

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

@@ -56,7 +56,7 @@ The result of the aggregation is a DoublesSketch that is the union of all sketch
 |name|A String for the output (result) name of the calculation.|yes|
 |fieldName|A String for the name of the input field (can contain sketches or raw numeric values).|yes|
 |k|Parameter that determines the accuracy and size of the sketch. Higher k means higher accuracy but more space to store sketches. Must be a power of 2 from 2 to 32768. See [accuracy information](https://datasketches.apache.org/docs/Quantiles/OrigQuantilesSketch) in the DataSketches documentation for details.|no, defaults to 128|
-|maxStreamLength|This parameter is a temporary solution to avoid a [known issue](https://github.com/apache/druid/issues/11544). It may be removed in a future release after the bug is fixed. This parameter defines the maximum number of items to store in each sketch. If a sketch reaches the limit, the query can throw `IllegalStateException`. To workaround this issue, increase the maximum stream length. See [accuracy information](https://datasketches.apache.org/docs/Quantiles/OrigQuantilesSketch) in the DataSketches documentation for how many bytes are required per stream length.|no, defaults to 1000000000|


Yeah, good call. Documenting something as a temporary solution is a great way to ensure it sticks around forever 🙂

Yes, the parameter was introduced to temporarily work around a bug, but after some discussion it was decided that it might be useful regardless

gianm · 2022-08-09T21:13:07Z

...a/org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchAggregatorFactory.java

+      return new KllDoublesSketchMergeAggregatorFactory(
+          getName(),
+          Math.max(getK(), ((KllDoublesSketchAggregatorFactory) other).getK()),
+          getMaxStreamLength()


It's nicer for this stuff to be symmetric; would it make sense to use max here?

Like, Math.max(getMaxStreamLength(), ((KllDoublesSketchAggregatorFactory) other).getMaxStreamLength())

OK, I think it makes sense.

gianm · 2022-08-09T21:26:39Z

...ain/java/org/apache/druid/query/aggregation/datasketches/kll/KllSketchAggregatorFactory.java

+  @Override
+  public boolean canVectorize(ColumnInspector columnInspector)
+  {
+    return true;


🚀 🚀 🚀

gianm · 2022-08-09T21:44:02Z

...org/apache/druid/query/aggregation/datasketches/kll/KllDoublesSketchToCDFPostAggregator.java

+    if (sketch.isEmpty()) {
+      final double[] cdf = new double[splitPoints.length + 1];
+      Arrays.fill(cdf, Double.NaN);
+      return cdf;


@clintropolis is double[] a valid return class for something declared as DOUBLE_ARRAY? (Or should it be Double[] or Object[]?) I didn't find javadocs where we codify this. Ideally it should be linked from ColumnType or ValueType.

I think double[] is ok to return here (it actually isn't currently, if the output of this is fed into an expression without a round-trip through Jackson, but this I think is a flaw and I'm going to fix that). When I fix the issue with double[] I will also update the javadocs for ColumnType to indicate the allowed return types.

gianm · 2022-08-09T21:51:48Z

...ache/druid/query/aggregation/datasketches/kll/KllDoublesSketchToQuantilesPostAggregator.java

+  {
+    final KllDoublesSketch sketch = (KllDoublesSketch) field.compute(combinedAggregators);
+    if (sketch.isEmpty()) {
+      final double[] quantiles = new double[fractions.length];


@clintropolis similar question here about DOUBLE_ARRAY and double[]?

gianm · 2022-08-09T21:52:59Z

...va/org/apache/druid/query/aggregation/datasketches/kll/KllFloatsSketchAggregatorFactory.java

+      return new KllFloatsSketchMergeAggregatorFactory(
+          getName(),
+          Math.max(getK(), ((KllFloatsSketchAggregatorFactory) other).getK()),
+          getMaxStreamLength()


Similar comment to KllDoublesSketchAggregatorFactory: It's nicer for this stuff to be symmetric; would it make sense to use max here?

Just to let you know, Alex is on vacation and won't be back until Aug 15th.

Thanks for letting us know. We took long enough to review this, so we can certainly wait a week to continue the conversation.

gianm

LGTM after CI passes!

The remaining issue in phase 1 of CI is the docs spellcheck. The following terms are not in the dictionary: KLL, KllFloatsSketch, KllDoublesSketch, PMF, CDF, maxStreamLength, and toString (https://app.travis-ci.com/github/apache/druid/jobs/580267197).

@AlexanderSaydakov you can address these by adding them to the dictionary (the website/.spelling file).

gianm · 2022-08-24T01:02:11Z

Two test runs failed with the below segfault in KllDoublesSketchAggregatorTest (first time) and KllFloatsSketchAggregatorTest (second time).

Judging by the fact it's BaseWritableMemoryImpl.getNativeOrderedLong, it could be an out-of-bound access or a use-after-free. I tried running the test case in a loop in my IDE, and unfortunately wasn't able to repro this.

In looking at the code, I suspect it's due to a use-after-free due to returning an object from KllSketchBuildBufferAggregatorHelper.get that references the underlying buffer. That's used to implement BufferAggregator.get and VectorAggregator.get, which by contract aren't permitted to return objects that reference the underlying buffer. (It may be freed, or reused, while those get'ed objects still exist.)

Similar code in other sketch helpers creates on-heap copies (hll uses copy(), quantiles uses compact()). Could we do the same thing here?

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007f69d737e924, pid=21102, tid=21103
#
# JRE version: OpenJDK Runtime Environment (11.0.2+9) (build 11.0.2+9)
# Java VM: OpenJDK 64-Bit Server VM (11.0.2+9, mixed mode, tiered, compressed oops, g1 gc, linux-amd64)
# Problematic frame:
# J 14439 c1 org.apache.datasketches.memory.internal.BaseWritableMemoryImpl.getNativeOrderedLong(J)J (38 bytes) @ 0x00007f69d737e924 [0x00007f69d737e7a0+0x0000000000000184]
#
# No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# An error report file with more information is saved as:
# /home/travis/build/apache/druid/extensions-core/datasketches/hs_err_pid21102.log
Could not load hsdis-amd64.so; library not loadable; PrintAssembly is disabled
#
# If you would like to submit a bug report, please visit:
#   http://bugreport.java.com/bugreport/crash.jsp
#

pulled from remote

gianm · 2022-08-25T16:55:09Z

So close! There's a conflict in ColumnType, probably from #12914, which was a response to the discussion in this PR about better supporting the arrays returned by the KLL postaggs. I took the liberty of resolving the conflict using GitHub's "resolve conflict" feature.

gianm · 2022-08-25T22:57:33Z

Hmm. Another crash.

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x00007f193747fdac, pid=21257, tid=0x00007f194d6bd700
#
# JRE version: OpenJDK Runtime Environment (8.0_292-b10) (build 1.8.0_292-8u292-b10-0ubuntu1~16.04.1-b10)
# Java VM: OpenJDK 64-Bit Server VM (25.292-b10 mixed mode linux-amd64 compressed oops)
# Problematic frame:
# J 14078 C1 org.apache.datasketches.memory.internal.BaseWritableMemoryImpl.getNativeOrderedLong(J)J (38 bytes) @ 0x00007f193747fdac [0x00007f193747fc20+0x18c]
#
# Failed to write core dump. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# An error report file with more information is saved as:
# /home/travis/build/apache/druid/extensions-core/datasketches/hs_err_pid21257.log
# If you would like to submit a bug report, please visit:
#   http://bugreport.java.com/bugreport/crash.jsp
#

gianm · 2022-08-26T00:04:18Z

I suspect the additional crash was due to a similar problem, but for unions. I pushed 7e117c7 to the branch which will copy those too. @AlexanderSaydakov if there's a better way to do this, please let me know (or just change it).

AlexanderSaydakov · 2022-08-26T02:38:08Z

I suspect the additional crash was due to a similar problem, but for unions. I pushed 7e117c7 to the branch which will copy those too. @AlexanderSaydakov if there's a better way to do this, please let me know (or just change it).

Looks fine to me. Sorry, I missed that part.

gianm · 2022-08-27T04:19:18Z

Looks like the latest build passed tests. I'll merge it. Thanks @AlexanderSaydakov !

KLL sketch

390a673

AlexanderSaydakov added the Feature label May 2, 2022

AlexanderSaydakov added 3 commits May 4, 2022 16:31

added documentation

5616648

direct static refs

b36b096

direct static refs

798fc9d

fixed test

f81d11d

gianm reviewed Aug 9, 2022

View reviewed changes

abhishekagarwal87 added the Release Notes label Aug 11, 2022

clintropolis mentioned this pull request Aug 17, 2022

tighten up array handling, fix bug with array_slice output type inference #12914

Merged

5 tasks

addressed review points

f574353

gianm approved these changes Aug 23, 2022

View reviewed changes

AlexanderSaydakov and others added 2 commits August 23, 2022 09:44

added KLL sketch related terms

bb2ad00

Merge branch 'master' into kll_sketch

1255fdb

AlexanderSaydakov and others added 3 commits August 24, 2022 17:01

return a copy from get

a267832

Merge branch 'kll_sketch' of github.com:apache/druid into kll_sketch

56db1ff

pulled from remote

Merge branch 'master' into kll_sketch

c54128a

clintropolis approved these changes Aug 25, 2022

View reviewed changes

Copy unions when returning them from "get".

7e117c7

gianm mentioned this pull request Aug 26, 2022

Use in heap memory to accelerate quantile calculation #12902

Closed

Remove redundant "final".

cfbbad5

gianm merged commit 7e2371b into master Aug 27, 2022

gianm deleted the kll_sketch branch August 27, 2022 04:19

kfaraz added this to the 25.0 milestone Nov 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KLL sketch #12498

KLL sketch #12498

AlexanderSaydakov commented May 2, 2022

gianm commented May 9, 2022

leerho commented May 9, 2022

AlexanderSaydakov commented May 12, 2022

gianm commented May 26, 2022

gianm left a comment

gianm Aug 9, 2022

clintropolis Aug 10, 2022

gianm Aug 9, 2022

AlexanderSaydakov Aug 18, 2022

gianm Aug 23, 2022

gianm Aug 9, 2022

AlexanderSaydakov Aug 18, 2022

gianm Aug 9, 2022

AlexanderSaydakov Aug 18, 2022

gianm Aug 9, 2022

AlexanderSaydakov Aug 18, 2022

gianm Aug 9, 2022

gianm Aug 9, 2022

clintropolis Aug 10, 2022

gianm Aug 9, 2022

gianm Aug 9, 2022

leerho Aug 10, 2022

gianm Aug 11, 2022

gianm left a comment

gianm commented Aug 24, 2022

gianm commented Aug 25, 2022

gianm commented Aug 25, 2022

gianm commented Aug 26, 2022

AlexanderSaydakov commented Aug 26, 2022

gianm commented Aug 27, 2022

KLL sketch #12498

KLL sketch #12498

Conversation

AlexanderSaydakov commented May 2, 2022

gianm commented May 9, 2022

leerho commented May 9, 2022

AlexanderSaydakov commented May 12, 2022

gianm commented May 26, 2022

gianm 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

gianm left a comment

Choose a reason for hiding this comment

gianm commented Aug 24, 2022

gianm commented Aug 25, 2022

gianm commented Aug 25, 2022

gianm commented Aug 26, 2022

AlexanderSaydakov commented Aug 26, 2022

gianm commented Aug 27, 2022