Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add interval response parameter to AutoDateInterval histogram #33254

Merged

Conversation

@pcsanwald
Copy link
Contributor

commented Aug 30, 2018

The goal of this PR is to add an interval parameter at the top level of the AutoIntervalHistogram. This allows clients to know what interval was used for the aggregation without comparing bucket key values.

The one bit I could use some advice on is the best way to persist this field onto ParsedAutoDateHistogram for HL REST client support: it's not clear to me how to add this for a response only field (i.e. one that isn't present on the AggregationBuilder)

Here's what the new response looks like:

	"aggregations": {
		"postal_codes": {
			"buckets": [{
				"key_as_string": "2017-01-01T00:00:00.000Z",
				"key": 1483228800000,
				"doc_count": 1718
			}, {
				"key_as_string": "2018-01-01T00:00:00.000Z",
				"key": 1514764800000,
				"doc_count": 3282
			}],
			"interval": "1y"
		}
	}
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Aug 30, 2018

Pinging @elastic/es-search-aggs

@pcsanwald pcsanwald changed the title Add interval to AutoDateInterval histogram Add interval response parameter to AutoDateInterval histogram Aug 30, 2018

@colings86
Copy link
Member

left a comment

@pcsanwald I left a suggestion about how we might be able to achieve this without recalculating the interval int he getter method

}
}
return new DateHistogramInterval(Integer.toString(roundingInfo.innerIntervals[0]) + unitAbbreviation);
}

This comment has been minimized.

Copy link
@colings86

colings86 Aug 30, 2018

Member

I'm not sure we should calculate the interval here as we could end up with bugs where this is different from the actual interval used. Instead could we have a field for the interval store in this class which is populated int he AutoDateHistogramAggregator.buildAggregation() and also in doReduce() when we build the new instance following a reduce? I think we will have all the information we need in both those places?

@colings86
Copy link
Member

left a comment

LGTM

@pcsanwald pcsanwald added backport pending and removed WIP labels Sep 5, 2018

@pcsanwald pcsanwald merged commit c303006 into elastic:master Sep 5, 2018

4 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@pcsanwald pcsanwald deleted the pcsanwald:add-interval-to-autointerval-histogram branch Sep 5, 2018

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 5, 2018
Merge branch 'master' into remote-cluster
* master:
  Fix deprecated setting specializations (elastic#33412)
  HLRC: split cluster request converters (elastic#33400)
  HLRC: Add ML get influencers API (elastic#33389)
  Add conditional token filter to elasticsearch (elastic#31958)
  Build: Merge xpack checkstyle config into core (elastic#33399)
  Disable IndexRecoveryIT.testRerouteRecovery.
  INGEST: Implement Drop Processor (elastic#32278)
  [ML] Add field stats to log structure finder (elastic#33351)
  Add interval response parameter to AutoDateInterval histogram (elastic#33254)
  MINOR+CORE: Remove Dead Methods ClusterService (elastic#33346)
pcsanwald added a commit to pcsanwald/elasticsearch that referenced this pull request Sep 5, 2018
Add interval response parameter to AutoDateInterval histogram (elasti…
…c#33254)

Adds the interval used to the aggregation response.
@pcsanwald

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

closes #32102

pcsanwald added a commit that referenced this pull request Sep 6, 2018
Add interval response parameter to AutoDateInterval histogram (#33254) (
#33437)

Adds the interval used to the aggregation response.

@pcsanwald pcsanwald added the v6.5.0 label Sep 6, 2018

@Mpdreamz Mpdreamz referenced this pull request Dec 13, 2018
61 of 107 tasks complete

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

@pcsanwald pcsanwald restored the pcsanwald:add-interval-to-autointerval-histogram branch Jun 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.