Moving_fn and gap_policy

[jcornez]

Possibly related to #42196

Here is a complete test case to show the problem. Tested on ES 7.0.1

PUT movfn_test

POST movfn_test/_doc/1
{
  "timestamp_utc": "2019-05-01",
  "foo": 1
}

POST movfn_test/_doc/3
{
  "timestamp_utc": "2019-05-03",
  "foo": 3
}

POST movfn_test/_search
{
  "size": 0,
  "aggs": {
    "days": {
      "date_histogram": {
        "min_doc_count": 0, 
        "field": "timestamp_utc",
        "interval": "1d"
      },
      "aggs": {
        "curr_foo": { "avg": {"field": "foo" }},
        "prev_foo": {
          "moving_fn": {
            "buckets_path": "curr_foo", "window": 1,
            "script": "MovingFunctions.sum(values)",
            "gap_policy": "skip"
        }}
    }}
  },
  "query": {
    "match_all": {}
  }
}

Returns

{
  "took" : 0,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 2,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  },
  "aggregations" : {
    "days" : {
      "buckets" : [
        {
          "key_as_string" : "2019-05-01T00:00:00.000Z",
          "key" : 1556668800000,
          "doc_count" : 1,
          "curr_foo" : {
            "value" : 1.0
          },
          "prev_foo" : {
            "value" : 0.0
          }
        },
        {
          "key_as_string" : "2019-05-02T00:00:00.000Z",
          "key" : 1556755200000,
          "doc_count" : 0,
          "curr_foo" : {
            "value" : null
          }
        },
        {
          "key_as_string" : "2019-05-03T00:00:00.000Z",
          "key" : 1556841600000,
          "doc_count" : 1,
          "curr_foo" : {
            "value" : 3.0
          },
          "prev_foo" : {
            "value" : 1.0
          }
        }
      ]
    }
  }
}

I expect prev_foo on day 03 to be null and not 1.0.

That is, I don't think gap_policy: skip means to extend the window back until a value is found. The window is 1 and I expect that to be honored. Here skip should not do anything. In the case of the fn being unweightedAvg or similar, I expect skip to mean that "missing" value should not be included in the set to be averaged.

[elasticmachine]

Pinging @elastic/es-analytics-geo

[polyfractal]

I think skip is doing the correct thing here; the bucket has no value so it is skipping it entirely. That aligns with its intended function.

I think, ultimately, what you are looking for is a different kind of gap_policy rather than skip/insert_zeros. Something like allow_empty, a gap_policy that will use null, NaN, Infinity, entirely missing, etc.

This does have some non-trivial semantics though. E.g. if you have a window of 5, a single missing value with that new gap_policy will "infect" the rest of the window for most of the moving functions, because you can't average null/NaN/etc into the rest of the values.

I'm not opposed to adding more gap policies, the original intent was to add more things like interpolation too. But in this case I think skip is probably "doing the right thing", just not the thing you're looking for :)

If a allow_empty style gap policy was added, it would probably also satisfy the needs of #42196.

[jcornez]

Sure - maybe someone wants "skip" to work this way. To me, I don't want to skip the bucket, but just the missing value.

Anyway, a rose by any other name...
So I'd be happy to have more gap_policies and that's what I'm really looking for, whatever the option is called, I'm fine.

Thank you.

[polyfractal]

Anyway, a rose by any other name...

For sure :) I was just wanting to clarify that I think this is new functionality that we should build, rather than a bug in the existing skip (in this situation, unlike the buggy aspects in other aggs).

Anyhow, labeling this enhancement and I think it should be doable. We'll need to verify that the existing functions can handle non-numeric values (null etc) but that shouldn't be a dealbreaker I don't think.

[jcornez]

I'd like to leave another thought on why I think the current skip isn't the best. I don't mind if not everyone agrees, but I think it is worth making the point

Here I am working with a date_histogram with interval: 1d. And if I say window: 7 then I expect that will mean one week. I will go further to say that I think this is quite reasonable and probably I am not alone.

So I find it very surprising that my choice of gap_policy along with the data itself can actually cause this to not be one week.

Again, thanks for your consideration.

[jcornez]

I'm curious about how things might be expected to progress at this point? Is it likely this could be part of ES 8.x? Sooner?

What can be done to encourage action? I'm a developer and work with other developers, but we haven't work on ES code before. So maybe there are bounties or similar? If there is just a link that I should read first, just let me know.

Thank you.

[polyfractal]

Hard to give a timeline to be honest. One of the analytics team might be starting on pipeline agg related issues soon, but it's tough to say when something like this might be addressed (we have a number of other semi-related issues too, as you've run into, so those might get addressed first). An enhancement like this can go in pretty much any release, there's no need to wait for 8.x. It's mainly a time issue and prioritizing the work.

I think adding a new gap policy should be relatively contained though, and not a bad first contribution project. We would happily take PRs for this! :)

We have some contributing guidelines here. Code style, how to run our build, testing, etc.

As to code, adding a new gap policy (allow_empty for example) would entail three main things:

1. Add a new enum value to the existing policies
2. Add logic for the new gap policy when it encounters a gap
3. Ensure that returning null/NaN from that new policy doesn't break existing aggs. This will be the difficult part I think, since I think we use null as a placeholder for "empty" and use that to drive reduction logic. And probably some aggs rely on it for their own functioning.

Adding the new policy should be easy enough, but I'm not sure what kind of troubles you'll run into with the null/NaN return values. It might also be easy, or non-trivial, I'm really not sure without digging a bit more myself.

If you want to work on this I'd say go ahead, and open a WIP PR if you get stuck on something, and we can work through the technical difficulties on the PR itself.

[jcornez]

Thanks for the detailed info. I don't know that I'll be able to contribute, but I do appreciate that the opportunity exists.

Beyond the code, I'm not clear on how specs and features are agreed upon. I think my question/comment above about gap_policy and window is at least worth some discussion. As I said, I find the current behavior surprising and I suspect that I might not be the only one. Before anyone dives into code, maybe such topic should be discussed?

[wchaparro]

related: #82808