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

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

Merged
merged 4 commits into from
Jan 23, 2018

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Jan 19, 2018

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
@jimczi 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 jimczi added v6.3.0 and removed v6.2.0 labels Jan 22, 2018
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed 2051507 to avoid writing the size twice

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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed 2051507

for (int i = 0; i < numFields; i++) {
sourceNames.add("field_" + i);
formats.add(DocValueFormat.RAW);
Copy link
Member

Choose a reason for hiding this comment

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

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

@cbuescher cbuescher self-assigned this Jan 22, 2018
@jimczi
Copy link
Contributor Author

jimczi commented Jan 22, 2018

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

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

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

@jimczi jimczi merged commit 19cfc25 into elastic:master Jan 23, 2018
@jimczi 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
…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
jimczi added a commit that referenced this pull request Jan 24, 2018
jimczi added a commit that referenced this pull request Jan 24, 2018
martijnvg added a commit that referenced this pull request Jan 24, 2018
* 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)
jasontedor added a commit to matarrese/elasticsearch that referenced this pull request Jan 24, 2018
* 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)
  ...
martijnvg added a commit that referenced this pull request Jan 25, 2018
* 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
martijnvg added a commit that referenced this pull request Jan 25, 2018
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants