[BEAM-409] Fixing incorrect use of Math.ceil in ApproximateQuantiles#3861
[BEAM-409] Fixing incorrect use of Math.ceil in ApproximateQuantiles#3861youngoli wants to merge 3 commits intoapache:masterfrom
Conversation
Casting the denominator in this division in the ceil call to a float to avoid an incorrect integer division causing a bug.
|
Thanks for this contribution. Please also include a unit test to verify. The unmerged PR #853 had a couple new tests, |
Adding unit tests written by swegner to confirm that the bug was fixed.
|
Thanks swegner. I added in the unit tests you made for PR #853 and added the |
|
retest this please |
1 similar comment
|
retest this please |
|
Changes Unknown when pulling 1875004 on youngoli:bugfix-beam409 into ** on apache:master**. |
|
LGTM. Adding @tgroh as Beam committer to merge. |
| long n = this.maxInputSize; | ||
|
|
||
| assertTrue("(b-2)2^(b-2) + 1/2 <= eN", (b - 2) * (1 << (b - 2)) + 0.5 <= this.epsilon * n); | ||
| assertTrue("k2^(b-1) >= N", Math.pow(k * 2, b - 1) >= n); |
There was a problem hiding this comment.
assertThat("what", actual, Matchers.lessThanOrEqualTo(expected)) and the same with the greater than or equal to.
Changing calls to assertTrue to assertThat.
|
Changes Unknown when pulling 8362ca3 on youngoli:bugfix-beam409 into ** on apache:master**. |
|
retest this please |
|
Changes Unknown when pulling 8362ca3 on youngoli:bugfix-beam409 into ** on apache:master**. |
|
retest this please |
1 similar comment
|
retest this please |
|
Changes Unknown when pulling 8362ca3 on youngoli:bugfix-beam409 into ** on apache:master**. |
4 participants
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.Fixed the FindBug issue by avoiding an incorrect integer division. I based this on an old unmerged pull request for this bug: #853
That PR also included a new unit test for ApproximateQuantiles.java, wasn't sure if I should also include that unit test.