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.

Sign up for GitHub

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

Jump to bottom

Remove ScriptDocValues#getValues #22919

Closed
nik9000 opened this issue Feb 2, 2017 · 12 comments
Closed

Remove ScriptDocValues#getValues #22919

nik9000 opened this issue Feb 2, 2017 · 12 comments
Labels
>breaking :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache good first issue low hanging fruit help wanted adoptme

Comments

@nik9000
Copy link
Member

nik9000 commented Feb 2, 2017

I expect ScriptDocValues#getValues was added for backwards compatibility a long time ago but at this point I don't think we need it. At this point everything every script that does doc.foo.values.something can do doc.foo.something instead.

Should we drop ScriptDocValues#getValues in 6.0?
@nik9000 nik9000 added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >breaking discuss labels Feb 2, 2017
@nik9000 nik9000 mentioned this issue Feb 2, 2017
Merged
@clintongormley
Copy link

+1 (with appropriate deprecation logging)

@nik9000 nik9000 removed the discuss label Feb 3, 2017
@nik9000
Copy link
Member Author

nik9000 commented Feb 3, 2017

Talked among a small group - we're fine with removing. I'll put together a change to deprecate in 5.x and remove in 6.0.

@jdconrad
Copy link
Contributor

@nik9000 Was this ever completed?

@nik9000
Copy link
Member Author

nik9000 commented Mar 13, 2018

No, but it feels like a legit thing we should do. Like, we should deprecate it in 6.x and remove in master.

@nik9000 nik9000 added good first issue low hanging fruit help wanted adoptme labels Sep 23, 2018
@nik9000 nik9000 removed their assignment Sep 23, 2018
@j-haj
Copy link
Contributor

j-haj commented Oct 1, 2018

I am interested in taking a look at this. @nik9000 can you give some direction? It seems like the plan was to deprecate and remove some time in 6.x. However, we are already at 6.4 and it doesn't seem like the deprecation warnings were ever added. At this point, are we only interested in adding deprecation info?

@nik9000 nik9000 changed the title Proposal: Remove ScriptDocValues#getValues Remove ScriptDocValues#getValues Oct 1, 2018
@nik9000
Copy link
Member Author

nik9000 commented Oct 1, 2018

Right @j-haj! We've traditionally done this sort of thing in a two phase process:

  1. Merge a PR that adds a deprecation warning to getValues to both 6.x and master.
  2. Merge a PR that drops the getValues entirely just to master.

We do this because it keeps 6.x as "mostly backports of stuff in master" and it allows us to always develop against master. So if you'd like to take this then step 1 is deprecating getValues.

One important thing: on DeprecationLogger there is a deprecated and a deprecatedAndMaybeLog method. Use deprecatedAndMaybeLog because it'll be much faster. It is quite common for search scripts to run millions of times per request and the savings really adds up.

@j-haj
Copy link
Contributor

j-haj commented Oct 1, 2018

Perfect - I will get working on this and comment if I have any questions.

@j-haj
Copy link
Contributor

j-haj commented Oct 3, 2018

@nik9000 I have added a call to deprecatedAndMaybeLog to ScriptDocValues#getValues. The message I am logging says "Deprecated getValues used" - I was not sure what else should be said. Summarizing the comment at the top of this thread seemed excessive. Is there anything else that should be communicated to the user?

Also, ScriptDocValues#getValue should be left as is, right?

@nik9000
Copy link
Member Author

nik9000 commented Oct 3, 2018

Also, ScriptDocValues#getValue should be left as is, right?

That depends on who you ask! Some of us aren't big fans of it to be honest. ScriptDocValues is a list and not treating it like one is kind of a problem. But one thing at a time.

I am logging says "Deprecated getValues used" - I was not sure what else should be said. Summarizing the comment at the top of this thread seemed excessive. Is there anything else that should be communicated to the user?

I think it'd be good to say something about how getValues isn't needed and you should just leave it out but I don't really know how to say it properly. getValues doesn't do anything doesn't quite convey that you can just drop it out of the calls.

@j-haj
Copy link
Contributor

j-haj commented Oct 3, 2018

Perfect - I can work with that and will have a PR out later this morning.

j-haj added a commit to j-haj/elasticsearch that referenced this issue Oct 3, 2018
@j-haj
Adds deprecation logging to ScriptDocValues#getValues.
b186bb7 
First commit addressing issue elastic#22919.

`ScriptDocValues#getValues` was added for backwards compatibility but no
longer needed. Scripts using the syntax `doc.foo.values.bar` should be
using `doc.foo.bar` instead.
@j-haj j-haj mentioned this issue Oct 3, 2018
Merged
j-haj added a commit to j-haj/elasticsearch that referenced this issue Oct 4, 2018
@j-haj
Adds deprecation logging to ScriptDocValues#getValues.
b8f383a 
First commit addressing issue elastic#22919.

