Move mov_fn agg to module #90836
Move mov_fn agg to module #90836
Conversation
This continues to populate the `aggregations` module with it's first pipeline aggregation and it's first custom script context. Relates to elastic#90283
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
There was a problem hiding this comment.
I'm happy to break this into two changes if folks want. It really has two sets of reviewers. But I stuck it together to sort of explain the painless stuff.
I didn't move the MovingFunctions implementations. Those are still in core because the ml plugin relies on them. We can move them, but that requires a few more decisions I didn't want to make at this time.
| public Set<ScriptContext<?>> getSupportedContexts() { | ||
| return Set.of(MovingFunctionScript.CONTEXT); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This is required because we MockScriptEngine can no longer provide a default implementation. So far as I can tell that implementation was only used for this test so I'm kind of happy to isolate it here anyway.
| script: "MovingFunctions.min(values)" | ||
|
|
||
| --- | ||
| "Bad window": |
There was a problem hiding this comment.
I combined the two test files we have for moving_fn
| 'painless/40_fields_api/filter script fields api', | ||
| 'painless/40_fields_api/script score fields api' | ||
| 'painless/40_fields_api/script score fields api', | ||
| 'painless/70_mov_fn_agg/*' // Agg moved to a module. |
There was a problem hiding this comment.
This is kind of a shame.
There was a problem hiding this comment.
Right, because the integration test cluster started here doesn't have the analytics module and therefor the moving average pipeline aggregation isn't available when running these bwc yaml tests. We won't have this test coverage in the aggregations module, since the tests didn't exist in this module in earlier versions (in fact the entire module doesn't exist).
We can maybe think about adding the aggregations module to the painless test cluster? Then we don't lose test coverage?
| request = new Request(new Script("MovingFunctions.max(new double[]{1, 3, 2})"), null, null); | ||
| response = innerShardOperation(request, scriptService, null); | ||
| assertEquals(3.0, Double.parseDouble((String) response.getResult()), .1); | ||
|
|
There was a problem hiding this comment.
I moved this to a yaml test in the aggregations module. It only works with that module enabled now.
| @@ -1,47 +0,0 @@ | |||
| # There are many more tests under modules/lang-painless/...moving_fn.yml so they can use painless | |||
| --- | |||
| "Bad window": | |||
There was a problem hiding this comment.
moved to the aggregations module
|
This fails because we are pulling the aggregations module into the rest-api-spec. Mostly we're doing that because of BWC. We're also doing that because at some point we'll get to tests where we want to move one of the aggs and not yet others. I think we should bite the bullet and remove the dependency. In a follow up PR though. |
Well, in a pre-committed PR actually. |
|
#90851 would fix the error here I think. |
| */ | ||
|
|
||
| package org.elasticsearch.search.aggregations.pipeline; | ||
| package org.elasticsearch.aggregations.pipeline; |
There was a problem hiding this comment.
maybe add a package level java docs for pipeline package?
...aggregations/src/main/java/org/elasticsearch/aggregations/AggregationsPainlessExtension.java
Outdated
Show resolved
Hide resolved
...aggregations/src/main/resources/org/elasticsearch/aggregations/moving_function_whitelist.txt
Outdated
Show resolved
Hide resolved
|
@elasticmachine, test this please |
5 participants
This continues to populate the
aggregationsmodule with it's firstpipeline aggregation and it's first custom script context. It also moves
the script context that painless uses for it's execute API into the SPI
so we can attach the
MovingFunctionsto it so we don't breakbackwards compatibility.
Relates to #90283