Puffin: Document stats ndv value representation

[findepi]

Follows discussion #10288 (comment)

[findepi]

cc @karuppayya

[jbonofre]

Good and useful details to me. Thanks !

[szehon-ho]

I am not sure, but does this mean an integer value like 2 now becomes 2.0? (if using java toString)

And in any case, as its not entirely backward compat, should we update the theta sketch version again? Maybe can bundle with the other pr of @amogh-jahagirdar : #10549

[szehon-ho]

Is 'integer' necessary? Not sure if its just me, but does it add confusion (i initially interpret it to mean 'number without decimal point')?

[findepi]

i actually used "integer" in "whole number without decimal point" meaning (and not eg as java integer, which is 32-bit integer value).
what's the best way to say this in English?

[szehon-ho]

Sorry I misunderstood the pr, 'decimal' threw me off.

[findepi]

i understand how words "integer" and "decimal" invite for misunderstanding. what would be a better way to write this?

[szehon-ho]

javadoc for Integer says 'decimal representation', maybe that? https://docs.oracle.com/javase/8/docs/api/java/lang/Integer.html#toString-int-

[szehon-ho]

Maybe something like "non-negative integer estimate of number of distinct values derived from the sketch, stored as a string using decimal representation"

[findepi]

I am not sure, but does this mean an integer value like 2 now becomes 2.0? (if using java toString)

no, this should be "2"

And in any case, as its not entirely backward compat, should we update the theta sketch version again?

this is supposed to be a clarification, not a change.

[szehon-ho]

@findepi Got it sorry i misinterpreted this pr to support double as per #10288 (comment) . This pr makes more sense then.

I think we should still do this for completness, but as theta-sketch-v2?

[szehon-ho]

This pr looks good to me, we can have an discuss long->double enhancements in another pr then.

[findepi]

misinterpreted this pr to support double as per #10288 (comment) .

sorry for the confusion!
in that PR review thread i was trying not to prescribe which way we should go and just outline consequences of using doubles
clarifying the spec to imply long values is just one of the ways and this is what this PR is doing.
clarifying the spec to imply fractional values is some other option. (I am slightly less in favor of it, so i didn't create a PR with it.)

I think we should still do this for completness, but as theta-sketch-v2?

The wording used for apache-datasketches-theta-v1 should have been better and clearly define both: allowed values and their representation. I.e. it was not specified what values are allowed and how they are represented.
This PR should not be understood as prohibiting fractional values. Rather, it should be seen as clarification of what's allowed and what's not, and as such shouldn't need a new version of the sketch.

[szehon-ho]

Yes this pr as is should not require a spec change.

The wording used for apache-datasketches-theta-v1 should have been better and clearly define both: allowed values and their representation. I.e. it was not specified what values are allowed and how they are represented.
This PR should not be understood as prohibiting fractional values. Rather, it should be seen as clarification of what's allowed and what's not, and as such shouldn't need a new version of the sketch.

Sorry I am still confused :( , this pr currently prohibits fractional value in this particular metadata "ndv" doesnt it, with the phrase "integer value"?

[amogh-jahagirdar]

Same question as @szehon-ho , by defining this as an integer we are necessarily saying it cannot be a fractional value.

I actually think that's ok and makes sense (what does it mean to have a fractional value for an NDV), since really this is just a clarification of a field that I don't think we should be worried about since I admittedly find it a difficult to believe if users were writing fractional values for this in the first place.

IMO this is different then the other case where we were proposing making the NDV field required since there it is arguable the structure of the metadata is materially different from the original spec.

[szehon-ho]

Thanks @amogh-jahagirdar. I guess I need to give the context. In #10288 (comment) we realize that in fact ndv as defined by theta-sketch algorithm and java library is a double, and the fact that we have stored it is a long in Trino/Spark PR means some precision is missing.

I am actually more in favor of making it a double to keep consistent with the algorithm, @findepi mention it is not too significant in the long run and favors keeping it a long. But above all, converting now from long to double in trino side is backward incompatible. Hence, was hoping that we can bundle this together with the bump to v2 in #10549 to allow decimal here.

[emkornfield]

Perhaps providing examples of allowed and not allowed values would also help with the clarification

[emkornfield]

Or maybe a grammar?

[findepi]

I am actually more in favor of making it a double to keep consistent with the algorithm

@szehon-ho i am totally fine with that approach too. We would need to define the string representation of the double value.
Would you want to shape a PR with appropriate wording?

[findepi]

It seems I forgot about the PR and the discussion here died. I believe it's better to merge as is than not to merge at all.
@szehon-ho your insights were good, sorry i wasn't able to fully complete them. Will you be able to follow up?

[szehon-ho]

Sure no problem! Yea ive been a bit busy, let me start a thread on the devlist when i get a chance