Begin moving date_histogram to offset rounding#50873
Conversation
We added a new rounding in elastic#50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset.
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
Neat! Real test failure. |
|
@elasticmachine, run elasticsearch-ci/2 |
not-napoleon
left a comment
There was a problem hiding this comment.
Looks good overall, but I'm a little concerned about the new & deprecated offset() method, as noted. If there's a plan for that, then I'm ok to merge as is, but I'd like to be explicit about the plan.
| Rounding effectiveRounding = rounding.withoutOffset(); | ||
| return new ExtendedBounds( | ||
| min != null ? effectiveRounding.round(min) : null, | ||
| max != null ? effectiveRounding.round(max) : null); |
There was a problem hiding this comment.
Excellent formatting choice, much more readable.
| }; | ||
| } | ||
|
|
||
| private long offsetAwareRounding(Rounding rounding, long value, long offset) { |
There was a problem hiding this comment.
Glad to see this getting moved into the rounding itself. Makes much more sense.
| * so keep any usage to migratory shims | ||
| */ | ||
| @Deprecated | ||
| public abstract long offset(); |
There was a problem hiding this comment.
What's the plan to migrate away from this? I'm not sure I understand the benefit to adding a deprecated method now that we intend to remove in the next PR, as opposed to just removing the need for it in this PR.
There was a problem hiding this comment.
I added this to cut the PR in half, basically. I'll remove it in a followup. I believe that follow up with have serialization changes so it'll feel pretty different than this PR.
There was a problem hiding this comment.
Seems reasonable, thanks for explaining!
|
Thanks @not-napoleon! |
…0873) We added a new rounding in elastic#50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset.
I caused a serialization bug in elastic#50873. It is kind of tricky to get: * Run a date_histogram aggregation * On a shard on 7.6 * That shard doesn't return any results * The coordinating node is before 7.6
This reverts commit b3a7728.
…lastic#50873) (elastic#50978)" This reverts commit 9a3d4db. It was subtly broken in ways we didn't have tests for.
|
I'm reverting this because it was subtly broken in ways we didn't have tests for. I'll make a PR that adds the tests and redoes it soon enough. |
…lastic#50873) (elastic#50978)" This reverts commit 9a3d4db. It was subtly broken in ways we don't have tests for.
…ic#50873)" (elastic#51238)" This reverts commit d114c9d.
We added a new rounding in elastic#50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset.
) We added a new rounding in elastic#50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset. This is a redo of elastic#50873 with more integration tests. This reverts commit d114c9d.
…51495) We added a new rounding in #50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset. This is a redo of #50873 with more integration tests. This reverts commit d114c9d.
We added a new rounding in #50609 that handles offsets to the start and
end of the rounding so that we could support
offsetin thecompositeaggregation. This starts moving
date_histogramto that new offset.