add quantileInterpolatedWeighted function

[bharatnc]

Changelog category (leave one):

• New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Add quantileInterpolatedWeighted/quantilesInterpolatedWeighted functions.

The current quantileExactWeighted function returns the exact quantile. Add another weighted quantile function that instead approximates the quantiles:

• sorts the input values and corresponding weights.
• builds a cumulative distribution based on weights.
• performs linear interpolation b/w the weights and the values to compute the quantiles.

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

[alexey-milovidov]

The name can be confusing, as it can be confused with approximate quantile sketches.
Better to name it quantileExactInterpolated and quantileExactInterpolatedWeighted.
Also the functions quantileExactInclusiveWeighted, quantileExactExclusiveWeighted, quantileExactLowWeighted, quantileExactHighWeighted should be added for consistency.

[bharatnc]

The name can be confusing, as it can be confused with approximate quantile sketches. Better to name it quantileExactInterpolated and quantileExactInterpolatedWeighted. Also the functions quantileExactInclusiveWeighted, quantileExactExclusiveWeighted, quantileExactLowWeighted, quantileExactHighWeighted should be added for consistency.

Sure, thank you for the comment. I'm thinking of replacing quantile(s)ApproximateWeighted with quantile(s)InterpolatedWeighted in this PR. quantile(s)InterpolatedWeighted sounds a little more apt to me (without the word exact).

[alexey-milovidov]

Ok.

[bharatnc]

• rename quantileApproximateWeighted to quantileInterpolatedWeighted everywhere.

(I'll do it finally since it's only a rename)

[bharatnc]

@evillique the PR is ready for your review, would appreciate your feedback.

[evillique]

I'm sorry that it took so long to get back to this PR, could you please update this branch to the latest master and add documentation similar to this?

ClickHouse/src/Functions/concatWithSeparator.cpp

Lines 158 to 163 in 6e5e2d5

	factory.registerFunction<FunctionConcatWithSeparator>({
	R"(
	Returns the concatenation strings separated by string separator. Syntax: concatWithSeparator(sep, expr1, expr2, expr3...)
	)",
	Documentation::Examples{{"concatWithSeparator", "SELECT concatWithSeparator('a', '1', '2', '3')"}},
	Documentation::Categories{"String"}});

[bharatnc]

@evillique thank you for taking a look again, sure I will rebase and update again today.

[bharatnc]

@evillique thank you for taking a look again, sure I will rebase and update again today.

@evillique rebased and fixed the build failures mostly (failing ones seem unrelated but obscure to me).

For docs, I added a separate markdown for this function. (If you want me to add docs in the way you have recommended - can I do that in an another PR ? Looks like it needs to be done for other quantile and possibly for other aggregate functions as well.)

[evillique]

@evillique rebased and fixed the build failures mostly (failing ones seem unrelated but obscure to me).

Sometimes builders fail for unrelated reasons. When the status of the build is something similar to BuilderDebMsan failed in the report, it is unrelated and the build process should be restarted in the near future.

For docs, I added a separate markdown for this function. (If you want me to add docs in the way you have recommended - can I do that in an another PR ? Looks like it needs to be done for other quantile and possibly for other aggregate functions as well.)

For now, internal documentation for aggregate functions is not required but is recommended. If you can also do it for other aggregate functions it would be greatly appreciated.

Also, it looks like AST fuzzer (ubsan) test failure is related to the changes in this PR:

../src/AggregateFunctions/QuantileInterpolatedWeighted.h:298:56: runtime error: -1.29127e+19 is outside the range of representable values of type 'long' Received signal -3 Received signal sanitizer trap (-3)

[bharatnc]

Interesting, the AST fuzzer for ubsan passed for the previous commit - no code changes. I'll take a look.

[bharatnc]

@evillique it would be nice to get this merged if the changes look good to you ? Let me know if you have any more changes that you want me to make.

[evillique]

We should probably also mention quantiles* function here: docs/en/sql-reference/aggregate-functions/reference/quantiles.md:12

[bharatnc]

We should probably also mention quantiles* function here: docs/en/sql-reference/aggregate-functions/reference/quantiles.md:12

sure added.

[qoega]

@bharatnc WDYT you think is it valid to add non integer weights as well? Like when user has % as weight with 1 as cumulative sum.

[bharatnc]

@bharatnc WDYT you think is it valid to add non integer weights as well? Like when user has % as weight with 1 as cumulative sum.

Hi, I think it'll be okay to support non integer weights depending on the use case. I chose to stick with integer weights for this implementation of quantileInterpolatedWeighted as the quantileExactWeighted also currently only supports integer weights. Perhaps it'll be nice to have both the functions allow non integer weights..