bug: getQuantiles() returns values that exceed max

[ginoledesma]

Fixes #3972

[ginoledesma]

Updated PR to include a proposed fix. May need discussion to ensure this is the right way to do things.

I fed getQuantiles() with more synthetic data sets to see that the numbers generated are sane. Since the algorithm is based off the BH/TT uniform() procedure, we know that uj should not exceed s given that uj should fall in the set of real numbers {u1 < . . . < u~B}, where s = u~B.

[gianm]

In addition to the one comment, @ginoledesma could you please fill out our CLA located at: http://druid.io/community/cla.html

Thanks for the contribution.

[gianm]

It's nice how simple, yet effective this tweak is. Would it make sense to do a similar tweak for mins?

[gianm]

In addition, if you have a clearly worded explanation of why the uj's can be greater than the max, a comment would be useful. Something about why it's necessary to clamp them to the max.

[gianm]

Does this help with the issue reported in #3972 (with regard to the median, rather than high percentiles)? It seems like max-clamping might not help with the low/mid quantiles like median.

[ginoledesma]

@gianm I've updated the PR to include a comment on why clamping to max needs to happen. It's mostly based on the BH/TT paper but I hope it's clear and makes sense. The short version is that we don't expect any "points" (values) to exist to the left of the known minimum or the right of the known maximum.

The clamping to min was already handled as a special case (line 1571 - https://github.com/druid-io/druid/pull/4744/files#diff-de22260542704e07a950d869cd124aefL1571).

As for #3972, yes, it handles that situation. I've tried to replicate the original poster's situation, but I took a guess in filling in the blanks for the full ingestion spec. Here's what I have:

• ingestion_spec.json.txt
• api_duration.csv.gz

Given the time series query:

{
  "queryType": "timeseries",
  "dataSource": "test_quantile_fix",
  "granularity": "all",
  "intervals": [
    "2017-09-01T00:00:00+08:00/2017-10-01T00:00:00+08:00"
  ],
  "aggregations": [
    {
      "type": "approxHistogramFold",
      "name": "hist_fold",
      "fieldName": "hist_fold_api_duration",
      "resolution": 10000,
      "numBuckets": 100
    }
  ],
  "postAggregations": [
    {
      "type": "quantile",
      "name": "myResult",
      "fieldName": "hist_fold",
      "probability": 0.5
    }
  ],
  "context": {}
}

Druid 0.10.1 Result for q=0.5: 1111.9833
druid_quantiles_stock.json.txt

Druid 0.10.1 + patch Result for q=0.5: 190.86847
druid_quantiles_patched.json.txt

Reference (computed using SciPy for q=0.5): 190.0

You'll notice the breaks are completely different between the behavior without the clamped values for max, but I think that has more to do with the folding of multiple approximate histograms, which gets exacerbated when the original values weren't clamped to begin with.

[ginoledesma]

I'll update the PR with the CLA tomorrow — I did use some personal/company time so I'll check with the company to get that ironed out.

[ginoledesma]

Updated unit tests to be more thorough with computing a range of quantiles (q=[.10, ..., 0.90] in 0.10 increments) including the explicit test for "outliers" (q=0.05 and q=0.95).

[ginoledesma]

Not sure why the travis builds are failing, but it seems to be affecting the other PRs as well — particularly on those with the -Xmx512m maven opts set.

[gianm]

@ginoledesma We've been having some unreliability with one of the tests recently, sorry about that. I re-ran it and it may pass when run a second time.

The patch looks good to me, thanks for the additional tests as well. Have you had a chance to get the CLA sorted?

[gianm]

@ginoledesma If you merge master into your branch, it should help with the CI issues. We just did a commit there that tweaks some Travis settings and seems to be helping.

[gianm]

LGTM.

[gianm]

@ginoledesma Hmm, this change does seem better, but I'm wondering how it helps with the test you describe in #4744 (comment). The patch only affects the getQuantiles(float[]) method, which is only called in the post-aggregation step, and so it shouldn't affect the breaks or folding behavior at all (which happen before that, in the aggregation step).

[gianm]

@ginoledesma, any thoughts on the question from #4744 (comment)?

[drcrallen]

waiting for answers to @gianm 's questions before merging.

[ginoledesma]

I don’t have a solid answer, unfortunately. I’m trying to come up with a smaller dataset that replicates the approximation breaks do it can be replicated in the tests properly, but I haven’t found any good leads to explain it.

[gianm]

@ginoledesma thanks for the details. If you do find any new info please raise a new issue. In the meantime I'll merge this patch since it does look like an improvement. Thanks!