LUCENE-5790: Fix compareTo in MutableValueDouble and MutableValueBool, this caused incorrect results when grouping on fields with missing values

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1608639 13f79535-47bb-0310-9956-ffa450edef68
This commit is contained in:
Chris M. Hostetter 2014-07-08 01:23:11 +00:00
parent e6ce493374
commit 474d846bb5
10 changed files with 537 additions and 20 deletions

View File

@ -138,6 +138,10 @@ Bug Fixes
* LUCENE-5796: Fixes the Scorer.getChildren() method for two combinations
of BooleanQuery. (Terry Smith via Robert Muir)
* LUCENE-5790: Fix compareTo in MutableValueDouble and MutableValueBool, this caused
incorrect results when grouping on fields with missing values.
(海老澤 志信, hossman)
Test Framework
* LUCENE-5786: Unflushed/ truncated events file (hung testing subprocess).

View File

@ -17,14 +17,17 @@
package org.apache.lucene.util.mutable;
/**
* {@link MutableValue} implementation of type
* <code>boolean</code>.
* {@link MutableValue} implementation of type <code>boolean</code>.
* When mutating instances of this object, the caller is responsible for ensuring
* that any instance where <code>exists</code> is set to <code>false</code> must also
* <code>value</code> set to <code>false</code> for proper operation.
*/
public class MutableValueBool extends MutableValue {
public boolean value;
@Override
public Object toObject() {
assert exists || (false == value);
return exists ? value : null;
}
@ -45,20 +48,23 @@ public class MutableValueBool extends MutableValue {
@Override
public boolean equalsSameType(Object other) {
assert exists || (false == value);
MutableValueBool b = (MutableValueBool)other;
return value == b.value && exists == b.exists;
}
@Override
public int compareSameType(Object other) {
assert exists || (false == value);
MutableValueBool b = (MutableValueBool)other;
if (value != b.value) return value ? 1 : 0;
if (value != b.value) return value ? 1 : -1;
if (exists == b.exists) return 0;
return exists ? 1 : -1;
}
@Override
public int hashCode() {
assert exists || (false == value);
return value ? 2 : (exists ? 1 : 0);
}
}

View File

@ -19,8 +19,8 @@ package org.apache.lucene.util.mutable;
import java.util.Date;
/**
* {@link MutableValue} implementation of type
* {@link Date}.
* {@link MutableValue} implementation of type {@link Date}.
* @see MutableValueLong
*/
public class MutableValueDate extends MutableValueLong {
@Override
@ -35,4 +35,4 @@ public class MutableValueDate extends MutableValueLong {
v.exists = this.exists;
return v;
}
}
}

View File

