@SmitaRKulkarni I had to close this #55305 because of a messed up rebase. Please review this instead. I added the docs and address comments from the other PR

I would appreciate it if someone could trigger workflows.

This is an automated comment for commit 068f6e2 with description of existing statuses. It's updated for the latest CI running

⏳ Click here to open a full report in a separate page

2023.11.06 11:36:06.177838 [ 895 ] {80ce4c2c-097e-47a1-8870-042595f45cd7} : Logical error: 'std::exception. Code: 1001, type: std::length_error, e.what() = vector (version 23.11.1.1), Stack trace:
2023.11.06 11:36:06.178402 [ 921 ] {} BaseDaemon: ########## Short fault info ############
2023.11.06 11:36:06.178441 [ 921 ] {} BaseDaemon: (version 23.11.1.1, build id: 548B41F3A0C69477122CCC46FB1DC905F5EA412C, git hash: 47efee857a245ba5f5a79a11eb1bbfa4b2bebb70) (from thread 895) Received signal 6
2023.11.06 11:36:06.178469 [ 921 ] {} BaseDaemon: Signal description: Aborted
2023.11.06 11:36:06.178489 [ 921 ] {} BaseDaemon:
2023.11.06 11:36:06.178518 [ 921 ] {} BaseDaemon: Stack trace: 0x00007f5df92f69fc 0x00007f5df92a2476 0x00007f5df92887f3 0x00005574a14112c8 0x00005574a141e055 0x00005574a14218f1 0x00005574b96432dd 0x00005574b966d967 0x00005574a16fff49 0x00005574a170e48d 0x000055748e2f70ab 0x00007f5df92f4ac3 0x00007f5df9386a40
2023.11.06 11:36:06.178546 [ 921 ] {} BaseDaemon: ########################################
2023.11.06 11:36:06.178616 [ 921 ] {} BaseDaemon: (version 23.11.1.1, build id: 548B41F3A0C69477122CCC46FB1DC905F5EA412C, git hash: 47efee857a245ba5f5a79a11eb1bbfa4b2bebb70) (from thread 895) (query_id: 80ce4c2c-097e-47a1-8870-042595f45cd7) (query: SELECT arrayMap(a -> round(0), quantilesDDSketch(1.1754943508222875e-38, 0, 0.001, 1., 1.1754943508222875e-38, 0.1, 1.1920928955078125e-7, 0.9999, 0.4, -0., -0., 0.9999, 0.5, 0.0001, 1.1920928955078125e-7, 0.999, 1)(x)) FROM (SELECT number AS x FROM system.numbers LIMIT 65536)) Received signal Aborted (6)

How can I get the fuzzer to pass a reasonable relative accuracy? The value 1.1754943508222875e-38 (understandably) crashes the build. I am thinking of throwing an exception in case of provided relative accuracy is too low. I think most practical applications will be fine with something like 0.0003 i.e. no more than 0.03%. Let me know what you think.

How can I get the fuzzer to pass a reasonable relative accuracy? The value 1.1754943508222875e-38 (understandably) crashes the build. I am thinking of throwing an exception in case of provided relative accuracy is too low. I think most practical applications will be fine with something like 0.0003 i.e. no more than 0.03%. Let me know what you think.

It looks like the msan fuzzer builds indicate possible memory violations here: src/AggregateFunctions/QuantileDDSketch.h:25 and here src/AggregateFunctions/QuantileDDSketch.h:31.

SELECT round(quantileDDSketch(1.1754943508222875e-38)(number)) FROM numbers(255) GROUP BY -2 / (number + 3) WITH ROLLUP

They are failing because the passed relative accuracy is too low which demands bins beyond allocatable and program crashes. If you change the relative accuracy to a reasonable value it works without crashing.

QuantileBFloat16 test is failing for ubsan, ParallelReplicas and QuantileDDSketch if failing for ubsan. It seems like a timeout. How can I troubleshoot or are they occasional intermittent failures?

I will resolve the conflicts

QuantileBFloat16 test is failing for ubsan, ParallelReplicas and QuantileDDSketch if failing for ubsan. It seems like a timeout. How can I troubleshoot or are they occasional intermittent failures?

I think QuantileBFloat16 test failure is un-related, it's failing because its not able to connect to the server (likely the server has crashed from the other test failure.)

The main error seems to be the following

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /build/src/AggregateFunctions/Store.h:106:35 in 

Possible overflows are happening here during addition. Probably, the easiest way is to fix is to use NO_SANITIZE_UNDEFINED (i.e. to suppress undefined behavior sanitizer) in the function signature of serialize. Here is an example for this. There are also arithmeticOverflow helper functions that basically do the same (suppresses the UBSAN) to handle the overflows for you while doing arithmetic operations. Better way is to actually handle overflows without suppressing the UBSAN if you're able to do so realistically .

