Skip to content

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Apr 17, 2019

Fixes #3673

Copy link
Contributor

@codebrain codebrain left a comment

Choose a reason for hiding this comment

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

Some small comments

@@ -745,15 +757,21 @@ private IBucket GetDateHistogramBucket(ref JsonReader reader, IJsonFormatterReso
reader.ReadNext(); // ,
reader.ReadNext(); // "key"
reader.ReadNext(); // :
var key = reader.ReadNullableLong().GetValueOrDefault(0);
var key = reader.ReadInt64();
Copy link
Contributor

Choose a reason for hiding this comment

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

No longer nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an artifact of porting over from Json.NET, where the conversion is made to long? - I don't believe nullability it's actually needed here, based on

https://github.com/elastic/elasticsearch/blob/daa2ec8a605d385a65b9ab3e89d016b3fd0dffe2/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalDateHistogram.java#L114-L116

reader.ReadNext(); // ,
reader.ReadNext(); // "doc_count"
reader.ReadNext(); // :
var docCount = reader.ReadNullableLong().GetValueOrDefault(0);
reader.ReadNext(); // ,
var docCount = reader.ReadInt64();
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, no longer nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to previous comment, this is an artifact of porting over from Json.NET. Based on

https://github.com/elastic/elasticsearch/blob/daa2ec8a605d385a65b9ab3e89d016b3fd0dffe2/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/InternalDateHistogram.java#L119-L121

there is always a long value for doc count.

@russcam russcam merged commit de61eb2 into master Apr 29, 2019
russcam added a commit that referenced this pull request May 3, 2019
@Mpdreamz Mpdreamz deleted the fix/3673 branch June 17, 2019 12:05
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.

7.0.0 - Exception when aggregating dates
2 participants