Replaced pre_offset and post_offset with offset #2820
Conversation
|
Could someone please review this PR? |
|
Thanks for the PR @jeroenkoknl, I'll take a look |
russcam
left a comment
russcam
left a comment
There was a problem hiding this comment.
Thanks again for the PR! I left a couple of comments for changes
| long? PreOffset { get; set; } | ||
|
|
||
| [JsonProperty("post_offset")] | ||
| long? PostOffset { get; set; } |
There was a problem hiding this comment.
For binary compatibility reasons, these can't be removed in a major version i.e. they can be removed in NEST 6.x but not 5.x. These should be marked with [Obsolete("removed in Elasticsearch 2.0. Will be removed in the next major version of NEST")] or something to that effect, and the JsonPropertyAttributes removed, so that setting them becomes a no-op.
| [JsonProperty("post_offset")] | ||
| long? PostOffset { get; set; } | ||
| [JsonProperty("offset")] | ||
| double? Offset { get; set; } |
There was a problem hiding this comment.
It looks like this can be a Time type for greater flexibility.
There was a problem hiding this comment.
I may be misunderstanding your request but I'm not sure this is correct. This change applies to the HistorygramAggregation and not to DateHistogramAggregation. Also in the java counterpart HistogramAggregationBuilder doesn't have this method. I may be missing something so could you please explain?
|
@russcam, any ideas why CI failed? The error message:
|
|
@jeroenkoknl seems like an intermittent travis bootstrap error. Again many thanks for the PR, much appreciated 👍 , rebased and squashed this commit without the merge commits via #2845 and forward ported it to |
|
Great! Thank you! |
3 participants
pre_offsetandpost_offsetare not supported in the search API,offsetis.