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

Add implicit this for class binding in Painless #40285

Merged
merged 7 commits into from
Mar 22, 2019

Conversation

jdconrad
Copy link
Contributor

@jdconrad jdconrad commented Mar 20, 2019

This change allows class bindings to add as their first argument, the base script class. The this reference to the base script class will be implicitly passed into a class binding as the first constructor argument upon initialization when specified as the first argument in whitelist entry for the class binding. This allows a class binding access to additional information added to the base script class such as more information about the current document or current shard. One extra requirement for this to work is the appropriate script base class must be whitelisted (should be empty). This should improve the situation for (#40186) as the user should only have to pass in the initial seed value.

Example:
Whitelist:

class org.elasticsearch.painless.BindingsTests$BindingsTestScript {
}

static_import {
int addThisWithState(BindingsTests.BindingsTestScript, int, int, int, double) bound_to org.elasticsearch.painless.BindingsTests$ThisBindingTestClass
}

Binding Class:

    public static class ThisBindingTestClass {
        private BindingsTestScript bindingsTestScript;
        private int state;

        public ThisBindingTestClass(BindingsTestScript bindingsTestScript, int state0, int state1) {
            this.bindingsTestScript = bindingsTestScript;
            this.state = state0 + state1;
        }

        public int addThisWithState(int istateless, double dstateless) {
            return istateless + state + (int)dstateless + bindingsTestScript.getTestValue();
        }
    }

Painless Script:

addThisWithState(4, bound, test, 0.0)

@jdconrad jdconrad added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 v7.2.0 labels Mar 20, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jdconrad
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/bwc

Copy link
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. Can you please add a comment somewhere to this explaining the rationale of using this hack and the desired eventual design?

@jdconrad
Copy link
Contributor Author

@rjernst Thanks for the review! Will find an appropriate place for and add the requested comment and then commit once CI passes.

@mayya-sharipova
Copy link
Contributor

@jdconrad Jack, thank so much for this change. This is really great.
I just have a question how to pass my base script class to ScoreScript, so that my base script class will be used for scoring?

@jdconrad
Copy link
Contributor Author

jdconrad commented Mar 22, 2019

@mayya-sharipova YW! Looks to me like the ScoreScript is the base class in this case unless I'm missing something. I noted that you already have a whitelist resource file specifically for the score context, so I believe (once this is commited) that you can just add ScoreScript to the whitelist as an empty class and then add it as the first argument for the binding and it will be implicitly passed in without the user needing to add it.

Example:
Whitelist:

class org.elasticsearch.script.ScoreScript no_import {
}
static_import {
  scoreScriptRandom(org.elasticsearch.script.ScoreScript, int) bound_to org.elasticsearch.script.ScoreScriptRandomClass
}

Painless Call:

scoreScriptRandom(seed)

where seed is considered to be read-only

From here you can add whatever methods you need to ScoreScript and call them directly from the ScoreScriptRandomClass since the ScoreScript will be passed into the constructor. You can also check out the example in BindingsTests.

Please let me know if you have further questions :)

@jdconrad jdconrad merged commit 8da56dc into elastic:master Mar 22, 2019
@mayya-sharipova
Copy link
Contributor

@jdconrad Jack, just tested - everything works fantastic! Thanks for this change

jdconrad added a commit that referenced this pull request Mar 22, 2019
This change allows class bindings to add as their first argument, the base script 
class. The this reference to the base script class will be implicitly passed into a 
class binding as the first constructor argument upon initialization when 
specified as the first argument in whitelist entry for the class binding. This 
allows a class binding access to additional information added to the base script 
class such as more information about the current document or current shard. 
One extra requirement for this to work is the appropriate script base class 
must be whitelisted (should be empty).
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 22, 2019
* elastic/master:
  [DOCS] Adds notable highlights tags (elastic#40330)
  [ML] making test more determinate (elastic#40374)
  [ML] adds support for non-numeric mapped types (elastic#40220)
  Move outbound message handling to OutboundHandler (elastic#40336)
  Add implicit this for class binding in Painless (elastic#40285)
  Muting test testExtractIndexCheckpointsInconsistentGlobalCheckpoints (elastic#40371)
  DOC: polish client docs
  Fix building bwc versions (elastic#40361)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 22, 2019
…-stop-time

* elastic/master:
  [DOCS] Adds notable highlights tags (elastic#40330)
  [ML] making test more determinate (elastic#40374)
  [ML] adds support for non-numeric mapped types (elastic#40220)
  Move outbound message handling to OutboundHandler (elastic#40336)
  Add implicit this for class binding in Painless (elastic#40285)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement v7.2.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants