Skip to content

Commit 4ff314a

Browse files
nik9000nik9000
authored
Begin moving date_histogram to offset rounding (take two) (#51271) (#51495)
We added a new rounding in #50609 that handles offsets to the start and end of the rounding so that we could support `offset` in the `composite` aggregation. This starts moving `date_histogram` to that new offset. This is a redo of #50873 with more integration tests. This reverts commit d114c9d.
1 parent faf6bb2 commit 4ff314a

File tree

10 files changed

+289
-35
lines changed

10 files changed

+289
-35
lines changed

‎rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/10_histogram.yml

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,3 +196,214 @@ setup:
196196
- match: { aggregations.histo.buckets.2.key_as_string: "2016-01-01T00:00:00.000Z" }
197197

198198
- match: { aggregations.histo.buckets.2.doc_count: 2 }
199+
200+
---
201+
"date_histogram":
202+
- skip:
203+
version: " - 7.1.99"
204+
reason: calendar_interval introduced in 7.2.0
205+
206+
- do:
207+
indices.create:
208+
index: test_2
209+
body:
210+
settings:
211+
# There was a BWC issue that only showed up on empty shards. This
212+
# test has 4 docs and 5 shards makes sure we get one empty.
213+
number_of_shards: 5
214+
mappings:
215+
properties:
216+
date:
217+
type : date
218+
219+
- do:
220+
bulk:
221+
index: test_2
222+
refresh: true
223+
body:
224+
- '{"index": {}}'
225+
- '{"date": "2016-01-01"}'
226+
- '{"index": {}}'
227+
- '{"date": "2016-01-02"}'
228+
- '{"index": {}}'
229+
- '{"date": "2016-02-01"}'
230+
- '{"index": {}}'
231+
- '{"date": "2016-03-01"}'
232+
233+
- do:
234+
search:
235+
body:
236+
size: 0
237+
aggs:
238+
histo:
239+
date_histogram:
240+
field: date
241+
calendar_interval: month
242+
243+
- match: { hits.total.value: 4 }
244+
- length: { aggregations.histo.buckets: 3 }
245+
- match: { aggregations.histo.buckets.0.key_as_string: "2016-01-01T00:00:00.000Z" }
246+
- match: { aggregations.histo.buckets.0.doc_count: 2 }
247+
- match: { aggregations.histo.buckets.1.key_as_string: "2016-02-01T00:00:00.000Z" }
248+
- match: { aggregations.histo.buckets.1.doc_count: 1 }
249+
- match: { aggregations.histo.buckets.2.key_as_string: "2016-03-01T00:00:00.000Z" }
250+
- match: { aggregations.histo.buckets.2.doc_count: 1 }
251+
252+
---
253+
"date_histogram with offset":
254+
- skip:
255+
version: " - 7.1.99"
256+
reason: calendar_interval introduced in 7.2.0
257+
258+
- do:
259+
indices.create:
260+
index: test_2
261+
body:
262+
settings:
263+
# There was a BWC issue that only showed up on empty shards. This
264+
# test has 4 docs and 5 shards makes sure we get one empty.
265+
number_of_shards: 5
266+
mappings:
267+
properties:
268+
date:
269+
type : date
270+
271+
- do:
272+
bulk:
273+
index: test_2
274+
refresh: true
275+
body:
276+
- '{"index": {}}'
277+
- '{"date": "2016-01-01"}'
278+
- '{"index": {}}'
279+
- '{"date": "2016-01-02"}'
280+
- '{"index": {}}'
281+
- '{"date": "2016-02-01"}'
282+
- '{"index": {}}'
283+
- '{"date": "2016-03-01"}'
284+
285+
- do:
286+
search:
287+
body:
288+
size: 0
289+
aggs:
290+
histo:
291+
date_histogram:
292+
field: date
293+
calendar_interval: month
294+
offset: +1d
295+
296+
- match: { hits.total.value: 4 }
297+
- length: { aggregations.histo.buckets: 3 }
298+
- match: { aggregations.histo.buckets.0.key_as_string: "2015-12-02T00:00:00.000Z" }
299+
- match: { aggregations.histo.buckets.0.doc_count: 1 }
300+
- match: { aggregations.histo.buckets.1.key_as_string: "2016-01-02T00:00:00.000Z" }
301+
- match: { aggregations.histo.buckets.1.doc_count: 2 }
302+
- match: { aggregations.histo.buckets.2.key_as_string: "2016-02-02T00:00:00.000Z" }
303+
- match: { aggregations.histo.buckets.2.doc_count: 1 }
304+
305+
306+
---
307+
"date_histogram on range":
308+
- skip:
309+
version: " - 7.1.99"
310+
reason: calendar_interval introduced in 7.2.0
311+
312+
- do:
313+
indices.create:
314+
index: test_2
315+
body:
316+
settings:
317+
# There was a BWC issue that only showed up on empty shards. This
318+
# test has 4 docs and 5 shards makes sure we get one empty.
319+
number_of_shards: 5
320+
mappings:
321+
properties:
322+
range:
323+
type : date_range
324+
325+
- do:
326+
bulk:
327+
index: test_2
328+
refresh: true
329+
body:
330+
- '{"index": {}}'
331+
- '{"range": {"gte": "2016-01-01", "lt": "2016-01-02"}}'
332+
- '{"index": {}}'
333+
- '{"range": {"gte": "2016-01-02", "lt": "2016-01-03"}}'
334+
- '{"index": {}}'
335+
- '{"range": {"gte": "2016-02-01", "lt": "2016-02-02"}}'
336+
- '{"index": {}}'
337+
- '{"range": {"gte": "2016-03-01", "lt": "2016-03-02"}}'
338+
339+
- do:
340+
search:
341+
body:
342+
size: 0
343+
aggs:
344+
histo:
345+
date_histogram:
346+
field: range
347+
calendar_interval: month
348+
349+
- match: { hits.total.value: 4 }
350+
- length: { aggregations.histo.buckets: 3 }
351+
- match: { aggregations.histo.buckets.0.key_as_string: "2016-01-01T00:00:00.000Z" }
352+
- match: { aggregations.histo.buckets.0.doc_count: 2 }
353+
- match: { aggregations.histo.buckets.1.key_as_string: "2016-02-01T00:00:00.000Z" }
354+
- match: { aggregations.histo.buckets.1.doc_count: 1 }
355+
- match: { aggregations.histo.buckets.2.key_as_string: "2016-03-01T00:00:00.000Z" }
356+
- match: { aggregations.histo.buckets.2.doc_count: 1 }
357+
358+
---
359+
"date_histogram on range with offset":
360+
- skip:
361+
version: " - 7.1.99"
362+
reason: calendar_interval introduced in 7.2.0
363+
364+
- do:
365+
indices.create:
366+
index: test_2
367+
body:
368+
settings:
369+
# There was a BWC issue that only showed up on empty shards. This
370+
# test has 4 docs and 5 shards makes sure we get one empty.
371+
number_of_shards: 5
372+
mappings:
373+
properties:
374+
range:
375+
type : date_range
376+
377+
- do:
378+
bulk:
379+
index: test_2
380+
refresh: true
381+
body:
382+
- '{"index": {}}'
383+
- '{"range": {"gte": "2016-01-01", "lt": "2016-01-02"}}'
384+
- '{"index": {}}'
385+
- '{"range": {"gte": "2016-01-02", "lt": "2016-01-03"}}'
386+
- '{"index": {}}'
387+
- '{"range": {"gte": "2016-02-01", "lt": "2016-02-02"}}'
388+
- '{"index": {}}'
389+
- '{"range": {"gte": "2016-03-01", "lt": "2016-03-02"}}'
390+
391+
- do:
392+
search:
393+
body:
394+
size: 0
395+
aggs:
396+
histo:
397+
date_histogram:
398+
field: range
399+
calendar_interval: month
400+
offset: +1d
401+
402+
- match: { hits.total.value: 4 }
403+
- length: { aggregations.histo.buckets: 3 }
404+
- match: { aggregations.histo.buckets.0.key_as_string: "2015-12-02T00:00:00.000Z" }
405+
- match: { aggregations.histo.buckets.0.doc_count: 1 }
406+
- match: { aggregations.histo.buckets.1.key_as_string: "2016-01-02T00:00:00.000Z" }
407+
- match: { aggregations.histo.buckets.1.doc_count: 2 }
408+
- match: { aggregations.histo.buckets.2.key_as_string: "2016-02-02T00:00:00.000Z" }
409+
- match: { aggregations.histo.buckets.2.doc_count: 1 }

‎server/src/main/java/org/elasticsearch/common/Rounding.java

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,19 @@ public void writeTo(StreamOutput out) throws IOException {
164164
*/
165165
public abstract long nextRoundingValue(long value);
166166

167+
/**
168+
* How "offset" this rounding is from the traditional "start" of the period.
169+
* @deprecated We're in the process of abstracting offset *into* Rounding
170+
* so keep any usage to migratory shims
171+
*/
172+
@Deprecated
173+
public abstract long offset();
174+
175+
/**
176+
* Strip the {@code offset} from these bounds.
177+
*/
178+
public abstract Rounding withoutOffset();
179+
167180
@Override
168181
public abstract boolean equals(Object obj);
169182

@@ -425,6 +438,16 @@ public long nextRoundingValue(long utcMillis) {
425438
}
426439
}
427440

441+
@Override
442+
public long offset() {
443+
return 0;
444+
}
445+
446+
@Override
447+
public Rounding withoutOffset() {
448+
return this;
449+
}
450+
428451
@Override
429452
public int hashCode() {
430453
return Objects.hash(unit, timeZone);
@@ -556,6 +579,16 @@ public long nextRoundingValue(long time) {
556579
.toInstant().toEpochMilli();
557580
}
558581

582+
@Override
583+
public long offset() {
584+
return 0;
585+
}
586+
587+
@Override
588+
public Rounding withoutOffset() {
589+
return this;
590+
}
591+
559592
@Override
560593
public int hashCode() {
561594
return Objects.hash(interval, timeZone);
@@ -617,8 +650,17 @@ public long round(long value) {
617650

618651
@Override
619652
public long nextRoundingValue(long value) {
620-
// This isn't needed by the current users. We'll implement it when we migrate other users to it.
621-
throw new UnsupportedOperationException("not yet supported");
653+
return delegate.nextRoundingValue(value - offset) + offset;
654+
}
655+
656+
@Override
657+
public long offset() {
658+
return offset;
659+
}
660+
661+
@Override
662+
public Rounding withoutOffset() {
663+
return delegate;
622664
}
623665

624666
@Override

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregationBuilder.java

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -499,22 +499,21 @@ protected ValuesSourceAggregatorFactory<ValuesSource> innerBuild(QueryShardConte
499499
AggregatorFactory parent,
500500
Builder subFactoriesBuilder) throws IOException {
501501
final ZoneId tz = timeZone();
502-
// TODO use offset here rather than explicitly in the aggregation
503-
final Rounding rounding = dateHistogramInterval.createRounding(tz, 0);
502+
final Rounding rounding = dateHistogramInterval.createRounding(tz, offset);
504503
final ZoneId rewrittenTimeZone = rewriteTimeZone(queryShardContext);
505504
final Rounding shardRounding;
506505
if (tz == rewrittenTimeZone) {
507506
shardRounding = rounding;
508507
} else {
509-
shardRounding = dateHistogramInterval.createRounding(rewrittenTimeZone, 0);
508+
shardRounding = dateHistogramInterval.createRounding(rewrittenTimeZone, offset);
510509
}
511510

512511
ExtendedBounds roundedBounds = null;
513512
if (this.extendedBounds != null) {
514513
// parse any string bounds to longs and round
515514
roundedBounds = this.extendedBounds.parseAndValidate(name, queryShardContext, config.format()).round(rounding);
516515
}
517-
return new DateHistogramAggregatorFactory(name, config, offset, order, keyed, minDocCount,
516+
return new DateHistogramAggregatorFactory(name, config, order, keyed, minDocCount,
518517
rounding, shardRounding, roundedBounds, queryShardContext, parent, subFactoriesBuilder, metaData);
519518
}
520519

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramAggregator.java

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -64,18 +64,16 @@ class DateHistogramAggregator extends BucketsAggregator {
6464
private final ExtendedBounds extendedBounds;
6565

6666
private final LongHash bucketOrds;
67-
private long offset;
6867

6968
DateHistogramAggregator(String name, AggregatorFactories factories, Rounding rounding, Rounding shardRounding,
70-
long offset, BucketOrder order, boolean keyed,
69+
BucketOrder order, boolean keyed,
7170
long minDocCount, @Nullable ExtendedBounds extendedBounds, @Nullable ValuesSource.Numeric valuesSource,
7271
DocValueFormat formatter, SearchContext aggregationContext,
7372
Aggregator parent, List<PipelineAggregator> pipelineAggregators, Map<String, Object> metaData) throws IOException {
7473

7574
super(name, factories, aggregationContext, parent, pipelineAggregators, metaData);
7675
this.rounding = rounding;
7776
this.shardRounding = shardRounding;
78-
this.offset = offset;
7977
this.order = InternalOrder.validate(order, this);
8078
this.keyed = keyed;
8179
this.minDocCount = minDocCount;
@@ -113,7 +111,7 @@ public void collect(int doc, long bucket) throws IOException {
113111
long value = values.nextValue();
114112
// We can use shardRounding here, which is sometimes more efficient
115113
// if daylight saving times are involved.
116-
long rounded = shardRounding.round(value - offset) + offset;
114+
long rounded = shardRounding.round(value);
117115
assert rounded >= previousRounded;
118116
if (rounded == previousRounded) {
119117
continue;
@@ -148,9 +146,9 @@ public InternalAggregation buildAggregation(long owningBucketOrdinal) throws IOE
148146
// value source will be null for unmapped fields
149147
// Important: use `rounding` here, not `shardRounding`
150148
InternalDateHistogram.EmptyBucketInfo emptyBucketInfo = minDocCount == 0
151-
? new InternalDateHistogram.EmptyBucketInfo(rounding, buildEmptySubAggregations(), extendedBounds)
149+
? new InternalDateHistogram.EmptyBucketInfo(rounding.withoutOffset(), buildEmptySubAggregations(), extendedBounds)
152150
: null;
153-
return new InternalDateHistogram(name, buckets, order, minDocCount, offset, emptyBucketInfo, formatter, keyed,
151+
return new InternalDateHistogram(name, buckets, order, minDocCount, rounding.offset(), emptyBucketInfo, formatter, keyed,
154152
pipelineAggregators(), metaData());
155153
}
156154

@@ -159,8 +157,8 @@ public InternalAggregation buildEmptyAggregation() {
159157
InternalDateHistogram.EmptyBucketInfo emptyBucketInfo = minDocCount == 0
160158
? new InternalDateHistogram.EmptyBucketInfo(rounding, buildEmptySubAggregations(), extendedBounds)
161159
: null;
162-
return new InternalDateHistogram(name, Collections.emptyList(), order, minDocCount, offset, emptyBucketInfo, formatter, keyed,
163-
pipelineAggregators(), metaData());
160+
return new InternalDateHistogram(name, Collections.emptyList(), order, minDocCount, rounding.offset(), emptyBucketInfo, formatter,
161+
keyed, pipelineAggregators(), metaData());
164162
}
165163

166164
@Override

0 commit comments

Comments
 (0)