Runtime fields to expose _source and stored fields like existing scripts #61775
Conversation
javanna
added
>enhancement
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
…fields This commit exposes stored fields to runtime fields (through params._fields) and replaces the current way to access _source (source['field']) in favour of the documented way for all existing scripts: params._source .
javanna
force-pushed
the
enhancement/runtime_fields_source_fields_access
branch
from
c085680 to
261b92a
Compare
| @@ -166,7 +166,7 @@ private static String painlessToLoadFromSource(String name, String type) { | |||
| return null; | |||
| } | |||
| StringBuilder b = new StringBuilder(); | |||
| b.append("def v = source['").append(name).append("'];\n"); | |||
| b.append("def v = params._source.").append(name).append(";\n"); | |||
There was a problem hiding this comment.
@javanna and I talked about this over video - I think that this syntax dates back to when we used mvel and the only reason for us to stick with it is backwards compatibility. But that aggregation scripts didn't really support _source before so maybe it isn't worth adding it to runtime fields like this. But we'll talk with a few folks about it before deciding for sure.
There was a problem hiding this comment.
@stu-elastic @jdconrad @rjernst what should we do here? We do want to give access to _source and stored fields in runtime fields, but we are wondering what the direction is on the long run and what syntax we should use. With this PR we are aligning with what is already supported but maybe that is not a good idea? What are your thoughts on this?
There was a problem hiding this comment.
Long-term we want all of these to be top-level parameters, so it's just _source, _fields, etc. This is possible now using getter methods and storing the appropriate values as part of the script during instance creation, but for most contexts it'll require additional deprecation. For runtime fields unless there are objections I would prefer to just start with these as top level variables.
There was a problem hiding this comment.
Sounds good, that means I will update this PR to expose source as _source (currently exposes as source) and stored fields as _fields (currently not exposed)
There was a problem hiding this comment.
it may make sense to start the deprecation dance in 7.x for existing scripts, at least that would allow us to have the new syntax in the docs, cause otherwise scripts for runtime fields will look completely different from anything listed in the docs. Or is it acceptable that runtime fields have their own separate scripting syntax in the docs too?
There was a problem hiding this comment.
I'd like to start that dance in 7.x, yes.
There was a problem hiding this comment.
I'd like to make sure with @rjernst that those are the expected names for exposing these things.
There was a problem hiding this comment.
We want to completely change how source and docvalues are accessed in scripts (see #61388). In the meantime, though, I think we should use existing patterns for access, to make it easier on users writing scripts across different contexts. While there are several ways we have of accessing source currently, the most common is params._source, so I think we should use that until we have a universal fields API.
I think that this syntax dates back to when we used mvel
History lesson! In mvel, we had _source. When we added painless, we wanted to deter users from using slow access methods, like source and fields. So we temporarily put these under params. The intent was to eventually formalize a way to access source in a different way in the global namespace of the script. Params was a dynamic place we could stash them in the meantime. However, we never got around to it fixing it (though we are looking at it now with the universal fields API), and params._source has stuck all these years.
…_source_fields_access
| // accessing doc_values through through params._doc and params.doc is deprecated in all existing script contexts, | ||
| // it makes little sense to expose it deprecated for runtime fields | ||
| if (entry.getKey().equals("_doc") == false && entry.getKey().equals("doc") == false) { | ||
| params.put(entry.getKey(), entry.getValue()); |
There was a problem hiding this comment.
Do you think it'd be more clear to:
params.put("_source", leafSearchLookup.searchLookup());
params.put("_fields", leafSearchLookup.fields());
?
There was a problem hiding this comment.
I would not rely on asMap(), but instead be explicit as Nik suggests.
There was a problem hiding this comment.
agreed, I already made this change
…_source_fields_access
|
run elasticsearch-ci/packaging-sample-windows |
This PR exposes stored fields to runtime fields (through params._fields) and replaces the current way to access _source (source['field']) in favour of the documented way for all existing scripts: params._source .
Relates to #59332