Skip to content

Commit

Permalink
Fix hashCode values of aggregations' BytesValues.
Browse files Browse the repository at this point in the history
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 elastic#5004
  • Loading branch information
jpountz committed Feb 4, 2014
1 parent ba415b8 commit 73b51ab
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 77 deletions.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -287,31 +287,34 @@ 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;
}
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());
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 73b51ab

Please sign in to comment.