-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Support offset in composite aggs #50609
Changes from 1 commit
3032d62
b8f692e
95da5cb
e7eea38
393388c
14de4c8
b271143
08fc267
2113c0e
c2256e2
54f53ed
9d3c354
fc77e34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -367,6 +367,40 @@ setup: | |
- match: { aggregations.test.buckets.0.key.date: "2017-10-21" } | ||
- match: { aggregations.test.buckets.0.doc_count: 1 } | ||
|
||
--- | ||
"Composite aggregation with date_histogram offset": | ||
- skip: | ||
version: " - 7.99.99" | ||
reason: offset introduced in 8.0.0 | ||
|
||
- do: | ||
search: | ||
rest_total_hits_as_int: true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use the new format, this option should be removed soon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! |
||
index: test | ||
body: | ||
aggregations: | ||
test: | ||
composite: | ||
sources: [ | ||
{ | ||
"date": { | ||
"date_histogram": { | ||
"field": "date", | ||
"calendar_interval": "1d", | ||
"offset": "4h", | ||
"format": "iso8601" # Format makes the comparisons a little more obvious | ||
} | ||
} | ||
} | ||
] | ||
|
||
- match: {hits.total: 6} | ||
- length: { aggregations.test.buckets: 2 } | ||
- match: { aggregations.test.buckets.0.key.date: "2017-10-19T04:00:00.000Z" } | ||
- match: { aggregations.test.buckets.0.doc_count: 1 } | ||
- match: { aggregations.test.buckets.1.key.date: "2017-10-21T04:00:00.000Z" } | ||
- match: { aggregations.test.buckets.1.doc_count: 1 } | ||
|
||
--- | ||
"Composite aggregation with after_key in the response": | ||
- do: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
package org.elasticsearch.common; | ||
|
||
import org.elasticsearch.ElasticsearchException; | ||
import org.elasticsearch.Version; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.common.io.stream.Writeable; | ||
|
@@ -177,6 +178,16 @@ public static Builder builder(TimeValue interval) { | |
return new Builder(interval); | ||
} | ||
|
||
/** | ||
* Create a rounding that offsets values before passing them into another rounding | ||
* and then un-offsets them after that rounding has done its job. | ||
* @param delegate the other rounding to offset | ||
* @param offset the offset, in milliseconds | ||
*/ | ||
public static Rounding offset(Rounding delegate, long offset) { | ||
return new OffsetRounding(delegate, offset); | ||
} | ||
|
||
public static class Builder { | ||
|
||
private final DateTimeUnit unit; | ||
|
@@ -556,19 +567,73 @@ public boolean equals(Object obj) { | |
} | ||
} | ||
|
||
static class OffsetRounding extends Rounding { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the intention to migrate the existing users of offsets (regular date histo agg, etc) over to this wrapper? E.g. it seems a bit heavyweight to create a whole new rounding, but if the idea is to followup with changes to the existing users of offset (rather than baking the logic into the aggs) it makes sense to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, we should probably add a javadoc explaining why/when to use this class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I did intend to migrate the rest over of the uses of I added javadoc on the public interface to the class which is what the rest of the classes in this file do. Do you think I should duplicate it onto this one? Or add something maybe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, missed that it was documented on the public method. Since the rest of the class does it that way, fine with me :) Thanks for the explanation! |
||
static final byte ID = 3; | ||
|
||
private final Rounding delegate; | ||
private final long offset; | ||
|
||
OffsetRounding(Rounding delegate, long offset) { | ||
this.delegate = delegate; | ||
this.offset = offset; | ||
} | ||
|
||
OffsetRounding(StreamInput in) throws IOException { | ||
delegate = Rounding.read(in); | ||
offset = in.readLong(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe read/write ZLong instead? I suspect offsets will be small-to-medium'ish sized and either positive or negative, so zlong might be a win? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In other places offsets are written as long and I just copied it. But I'd be fine with zlong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pushed the switch to zlong. |
||
} | ||
|
||
@Override | ||
public void innerWriteTo(StreamOutput out) throws IOException { | ||
if (out.getVersion().before(Version.V_8_0_0)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't actually serialize this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Modulo the version check which I discuss below. |
||
throw new IllegalArgumentException("Offset rounding not supported before 8.0.0"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should the message be before 7.6.0 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. I'll fix it. |
||
} | ||
delegate.writeTo(out); | ||
out.writeLong(offset); | ||
} | ||
|
||
@Override | ||
public byte id() { | ||
return ID; | ||
} | ||
|
||
@Override | ||
public long round(long value) { | ||
return delegate.round(value - offset) + offset; | ||
} | ||
|
||
@Override | ||
public long nextRoundingValue(long value) { | ||
// This isn't needed by the current users. We'll implement it when we migrate other users to it. | ||
throw new UnsupportedOperationException("not yet supported"); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(delegate, offset); | ||
} | ||
|
||
@Override | ||
public boolean equals(Object obj) { | ||
if (obj == null || getClass() != obj.getClass()) { | ||
return false; | ||
} | ||
OffsetRounding other = (OffsetRounding) obj; | ||
return delegate.equals(other.delegate) && offset == other.offset; | ||
} | ||
} | ||
|
||
public static Rounding read(StreamInput in) throws IOException { | ||
Rounding rounding; | ||
byte id = in.readByte(); | ||
switch (id) { | ||
case TimeUnitRounding.ID: | ||
rounding = new TimeUnitRounding(in); | ||
break; | ||
return new TimeUnitRounding(in); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙏 for the cleanup here :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure everyone would consider that cleaner, but I sure do! |
||
case TimeIntervalRounding.ID: | ||
rounding = new TimeIntervalRounding(in); | ||
break; | ||
return new TimeIntervalRounding(in); | ||
case OffsetRounding.ID: | ||
return new OffsetRounding(in); | ||
default: | ||
throw new ElasticsearchException("unknown rounding id [" + id + "]"); | ||
} | ||
return rounding; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
|
||
package org.elasticsearch.search.aggregations.bucket.composite; | ||
|
||
import org.elasticsearch.Version; | ||
import org.elasticsearch.common.ParseField; | ||
import org.elasticsearch.common.Rounding; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
|
@@ -31,9 +32,11 @@ | |
import org.elasticsearch.index.query.QueryShardContext; | ||
import org.elasticsearch.script.Script; | ||
import org.elasticsearch.search.DocValueFormat; | ||
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder; | ||
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval; | ||
import org.elasticsearch.search.aggregations.bucket.histogram.DateIntervalConsumer; | ||
import org.elasticsearch.search.aggregations.bucket.histogram.DateIntervalWrapper; | ||
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; | ||
import org.elasticsearch.search.aggregations.support.ValueType; | ||
import org.elasticsearch.search.aggregations.support.ValuesSource; | ||
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; | ||
|
@@ -56,6 +59,13 @@ public class DateHistogramValuesSourceBuilder | |
PARSER = new ObjectParser<>(DateHistogramValuesSourceBuilder.TYPE); | ||
PARSER.declareString(DateHistogramValuesSourceBuilder::format, new ParseField("format")); | ||
DateIntervalWrapper.declareIntervalFields(PARSER); | ||
PARSER.declareField(DateHistogramValuesSourceBuilder::offset, p -> { | ||
if (p.currentToken() == XContentParser.Token.VALUE_NUMBER) { | ||
return p.longValue(); | ||
} else { | ||
return DateHistogramAggregationBuilder.parseStringOffset(p.text()); | ||
} | ||
}, Histogram.OFFSET_FIELD, ObjectParser.ValueType.LONG); | ||
PARSER.declareField(DateHistogramValuesSourceBuilder::timeZone, p -> { | ||
if (p.currentToken() == XContentParser.Token.VALUE_STRING) { | ||
return ZoneId.of(p.text()); | ||
|
@@ -71,6 +81,7 @@ static DateHistogramValuesSourceBuilder parse(String name, XContentParser parser | |
|
||
private ZoneId timeZone = null; | ||
private DateIntervalWrapper dateHistogramInterval = new DateIntervalWrapper(); | ||
private long offset = 0; | ||
|
||
public DateHistogramValuesSourceBuilder(String name) { | ||
super(name, ValueType.DATE); | ||
|
@@ -80,12 +91,18 @@ protected DateHistogramValuesSourceBuilder(StreamInput in) throws IOException { | |
super(in); | ||
dateHistogramInterval = new DateIntervalWrapper(in); | ||
timeZone = in.readOptionalZoneId(); | ||
if (in.getVersion().after(Version.V_8_0_0)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the way to do this, but it has been a long time. I want to land this change in master and 7.x, but these tests have no change of passing the bwc tests until I backport it. Now that I think about it, maybe this version check should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests caught an error - this at least be |
||
offset = in.readLong(); | ||
} | ||
} | ||
|
||
@Override | ||
protected void innerWriteTo(StreamOutput out) throws IOException { | ||
dateHistogramInterval.writeTo(out); | ||
out.writeOptionalZoneId(timeZone); | ||
if (out.getVersion().after(Version.V_8_0_0)) { | ||
out.writeLong(offset); | ||
} | ||
} | ||
|
||
@Override | ||
|
@@ -215,9 +232,28 @@ public ZoneId timeZone() { | |
return timeZone; | ||
} | ||
|
||
/** | ||
* Get the offset to use when rounding, which is a number of milliseconds. | ||
*/ | ||
public long offset() { | ||
return offset; | ||
} | ||
|
||
/** | ||
* Set the offset on this builder, which is a number of milliseconds. | ||
* @return this for chaining | ||
*/ | ||
public DateHistogramValuesSourceBuilder offset(long offset) { | ||
this.offset = offset; | ||
return this; | ||
} | ||
|
||
@Override | ||
protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig<?> config) throws IOException { | ||
Rounding rounding = dateHistogramInterval.createRounding(timeZone()); | ||
if (offset != 0) { | ||
rounding = Rounding.offset(rounding, offset); | ||
} | ||
ValuesSource orig = config.toValuesSource(queryShardContext); | ||
if (orig == null) { | ||
orig = ValuesSource.Numeric.EMPTY; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -195,6 +195,16 @@ public void testTimeUnitRoundingDST() { | |
assertThat(tzRounding_chg.round(time("2014-11-02T06:01:01", chg)), isDate(time("2014-11-02T06:00:00", chg), chg)); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, do you know if we have any serialization tests for If we have them somewhere, let's add a test for the id byte. If not, probably too much to add to this PR but we should file a ticket so we don't forget to add some tests... makes me uneasy that such a widespread class doesn't have serialization tests :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we end up testing serialization as part of testing things like extended bounds bucket response. Adding a unit test for just this class's serialization makes sense to me though. I figured I'd wait until I used it in a context where we serialized it but since I'm writing the serialization code now I probably ought to write the test now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed a wire test case. |
||
public void testOffsetRounding() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add another test that does negative offsets too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I pushed a test with negative offsets. |
||
long twoHours = TimeUnit.HOURS.toMillis(2); | ||
long oneDay = TimeUnit.DAYS.toMillis(1); | ||
Rounding dayOfMonth = Rounding.builder(Rounding.DateTimeUnit.DAY_OF_MONTH).build(); | ||
Rounding offsetRounding = Rounding.offset(dayOfMonth, twoHours); | ||
assertThat(offsetRounding.round(0), equalTo(-oneDay + twoHours)); | ||
|
||
assertThat(offsetRounding.round(twoHours), equalTo(twoHours)); | ||
} | ||
|
||
/** | ||
* Randomized test on TimeUnitRounding. Test uses random | ||
* {@link DateTimeUnit} and {@link ZoneId} and often (50% of the time) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured because the option is the same I should just include the docs from the normal date histogram.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