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

Static return values for scripting runtime fields #59647

Closed
stu-elastic opened this issue Jul 15, 2020 · 6 comments
Closed

Static return values for scripting runtime fields #59647

stu-elastic opened this issue Jul 15, 2020 · 6 comments
Assignees
@stu-elastic
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement Team:Core/Infra Meta label for core/infra team

Comments

@stu-elastic
Copy link
Contributor

Runtime fields need to ensure scripts have the return type specified in the mapping. Returning Object would incur dramatic boxing penalties.

We can implement this in the scripting infrastructure by having a context per mapping type, with the different contexts differing only on the execute return value.

Related: #59332
@stu-elastic stu-elastic added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache needs:triage Requires assignment of a team area label labels Jul 15, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 15, 2020
@javanna javanna mentioned this issue Jul 15, 2020
Closed
30 tasks
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 15, 2020
@stu-elastic
Scripting: Augment String with sha1 and sha256
46c64ab 
Only available in the ingest context for use in ingest pipelines.

Digests are computed on the UTF-8 encoding of the String and are
returned as hex strings.

sha1() return hex strings of length 40, sha256() returns length 64

Fixes: elastic#59647
@stu-elastic stu-elastic mentioned this issue Jul 15, 2020
Merged
@andreidan andreidan removed the needs:triage Requires assignment of a team area label label Jul 16, 2020
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 16, 2020
@stu-elastic
Scripting: Augment String with sha1 and sha256 (elastic#59671)
685546a 
Only available in the ingest context for use in ingest pipelines.

Digests are computed on the UTF-8 encoding of the String and are
returned as hex strings.

sha1() return hex strings of length 40, sha256() returns length 64

Fixes: elastic#59647
Backport: 3c85272
@stu-elastic stu-elastic mentioned this issue Jul 16, 2020
Merged
stu-elastic added a commit that referenced this issue Jul 16, 2020
@stu-elastic
Scripting: Augment String with sha1 and sha256 (#59671) (#59723)
8fdaed0 
Only available in the ingest context for use in ingest pipelines.

Digests are computed on the UTF-8 encoding of the String and are
returned as hex strings.

sha1() return hex strings of length 40, sha256() returns length 64

Fixes: #59647
Backport: 3c85272
@stu-elastic stu-elastic reopened this Jul 20, 2020
@stu-elastic
Copy link
Contributor Author

Wrongly closed.

@stu-elastic
Copy link
Contributor Author

  • We will have contexts that all return primitive arrays
  • We will optimize for single value return by default allocating length one array
    • If script returns non-array, we will automatically fill this array
    • We can also auto-convert List<BoxedTyped> to type[]
  • RT field team will select the appropriate context based on their knowledge of the mappings.

@stu-elastic stu-elastic self-assigned this Jul 23, 2020
@rjernst
Copy link
Member

rjernst commented Jul 27, 2020

One thought regarding the return type conversions. We've gone to great lengths to make painless agnostic to the signature for particular contexts. Rather than having some kind of magic living within painless to do conversions, what if we were to have conversion methods on the script class itself? This would be similar to the special getters we have which painless looks for, in that the script class (ie script context) completely defines the input and output of the script. So painless would see the return types don't match up, and have some registered conversion methods to look at from when we registered the script context.

@stu-elastic stu-elastic mentioned this issue Aug 5, 2020
Closed
@stu-elastic
Copy link
Contributor Author

stu-elastic commented Aug 5, 2020 •
edited
Loading

Phase 1 - improve casting model:

  • ScriptContexts define a set of cast methods that take various types and return the context return value
  • return x is wrapped return cast(x) to ensure the proper return value is always returned

This is along with the current casting model (casting long when double is expected, for example).

stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Aug 10, 2020
@stu-elastic
Scripting: Converters can adapt return values
03cf8a6 
Add ability to explicitly coerce the return value from a script.

Runtime fields want to avoid returning `Object` from the execute method
in each context.  Instead, they will return an array of primitive
objects, such as `long[]`.

However, it's convenient to allow a user to return a single primitive
type rather than allocating a length-one array.

To achieve this, an implementer can add explicit conversion functions to
a context with signature:
`public static <context-return-value> convertFrom<Suffix>(<any type>)`

When a user returns a type other than the context return value, at
compile-time, painless will insert a call to their `convertFrom` method.

This commit is Phase 1 of this work.  It handles explicit converters for
all painless types EXCEPT def type.

Refs: elastic#59647
@stu-elastic stu-elastic mentioned this issue Aug 10, 2020
Merged
stu-elastic added a commit that referenced this issue Aug 11, 2020
@stu-elastic
Scripting: Converters can adapt return values (#60937)
5752340 
Add ability to explicitly coerce the return value from a script.

Runtime fields wants to avoid returning `Object` from the execute method
in each context.  Instead, they will return an array of primitives, such as
`long[]`.

However, it's convenient to allow a user to return a single primitive
type rather than allocating a length-one array.

To achieve this, an implementer can add explicit conversion functions to
a context with the signature:
`public static <context-return-value> convertFrom<Suffix>(<any type>)`

When a user returns a type other than the context return value, at
compile-time, painless will insert a call to their `convertFrom` method.

This commit is Phase 1 of this work.  It handles explicit converters for
all painless types EXCEPT def type.

Refs: #59647
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Aug 19, 2020
@stu-elastic
Scripting: Converts casting and def support
c858a5a 
Painless will cast returned values to a converter
argument type, if necessary.

Painless will also look for a special `convertFromDef`
converter which is called to explicitly handle `def`
conversions.

`convertFromDef` must handle all valid def conversions.

Refs: elastic#59647
@stu-elastic stu-elastic mentioned this issue Aug 19, 2020
Merged
stu-elastic added a commit that referenced this issue Aug 20, 2020
@stu-elastic
Scripting: Converts casting and def support (#61350)
bb48a6d 
Painless will cast returned values to a converter
argument type, if necessary.

Painless will also look for a special `convertFromDef`
converter which is called to explicitly handle `def`
conversions.

`convertFromDef` must handle all valid def conversions.

Refs: #59647
@stu-elastic stu-elastic mentioned this issue Aug 20, 2020
Closed
@stu-elastic
Copy link
Contributor Author

Closing this as all requirements are implemented, created #61389 to make them easier on implementers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stu-elastic stu-elastic

Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement Team:Core/Infra Meta label for core/infra team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@rjernst @andreidan @elasticmachine @stu-elastic