Skip to content

Scripting: Cache script results if deterministic#50106

Merged
stu-elastic merged 49 commits intoelastic:masterfrom
stu-elastic:feature/painless-caching-part-2
Dec 18, 2019
Merged

Scripting: Cache script results if deterministic#50106
stu-elastic merged 49 commits intoelastic:masterfrom
stu-elastic:feature/painless-caching-part-2

Conversation

@stu-elastic
Copy link
Copy Markdown
Contributor

@stu-elastic stu-elastic commented Dec 11, 2019

Cache results from queries that use scripts if they use only
deterministic API calls. Nondeterministic API calls are marked in the
whitelist with the @nondeterministic annotation. Examples are
Math.random() and new Date().

Refs: #49466

@stu-elastic stu-elastic added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Dec 11, 2019
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Dec 11, 2019

Pinging @elastic/es-core-infra (:Core/Infra/Scripting)

jdconrad
jdconrad approved these changes Dec 12, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jdconrad jdconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once we address the couple of comments I left that require further discussion.

rjernst
rjernst approved these changes Dec 12, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left a few suggestions.

@stu-elastic stu-elastic removed the WIP label Dec 12, 2019
@stu-elastic
Copy link
Copy Markdown
Contributor Author

stu-elastic commented Dec 12, 2019

Currently debugging NPE when using scripts for weighted avg, fieldName coming in as null.

@stu-elastic
Copy link
Copy Markdown
Contributor Author

stu-elastic commented Dec 12, 2019

Ok, now that scripts are cachable, here's what's causing the NPE

SearchService.loadOrExecuteQueryPhase

calls into

IndicesService.loadIntoContext (instead of queryPhase.execute)

which calls

ShardSearchRequest.cacheKey

which eventually calls

MultiValuesSourceFieldConfig.writeTo

which nulls out because there is no fieldName.

Here's the series of calls to reproduce:

curl -H 'Content-Type: application/json' -XPUT --user 'elastic-admin:elastic-password' localhost:9200/exams -d '{ "mappings": { "properties": { "grade": { "type": "byte" } } } }'

curl -H 'Content-Type: application/json' -XPOST --user 'elastic-admin:elastic-password' localhost:9200/exams/_doc -d '{"grade": 100, "weight": 2}'

curl -H 'Content-Type: application/json' -XPOST --user 'elastic-admin:elastic-password' localhost:9200/exams/_doc -d '{"grade": 50, "weight": 3}'

curl -H 'Content-Type: application/json' -XPOST --user 'elastic-admin:elastic-password' localhost:9200/exams/_search -d '{
    "size": 0,
    "aggs" : {
        "weighted_grade": {
            "weighted_avg": {
                "value": {
                    "script": "doc.grade.value + 1"
                },
                "weight": {
                    "script": "doc.weight.value + 1"
                }
            }
        }
    }
}'

@stu-elastic
Copy link
Copy Markdown
Contributor Author

stu-elastic commented Dec 12, 2019

This just happened to be caught by a test. Are there other places where cacheKey calls writeTo with fields that are not available if a script is used?

@stu-elastic stu-elastic requested a review from jpountz December 13, 2019 19:15
@stu-elastic
Copy link
Copy Markdown
Contributor Author

stu-elastic commented Dec 18, 2019

@elasticmachine run elasticsearch-ci/1

@jdconrad
Copy link
Copy Markdown
Contributor

jdconrad commented Dec 18, 2019

@stu-elastic Thanks again for all the work getting this done. The tests look especially good.

@stu-elastic stu-elastic merged commit 7794625 into elastic:master Dec 18, 2019
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this pull request Dec 18, 2019
@stu-elastic
Scripting: Cache script results if deterministic (elastic#50106)
e02e10c 
Cache results from queries that use scripts if they use only
deterministic API calls.  Nondeterministic API calls are marked in the
whitelist with the `@nondeterministic` annotation.  Examples are
`Math.random()` and `new Date()`.

Refs: elastic#49466
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this pull request Dec 18, 2019
@stu-elastic
[TEST] BWC re-enable after backport of elastic#50106
44bd648
stu-elastic added a commit that referenced this pull request Dec 18, 2019
@stu-elastic
Scripting: Cache script results if deterministic (#50106) (#50329)
06a24f0 
Cache results from queries that use scripts if they use only
deterministic API calls.  Nondeterministic API calls are marked in the
whitelist with the `@nondeterministic` annotation.  Examples are
`Math.random()` and `new Date()`.

Refs: #49466
stu-elastic added a commit that referenced this pull request Dec 18, 2019
@stu-elastic
[TEST] BWC re-enable after backport of #50106 (#50336)
a3ca04d
@stu-elastic stu-elastic mentioned this pull request Dec 19, 2019
Closed
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
@stu-elastic @SivagurunathanV
Scripting: Cache script results if deterministic (elastic#50106)
85d6941 
Cache results from queries that use scripts if they use only
deterministic API calls.  Nondeterministic API calls are marked in the
whitelist with the `@nondeterministic` annotation.  Examples are
`Math.random()` and `new Date()`.

Refs: elastic#49466
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
@stu-elastic @SivagurunathanV
[TEST] BWC re-enable after backport of elastic#50106 (elastic#50336)
49c05ee
This was referenced Feb 3, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdconrad jdconrad jdconrad approved these changes

@rjernst rjernst rjernst approved these changes

+1 more reviewer

@jpountz jpountz jpountz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement v7.6.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@stu-elastic @elasticmachine @jdconrad @rjernst @jpountz @jakelandis