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

Return new lists on calls to getValues. #8591

Closed
wants to merge 2 commits into from

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Nov 21, 2014

Scripts currently share the same list across invocations to getValues. This
caused a bug in script fields where all documents coming from the same segment
would get the same values (basically, for the next document for which script
values have been requested). Scripts now return a fresh new list on every
invocation to getValues.

Close #8576

Scripts currently share the same list across invocations to getValues. This
caused a bug in script fields where all documents coming from the same segment
would get the same values (basically, for the next document for which script
values have been requested). Scripts now return a fresh new list on every
invocation to `getValues`.

Close elastic#8576
@jpountz jpountz added the review label Nov 21, 2014
@s1monw
Copy link
Contributor

s1monw commented Nov 21, 2014

I wonder if we should fix this Interface and return Iterable<T> instead such that folks need to make copies of it if they really need and we can just lazily fetch the values?

@jpountz
Copy link
Contributor Author

jpountz commented Nov 21, 2014

I am not sure it would fix the issue? Someone could still keep references to the Iterable instance?

I think it is the right trade-off to have these interfaces potentially slow but easy to use. It avoids surprises, and if someone needs better performance it is still possible to access directly the fielddata interfaces (via the getInternalValues method).

@s1monw
Copy link
Contributor

s1monw commented Nov 23, 2014

I am not sure it would fix the issue? Someone could still keep references to the Iterable instance?

yes they should becuse each call to getValues would return a new iterable. but it would produce the values each time. The common case here is that stuff iterated once if you need it more often you should copy it?

@jpountz
Copy link
Contributor Author

jpountz commented Nov 24, 2014

@s1monw I updated the patch with your suggestion. So now we have doc['field'] which implements java.util.List and its scope is limited to the current document. And we also have doc['field'].values which returns a copy so that it can survive changes of scope. So for instance doc['field'].values is required in script fields for things to work correctly but it is perfectly fine to use doc['field'] in the context of an aggregation since we don't keep references to the produced list. Is it what you had in mind?

@s1monw
Copy link
Contributor

s1monw commented Nov 25, 2014

awesome - LGTM

@jpountz jpountz removed the review label Nov 25, 2014
@clintongormley clintongormley added v1.5.0 v1.4.1 v2.0.0-beta1 :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >bug labels Nov 25, 2014
@jpountz jpountz closed this Nov 25, 2014
@clintongormley clintongormley changed the title Scripts: Return new lists on calls to getValues. Scripting: Return new lists on calls to getValues. Nov 26, 2014
@clintongormley clintongormley changed the title Scripting: Return new lists on calls to getValues. Return new lists on calls to getValues. Jun 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v1.4.1 v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scripting: ScriptDocValues.getValues() returns an reused list
3 participants