Adds the ability to specify a format on composite date_histogram source by jimczi · Pull Request #28310 · elastic/elasticsearch

jimczi · 2018-01-19T11:30:02Z

This commit adds the ability to specify a date format on the date_histogram composite source.
If the format is defined, the key for the source is returned as a formatted date.

Closes #27923

This commit adds the ability to specify a date format on the `date_histogram` composite source. If the format is defined, the key for the source is returned as a formatted date. Closes elastic#27923

cbuescher

@jimczi I had a great time looking at this PR and trying to undestand a bit more about composite aggs. I left a few minor comments, mostly questions though.

cbuescher · 2018-01-22T14:45:05Z

.../src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java

                      List<PipelineAggregator> pipelineAggregators, Map<String, Object> metaData) {
        super(name, pipelineAggregators, metaData);
        this.sourceNames = sourceNames;
+        this.formats = formats;


I saw in the constructor in CompositeAggregator that sorceNames and formats always need to be of same size, but still needed to re-check when looking at this classes serialization/deserialization. Maybe it would make sense to add a simple assert for this invariant here in the constructor to make this more obvious and also detect potential future violations of this.

I pushed 2051507 to avoid writing the size twice

cbuescher · 2018-01-22T14:46:12Z

.../src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java

+        if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
+            this.formats = in.readNamedWriteableList(DocValueFormat.class);
+        } else {
+            this.formats = new ArrayList<>(sourceNames.size());


I think this is where I needed to pause for a second to check that formats/sourceNames are always same size, since we don't write the formats length to the stream this wasn't immediately obvious for me. Maybe worth a small comment.

cbuescher · 2018-01-22T15:07:55Z

...st/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java

+                    .dateHistogramInterval(DateHistogramInterval.days(1))
+                    .format("yyyy-MM-dd");
+                return new CompositeAggregationBuilder("name", Collections.singletonList(histo))
+                    .aggregateAfter(createAfterKey("date", 1474329600000L));


Reading this I have a short question: does the after parameter in the rest API support formated dates now? Or would users have to get the raw value to be able to use it?

Also for understanding this test better, adding which date the long here refers to in a comment would help, although it can be infered from the result.

Good catch, thanks, we should only allow formatted after key here. So the format should be a string: "2016-09-20". I'll fix this

I pushed 2051507

cbuescher · 2018-01-22T15:15:06Z

...test/java/org/elasticsearch/search/aggregations/bucket/composite/InternalCompositeTests.java

+        types = new int[numFields];
        for (int i = 0; i < numFields; i++) {
            sourceNames.add("field_" + i);
+            formats.add(DocValueFormat.RAW);


Maybe also randomize this? It might not be necessary though.

jimczi · 2018-01-22T16:22:20Z

Thanks @cbuescher I pushed a commit to address your comments.

cbuescher

@jimczi thanks for the additional changes, ready to go once CI is green I think

…ce (#28310) This commit adds the ability to specify a date format on the `date_histogram` composite source. If the format is defined, the key for the source is returned as a formatted date. Closes #27923

* es/master: Remove redundant argument for buildConfiguration of s3 plugin (#28281) Completely remove Painless Type from AnalyzerCaster in favor of Java Class. (#28329) Fix spelling error Reindex: Wait for deletion in test Reindex: log more on rare test failure Ensure we protect Collections obtained from scripts from self-referencing (#28335) [Docs] Fix asciidoc style in composite agg docs Adds the ability to specify a format on composite date_histogram source (#28310) Provide a better error message for the case when all shards failed (#28333) [Test] Re-Add integer_range and date_range field types for query builder tests (#28171) Added Put Mapping API to high-level Rest client (#27869) Revert change that does not return all indices if a specific alias is requested via get alias api. (#28294) Painless: Replace Painless Type with Java Class during Casts (#27847) Notify affixMap settings when any under the registered prefix matches (#28317)

* master: (94 commits) Completely remove Painless Type from AnalyzerCaster in favor of Java Class. (elastic#28329) Fix spelling error Reindex: Wait for deletion in test Reindex: log more on rare test failure Ensure we protect Collections obtained from scripts from self-referencing (elastic#28335) [Docs] Fix asciidoc style in composite agg docs Adds the ability to specify a format on composite date_histogram source (elastic#28310) Provide a better error message for the case when all shards failed (elastic#28333) [Test] Re-Add integer_range and date_range field types for query builder tests (elastic#28171) Added Put Mapping API to high-level Rest client (elastic#27869) Revert change that does not return all indices if a specific alias is requested via get alias api. (elastic#28294) Painless: Replace Painless Type with Java Class during Casts (elastic#27847) Notify affixMap settings when any under the registered prefix matches (elastic#28317) Trim down usages of `ShardOperationFailedException` interface (elastic#28312) Do not return all indices if a specific alias is requested via get aliases api. [Test] Lower bwc version for rank-eval rest tests CountedBitSet doesn't need to extend BitSet. (elastic#28239) Calculate sum in Kahan summation algorithm in aggregations (elastic#27807) (elastic#27848) Remove the `update_all_types` option. (elastic#28288) Add information when master node left to DiscoveryNodes' shortSummary() (elastic#28197) ...

* es/master: [Docs] Fix explanation for `from` and `size` example (#28320) Adapt bwc version after backport #28358 Always return the after_key in composite aggregation response (#28358) Adds test name to MockPageCacheRecycler exception (#28359) Adds a note in the `terms` aggregation docs regarding pagination (#28360) [Test] Fix DiscoveryNodesTests.testDeltas() (#28361) Update packaging tests to work with meta plugins (#28336) Remove Painless Type from MethodWriter in favor of Java Class. (#28346) [Doc] Fixs typo in reverse-nested-aggregation.asciidoc (#28348) Reindex: Shore up rethrottle test Only assert single commit iff index created on 6.2 isHeldByCurrentThread should return primitive bool [Docs] Clarify `html` encoder in highlighting.asciidoc (#27766) Fix GeoDistance query example (#28355) Settings: Introduce settings updater for a list of settings (#28338) Adapt bwc version after backport #28310

* es/6.x: [Docs] Fix explanation for `from` and `size` example (#28320) Adapt bwc version after backport #28358 Always return the after_key in composite aggregation response (#28358) Adds a note in the `terms` aggregation docs regarding pagination (#28360) Update packaging tests to work with meta plugins (#28336) Remove Painless Type from MethodWriter in favor of Java Class. (#28346) [Doc] Fixs typo in reverse-nested-aggregation.asciidoc (#28348) [Docs] Fixed Indices information breaking changes (#27914) Reindex: Shore up rethrottle test isHeldByCurrentThread should return primitive bool [Docs] Clarify `html` encoder in highlighting.asciidoc (#27766) Only assert single commit iff index created on 6.2 Deprecate the `update_all_types` option. (#28284) Fix GeoDistance query example Settings: Introduce settings updater for a list of settings (#28338) [Docs] Fix asciidoc style in composite agg docs Adapt bwc version after backport #28310 Adds the ability to specify a format on composite date_histogram source (#28310)

Adds the ability to specify a format on date_histogram source

9ca68af

This commit adds the ability to specify a date format on the `date_histogram` composite source. If the format is defined, the key for the source is returned as a formatted date. Closes elastic#27923

jimczi added >feature :Analytics/Aggregations Aggregations v7.0.0 v6.2.0 labels Jan 19, 2018

fix serialization tests

360a28a

jimczi changed the title ~~Adds the ability to specify a format on date_histogram source~~ Adds the ability to specify a format on composite date_histogram source Jan 19, 2018

jimczi added v6.3.0 and removed v6.2.0 labels Jan 22, 2018

cbuescher reviewed Jan 22, 2018

View reviewed changes

cbuescher self-assigned this Jan 22, 2018

Handle after key with formatted date histogram + address reviews

2051507

add rest test for the new option

d1a9447

cbuescher approved these changes Jan 23, 2018

View reviewed changes

jimczi merged commit 19cfc25 into elastic:master Jan 23, 2018

jimczi deleted the enhancements/composite_date_format branch January 23, 2018 14:14

jimczi added a commit that referenced this pull request Jan 24, 2018

Adapt bwc version after backport #28310

9bf6597

jimczi added a commit that referenced this pull request Jan 24, 2018

Adapt bwc version after backport #28310

b10d166

russcam mentioned this pull request Jul 9, 2018

Add date format to DateHistogram source for Composite Aggregation elastic/elasticsearch-net#3317

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the ability to specify a format on composite date_histogram source#28310

Adds the ability to specify a format on composite date_histogram source#28310
jimczi merged 4 commits intoelastic:masterfrom
jimczi:enhancements/composite_date_format

jimczi commented Jan 19, 2018

Uh oh!

cbuescher left a comment

Uh oh!

cbuescher Jan 22, 2018

Uh oh!

jimczi Jan 22, 2018

Uh oh!

cbuescher Jan 22, 2018

Uh oh!

cbuescher Jan 22, 2018

Uh oh!

cbuescher Jan 22, 2018

Uh oh!

jimczi Jan 22, 2018

Uh oh!

jimczi Jan 22, 2018

Uh oh!

cbuescher Jan 22, 2018

Uh oh!

jimczi commented Jan 22, 2018

Uh oh!

cbuescher left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jimczi commented Jan 19, 2018

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimczi commented Jan 22, 2018

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants