Skip to content

Conversation

@stevejgordon
Copy link
Contributor

Please consider this a WIP for review to check I'm on the best track.

Adds hard_bounds parameter to histogram aggregations per elastic/elasticsearch#59175.

Questions:

For now, I've made HardBounds a copy of ExtendedBounds. I suspect we can't safely rename this public type to something more general. An option would be to define the properties in a base class from which these two could derive. Perhaps overkill here though.

I've updated two usage tests to confirm that the final JSON is built as expected. Are there any further tests we should be considering on additional such as this one?

@Mpdreamz
Copy link
Member

Those tests are the correct place to add them but they are breaking the integration assertions:

https://github.com/elastic/elasticsearch-net/pull/5098/files#diff-abdcda181162c237e14568b13e1eda0e3aae3cc12efcc1f9b0c33e86bf203ccfR127

[xUnit.net 00:01:51.9685150]     Tests.Aggregations.Bucket.DateHistogram.DateHistogramAggregationUsageTests.ReturnsExpectedResponse [FAIL]
  X Tests.Aggregations.Bucket.DateHistogram.DateHistogramAggregationUsageTests.ReturnsExpectedResponse [939ms]
  Error Message:
   Tests.Framework.EndpointTests.ResponseAssertionException : Expected value to be greater than 10, but found 0.
Response Under Test:
Valid NEST response built from a successful (200) low level call on POST: /projects-only/_search?pretty=true&error_trace=true&typed_keys=false
# Audit trail of this API call:
 - [1] HealthyResponse: Node: http://localhost:9200/ Took: 00:00:00.0964947
# Request:
{"aggs":{"projects_started_per_month":{"aggs":{"project_tags":{"aggs":{"tags":{"terms":{"field":"tags.name"}}},"nested":{"path":"tags"}}},"date_histogram":{"extended_bounds":{"max":"2016-06-06T12:01:02.123","min":"2014-06-06T12:01:02.123"},"hard_bounds":{"max":"2016-06-06T12:01:02.123","min":"2014-06-06T12:01:02.123"},"field":"startedOn","format":"yyyy-MM-dd'T'HH:mm:ss||date_optional_time","calendar_interval":"month","min_doc_count":2,"missing":"2015-06-06T12:01:02.1230000","order":{"_count":"asc"}}}},"query":{"term":{"type":{"value":"project"}}},"size":0}
# Response:
{
  "took" : 7,
  "timed_out" : false,
  "_shards" : {
    "total" : 2,
    "successful" : 2,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 100,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  },
  "aggregations" : {
    "projects_started_per_month" : {
      "buckets" : [ ]
    }
  }
}

This might warrant their own *UsageTests.

You can run just a particular test with:

.\built.bat integrate latest readonly DateHistogramAggregation

@stevejgordon stevejgordon marked this pull request as ready for review November 18, 2020 15:43
@stevejgordon
Copy link
Contributor Author

@Mpdreamz I've reworked the tests, adding specific ones as you suggested. These seem to pass locally now too using .\build.bat integrate 7.10.0 readonly and .\build.bat test.

As with extended_bounds, we should append the date_optional_time format
to the format starting on the aggregation. This avoids parsing errors on
the server.

Includes an update the documentation.
@stevejgordon
Copy link
Contributor Author

@Mpdreamz / @russcam - On further review I noticed that this was already handled for extended_bounds by adding date_optional_time to the format string. I've updated the code so we apply the same here and now formatting works as expected.

One thing I did notice is that the callout from https://github.com/elastic/elasticsearch-net/blob/master/tests/Tests/Aggregations/Bucket/DateHistogram/DateHistogramAggregationUsageTests.cs#L42 is not rendered into the final documentation - https://github.com/elastic/elasticsearch-net/blob/master/docs/aggregations/bucket/date-histogram/date-histogram-aggregation-usage.asciidoc

Is there something extra needed to make that work? It's affecting the current 7.x documentation. https://www.elastic.co/guide/en/elasticsearch/client/net-api/current/date-histogram-aggregation-usage.html

get => !string.IsNullOrEmpty(_format) &&
!_format.Contains("date_optional_time") &&
(ExtendedBounds != null || Missing.HasValue)
(ExtendedBounds != null || HardBounds != null || Missing.HasValue)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ this hack comes back to me now 😄

@stevejgordon stevejgordon merged commit 19b8c5f into master Nov 19, 2020
@stevejgordon stevejgordon deleted the feature/hard-bounds branch November 19, 2020 14:37
github-actions bot pushed a commit that referenced this pull request Nov 19, 2020
* Add hard_bounds for histograms
* Apply date_optional_time format for hard_bounds
* Add new hard_bounds tests

As with extended_bounds, we append the date_optional_time format
to the format starting on the aggregation. This avoids parsing errors on
the server.

Includes an update to the documentation which we be generated and 
added in a subsequent PR.
github-actions bot pushed a commit that referenced this pull request Nov 19, 2020
* Add hard_bounds for histograms
* Apply date_optional_time format for hard_bounds
* Add new hard_bounds tests

As with extended_bounds, we append the date_optional_time format
to the format starting on the aggregation. This avoids parsing errors on
the server.

Includes an update to the documentation which we be generated and 
added in a subsequent PR.
stevejgordon added a commit that referenced this pull request Nov 20, 2020
* Add hard_bounds for histograms
* Apply date_optional_time format for hard_bounds
* Add new hard_bounds tests

As with extended_bounds, we append the date_optional_time format
to the format starting on the aggregation. This avoids parsing errors on
the server.

Includes an update to the documentation which we be generated and 
added in a subsequent PR.

Co-authored-by: Steve Gordon <[email protected]>
stevejgordon added a commit that referenced this pull request Nov 20, 2020
* Add hard_bounds for histograms
* Apply date_optional_time format for hard_bounds
* Add new hard_bounds tests

As with extended_bounds, we append the date_optional_time format
to the format starting on the aggregation. This avoids parsing errors on
the server.

Includes an update to the documentation which we be generated and 
added in a subsequent PR.

Co-authored-by: Steve Gordon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants