Add approx_percentile_cont() aggregation function

[domodwyer]

Which issue does this PR close?

Closes #1538.

What changes are included in this PR?

A new approx_quantile() aggregation function.

This PR also includes support for aggreagte functions with multiple arguments - all existing aggregations had a signature such as fn(column), whereas this fn requires approx_quantile(column, quantile).

This involved two main changes to existing code:

• Support for TypeSignature::OneOf for aggregation functions
• Changing the protobuf format to allow AggregateExprNode to carry multiple LogicalExprNode (see commit messages)

I'd like to draw attention to the fact this TDigest impl is largely taken from (the Apache 2.0 licensed) work by MnO2 and modified to fit datafusion efficiently - itself a port of Facebook's C++ implementation. I've included this information in a comment in the file, but let me know if I should provide attribution in some other way too!

Are there any user-facing changes?

A new approx_quantile() aggregation is available to the user, both via SQL and the dataframe API.

I added the approx_quantile() aggregation to the prelude - let me know if that's not right.

Future

I did not spend much time trying to optimise this, and I suspect it can be polished up a bit but wanted to land something working first. I may also investiage the uddsketch algorithm in the future and potentially swap out the tdigest approach used here if it proves to be superior.

Should I include documenting this new fn anywhere as part of the PR?

---

• feat: implement TDigest for approx quantile (b72d21c)

  Adds a [TDigest] implementation providing approximate quantile estimations of
  large inputs using a small amount of (bounded) memory.
  
  A TDigest is most accurate near either "end" of the quantile range (that is,
  0.1, 0.9, 0.95, etc) due to the use of a scalaing function that increases
  resolution at the tails. The paper claims single digit part per million errors
  for q ≤ 0.001 or q ≥ 0.999 using 100 centroids, and in practice I have found
  accuracy to be more than acceptable for an apprixmate function across the
  entire quantile range.
  
  The implementation is a modified copy of https://github.com/MnO2/t-digest,
  itself a Rust port of [Facebook's C++ implementation]. Both Facebook's
  implementation, and Mn02's Rust port are Apache 2.0 licensed.
  
  [TDigest]: https://arxiv.org/abs/1902.04023
  [Facebook's C++ implementation]:
  https://github.com/facebook/folly/blob/main/folly/stats/TDigest.h
  

• feat: approx_quantile aggregation (d5fc006)

  Adds the ApproxQuantile physical expression, plumbing & test cases.
  
  The function signature is:
  
  	approx_quantile(column, quantile)
  
  Where column can be any numeric type (that can be cast to a float64) and 
  quantile is a float64 literal between 0 and 1.
  

• feat: approx_quantile dataframe function (8714293)

  Adds the approx_quantile() dataframe function, and exports it in the prelude.
  

• refactor: bastilla approx_quantile support (b4ff5b3)

  Adds bastilla wire encoding for approx_quantile.
  
  Adding support for this required modifying the AggregateExprNode proto message
  to support propigating multiple LogicalExprNode aggregate arguments - all the
  existing aggregations take a single argument, so this wasn't needed before.
  
  This commit adds "repeated" to the expr field, which I believe is backwards
  compatible as described here:
  
  	https://developers.google.com/protocol-buffers/docs/proto3#updating
  
  Specifically, adding "repeated" to an existing message field:
  
  	"For ... message fields, optional is compatible with repeated"
  
  No existing tests needed fixing, and a new roundtrip test is included that
  covers the change to allow multiple expr.
  

• refactor: use input type as return type (d72ca10)

  Casts the calculated quantile value to the same type as the input data.
  

[hntd187]

Is there a reason this is pub(crate) while everything else is open?

[domodwyer]

I modelled approx_quantile() after approx_distinct() which uses HyperLogLog internally - the hyperloglog mod is also pub(crate) so I did the same - I can change tdigest to pub if that makes more sense?

Should hyperloglog also be pub?

[alamb]

I think pub(crate) is a good place to start, and we can make it pub later on if there is some usecase

[Dandandan]

The branches are the same for less and equal so I think the whole match could be simplified to

if centroids[i] <= centroids[j] {
    result.push(centroids[i].clone());
    i += 1;
} else {
    result.push(centroids[j].clone());
    j += 1;
}

[realno]

This is a very useful feature, thank you 👍

DataFusion usually try to match Postgres' function definition. So for quantile they use percentile instead, and implemented in two ways percentile_disc and persentile_cont as specified here: https://www.postgresql.org/docs/9.4/functions-aggregate.html

It would be nice we follow the same convention.

[domodwyer]

The T-Digest algorithm returns an interpolated result, so I think percentile_cont makes most sense - however T-digest is an approximation of the quantile. Should we include approx in the name to signify this? If so, would approx_percentile_cont be the desired name, keeping in line with the existing approx_distinct aggregate?

[realno]

The T-Digest algorithm returns an interpolated result, so I think percentile_cont makes most sense - however T-digest is an approximation of the quantile. Should we include approx in the name to signify this? If so, would approx_percentile_cont be the desired name, keeping in line with the existing approx_distinct aggregate?

This sounds reasonable. I suggest also check if anyone from the community is more familiar with Postgres implementation. I remember I read somewhere its quantile is more efficient than median function, so I assume it uses something like KLL., if that's the case it may not be a big deal if we use approx or not.

[realno]

@alamb is this test sufficient for testing merge functions?

[domodwyer]

Merge appears to always be called, so it is definitely exercised - I don't know if this is sufficent coverage though!

[realno]

Looked at it again, it should be fine since the compute logic is tested in the algo.

[realno]

I think we can use more unit tests here if necessary.

[domodwyer]

Thanks for the reviews - I shall make some time to rebase this and address the comments soon 👍

[domodwyer]

I have rebased onto master and made the requested changes - thanks for the reviews! 👍

I still need to rename approx_quantile() to approx_percentile_cont() - I just wanted to confirm this is the desired name before making the changes.

If no one objects, I shall do the rename in a couple of days!

[domodwyer]

Done!

+-------------------------------------------+
| APPROXPERCENTILECONT(test.b,Float64(0.5)) |
+-------------------------------------------+
| 10                                        |
+-------------------------------------------+

This should be good for another review now - thanks all 👍

[realno]

LGTM, nice work, thanks! @domodwyer

[alamb]

👍 nice work @domodwyer . Thank you for the review @realno

I didn't check the math, but I reviewed the structure of this PR and tests.

I also took it for a test drive

cargo run --bin datafusion-cli

❯ create table t1 as select * from (values (11, 'a'), (22, 'b'), (33, 'c'), (44, 'd'), (77, 'e')) as sq;
0 rows in set. Query took 0.016 seconds.
❯ select * from t1;
+---------+---------+
| column1 | column2 |
+---------+---------+
| 11      | a       |
| 22      | b       |
| 33      | c       |
| 44      | d       |
| 77      | e       |
+---------+---------+
5 rows in set. Query took 0.008 seconds.
❯ select approx_percentile_cont(column1, 0.2) from t1;
+-----------------------------------------------+
| APPROXPERCENTILECONT(t1.column1,Float64(0.2)) |
+-----------------------------------------------+
| 16                                            |
+-----------------------------------------------+
1 row in set. Query took 0.012 seconds.
❯ select approx_percentile_cont(column1, 0.2) from t1 group by column2;
+-----------------------------------------------+
| APPROXPERCENTILECONT(t1.column1,Float64(0.2)) |
+-----------------------------------------------+
| 11                                            |
| 44                                            |
| 22                                            |
| 77                                            |
| 33                                            |
+-----------------------------------------------+
5 rows in set. Query took 0.011 seconds.

[alamb]

I think pub(crate) is a good place to start, and we can make it pub later on if there is some usecase