Skip to content

Commit e232495

Browse files
nik9000nik9000
authored
Revert "Begin moving date_histogram to offset rounding (backport of #50873) (#50978)" (#51240)
This reverts commit 9a3d4db. It was subtly broken in ways we don't have tests for.
1 parent d6cf64d commit e232495

File tree

10 files changed

+33
-77
lines changed

10 files changed

+33
-77
lines changed

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

Lines changed: 2 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -164,19 +164,6 @@ 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-
180167
@Override
181168
public abstract boolean equals(Object obj);
182169

@@ -438,16 +425,6 @@ public long nextRoundingValue(long utcMillis) {
438425
}
439426
}
440427

441-
@Override
442-
public long offset() {
443-
return 0;
444-
}
445-
446-
@Override
447-
public Rounding withoutOffset() {
448-
return this;
449-
}
450-
451428
@Override
452429
public int hashCode() {
453430
return Objects.hash(unit, timeZone);
@@ -579,16 +556,6 @@ public long nextRoundingValue(long time) {
579556
.toInstant().toEpochMilli();
580557
}
581558

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

651618
@Override
652619
public long nextRoundingValue(long value) {
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;
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");
664622
}
665623

666624
@Override

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -500,21 +500,21 @@ protected ValuesSourceAggregatorFactory<ValuesSource> innerBuild(QueryShardConte
500500
Builder subFactoriesBuilder) throws IOException {
501501
final ZoneId tz = timeZone();
502502
// TODO use offset here rather than explicitly in the aggregation
503-
final Rounding rounding = dateHistogramInterval.createRounding(tz, offset);
503+
final Rounding rounding = dateHistogramInterval.createRounding(tz, 0);
504504
final ZoneId rewrittenTimeZone = rewriteTimeZone(queryShardContext);
505505
final Rounding shardRounding;
506506
if (tz == rewrittenTimeZone) {
507507
shardRounding = rounding;
508508
} else {
509-
shardRounding = dateHistogramInterval.createRounding(rewrittenTimeZone, offset);
509+
shardRounding = dateHistogramInterval.createRounding(rewrittenTimeZone, 0);
510510
}
511511

512512
ExtendedBounds roundedBounds = null;
513513
if (this.extendedBounds != null) {
514514
// parse any string bounds to longs and round
515515
roundedBounds = this.extendedBounds.parseAndValidate(name, queryShardContext, config.format()).round(rounding);
516516
}
517-
return new DateHistogramAggregatorFactory(name, config, order, keyed, minDocCount,
517+
return new DateHistogramAggregatorFactory(name, config, offset, order, keyed, minDocCount,
518518
rounding, shardRounding, roundedBounds, queryShardContext, parent, subFactoriesBuilder, metaData);
519519
}
520520

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

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

6666
private final LongHash bucketOrds;
67+
private long offset;
6768

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

7475
super(name, factories, aggregationContext, parent, pipelineAggregators, metaData);
7576
this.rounding = rounding;
7677
this.shardRounding = shardRounding;
78+
this.offset = offset;
7779
this.order = InternalOrder.validate(order, this);
7880
this.keyed = keyed;
7981
this.minDocCount = minDocCount;
@@ -111,7 +113,7 @@ public void collect(int doc, long bucket) throws IOException {
111113
long value = values.nextValue();
112114
// We can use shardRounding here, which is sometimes more efficient
113115
// if daylight saving times are involved.
114-
long rounded = shardRounding.round(value);
116+
long rounded = shardRounding.round(value - offset) + offset;
115117
assert rounded >= previousRounded;
116118
if (rounded == previousRounded) {
117119
continue;
@@ -148,7 +150,7 @@ public InternalAggregation buildAggregation(long owningBucketOrdinal) throws IOE
148150
InternalDateHistogram.EmptyBucketInfo emptyBucketInfo = minDocCount == 0
149151
? new InternalDateHistogram.EmptyBucketInfo(rounding, buildEmptySubAggregations(), extendedBounds)
150152
: null;
151-
return new InternalDateHistogram(name, buckets, order, minDocCount, rounding.offset(), emptyBucketInfo, formatter, keyed,
153+
return new InternalDateHistogram(name, buckets, order, minDocCount, offset, emptyBucketInfo, formatter, keyed,
152154
pipelineAggregators(), metaData());
153155
}
154156

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

164166
@Override

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
public final class DateHistogramAggregatorFactory
4040
extends ValuesSourceAggregatorFactory<ValuesSource> {
4141

42+
private final long offset;
4243
private final BucketOrder order;
4344
private final boolean keyed;
4445
private final long minDocCount;
@@ -47,11 +48,12 @@ public final class DateHistogramAggregatorFactory
4748
private final Rounding shardRounding;
4849

4950
public DateHistogramAggregatorFactory(String name, ValuesSourceConfig<ValuesSource> config,
50-
BucketOrder order, boolean keyed, long minDocCount,
51+
long offset, BucketOrder order, boolean keyed, long minDocCount,
5152
Rounding rounding, Rounding shardRounding, ExtendedBounds extendedBounds, QueryShardContext queryShardContext,
5253
AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder,
5354
Map<String, Object> metaData) throws IOException {
5455
super(name, config, queryShardContext, parent, subFactoriesBuilder, metaData);
56+
this.offset = offset;
5557
this.order = order;
5658
this.keyed = keyed;
5759
this.minDocCount = minDocCount;
@@ -102,7 +104,7 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource,
102104
private Aggregator createAggregator(ValuesSource.Numeric valuesSource, SearchContext searchContext,
103105
Aggregator parent, List<PipelineAggregator> pipelineAggregators,
104106
Map<String, Object> metaData) throws IOException {
105-
return new DateHistogramAggregator(name, factories, rounding, shardRounding, order, keyed, minDocCount, extendedBounds,
107+
return new DateHistogramAggregator(name, factories, rounding, shardRounding, offset, order, keyed, minDocCount, extendedBounds,
106108
valuesSource, config.format(), searchContext, parent, pipelineAggregators, metaData);
107109
}
108110

@@ -111,7 +113,7 @@ private Aggregator createRangeAggregator(ValuesSource.Range valuesSource,
111113
Aggregator parent,
112114
List<PipelineAggregator> pipelineAggregators,
113115
Map<String, Object> metaData) throws IOException {
114-
return new DateRangeHistogramAggregator(name, factories, rounding, shardRounding, order, keyed, minDocCount, extendedBounds,
116+
return new DateRangeHistogramAggregator(name, factories, rounding, shardRounding, offset, order, keyed, minDocCount, extendedBounds,
115117
valuesSource, config.format(), searchContext, parent, pipelineAggregators, metaData);
116118
}
117119

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

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,10 @@ class DateRangeHistogramAggregator extends BucketsAggregator {
6767
private final ExtendedBounds extendedBounds;
6868

6969
private final LongHash bucketOrds;
70+
private long offset;
7071

7172
DateRangeHistogramAggregator(String name, AggregatorFactories factories, Rounding rounding, Rounding shardRounding,
72-
BucketOrder order, boolean keyed,
73+
long offset, BucketOrder order, boolean keyed,
7374
long minDocCount, @Nullable ExtendedBounds extendedBounds, @Nullable ValuesSource.Range valuesSource,
7475
DocValueFormat formatter, SearchContext aggregationContext,
7576
Aggregator parent, List<PipelineAggregator> pipelineAggregators,
@@ -78,6 +79,7 @@ class DateRangeHistogramAggregator extends BucketsAggregator {
7879
super(name, factories, aggregationContext, parent, pipelineAggregators, metaData);
7980
this.rounding = rounding;
8081
this.shardRounding = shardRounding;
82+
this.offset = offset;
8183
this.order = InternalOrder.validate(order, this);
8284
this.keyed = keyed;
8385
this.minDocCount = minDocCount;
@@ -124,8 +126,8 @@ public void collect(int doc, long bucket) throws IOException {
124126
// The encoding should ensure that this assert is always true.
125127
assert from >= previousFrom : "Start of range not >= previous start";
126128
final Long to = (Long) range.getTo();
127-
final long startKey = shardRounding.round(from);
128-
final long endKey = shardRounding.round(to);
129+
final long startKey = offsetAwareRounding(shardRounding, from, offset);
130+
final long endKey = offsetAwareRounding(shardRounding, to, offset);
129131
for (long key = startKey > previousKey ? startKey : previousKey; key <= endKey;
130132
key = shardRounding.nextRoundingValue(key)) {
131133
if (key == previousKey) {
@@ -151,6 +153,10 @@ public void collect(int doc, long bucket) throws IOException {
151153
};
152154
}
153155

156+
private long offsetAwareRounding(Rounding rounding, long value, long offset) {
157+
return rounding.round(value - offset) + offset;
158+
}
159+
154160
@Override
155161
public InternalAggregation buildAggregation(long owningBucketOrdinal) throws IOException {
156162
assert owningBucketOrdinal == 0;
@@ -169,7 +175,7 @@ public InternalAggregation buildAggregation(long owningBucketOrdinal) throws IOE
169175
InternalDateHistogram.EmptyBucketInfo emptyBucketInfo = minDocCount == 0
170176
? new InternalDateHistogram.EmptyBucketInfo(rounding, buildEmptySubAggregations(), extendedBounds)
171177
: null;
172-
return new InternalDateHistogram(name, buckets, order, minDocCount, rounding.offset(), emptyBucketInfo, formatter, keyed,
178+
return new InternalDateHistogram(name, buckets, order, minDocCount, offset, emptyBucketInfo, formatter, keyed,
173179
pipelineAggregators(), metaData());
174180
}
175181

@@ -178,8 +184,8 @@ public InternalAggregation buildEmptyAggregation() {
178184
InternalDateHistogram.EmptyBucketInfo emptyBucketInfo = minDocCount == 0
179185
? new InternalDateHistogram.EmptyBucketInfo(rounding, buildEmptySubAggregations(), extendedBounds)
180186
: null;
181-
return new InternalDateHistogram(name, Collections.emptyList(), order, minDocCount, rounding.offset(), emptyBucketInfo, formatter,
182-
keyed, pipelineAggregators(), metaData());
187+
return new InternalDateHistogram(name, Collections.emptyList(), order, minDocCount, offset, emptyBucketInfo, formatter, keyed,
188+
pipelineAggregators(), metaData());
183189
}
184190

185191
@Override

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,7 @@ ExtendedBounds parseAndValidate(String aggName, QueryShardContext queryShardCont
166166
}
167167

168168
ExtendedBounds round(Rounding rounding) {
169-
// Extended bounds shouldn't be effected by the offset
170-
Rounding effectiveRounding = rounding.withoutOffset();
171-
return new ExtendedBounds(
172-
min != null ? effectiveRounding.round(min) : null,
173-
max != null ? effectiveRounding.round(max) : null);
169+
return new ExtendedBounds(min != null ? rounding.round(min) : null, max != null ? rounding.round(max) : null);
174170
}
175171

176172
@Override

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -497,7 +497,7 @@ public Number getKey(MultiBucketsAggregation.Bucket bucket) {
497497

498498
@Override
499499
public Number nextKey(Number key) {
500-
return emptyBucketInfo.rounding.nextRoundingValue(key.longValue());
500+
return emptyBucketInfo.rounding.nextRoundingValue(key.longValue() - offset) + offset;
501501
}
502502

503503
@Override

‎server/src/test/java/org/elasticsearch/common/RoundingTests.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -201,18 +201,10 @@ public void testOffsetRounding() {
201201
Rounding rounding = Rounding.builder(Rounding.DateTimeUnit.DAY_OF_MONTH).offset(twoHours).build();
202202
assertThat(rounding.round(0), equalTo(-oneDay + twoHours));
203203
assertThat(rounding.round(twoHours), equalTo(twoHours));
204-
assertThat(rounding.nextRoundingValue(-oneDay), equalTo(-oneDay + twoHours));
205-
assertThat(rounding.nextRoundingValue(0), equalTo(twoHours));
206-
assertThat(rounding.withoutOffset().round(0), equalTo(0L));
207-
assertThat(rounding.withoutOffset().nextRoundingValue(0), equalTo(oneDay));
208204

209205
rounding = Rounding.builder(Rounding.DateTimeUnit.DAY_OF_MONTH).offset(-twoHours).build();
210206
assertThat(rounding.round(0), equalTo(-twoHours));
211207
assertThat(rounding.round(oneDay - twoHours), equalTo(oneDay - twoHours));
212-
assertThat(rounding.nextRoundingValue(-oneDay), equalTo(-twoHours));
213-
assertThat(rounding.nextRoundingValue(0), equalTo(oneDay - twoHours));
214-
assertThat(rounding.withoutOffset().round(0), equalTo(0L));
215-
assertThat(rounding.withoutOffset().nextRoundingValue(0), equalTo(oneDay));
216208
}
217209

218210
/**

‎server/src/test/java/org/elasticsearch/search/aggregations/pipeline/PipelineAggregationHelperTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ static AggregatorFactory getRandomSequentiallyOrderedParentAgg() throws IOExcept
164164
new AggregatorFactories.Builder(), Collections.emptyMap());
165165
break;
166166
case 1:
167-
factory = new DateHistogramAggregatorFactory("name", mock(ValuesSourceConfig.class),
167+
factory = new DateHistogramAggregatorFactory("name", mock(ValuesSourceConfig.class), 0L,
168168
mock(InternalOrder.class), false, 0L, mock(Rounding.class), mock(Rounding.class),
169169
mock(ExtendedBounds.class), mock(QueryShardContext.class), mock(AggregatorFactory.class),
170170
new AggregatorFactories.Builder(), Collections.emptyMap());

‎x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ public void testParentValidations() throws IOException {
131131
// Date Histogram
132132
aggBuilders.clear();
133133
aggBuilders.add(new CumulativeCardinalityPipelineAggregationBuilder("cumulative_card", "sum"));
134-
parent = new DateHistogramAggregatorFactory("name", valuesSourceConfig,
134+
parent = new DateHistogramAggregatorFactory("name", valuesSourceConfig, 0L,
135135
mock(InternalOrder.class), false, 0L, mock(Rounding.class), mock(Rounding.class),
136136
mock(ExtendedBounds.class), mock(QueryShardContext.class), mock(AggregatorFactory.class),
137137
new AggregatorFactories.Builder(), Collections.emptyMap());

0 commit comments

Comments
 (0)