From 9333a3c28a147164ec66680a384d2dd05d9a05b7 Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Tue, 4 Feb 2014 12:14:29 +0100 Subject: [PATCH] Fix hashCode values of aggregations' BytesValues. This commit removes FilterBytesValues which is very trappy as the default implementation forwards all method calls to the delegate. So if you do any non-trivial modification to the terms or to the order of the terms, you need to remember to override currentValueHash, copyShared, and this is very error-prone. FieldDataSource.WithScript.BytesValues and ScriptBytesValues now return correct hash codes, future bugs here would be catched by the new assertion in SortedUniqueBytesValues. This bug was causing performance issues with scripts as all terms were assumed to have the same hash code. Close #5004 --- .../index/fielddata/FilterBytesValues.java | 61 -------- .../aggregations/support/FieldDataSource.java | 26 ++-- .../support/bytes/ScriptBytesValues.java | 1 - .../support/FieldDataSourceTests.java | 141 ++++++++++++++++++ 4 files changed, 152 insertions(+), 77 deletions(-) delete mode 100644 src/main/java/org/elasticsearch/index/fielddata/FilterBytesValues.java create mode 100644 src/test/java/org/elasticsearch/search/aggregations/support/FieldDataSourceTests.java diff --git a/src/main/java/org/elasticsearch/index/fielddata/FilterBytesValues.java b/src/main/java/org/elasticsearch/index/fielddata/FilterBytesValues.java deleted file mode 100644 index a652106cf54be..0000000000000 --- a/src/main/java/org/elasticsearch/index/fielddata/FilterBytesValues.java +++ /dev/null @@ -1,61 +0,0 @@ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch 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.elasticsearch.index.fielddata; - -import org.apache.lucene.util.BytesRef; - -/** - * FilterBytesValues contains another {@link BytesValues}, which it - * uses as its basic source of data, possibly transforming the data along the - * way or providing additional functionality. - */ -public abstract class FilterBytesValues extends BytesValues { - - protected final BytesValues delegate; - - protected FilterBytesValues(BytesValues delegate) { - super(delegate.isMultiValued()); - this.delegate = delegate; - } - - @Override - public BytesRef copyShared() { - return delegate.copyShared(); - } - - @Override - public int setDocument(int docId) { - return delegate.setDocument(docId); - } - - @Override - public BytesRef nextValue() { - return delegate.nextValue(); - } - - @Override - public int currentValueHash() { - return delegate.currentValueHash(); - } - - @Override - public AtomicFieldData.Order getOrder() { - return delegate.getOrder(); - } -} \ No newline at end of file diff --git a/src/main/java/org/elasticsearch/search/aggregations/support/FieldDataSource.java b/src/main/java/org/elasticsearch/search/aggregations/support/FieldDataSource.java index 223e1502284ef..5d1152aefae54 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/support/FieldDataSource.java +++ b/src/main/java/org/elasticsearch/search/aggregations/support/FieldDataSource.java @@ -287,23 +287,23 @@ public org.elasticsearch.index.fielddata.BytesValues bytesValues() { return bytesValues; } - static class SortedUniqueBytesValues extends FilterBytesValues { + static class SortedUniqueBytesValues extends BytesValues { - final BytesRef spare; + final BytesValues delegate; int[] sortedIds; final BytesRefHash bytes; int numUniqueValues; int pos = Integer.MAX_VALUE; public SortedUniqueBytesValues(BytesValues delegate) { - super(delegate); + super(delegate.isMultiValued()); + this.delegate = delegate; bytes = new BytesRefHash(); - spare = new BytesRef(); } @Override public int setDocument(int docId) { - final int numValues = super.setDocument(docId); + final int numValues = delegate.setDocument(docId); if (numValues == 0) { sortedIds = null; return 0; @@ -311,7 +311,10 @@ public int setDocument(int docId) { bytes.clear(); bytes.reinit(); for (int i = 0; i < numValues; ++i) { - bytes.add(super.nextValue(), super.currentValueHash()); + final BytesRef next = delegate.nextValue(); + final int hash = delegate.currentValueHash(); + assert hash == next.hashCode(); + bytes.add(next, hash); } numUniqueValues = bytes.size(); sortedIds = bytes.sort(BytesRef.getUTF8SortedAsUnicodeComparator()); @@ -321,13 +324,8 @@ public int setDocument(int docId) { @Override public BytesRef nextValue() { - bytes.get(sortedIds[pos++], spare); - return spare; - } - - @Override - public int currentValueHash() { - return spare.hashCode(); + bytes.get(sortedIds[pos++], scratch); + return scratch; } @Override @@ -738,13 +736,11 @@ static class BytesValues extends org.elasticsearch.index.fielddata.BytesValues { private final FieldDataSource source; private final SearchScript script; - private final BytesRef scratch; public BytesValues(FieldDataSource source, SearchScript script) { super(true); this.source = source; this.script = script; - scratch = new BytesRef(); } @Override diff --git a/src/main/java/org/elasticsearch/search/aggregations/support/bytes/ScriptBytesValues.java b/src/main/java/org/elasticsearch/search/aggregations/support/bytes/ScriptBytesValues.java index cd612b6102222..b7d46a74a326b 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/support/bytes/ScriptBytesValues.java +++ b/src/main/java/org/elasticsearch/search/aggregations/support/bytes/ScriptBytesValues.java @@ -37,7 +37,6 @@ public class ScriptBytesValues extends BytesValues implements ScriptValues { private Iterator iter; private Object value; - private BytesRef scratch = new BytesRef(); public ScriptBytesValues(SearchScript script) { super(true); // assume multi-valued diff --git a/src/test/java/org/elasticsearch/search/aggregations/support/FieldDataSourceTests.java b/src/test/java/org/elasticsearch/search/aggregations/support/FieldDataSourceTests.java new file mode 100644 index 0000000000000..b9c1954b164dc --- /dev/null +++ b/src/test/java/org/elasticsearch/search/aggregations/support/FieldDataSourceTests.java @@ -0,0 +1,141 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch 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.elasticsearch.search.aggregations.support; + +import org.apache.lucene.index.AtomicReaderContext; +import org.apache.lucene.search.Scorer; +import org.apache.lucene.util.BytesRef; +import org.elasticsearch.index.fielddata.BytesValues; +import org.elasticsearch.script.SearchScript; +import org.elasticsearch.test.ElasticsearchTestCase; +import org.junit.Test; + +import java.util.Map; + +public class FieldDataSourceTests extends ElasticsearchTestCase { + + private static BytesValues randomBytesValues() { + final boolean multiValued = randomBoolean(); + return new BytesValues(multiValued) { + @Override + public int setDocument(int docId) { + return randomInt(multiValued ? 10 : 1); + } + @Override + public BytesRef nextValue() { + scratch.copyChars(randomAsciiOfLength(10)); + return scratch; + } + + }; + } + + private static SearchScript randomScript() { + return new SearchScript() { + + @Override + public void setNextVar(String name, Object value) { + } + + @Override + public Object run() { + return randomAsciiOfLength(5); + } + + @Override + public Object unwrap(Object value) { + return value; + } + + @Override + public void setNextReader(AtomicReaderContext reader) { + } + + @Override + public void setScorer(Scorer scorer) { + } + + @Override + public void setNextDocId(int doc) { + } + + @Override + public void setNextSource(Map source) { + } + + @Override + public void setNextScore(float score) { + } + + @Override + public float runAsFloat() { + throw new UnsupportedOperationException(); + } + + @Override + public long runAsLong() { + throw new UnsupportedOperationException(); + } + + @Override + public double runAsDouble() { + throw new UnsupportedOperationException(); + } + + }; + } + + private static void assertConsistent(BytesValues values) { + for (int i = 0; i < 10; ++i) { + final int valueCount = values.setDocument(i); + for (int j = 0; j < valueCount; ++j) { + final BytesRef term = values.nextValue(); + assertEquals(term.hashCode(), values.currentValueHash()); + assertTrue(term.bytesEquals(values.copyShared())); + } + } + } + + @Test + public void bytesValuesWithScript() { + final BytesValues values = randomBytesValues(); + FieldDataSource source = new FieldDataSource.Bytes() { + + @Override + public BytesValues bytesValues() { + return values; + } + + @Override + public MetaData metaData() { + throw new UnsupportedOperationException(); + } + + }; + SearchScript script = randomScript(); + assertConsistent(new FieldDataSource.WithScript.BytesValues(source, script)); + } + + @Test + public void sortedUniqueBytesValues() { + assertConsistent(new FieldDataSource.Bytes.SortedAndUnique.SortedUniqueBytesValues(randomBytesValues())); + } + +}