[SPARK-17306] [SQL] QuantileSummaries doesn't compress

[srowen]

What changes were proposed in this pull request?

• Actually call compress() in insert() when the threshold is exceeded
• Also, avoid using ArrayBuffer where all data is prepended, because this is O(n)

How was this patch tested?

Existing tests.

[srowen]

CC @thunterdb -- is this the right fix?
I also 'adjusted' calls to .append() which is actually a varargs method; += appends an element

[clockfly]

@srowen
I think the compression decision need to be related to relative error setting. (The smaller the relative error is, the less frequent we do compression)

When implementing aggregation function percentile_approx, I have implemented compression like this:

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/ApproximatePercentile.scala#L214

[clockfly]

By the way, would you mind change ApproximatePercentile altogether, the compression in ApproximatePercentile will not be necessary if we have done compression at QuantileSummaries

[thunterdb]

Yes, this is the fix, thanks for doing it. I had never realized that .append takes a vararg input, thanks for the hint.

[srowen]

@clockfly oh, hm, this code just landed recently? it seems to call compress() itself rather than leave it to QuantileSummaries, in which case I'm not clear why there's a compressThreshold in QuantileSummaries It seems like the new class is trying to manage it. What's the right way to rationalize this -- are you saying QuantileSummaries shouldn't manage compression at all? that's fine too (in which case this can just turn into a very small optimization change).

[thunterdb]

@clockfly @srowen the compression threshold is just here to amortize the cost of performing compression. If you wanted to, you could run compression every iteration (it is an idempotent operation). Internally, the compress method uses a merging threshold that indeeds depends on the number of elements seen, but it operates on a number of samples that is bounded by O(1/\epsi).
This patch will work. @clockfly I suspect some of the wrappers done in the Approximate percentile are not required either, once I submit a PR that fixes an off-by-1 error.

[clockfly]

@srowen @thunterdb

I think the compression still need to be done in QuantileSummaries. I added some compression implementation in wrapper class ApproximatePercentile because ApproximatePercentile need to know whether the QuantileSummaries is compressed or not, otherwise ApproximatePercentile don't know whether it is OK to call def query(quantile: Double) of QuantileSummaries.

Maybe QuantileSummaries should expose an API like "isCompressed"? So that the caller can skip calling compress if QuantileSummaries is already compressed.

[SparkQA]

Test build #64997 has finished for PR 14976 at commit 7226aa0.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[thunterdb]

@srowen LGTM, thanks!

[thunterdb]

By the way, you gave some great advice here. Is there a page on the wiki where we collect all this internal knowledge?

[SparkQA]

Test build #65032 has finished for PR 14976 at commit 75cb088.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

Superseded by #15002