[BEAM-6693] replace mmh3 with default hash function #8799
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hannah-Jiang
changed the title
|
R: @pabloem |
pabloem
reviewed
Jun 10, 2019
| label='assert:global_by_error_with_samll_population') | ||
| pipeline.run() | ||
|
|
||
| def test_approximate_unique_global_by_error_with_big_population(self): |
There was a problem hiding this comment.
Why are you removing this test?
There was a problem hiding this comment.
This test is more for estimation algorithm performance testing, rather than for testing functionality. With py3 default hash algorithm, it would fail sometimes (2%) because it is out of estimation error range, so I decided to remove it.
|
Thanks Hannah. This LGTM. I just left one question. Thanks! |
aaltay
reviewed
Jun 10, 2019
| @@ -214,7 +212,7 @@ def create_accumulator(self, *args, **kwargs): | |||
|
|
|||
There was a problem hiding this comment.
Suggestion (Maybe add a JIRA (does not have to be fixed in this PR) for future):
- Pass a hash_fn argument to ApproximateUniqueCombineFn, that can be passed by users to customize the hash_fn they would like to use. It can default to
hash
There was a problem hiding this comment.
Ticket is created: https://issues.apache.org/jira/browse/BEAM-7525
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace mmh3 with default python hash function, because it's not reasonable to bring this inconvenience to production with only a little bit improvement with estimation for a loose estimation algorithm.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.