Moments Sketch custom aggregator #6581
Conversation
|
@edgan8 we'll review soon! Thanks for the contrib! |
| private MomentSketchWrapper momentsSketch; | ||
|
|
||
| public MomentSketchBuildAggregator( | ||
| final ColumnValueSelector<Double> valueSelector, |
There was a problem hiding this comment.
Should use BaseDoubleColumnValueSelector instead
| private final boolean compress; | ||
| private final byte cacheTypeId; | ||
|
|
||
| private static final byte MOMENTS_SKETCH_CACHE_ID = 0x51; |
There was a problem hiding this comment.
This constant should belong to AggregatorUtil
| @Override | ||
| public int getMaxIntermediateSize() | ||
| { | ||
| return (k + 2) * 8 + 2 * 4 + 8; |
There was a problem hiding this comment.
Should use constants like Integer.BYTES and also explain each addend in comments
| @Override | ||
| public Object combine(Object lhs, Object rhs) | ||
| { | ||
| MomentSketchWrapper union = (MomentSketchWrapper) lhs; |
| final byte cacheTypeId | ||
| ) | ||
| { | ||
| if (name == null) { |
There was a problem hiding this comment.
Objects.requireNonNull is shorter
| private final boolean compress; | ||
|
|
||
| public MomentSketchBuildBufferAggregator( | ||
| final ColumnValueSelector<Double> valueSelector, |
There was a problem hiding this comment.
Should use BaseDoubleColumnValueSelector instead
| } | ||
|
|
||
| @Override | ||
| public synchronized void aggregate(final ByteBuffer buffer, final int position) |
There was a problem hiding this comment.
Should use finer, read-write concurrency; see HllSketchBuildBufferAggregator for example
There was a problem hiding this comment.
I was told here #6381 that synchronization is not needed in buffer aggregators.
There was a problem hiding this comment.
@leventov do you suggestions on how to proceed here?
There was a problem hiding this comment.
the buffer aggregator doesn't need synchronization, but the non-buffer aggregator does since those are used in realtime ingestion tasks which can be queried as they're ingesting
There was a problem hiding this comment.
Some buffer aggregators do need synchronization for OffheapIncrementalIndex. See #3956.
There was a problem hiding this comment.
OK I will keep both the buffer and non-buffer aggregators synchronized then.
| } | ||
|
|
||
| @Override | ||
| public synchronized void close() |
There was a problem hiding this comment.
Doesn't need to be synchronized
| public class MomentSketchMergeAggregatorFactory extends MomentSketchAggregatorFactory | ||
| { | ||
| public static final String TYPE_NAME = "momentSketchMerge"; | ||
| private static final byte MOMENTS_SKETCH_MERGE_CACHE_ID = 0x52; |
There was a problem hiding this comment.
This constants should belong to AggregatorUtil
| public byte[] getCacheKey() | ||
| { | ||
| final CacheKeyBuilder builder = new CacheKeyBuilder( | ||
| AggregatorUtil.QUANTILES_DOUBLES_SKETCH_TO_QUANTILES_CACHE_TYPE_ID).appendCacheable(field); |
There was a problem hiding this comment.
Not sure that this constant should be used
There was a problem hiding this comment.
According to the Druid style, should ).appendCacheable(field); should be on a separate line
|
We just became aware of the underlying paper for this submission a few days ago and are still in the process of reviewing it. It is not up to me whether this code should be merged into Druid as a custom aggregator. However, the code has almost no Javadocs and the paper may be difficult for many users to fully understanding the trade-offs, advantages and disadvantages for how and when to use this kind of quantiles sketch as compared to the DataSketches (DS) quantiles sketches already available in Druid. Quoting from the paper's Abstract:
This is a very exciting claim, but understanding some of the assumptions behind this claim requires a bit of a deep-dive into the paper and unraveling what this sketch would be great at doing, and where it may not be so great. The Moments-Quantiles (M-Sketch) has been optimized for merge-time performance and for this metric the M-Sketch really shines. It's merge speed can be an order-of-magnitude faster than the DS-sketch. Meanwhile, obtaining a M-Sketch quantile estimate can be milliseconds, while the DS-Sketch is in the microsecond range. The paper defines the primary metric for sketch performance as total query-time, where the number of merges are large and the number of get-quantile estimate is rare, and perhaps for many Druid queries, this trade-off makes sense. The other major tradeoff is how sketch accuracy is defined and measured.
This difference has several practical real-world implications. The DS-Sketch doesn't care how ugly your input data stream is. It can have negative values, zeros, gaps and multiple spikes or blocks in its distribution of values, the values can range over many orders-of-magnitude, and the error guarantees of the DS-Sketch will still hold. However, the authors of the paper make it clear in several places that M-Sketch error gurantees only apply to relatively well-behaved and smooth value distributions (what the paper calls "non-pathological"). Unfortunately the term "non-pathological" is not well-defined and the user has no way of knowing whether or not any given input data stream is appropriately "non-pathological" without performing extensive brute-force quantile analysis of the stream and comparing it with the M-Sketch results. Another subtle difference between the two types of sketches is how error performance is measured and quoted. (This part is greatly simplified.) The M-sketch paper effectively defines a total area difference between two distribution curves; one being the real underlying distribution, and the other being the curve effectively modeled by the moments computed by the sketch. Then the paper defines the maximum error as effectively the maximum average error (the integral) of all queries along all points of the distribution. The DS-Sketch defines the maximum error as the maximum difference between the two curves at any point along the full range of the distribution. This means that the actual error from the M-Sketch could be huge (many times the quoted maximum error) for parts of the distribution, and very small for other parts of the curve so that, on average, if you perform queries over the full range of values of the distribution, the average error would be pretty good. But the user would have no clue where along the distribution the error is very low, or outrageously high. In contrast, the DS-Sketch error guarantee is for any single query and for all queries. The other consequence of M-Sketch's error dependance on the data is that the M-sketch cannot give the user any before-the-fact guidance on what the error will be after the data has been sketched. The DS-Sketch does provide this guidance. So as long as you know a great deal about the underlying distributions of your data, or you don't care too much about error, and you are only concerned about total-query-time, go ahead and use the M-Sketch. If you don't know anything about the underlying distributions of your data and you do care about error, and slower total-query-time is an affordable trade-off, then I would advise you use the DataSketches/quantiles sketch. Cheers, Lee. |
|
@leerho thanks for posting the great summary! Author of the paper here, I wanted to confirm that I fully agree with your analysis of when the M-sketch would or would not be appropriate. The actual error is data dependent. In many situations I would prefer to use the datasketches library as well which I found to be fairly reliable in my experiments. One other point to keep in mind is that the space usage of the M-sketch is also extremely low (usually < 150 bytes) so I often think of it as serving as a supplement to basic statistics and tiny histograms rather than a full-fledged sketch. |
...rg/apache/druid/query/aggregation/momentsketch/aggregator/MomentSketchAggregatorFactory.java
Outdated
Show resolved
Hide resolved
...ache/druid/query/aggregation/momentsketch/aggregator/MomentSketchQuantilePostAggregator.java
Outdated
Show resolved
Hide resolved
...ache/druid/query/aggregation/momentsketch/aggregator/MomentSketchQuantilePostAggregator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
What do you think about adding a min/max post-agg for moments sketch, similar to http://druid.io/docs/latest/development/extensions-core/approximate-histograms.html#min-post-aggregator?
|
@edgan8 Is this ready for review again? |
|
@jon-wei not yet, I need to write the documentation and fix some more issues. Will ping this thread in the next day or two! |
docs/content/development/extensions-contrib/momentsketch-quantiles.md
Outdated
Show resolved
Hide resolved
| @@ -103,6 +107,8 @@ public MomentSolver getSolver() | |||
| public double[] getQuantiles(double[] fractions) | |||
| { | |||
| MomentSolver ms = new MomentSolver(data); | |||
| // Constants here are chosen to yield maximum precision while keeping solve times ~1ms on 2Ghz cpu | |||
| // Grid size can be increased if longer solve times are acceptable | |||
There was a problem hiding this comment.
When multiple quantiles are requested, the estimation could be more efficient if MomentSolver had a getQuantiles method that accepts an array, the cdf could be reused and you would only need one pass of the latter for loop.
| } | ||
|
|
||
| @Override | ||
| public synchronized void aggregate(final ByteBuffer buffer, final int position) |
There was a problem hiding this comment.
the buffer aggregator doesn't need synchronization, but the non-buffer aggregator does since those are used in realtime ingestion tasks which can be queried as they're ingesting
processing/src/main/java/org/apache/druid/query/aggregation/AggregatorUtil.java
Outdated
Show resolved
Hide resolved
|
Hi @edgan8, can you take another look, I've left some comments |
| public Object extractValue(final InputRow inputRow, final String metricName) | ||
| { | ||
| Object rawValue = inputRow.getRaw(metricName); | ||
| if (rawValue instanceof MomentSketchWrapper) { |
There was a problem hiding this comment.
This explicitness doesn't add a lot of value, just cast and let it throw ClassCastException. It will also log the wrong class, unlike the current code.
|
|
||
| public class MomentSketchComplexMetricSerde extends ComplexMetricSerde | ||
| { | ||
| private static final MomentSketchObjectStrategy strategy = new MomentSketchObjectStrategy(); |
There was a problem hiding this comment.
Static field name should be all uppercase
| MomentSketchMaxPostAggregator.TYPE_NAME | ||
| ) | ||
| ).addSerializer( | ||
| MomentSketchWrapper.class, |
| public List<? extends Module> getJacksonModules() | ||
| { | ||
| return ImmutableList.of( | ||
| new SimpleModule(getClass().getSimpleName() |
There was a problem hiding this comment.
Current formatting is not aligned with the Druid style. It could be
new SimpleModule(
getClass().getSimpleName()
).registerSubtypes(
...Or
new SimpleModule(getClass().getSimpleName())
.registerSubtypes(
...| public void configure(Binder binder) | ||
| { | ||
| String typeName = MomentSketchAggregatorFactory.TYPE_NAME; | ||
| if (ComplexMetrics.getSerdeForType(typeName) == null) { |
There was a problem hiding this comment.
Could you please replace this boilerplate pattern with a single method registerSerde(String, Supplier<ComplexMetricSerde>) thoughout the code?
There was a problem hiding this comment.
I'll create the method and updated my module to use it but don't feel comfortable updating other modules in this PR. Maybe this could be migrated in a future PR?
| public MomentSketchWrapper fromByteBuffer(ByteBuffer buffer, int numBytes) | ||
| { | ||
| if (numBytes == 0) { | ||
| return EMPTY_SKETCH; |
There was a problem hiding this comment.
Does EMPTY_SKETCH.toByteArray() result in an empty array? Currently there is a discrepancy between fromByteBuffer() and toBytes() that looks suspicious when just reading the code.
There was a problem hiding this comment.
Good point, I'll make empty_bytes consistently correspond to a null sketch
|
|
||
| import java.nio.ByteBuffer; | ||
|
|
||
| public class MomentSketchWrapper |
There was a problem hiding this comment.
This class contains mathematical bits that are not obvious. It's javadoc comment should refer to some document (a paper at least, but preferably something more approachable) after reading which a reader of this class could understand what is "moment solving", ArcSinh compression concept, what does log(x + sqrt(1 + x^2)) mean, etc.
Ideally it's just explained inline.
P. S. I see those things are explained to some extent in MomentSketchAggregatorFactory. Please link to this class in the javadoc comment too.
| } | ||
|
|
||
| @Override | ||
| public Object combine(Object lhs, Object rhs) |
There was a problem hiding this comment.
Parameters should be annotated @Nullable.
| return serializedSketch; | ||
| } | ||
| throw new ISE( | ||
| "Object is not of a type that can be deserialized to a Moments Sketch" |
There was a problem hiding this comment.
Add space in the end of the string
| } | ||
|
|
||
| @Override | ||
| public synchronized void aggregate(final ByteBuffer buffer, final int position) |
There was a problem hiding this comment.
Some buffer aggregators do need synchronization for OffheapIncrementalIndex. See #3956.
|
Thank you for the comments, I will update this later in the week! |
|
@edgan8 as a side note, please don't "mark converations as resolved". I wish this Github feature could be turned off. As a reviewer I anyway have to revisit each conversation and verify myself that it's resolved, marking it as resolved just adds clicking work for me. |
|
@leventov Ah I did not realize it affected your workflow like that. Sure thing, I will be careful with that in the future! |
|
@edgan8 There are a couple of checkstyle errors |
|
@edgan8 Can you fix the checkstyle errors and conflict when you get a chance? |
|
@jon-wei just pushed |
|
@edgan8 please don't force-push your branch in the future, as noted here: https://github.com/apache/incubator-druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master |
| |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. Usable range is generally [3,15] |no, defaults to 13| | ||
| |compress|Flag for whether the aggregator compresses numeric values using arcsinh. Can improve robustness to skewed and long-tailed distributions, but reduces accuracy slightly on more uniform distributions.|| no, defaults to true |
There was a problem hiding this comment.
distributions.|| no, defaults to true has a formatting error
| { | ||
| final CacheKeyBuilder builder = new CacheKeyBuilder( | ||
| PostAggregatorIds.MOMENTS_SKETCH_TO_QUANTILES_CACHE_TYPE_ID | ||
| ).appendCacheable( |
There was a problem hiding this comment.
nit: I think it'd be better to keep each builder call on the same line, e.g. .appendCacheable(field)
| public MomentSketchAggregatorFactory( | ||
| @JsonProperty("name") final String name, | ||
| @JsonProperty("fieldName") final String fieldName, | ||
| @JsonProperty("k") final Integer k, |
There was a problem hiding this comment.
this should have @Nullable as well
|
I just completed a comparative analysis of the Druid Approximate Histogram, the Moments Sketch, and the DS-Quantiles sketch against some actual time-spent data we collected from one of our servers. This data has a well-behaved and smooth distribution so any reasonable quantile or histogram tool should be able to handle it without any issues. |
|
@leerho thank you for the careful analysis. I believe the larger than expected errors are due to the huge spike of zero values. I can update the moments sketch to handle zero values separately since they are so common and see how they affect accuracy, though that should not affect any of the druid integration code. |
|
@edgan8 Thank you for your reply.
Unfortunately, Druid end users did not bother to find and read the AH paper where the authors admit that the AH algorithm has serious limitations. And they likely didn't have the necessary skills to do a deep dive into the Druid HLL sketch algorithm to uncover its problems. Now, unfortunately, both groups of users are stuck with lots of historical data of dubious quality with no means of recovery. |
|
Currently the Moments Sketch can estimate the quantile value for a given a rank. |
|
@leerho I completely agree, it seems the concern is about the recommendations we provide to users. Due to concerns about robustness, I don't think the moments sketch should be a recommended first choice in standard environments either. For users with extreme demands, my understanding was that putting an extension package in contrib was a convenient to location allow them to experiment and can work on updating the documentation to make that more clear. If the druid maintainers have different plans for contrib then I can move this back to an external repository, I don't have a stake. @jon-wei please let me know your thoughts on the best place to put this package moving forward, I am also happy keeping it in my own repository if that is more convenient. @AlexanderSaydakov those features are not difficult to add if people find them important. |
I think extensions-contrib is a good place for this aggregator, we can make the characteristics and limitations clear in the docs, and have it as something that users can experiment with while recommending the DS-Sketch for general use. |
|
@jon-wei I hope that the studies that I reference in this thread as well as my concerns and @edgan8's agreement should provide sufficient information for someone to generate relevant documentation. Since I don't really know how (style, place, format, etc.) the Druid team wants to document these algorithms, someone on the Druid team should do that. Once generated I would be happy to review and comment, but I really need someone to take ownership of the documentation of these algorithms for Druid. |
|
@jon-wei you may want to update the docs for the Approximate Histogram and Druid HLL as well. |
|
@AlexanderSaydakov that sounds good, the 0.14.0 docs will try to move users away from ApproximateHistogram and the old Druid HLL |
|
@leventov did you have more comments on this |
|
I'm OK with this PR. |
|
@edgan8 thanks for the contrib! |
|
This seems to have broken CI on master. |
|
@glasser I'm making a PR to fix the build issues |
|
@jon-wei do you have an update on when / how this will be included in a future build? Just curious if I should follow-up on anything. |
|
@edgan8 This'll be available as a contrib extension starting in 0.15.0. No need to do anything on your end! |
Initial pull request for a druid aggregation extension that supports the moments sketch. The moments sketch is a compact, efficiently mergeable approximate quantile sketch. This extension wraps the library available here: https://github.com/stanford-futuredata/momentsketch . The post aggregator can be used to extract quantile estimates from the aggregator.
The aggregator is parameterized by k, the size of the sketch, and a boolean parameter "compress" which will compress the range of input values, improving accuracy for very long-tailed distributions, but slightly reducing accuracy for values more uniformly distributed across their range.
MomentSketchAggregatorTest shows how the custom aggregation can be constructed during either ingest or query time.