regression: quantilesDoublesSketch returns null instead of empty array

[pdeva]

This seems to be new behavior in 0.14

Affected Version

0.14.0-incubating-iap3

Description

query code:

"queryType": "timeseries",
"aggregations": [
    {
      "type": "quantilesDoublesSketch",
      "name": "value",
      "fieldName": "value",
      "k": 512
    }
  ],
  "postAggregations": [
    {
      "type": "quantilesDoublesSketchToQuantiles",
      "name": "qtl",
      "field": {
        "type": "fieldAccess",
        "fieldName": "value"
      },
      "fractions": [
        0.5,
        0.9,
        0.95,
        0.99
      ]
    }
  ],

in the response, whenever there is no value present for the value column, the result would look like this:

{
    "timestamp": "2019-04-16T03:15:00.000Z",
    "result": {
      "qtl": null,
      "value": 0
    }
  },

Previous druid versions would return "qtl": [], which seems like correct behavior rather than just returning null

[AlexanderSaydakov]

Let's see:
Druid 0.14 uses sketches-core-0.12.0
Druid 0.13 uses sketches-core-0.10.3
This behavior changed in sketches-core-0.11.0
I believe that previous behavior was to always return an array of quantile values corresponding to the specified ranks. In case of an empty sketch the values returned are NaN, not an empty array. We thought that array of NaN values does not make much sense, so we changed this to return null. This affects all functions returning arrays: getQuantiles(), getPMF() and getCDF()

If null result is undesirable in Druid, it should be changed in the post-aggs.
Could you clarify why null result from an empty sketch is not desirable? Why an array of NaNs was better?

[pdeva]

My suggestion would be to return an empty array [] instead of null.

With an empty array, all code that works on array continues working naturally and unmodified.
Eg, methods like .map(), .filter() etc continue to work and handle the case of empty array.

With null we have to put in special checks in code everywhere to check for null.

If null result is undesirable in Druid, it should be changed in the post-aggs.

what is the right way to change null to empty array in post agg? i dont even know. could you please clarify.

[AlexanderSaydakov]

In your example query you wanted four quantile values for ranks 0.5, 0.9, 0.95 and 0.99. How would you process the result? What if it is an empty array, not an array with four values you would expect?

This is about DoublesSketchToQuantilesPostAggregator.java
Currently it just returns the result from the sketch like so:
return sketch.getQuantiles(fractions);
We might change that to something like this:
double[] quantiles = sketch.getQuantiles(fractions);
if (quantiles == null) quantiles = new double[0];
return quantiles;

And I think we have a bigger problem in DoublesSketchToHistogramPostAggregator.java
It is going to throw a null pointer exception.

We may also want to add more post-aggs to get rank of a given value and get CDF.

[pdeva]

In your example query you wanted four quantile values for ranks 0.5, 0.9, 0.95 and 0.99. How would you process the result? What if it is an empty array, not an array with four values you would expect?

as i mentioned, the code to handle arrays gracefully handles empty arrays, whether its charting libraries or our own code. but null throws a wrench into everything..

Even in the code you pointed out:

return sketch.getQuantiles(fractions);

as you yourself point out, this now has to be special cased to handle null, whereas it worked fine with empty arrays previously.
returning an empty array instead would allow all existing code to continue working

[AlexanderSaydakov]

it worked fine with empty arrays previously

Just to be precise, I don't believe it was an empty array. It was an array with NaNs. If you asked for four quantiles, you would get four NaNs. Would you like this to happen or would you like an empty array?

[pdeva]

i would prefer an empty array.

[AlexanderSaydakov]

So you would need to check if the size is zero. How this special treatment better than checking for null?

[drcrallen]

If I recall, all of the aggregators have a "default" state. Things like sum are 0, things like max are MIN_VALUE, things like min are MAX_VALUE. From the returned results the quantilesDoublesSketch returns 0 on empty. Would it ever make sense for the quantilesDoublesSketchToQuantiles to return an array of 0's? I suspect that will end up causing more confusion than clarity, and isn't really representative of the base state for quantiles.

For null I can't think of any other aggregator which has null as the default state. If there is one it would provide precedence here to use null for "no data".

In other parts of the code org.apache.druid.query.aggregation.histogram.ApproximateHistogram#getQuantiles uses an array of NaN. But org.apache.druid.query.aggregation.histogram.FixedBucketsHistogram#percentilesFloat seems to return 0. So handling this case is a bit all over the map :-/

[AlexanderSaydakov]

Are you referring to the finalizeComputation() method as the "default" state?
This notion might make more or less sense for different sketches. Let's say, for distinct counting sketches like Theta or HLL this makes perfect sense: it must be the estimate of the distinct count. It makes much less sense for quantiles sketch that represents an arbitrary distribution of some values. We decided that the most sensible single number for this purpose would be the total weight of the distribution (also known as the stream length or the number of values, if they all have the weight of 1).
Now consider an empty quantiles sketch. Nothing is known about the distribution. The total weight is zero, so there is no problem with this questionable finalizeComputation(), but we cannot say anything at all about quantiles or ranks. What is the min value, median or max or any other quantile? If a single quantile is requested, then the best answer must be NaN, not zero since zero is a perfectly good number and would be deeply misleading. What to do if an array of quantiles is requested? Options are: null, empty array and array of NaNs (the same number as in the request). We used to do the latter in the core library, but at some point (between versions 0.10.3 and 0.11.0) we decided that it did not make much sense to spend resources in the core library to create this array and fill it with NaN values. We decided to return null and let the user decide what to do in the presentation layer. So let's decide what do we want Druid to return from such a post-agg.

[drcrallen]

This discussion probably needs posted to the dev list, or else it risks being part of non-apache-way discussion paths.

[drcrallen]

I'm going to start a thread

[gianm]

This discussion probably needs posted to the dev list, or else it risks being part of non-apache-way discussion paths.

Fwiw, my understanding is that GH discussions are OK, because they are all copied to commits@druid.apache.org and so they are recorded for posterity. But nothing wrong with raising dev threads for additional visibility.

[AlexanderSaydakov]

Should we proceed with returning an empty array?

[jon-wei]

@AlexanderSaydakov The consensus so far on the discussion thread @drcrallen opened is that the array of NaN is the best option (https://lists.apache.org/thread.html/32c816487529dd3b6f04b9f029f2136050a10d54f19f5c1f3f15fc2d@%3Cdev.druid.apache.org%3E)