`ScriptDocValues#getValues` was added for backwards compatibility but no
longer needed. Scripts using the syntax `doc.foo.values.bar` should be
using `doc.foo.bar` instead.
j-haj added a commit to j-haj/elasticsearch that referenced this issue Oct 5, 2018
@j-haj
Adds deprecation logging to ScriptDocValues#getValues.
570c745 
First commit addressing issue elastic#22919.

`ScriptDocValues#getValues` was added for backwards compatibility but no
longer needed. Scripts using the syntax `doc.foo.values.bar` should be
using `doc.foo.bar` instead.
j-haj added a commit to j-haj/elasticsearch that referenced this issue Oct 25, 2018
@j-haj
Adds deprecation logging to ScriptDocValues#getValues.
970c3e1 
First commit addressing issue elastic#22919.

`ScriptDocValues#getValues` was added for backwards compatibility but no
longer needed. Scripts using the syntax `doc.foo.values.bar` should be
using `doc.foo.bar` instead.
j-haj added a commit to j-haj/elasticsearch that referenced this issue Oct 25, 2018
@j-haj
Adds deprecation logging to ScriptDocValues#getValues.
d643b9e 
First commit addressing issue elastic#22919.

`ScriptDocValues#getValues` was added for backwards compatibility but no
longer needed. Scripts using the syntax `doc.foo.values.bar` should be
using `doc.foo.bar` instead.
j-haj added a commit to j-haj/elasticsearch that referenced this issue Nov 1, 2018
@j-haj
Adds deprecation logging to ScriptDocValues#getValues.
51a6c6d 
First commit addressing issue elastic#22919.

`ScriptDocValues#getValues` was added for backwards compatibility but no
longer needed. Scripts using the syntax `doc['foo'].values` when
`doc['foo']` is a list should be using `doc['foo']` instead.
j-haj added a commit to j-haj/elasticsearch that referenced this issue Nov 1, 2018
@j-haj
Adds deprecation logging to ScriptDocValues#getValues.
c5704a4 
First commit addressing issue elastic#22919.

`ScriptDocValues#getValues` was added for backwards compatibility but no
longer needed. Scripts using the syntax `doc['foo'].values` when
`doc['foo']` is a list should be using `doc['foo']` instead.
@nik9000 nik9000 reopened this Nov 28, 2018
@nik9000
Copy link
Member Author

nik9000 commented Nov 28, 2018

I closed this without thinking earlier today. The issue isn't resolved by #34279. I'm not as good at reading as I thought!

nik9000 pushed a commit that referenced this issue Nov 28, 2018
@j-haj @nik9000
Adds deprecation logging to ScriptDocValues#getValues. (#34279)
185f189 
`ScriptDocValues#getValues` was added for backwards compatibility but no
longer needed. Scripts using the syntax `doc['foo'].values` when
`doc['foo']` is a list should be using `doc['foo']` instead.

Closes #22919
@j-haj j-haj mentioned this issue Dec 3, 2018
Merged
nik9000 pushed a commit that referenced this issue Dec 14, 2018
@j-haj @nik9000
Delete deprecated getValues from ScriptDocValues (#36183)
f1f3b28 
* Adds deprecation logging to ScriptDocValues#getValues.

First commit addressing issue #22919.

`ScriptDocValues#getValues` was added for backwards compatibility but no
longer needed. Scripts using the syntax `doc['foo'].values` when
`doc['foo']` is a list should be using `doc['foo']` instead.

* Fixes two build errors in #34279

* Removes unused import in ScriptDocValuesDatesTest
* Removes used of `.values` in example in diversified-sampler-aggregation.asciidoc

* Removes use of .values from painless test.

Part of #34279

* Updates tests to use `doc[foo]` syntax rather than `doc[foo].values`.

* Removes use of `getValues()` and replaces use of `doc[foo].values` with `doc[foo]`.

* Indentation fix.

* Remove unnecessary list construction at previous `getValues()` callsite in ScriptDocValues.GeoPoints.

* Update migration doc and add link to `getValue` in ScriptDocValues javadoc.

* Fix compile

* Fix javadoc issue

* Removes ScriptDocValues#getValues usage from painless whitelist.
@rjernst
Copy link
Member

rjernst commented Apr 26, 2019

This was removed in #36183

@rjernst rjernst closed this as completed Apr 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
>breaking :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache good first issue low hanging fruit help wanted adoptme
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@clintongormley @nik9000 @rjernst @jdconrad @j-haj @tomcallahan