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

Reorganise scripting docs #18132

Merged
merged 5 commits into from May 4, 2016

Conversation

Projects
None yet
3 participants
@clintongormley
Copy link
Member

commented May 4, 2016

This PR is based on the reorganisation started by @rmuir in #18116. It tidies up some of the asciidoc layout, adds more examples using Lucene expressions and rewrites some of the more confusing sections.

I've also moved the doc-values properties/methods under the Groovy section as these aren't supported by Painless (yet?) of Expressions. I added the experimental label to text-scoring-in-scripts as its value and implementation is questionable.

Closes #18116

rmuir and others added some commits May 3, 2016

@clintongormley

This comment has been minimized.

Copy link
Member Author

commented May 4, 2016

@jdconrad @rmuir could you take a look at this when you have a moment?

When a document is missing the field completely, by default the value will be treated as `0`.

Boolean fields are exposed as numerics, with `true` mapped to `1` and `false` mapped to `0`.
For example: `doc['on_sale'] ? doc['price'] * 0.5 : doc['price']`

This comment has been minimized.

Copy link
@jpountz

jpountz May 4, 2016

Contributor

should we append .value to be consistent with other examples?

This comment has been minimized.

Copy link
@clintongormley

clintongormley May 4, 2016

Author Member

.value isn't required here. honestly, i'm not certain when it IS required

This comment has been minimized.

Copy link
@rmuir

rmuir May 4, 2016

Contributor

+1 to add .value

yes for expressions, .value is optional, in the sense its the default if you don't specify anything else.

e.g. doc['foo'] == doc['foo'].value == doc['foo'].min()

Its always been this way, so I don't think we need to take away this shortcut (break any scripts), but i think as far as docs go, consistency is better and we should use .value? Its slightly more verbose, but this makes it appear more as a "subset" of the scripting API, which I think is what we want as much as possible, rather than something totally strange.

I think i was just lazy in this case.


|`doc['field_name'].lat` |The latitude of the geo point.

|`doc['field_name'].lon` |The longitude of the geo point.

This comment has been minimized.

Copy link
@jpountz

jpountz May 4, 2016

Contributor

maybe we need to document that these accessors have no default value. Ie. if a document does not have a value for a geo point this will throw a NPE. So protecting access with empty() is important.

This comment has been minimized.

Copy link
@jpountz

jpountz May 4, 2016

Contributor

alternatively we could fix the code to give them default values, but I am not sure what default value makes sense for a geopoint

This comment has been minimized.

Copy link
@clintongormley

clintongormley May 4, 2016

Author Member

I've added ", or null" to both lines, and "or an empty array" to the plural forms

This comment has been minimized.

Copy link
@rmuir

rmuir May 4, 2016

Contributor

I think this is wrong for the expressions case. Expressions only have doubles, null does not exist.

Just like other fields, if its missing, latitude and longitude is treated as 0. if you don't like that, you can use .empty to check if its empty, and substitute something else. E.G. doc['field'].empty ? 90 : doc['field'].lat to treat missing values as 90, for example. just like numeric/date fields.

See https://github.com/elastic/elasticsearch/blob/master/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoLatitudeValueSource.java#L53-L54

This comment has been minimized.

Copy link
@rmuir

rmuir May 4, 2016

Contributor

I pushed a little fix for this here: 7656d7e

The only time it really makes sense to use stored fields instead of the
`_source` field is when the `_source` is very large and it is less costly to
access a few small stored fields instead of the entire `_source`.

This comment has been minimized.

Copy link
@jpountz

jpountz May 4, 2016

Contributor

For the record another difference is that stored fields store a more primitive representation of the data (milliseconds since 1970 for dates for instance). I am not sure whether this is worth documenting.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

Thanks for improving this!

FYI I tried to build this locally to look but hit a snag:

a2x: executing: "xmllint" --nonet --noout --valid "/home/rmuir/workspace/elasticsearch-docs/index.xml"

/home/rmuir/workspace/elasticsearch-docs/index.xml:53014: element link: validity error : IDREF attribute linkend references an unknown ID "aggregations"
/home/rmuir/workspace/elasticsearch-docs/index.xml:52863: element link: validity error : IDREF attribute linkend referencesa2x: ERROR: "xmllint" --nonet --noout --valid "/home/rmuir/workspace/elasticsearch-docs/index.xml" returned non-zero exit status 4
 an unknown ID "script-reloading"

clintongormley added some commits May 4, 2016

@clintongormley

This comment has been minimized.

Copy link
Member Author

commented May 4, 2016

thanks @rmuir and @jpountz - i've addressed the issues and will merge

@clintongormley clintongormley merged commit 34d90b0 into elastic:master May 4, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@clintongormley clintongormley deleted the clintongormley:scripting-docs branch May 4, 2016

rmuir added a commit that referenced this pull request May 4, 2016

docs: remove null from expressions case.
Expressions don't have nulls, only doubles. If the field is missing, then its
treated as 0.0. You can query .empty to see if its missing and substitute something else.

See #18132 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.