Skip to content
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

ScriptBytesValues.currentValueHash is wrong #5004

Closed
jpountz opened this issue Feb 4, 2014 · 2 comments
Closed

ScriptBytesValues.currentValueHash is wrong #5004

jpountz opened this issue Feb 4, 2014 · 2 comments

Comments

@jpountz
Copy link
Contributor

jpountz commented Feb 4, 2014

ScriptBytesValues.currentValueHash doesn't return the hash value of the last returned term. The reason is that it has its own BytesRef (ScriptDocValues.scratch) to store the term while the hash code is computed on the parent's term (BytesValues.scratch).

@jpountz jpountz self-assigned this Feb 4, 2014
@s1monw
Copy link
Contributor

s1monw commented Feb 4, 2014

Damned! good catch

@jpountz
Copy link
Contributor Author

jpountz commented Feb 4, 2014

FieldDataSource.WithScript.BytesValues has the same issue.

jpountz added a commit to jpountz/elasticsearch that referenced this issue Feb 4, 2014
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
@jpountz jpountz closed this as completed in 9333a3c Feb 4, 2014
jpountz added a commit that referenced this issue Feb 4, 2014
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
jpountz added a commit that referenced this issue Feb 4, 2014
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
mute pushed a commit to mute/elasticsearch that referenced this issue Jul 29, 2015
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants