-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix tie-break bug in various Facets implementations #11768
Conversation
a7452c2
to
eb21096
Compare
Thanks @gsmiller for discovering this issue! The changes look good to me. I am curious if the |
@Yuti-G could you help me understand what faceting implementation or part of the code you're referring to? Thanks! |
Sure, I just updated the previous comment with links. Thanks! |
@Yuti-G thanks for the links. In this case, the contract is that we break ties by the value (of the long) itself (low-to-high), which the PQ is already doing. So this appears to be correct to me, but let me know if I'm overlooking something. Also, it's not possible to have identical values between two results since the counting structures guarantee unique indexes/keys right? |
I see.. Thanks for the explanation of indexes! |
@@ -189,10 +190,11 @@ private TopChildrenForPath getTopChildrenForPath(DimConfig dimConfig, int pathOr | |||
|
|||
TopOrdAndFloatQueue.OrdAndValue reuse = null; | |||
while (ord != TaxonomyReader.INVALID_ORDINAL) { | |||
if (values[ord] > 0) { | |||
float value = values[ord]; | |||
if (value > 0) { | |||
aggregatedValue = aggregationFunction.aggregate(aggregatedValue, values[ord]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might as well use value
here too (and check if we you can replace values[ord]
with value
elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch. I intended to do this everyone but looks like I missed a spot. I'll go back through and re-check.
@@ -626,7 +626,7 @@ public void testBasicWithCollectorManager() throws Exception { | |||
List<FacetResult> topNDimsResult = r.facets.getTopDims(1, 2); | |||
assertEquals(1, topNDimsResult.size()); | |||
assertEquals( | |||
"dim=Author path=[] value=5 childCount=4\n Lisa (2)\n Susan (1)\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's disturbing that these tests were "wrong" and we just let them be like that. I'm glad that you fixed them, but makes me wonder if it was possible to catch this bug earlier by scrutinizing these tests better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, hindsight is 20/20 I suppose. We'll eventually converge on something that's correct :)
eb21096
to
5baa18b
Compare
Description
There are a number of places in
Facets
implementations wheregetTopChildren
is incorrectly handling count/value ties. The behavior should prefer smaller ordinals when counts/values are equal, but it's not always doing that. Some tests were also incorrect and needed to be updated.