@ -17,14 +17,17 @@
package org.apache.lucene.util.mutable;
/**
* {@link MutableValue} implementation of type
* <code>double</code>.
* {@link MutableValue} implementation of type <code>double</code>.
* When mutating instances of this object, the caller is responsible for ensuring
* that any instance where <code>exists</code> is set to <code>false</code> must also
* <code>value</code> set to <code>0.0D</code> for proper operation.
*/
public class MutableValueDouble extends MutableValue {
public double value;
public double value = 0.0D;
@Override
public Object toObject() {
assert exists || 0.0D == value;
return exists ? value : null;
}
@ -45,22 +48,24 @@ public class MutableValueDouble extends MutableValue {
@Override
public boolean equalsSameType(Object other) {
assert exists || 0.0D == value;
MutableValueDouble b = (MutableValueDouble)other;
return value == b.value && exists == b.exists;
}
@Override
public int compareSameType(Object other) {
assert exists || 0.0D == value;
MutableValueDouble b = (MutableValueDouble)other;
int c = Double.compare(value, b.value);
if (c != 0) return c;
if (!exists) return -1;
if (!b.exists) return 1;
return 0;
if (exists == b.exists) return 0;
return exists ? 1 : -1;
}
@Override
public int hashCode() {
assert exists || 0.0D == value;
long x = Double.doubleToLongBits(value);
return (int)x + (int)(x>>>32);
}

View File

@ -17,14 +17,17 @@
package org.apache.lucene.util.mutable;
/**
* {@link MutableValue} implementation of type
* <code>float</code>.
* {@link MutableValue} implementation of type <code>float</code>.
* When mutating instances of this object, the caller is responsible for ensuring
* that any instance where <code>exists</code> is set to <code>false</code> must also
* <code>value</code> set to <code>0.0F</code> for proper operation.
*/
public class MutableValueFloat extends MutableValue {
public float value;
@Override
public Object toObject() {
assert exists || 0.0F == value;
return exists ? value : null;
}
@ -45,12 +48,14 @@ public class MutableValueFloat extends MutableValue {
@Override
public boolean equalsSameType(Object other) {
assert exists || 0.0F == value;
MutableValueFloat b = (MutableValueFloat)other;
return value == b.value && exists == b.exists;
}
@Override
public int compareSameType(Object other) {
assert exists || 0.0F == value;
MutableValueFloat b = (MutableValueFloat)other;
int c = Float.compare(value, b.value);
if (c != 0) return c;
@ -60,6 +65,7 @@ public class MutableValueFloat extends MutableValue {
@Override
public int hashCode() {
assert exists || 0.0F == value;
return Float.floatToIntBits(value);
}
}

View File

@ -17,14 +17,17 @@
package org.apache.lucene.util.mutable;
/**
* {@link MutableValue} implementation of type
* <code>int</code>.
* {@link MutableValue} implementation of type <code>int</code>.
* When mutating instances of this object, the caller is responsible for ensuring
* that any instance where <code>exists</code> is set to <code>false</code> must also
* <code>value</code> set to <code>0</code> for proper operation.
*/
public class MutableValueInt extends MutableValue {
public int value;
@Override
public Object toObject() {
assert exists || 0 == value;
return exists ? value : null;
}
@ -45,12 +48,14 @@ public class MutableValueInt extends MutableValue {
@Override
public boolean equalsSameType(Object other) {
assert exists || 0 == value;
MutableValueInt b = (MutableValueInt)other;
return value == b.value && exists == b.exists;
}
@Override
public int compareSameType(Object other) {
assert exists || 0 == value;
MutableValueInt b = (MutableValueInt)other;
int ai = value;
int bi = b.value;
@ -64,6 +69,7 @@ public class MutableValueInt extends MutableValue {
@Override
public int hashCode() {
assert exists || 0 == value;
// TODO: if used in HashMap, it already mixes the value... maybe use a straight value?
return (value>>8) + (value>>16);
}

View File

@ -17,14 +17,17 @@
package org.apache.lucene.util.mutable;
/**
* {@link MutableValue} implementation of type
* <code>long</code>.
* {@link MutableValue} implementation of type <code>long</code>.
* When mutating instances of this object, the caller is responsible for ensuring
* that any instance where <code>exists</code> is set to <code>false</code> must also
* <code>value</code> set to <code>0L</code> for proper operation.
*/
public class MutableValueLong extends MutableValue {
public long value;
@Override
public Object toObject() {
assert exists || 0L == value;
return exists ? value : null;
}
@ -45,12 +48,14 @@ public class MutableValueLong extends MutableValue {
@Override
public boolean equalsSameType(Object other) {
assert exists || 0L == value;
MutableValueLong b = (MutableValueLong)other;
return value == b.value && exists == b.exists;
}
@Override
public int compareSameType(Object other) {
assert exists || 0L == value;
MutableValueLong b = (MutableValueLong)other;
long bv = b.value;
if (value<bv) return -1;
@ -62,6 +67,7 @@ public class MutableValueLong extends MutableValue {
@Override
public int hashCode() {
assert exists || 0L == value;
return (int)value + (int)(value>>32);
}
}

View File

@ -19,14 +19,17 @@ package org.apache.lucene.util.mutable;
import org.apache.lucene.util.BytesRef;
/**
* {@link MutableValue} implementation of type
* {@link String}.
* {@link MutableValue} implementation of type {@link String}.
* When mutating instances of this object, the caller is responsible for ensuring
* that any instance where <code>exists</code> is set to <code>false</code> must also
* have a <code>value</code> with a length set to 0.
*/
public class MutableValueStr extends MutableValue {
public BytesRef value = new BytesRef();
@Override
public Object toObject() {
assert exists || 0 == value.length;
return exists ? value.utf8ToString() : null;
}
@ -47,12 +50,14 @@ public class MutableValueStr extends MutableValue {
@Override
public boolean equalsSameType(Object other) {
assert exists || 0 == value.length;
MutableValueStr b = (MutableValueStr)other;
return value.equals(b.value) && exists == b.exists;
}
@Override
public int compareSameType(Object other) {
assert exists || 0 == value.length;
MutableValueStr b = (MutableValueStr)other;
int c = value.compareTo(b.value);
if (c != 0) return c;
@ -63,6 +68,7 @@ public class MutableValueStr extends MutableValue {
@Override
public int hashCode() {
assert exists || 0 == value.length;
return value.hashCode();
}
}

View File

@ -0,0 +1,296 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.lucene.util.mutable;
import org.apache.lucene.util.LuceneTestCase;
import org.apache.lucene.util.BytesRef;
/**
* Simple test of the basic contract of the various {@link MutableValue} implementaitons.
*/
public class TestMutableValues extends LuceneTestCase {
public void testStr() {
MutableValueStr xxx = new MutableValueStr();
assert xxx.value.equals(new BytesRef()) : "defaults have changed, test utility may not longer be as high";
assert xxx.exists : "defaults have changed, test utility may not longer be as high";
assertSanity(xxx);
MutableValueStr yyy = new MutableValueStr();
assertSanity(yyy);
assertEquality(xxx, yyy);
xxx.exists = false;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.exists = false;
assertEquality(xxx, yyy);
xxx.value.length = 0;
xxx.value.copyChars("zzz");
xxx.exists = true;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.value.length = 0;
yyy.value.copyChars("aaa");
yyy.exists = true;
assertSanity(yyy);
assertInEquality(xxx,yyy);
assertTrue(0 < xxx.compareTo(yyy));
assertTrue(yyy.compareTo(xxx) < 0);
xxx.copy(yyy);
assertSanity(xxx);
assertEquality(xxx, yyy);
// special BytesRef considerations...
xxx.exists = false;
xxx.value.length = 0; // but leave bytes alone
assertInEquality(xxx,yyy);
yyy.exists = false;
yyy.value.length = 0; // but leave bytes alone
assertEquality(xxx, yyy);
}
public void testDouble() {
MutableValueDouble xxx = new MutableValueDouble();
assert xxx.value == 0.0D : "defaults have changed, test utility may not longer be as high";
assert xxx.exists : "defaults have changed, test utility may not longer be as high";
assertSanity(xxx);
MutableValueDouble yyy = new MutableValueDouble();
assertSanity(yyy);
assertEquality(xxx, yyy);
xxx.exists = false;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.exists = false;
assertEquality(xxx, yyy);
xxx.value = 42.0D;
xxx.exists = true;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.value = -99.0D;
yyy.exists = true;
assertSanity(yyy);
assertInEquality(xxx,yyy);
assertTrue(0 < xxx.compareTo(yyy));
assertTrue(yyy.compareTo(xxx) < 0);
xxx.copy(yyy);
assertSanity(xxx);
assertEquality(xxx, yyy);
}
public void testInt() {
MutableValueInt xxx = new MutableValueInt();
assert xxx.value == 0 : "defaults have changed, test utility may not longer be as high";
assert xxx.exists : "defaults have changed, test utility may not longer be as high";
assertSanity(xxx);
MutableValueInt yyy = new MutableValueInt();
assertSanity(yyy);
assertEquality(xxx, yyy);
xxx.exists = false;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.exists = false;
assertEquality(xxx, yyy);
xxx.value = 42;
xxx.exists = true;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.value = -99;
yyy.exists = true;
assertSanity(yyy);
assertInEquality(xxx,yyy);
assertTrue(0 < xxx.compareTo(yyy));
assertTrue(yyy.compareTo(xxx) < 0);
xxx.copy(yyy);
assertSanity(xxx);
assertEquality(xxx, yyy);
}
public void testFloat() {
MutableValueFloat xxx = new MutableValueFloat();
assert xxx.value == 0.0F : "defaults have changed, test utility may not longer be as high";
assert xxx.exists : "defaults have changed, test utility may not longer be as high";
assertSanity(xxx);
MutableValueFloat yyy = new MutableValueFloat();
assertSanity(yyy);
assertEquality(xxx, yyy);
xxx.exists = false;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.exists = false;
assertEquality(xxx, yyy);
xxx.value = 42.0F;
xxx.exists = true;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.value = -99.0F;
yyy.exists = true;
assertSanity(yyy);
assertInEquality(xxx,yyy);
assertTrue(0 < xxx.compareTo(yyy));
assertTrue(yyy.compareTo(xxx) < 0);
xxx.copy(yyy);
assertSanity(xxx);
assertEquality(xxx, yyy);
}
public void testLong() {
MutableValueLong xxx = new MutableValueLong();
assert xxx.value == 0L : "defaults have changed, test utility may not longer be as high";
assert xxx.exists : "defaults have changed, test utility may not longer be as high";
assertSanity(xxx);
MutableValueLong yyy = new MutableValueLong();
assertSanity(yyy);
assertEquality(xxx, yyy);
xxx.exists = false;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.exists = false;
assertEquality(xxx, yyy);
xxx.value = 42L;
xxx.exists = true;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.value = -99L;
yyy.exists = true;
assertSanity(yyy);
assertInEquality(xxx,yyy);
assertTrue(0 < xxx.compareTo(yyy));
assertTrue(yyy.compareTo(xxx) < 0);
xxx.copy(yyy);
assertSanity(xxx);
assertEquality(xxx, yyy);
}
public void testBool() {
MutableValueBool xxx = new MutableValueBool();
assert xxx.value == false : "defaults have changed, test utility may not longer be as high";
assert xxx.exists : "defaults have changed, test utility may not longer be as high";
assertSanity(xxx);
MutableValueBool yyy = new MutableValueBool();
assertSanity(yyy);
assertEquality(xxx, yyy);
xxx.exists = false;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.exists = false;
assertEquality(xxx, yyy);
xxx.value = true;
xxx.exists = true;
assertSanity(xxx);
assertInEquality(xxx,yyy);
yyy.value = false;
yyy.exists = true;
assertSanity(yyy);
assertInEquality(xxx,yyy);
assertTrue(0 < xxx.compareTo(yyy));
assertTrue(yyy.compareTo(xxx) < 0);
xxx.copy(yyy);
assertSanity(xxx);
assertEquality(xxx, yyy);
}
private void assertSanity(MutableValue x) {
assertEquality(x, x);
MutableValue y = x.duplicate();
assertEquality(x, y);
}
private void assertEquality(MutableValue x, MutableValue y) {
assertEquals(x.hashCode(), y.hashCode());
assertEquals(x, y);
assertEquals(y, x);
assertTrue(x.equalsSameType(y));
assertTrue(y.equalsSameType(x));
assertEquals(0, x.compareTo(y));
assertEquals(0, y.compareTo(x));
assertEquals(0, x.compareSameType(y));
assertEquals(0, y.compareSameType(x));
}
private void assertInEquality(MutableValue x, MutableValue y) {
assertFalse(x.equals(y));
assertFalse(y.equals(x));
assertFalse(x.equalsSameType(y));
assertFalse(y.equalsSameType(x));
assertFalse(0 == x.compareTo(y));
assertFalse(0 == y.compareTo(x));
}
}

View File

@ -0,0 +1,182 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.solr.search;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.common.SolrInputDocument;
import org.apache.lucene.util.TestUtil;
import org.junit.BeforeClass;
import org.junit.After;
import java.util.Set;
import java.util.HashSet;
import java.util.List;
import java.util.ArrayList;
import java.util.Map;
import java.util.HashMap;
/** Inspired by LUCENE-5790 */
public class TestMissingGroups extends SolrTestCaseJ4 {
@BeforeClass
public static void beforeTests() throws Exception {
initCore("solrconfig.xml", "schema15.xml");
}
@After
public void cleanup() throws Exception {
clearIndex();
assertU(optimize());
}
public void testGroupsOnMissingValues() throws Exception {
final int numDocs = atLeast(500);
// setup some key values for some random docs in our index
// every other doc will have no values for these fields
// NOTE: special values may be randomly assigned to the *same* docs
final List<SpecialField> specials = new ArrayList<SpecialField>(7);
specials.add(new SpecialField(numDocs, "group_s1", "xxx","yyy"));
specials.add(new SpecialField(numDocs, "group_ti", "42","24"));
specials.add(new SpecialField(numDocs, "group_td", "34.56","12.78"));
specials.add(new SpecialField(numDocs, "group_tl", "66666666","999999999"));
specials.add(new SpecialField(numDocs, "group_tf", "56.78","78.45"));
specials.add(new SpecialField(numDocs, "group_b", "true", "false"));
specials.add(new SpecialField(numDocs, "group_tdt",
"2009-05-10T03:30:00Z","1976-03-06T15:06:00Z"));
// build up our index of docs
for (int i = 1; i < numDocs; i++) { // NOTE: start at 1, doc#0 is below...
SolrInputDocument d = sdoc("id", i);
if (SpecialField.special_docids.contains(i)) {
d.addField("special_s","special");
for (SpecialField f : specials) {
if (f.docX == i) {
d.addField(f.field, f.valueX);
} else if (f.docY == i) {
d.addField(f.field, f.valueY);
}
}
} else {
// doc isn't special, give it a random chances of being excluded from some queries
d.addField("filter_b", random().nextBoolean());
}
assertU(adoc(d));
if (rarely()) {
assertU(commit()); // mess with the segment counts
}
}
// doc#0: at least one doc that is garunteed not special and has no chance of being filtered
assertU(adoc(sdoc("id","0")));
assertU(commit());
// sanity check
assertQ(req("q", "*:*"), "//result[@numFound="+numDocs+"]");
for (SpecialField special : specials) {
// sanity checks
assertQ(req("q", "{!term f=" + special.field + "}" + special.valueX),
"//result[@numFound=1]");
assertQ(req("q", "{!term f=" + special.field + "}" + special.valueY),
"//result[@numFound=1]");
// group on special field, and confirm all docs w/o group field get put into a single group
final String xpre = "//lst[@name='grouped']/lst[@name='"+special.field+"']";
assertQ(req("q", (random().nextBoolean() ? "*:*" : "special_s:special id:[0 TO 400]"),
"fq", (random().nextBoolean() ? "*:*" : "-filter_b:"+random().nextBoolean()),
"group","true",
"group.field",special.field,
"group.ngroups", "true")
// basic grouping checks
, xpre + "/int[@name='ngroups'][.='3']"
, xpre + "/arr[@name='groups'][count(lst)=3]"
// sanity check one group is the missing values
, xpre + "/arr[@name='groups']/lst/null[@name='groupValue']"
// check we have the correct groups for the special values with a single doc
, xpre + "/arr[@name='groups']/lst/*[@name='groupValue'][.='"+special.valueX+"']/following-sibling::result[@name='doclist'][@numFound=1]/doc/str[@name='id'][.="+special.docX+"]"
, xpre + "/arr[@name='groups']/lst/*[@name='groupValue'][.='"+special.valueY+"']/following-sibling::result[@name='doclist'][@numFound=1]/doc/str[@name='id'][.="+special.docY+"]"
);
// now do the same check, but exclude one special doc to force only 2 groups
final int doc = random().nextBoolean() ? special.docX : special.docY;
final Object val = (doc == special.docX) ? special.valueX : special.valueY;
assertQ(req("q", (random().nextBoolean() ? "*:*" : "special_s:special id:[0 TO 400]"),
"fq", (random().nextBoolean() ? "*:*" : "-filter_b:"+random().nextBoolean()),
"fq", "-id:" + ((doc == special.docX) ? special.docY : special.docX),
"group","true",
"group.field",special.field,
"group.ngroups", "true")
// basic grouping checks
, xpre + "/int[@name='ngroups'][.='2']"
, xpre + "/arr[@name='groups'][count(lst)=2]"
// sanity check one group is the missing values
, xpre + "/arr[@name='groups']/lst/null[@name='groupValue']"
// check we have the correct group for the special value with a single doc
, xpre + "/arr[@name='groups']/lst/*[@name='groupValue'][.='"+val+"']/following-sibling::result[@name='doclist'][@numFound=1]/doc/str[@name='id'][.="+doc+"]"
);
// one last check, exclude both docs and verify the only group is the missing value group
assertQ(req("q", (random().nextBoolean() ? "*:*" : "special_s:special id:[0 TO 400]"),
"fq", (random().nextBoolean() ? "*:*" : "-filter_b:"+random().nextBoolean()),
"fq", "-id:" + special.docX,
"fq", "-id:" + special.docY,
"group","true",
"group.field",special.field,
"group.ngroups", "true")
// basic grouping checks
, xpre + "/int[@name='ngroups'][.='1']"
, xpre + "/arr[@name='groups'][count(lst)=1]"
// the only group should be the missing values
, xpre + "/arr[@name='groups']/lst/null[@name='groupValue']"
);
}
}
private static final class SpecialField {
// fast lookup of which docs are special
public static final Set<Integer> special_docids = new HashSet<>();
public final String field;
public final int docX;
public final Object valueX;
public final int docY;
public final Object valueY;
public SpecialField(int numDocs, String field, Object valueX, Object valueY) {
this.field = field;
this.valueX = valueX;
this.valueY = valueY;
this.docX = TestUtil.nextInt(random(),1,numDocs-1);
this.docY = (docX < (numDocs / 2))
? TestUtil.nextInt(random(),docX+1,numDocs-1)
: TestUtil.nextInt(random(),1,docX-1);
special_docids.add(docX);
special_docids.add(docY);
}
}
}