Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change LOWEST_MAX_STANDARD_ERROR for approx_distinct function #9397

Merged
merged 1 commit into from Nov 25, 2017

Conversation

@shixuan-fan
Copy link
Contributor

@shixuan-fan shixuan-fan commented Nov 22, 2017

Maximum number of buckets in HyperLogLog changed from 8192 to 65536,
so now we could support lower max standard errors.

Copy link
Contributor

@martint martint left a comment

Looks good, but change the commit message to say: "Reduce lowest standard error for approx_distinct".

@shixuan-fan
Copy link
Contributor Author

@shixuan-fan shixuan-fan commented Nov 22, 2017

Thanks for the review! I think it should be "lowest max standard error" :)

@shixuan-fan shixuan-fan force-pushed the shixuan-fan:approx branch from ccaa3df to 411e9c4 Nov 22, 2017
@martint
Copy link
Contributor

@martint martint commented Nov 22, 2017

No, the "max" in the names of those constants is wrong. It's not an upper bound on the error. It's a "standard error" (i.e., the standard deviation of the distribution of possible errors). So, this changing the "lowest standard error the function can operate with".

I.e., there's not such a thing as a "max standard error".

Maximum number of buckets in HyperLogLog changed from 8192 to 65536,
so now we could support lower max standard errors.
@shixuan-fan shixuan-fan force-pushed the shixuan-fan:approx branch from 411e9c4 to 2e88e2f Nov 22, 2017
@shixuan-fan shixuan-fan merged commit c2eaae8 into prestodb:master Nov 25, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants