Hey folks, I have a PR to offer which implements

approx_distinct

. Should I fork and make a PR as I don't have push rights to the repo?

Hi Neil! Yeah forking and making a PR is how we normally go about it. Just wanted to confirm, is this a different expression from https://www.getdaft.io/projects/docs/en/stable/api_docs/doc_gen/expression_methods/daft.Expression.approx_count_distinct.html ?

Yes, Approx count distinct "counts" Approx distinct "lists"

Tagged @Raunak Bhagat as a reviewer as he recently did some work on aggregations I mentioned this on the PR, but I do think that this is not an implementation of an

approx_distinct

, but rather a

distinct

as it uses

HashSet

under the hood. This is great because

distinct

is what we really want! Could we remove all the references to sketches and

approx_

from the names?

I kept

approx

since like the other implementation, it ignores

None

values. Is the expected behavior of

distinct

as well? Also, I could use some help with the test issues - I have a feeling there's some under-the-hood rust behavior I'm not catching but I'm not sure how to move forward on that front. Finally, How can improve the performance? it looks like I'm slowing

daft

down by 50%?

Is the expected behavior of

distinct

as well?

I believe that NULLs are typically treated as a value, e.g. snowflake's array_distinct @Andrew Gazelka do you remember what decision we came to regarding NULLs and `value_counts`? At the very least we should be consistent

Finally, How can improve the performance? it looks like I'm slowing

daft

down by 50%?

Let's ignore the codspeed results, they're kinda noisy now and seems like network or i/o is what leads to the huge variance