@bharatnc Thanks, that was helpful. I saw other files but didn't pay attention to stderr.log. This happens when the store is empty. I pushed a fix (check for the empty state) to solve it.

I don't have solid expertise with C++. Few suggestions from the previous review didn't make much difference. I will be happy to make changes if it gives such a significant improvement.

I would do something like this: https://pastila.nl/?0052513d/409c97e2bb061b060eacf41ae73ceae2#IkYx5MhJoZnbPjohZ4NQNw== (untested), on the off chance you're in the mood for rewriting everything.

I don't have solid expertise with C++. Few suggestions from the previous review didn't make much difference. I will be happy to make changes if it gives such a significant improvement.

I would do something like this: https://pastila.nl/?0052513d/409c97e2bb061b060eacf41ae73ceae2#IkYx5MhJoZnbPjohZ4NQNw== (untested), on the off chance you're in the mood for rewriting everything.

I will give it a try. If you believe it could improve the performance by multifold I want to give it a try.

   /// (No need to write mapping and scale, they're determined by parameters, which should match

    ///  between serializer and deserializer. Unless again you need to be compatible with some
    ///  other system for some reason. No need for changeMapping() for the same reason.
    ///  Unless I'm missing something?)

To give some clarity on this, I was trying to keep the state compatible with the DDSketch proto https://github.com/DataDog/sketches-java/blob/master/src/protobuf/proto/DDSketch.proto. The clickHouse-go client would directly write the rows with the compatible serialized state. This will enable ingesting the pre-aggregated sketches (usually on the web service application) and then computing percentiles at the query time.

I will give it a try. If you believe it could improve the performance by multifold I want to give it a try.

Cool! Hack a quick prototype first, to check if it's really fast enough to be worth rewriting the rest. (Though it's also less code, so might be worth rewriting even if not much faster, but that's less important and more subjective.)

   /// (No need to write mapping and scale, they're determined by parameters, which should match

    ///  between serializer and deserializer. Unless again you need to be compatible with some
    ///  other system for some reason. No need for changeMapping() for the same reason.
    ///  Unless I'm missing something?)

To give some clarity on this, I was trying to keep the state compatible with the DDSketch proto https://github.com/DataDog/sketches-java/blob/master/src/protobuf/proto/DDSketch.proto. The clickHouse-go client would directly write the rows with the compatible serialized state. This will enable ingesting the pre-aggregated sketches (usually on the web service application) and then computing percentiles at the query time.

Makes sense, please add a comment about it in the code, with the link. Otherwise it just looks like random crazy overengineering tbh :)

@al13n321 I tried the suggestion in a separate branch and it improved the performance by ~20%. And then I revisited your comment here about the virtual functions #56342 (comment). The last time when I added final it didn't make a difference but I removed the inheritance and virtual functions and that change also improved the performance, i.e. ~20%. I pushed the updated changes, please take a look again.

I started looking into small object optimization. Is it okay if that is addressed in the follow-up PR? This has gone through many changes at this point and has been reviewed. Since it will be an enhancement that doesn't break anything, I think it's better to address it in a follow-up PR without adding more new changes to this already reviewed PR. Let me know what do you think?

@srikanthccv, totally ok.

Looks mergable, except for the fuzzer failure from before: https://s3.amazonaws.com/clickhouse-test-reports/56342/b55529b5850abc4cc3578b96582c92476c1b558a/fuzzer_astfuzzerubsan/report.html - did you explain or fix it? It didn't happen this time, but I don't see anything that looks like a fix, so it's probably not fixed and will come back.

Maybe try to reproduce it by running the last few queries with substring "DDSketch" from fuzzer.log and server.log.zst (from the link above), in UBSAN build (or with a manual overflow check for the particular subtraction that overflowed). If that doesn't work, just add an overflow check in getNewLength(), and we'll hope that my explanation in #56342 (comment) is correct.

hmm, I posted a comment before resolved but it seems to have been not delivered.

@al13n321 The commit I made which caused the overflow was an incorrect change and shouldn't have been committed.

Here is what happened. The new_length returned occasionally would be smaller than the length of the current bins after the suggestion bins.resize(bins.capacity()); is incorporated - the size of the current bins is enough to adjust the layout. Originally there was no check before resizing and as a result, some previous counts got wiped out. I added the check in this commit d831792 (#56342). The overflow shouldn't happen the keys are going to be well within the range. We do not allow relative accuracy no less than 1e-6. So the max possible value is 1.79769e+308 and it's the corresponding key is 354891355 so we are fine. Let me know if this answers your question.

@al13n321 @alexey-milovidov @bharatnc Thanks for the reviews and help. I will send a follow-up PR for small object optimization after testing against the ClickBench data.