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.

Sign up for GitHub

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

Jump to bottom

Allows nanosecond resolution in search_after #60328

Merged
merged 4 commits into from
Jul 29, 2020
Merged

Allows nanosecond resolution in search_after #60328

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69025d7
Allows nanosecond resolution in search_after
nik9000 Jul 28, 2020
a8fcba8
Merge branch 'master' into allow_nano_resolution_in_search_after
nik9000 Jul 28, 2020
81beee7
Iter
nik9000 Jul 28, 2020
b298836
Skip
nik9000 Jul 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 124 additions & 4 deletions rest-api-spec/src/main/resources/rest-api-spec/test/search/90_search_after.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
setup:
"search with search_after parameter":
- do:
indices.create:
index: test
Expand All @@ -24,9 +24,6 @@ setup:
indices.refresh:
index: test

---
"search with search_after parameter":

- do:
search:
rest_total_hits_as_int: true
Expand Down Expand Up @@ -94,3 +91,126 @@ setup:

- match: {hits.total: 3}
- length: {hits.hits: 0 }

---
"date":
- do:
indices.create:
index: test
body:
mappings:
properties:
timestamp:
type: date
format: yyyy-MM-dd HH:mm:ss.SSS
- do:
bulk:
refresh: true
index: test
body: |
{"index":{}}
{"timestamp":"2019-10-21 00:30:04.828"}
{"index":{}}
{"timestamp":"2019-10-21 08:30:04.828"}

- do:
search:
index: test
body:
size: 1
sort: [{ timestamp: desc }]
- match: {hits.total.value: 2 }
- length: {hits.hits: 1 }
- match: {hits.hits.0._index: test }
- match: {hits.hits.0._source.timestamp: "2019-10-21 08:30:04.828" }
- match: {hits.hits.0.sort: [1571646604828] }

# search_after with the sort
- do:
search:
index: test
body:
size: 1
sort: [{ timestamp: desc }]
search_after: [1571646604828]
- match: {hits.total.value: 2 }
- length: {hits.hits: 1 }
- match: {hits.hits.0._index: test }
- match: {hits.hits.0._source.timestamp: "2019-10-21 00:30:04.828" }
- match: {hits.hits.0.sort: [1571617804828] }

# search_after with the formatted date
- do:
search:
index: test
body:
size: 1
sort: [{ timestamp: desc }]
search_after: ["2019-10-21 08:30:04.828"]
- match: {hits.total.value: 2 }
- length: {hits.hits: 1 }
- match: {hits.hits.0._index: test }
- match: {hits.hits.0._source.timestamp: "2019-10-21 00:30:04.828" }
- match: {hits.hits.0.sort: [1571617804828] }

---
"date_nanos":
- do:
indices.create:
index: test
body:
mappings:
properties:
timestamp:
type: date_nanos
format: yyyy-MM-dd HH:mm:ss.SSSSSS
- do:
bulk:
refresh: true
index: test
body: |
{"index":{}}
{"timestamp":"2019-10-21 00:30:04.828740"}
{"index":{}}
{"timestamp":"2019-10-21 08:30:04.828733"}

- do:
search:
index: test
body:
size: 1
sort: [{ timestamp: desc }]
- match: {hits.total.value: 2 }
- length: {hits.hits: 1 }
- match: {hits.hits.0._index: test }
- match: {hits.hits.0._source.timestamp: "2019-10-21 08:30:04.828733" }
- match: {hits.hits.0.sort: [1571646604828733000] }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should use the formatted version in the sort output ? We don't allow this format (nanoseconds since epoch) when ingesting or querying a date_nanos field so that's an exception here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm honestly not sure! I kind of figured the contact of search_after was "take the last sort value and stick it into search_after" but I see that we support formatted.

I also wonder what counts as breaking here. If we were to change the result to be formatted I imagine we'll break someone.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do support formatted output so my point was that the sort section should return the formatted version in order to avoid returning long that represents nanoseconds since epoch. I agree that this is not in the scope of this change so let's merge this one and we can discuss the formatting of sort values in a follow up (I can open the issue) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that this is not in the scope of this change so let's merge this one and we can discuss the formatting of sort values in a follow up (I can open the issue) ?

👍


# search_after with the sort
- do:
search:
index: test
body:
size: 1
sort: [{ timestamp: desc }]
search_after: [1571646604828733000]
- match: {hits.total.value: 2 }
- length: {hits.hits: 1 }
- match: {hits.hits.0._index: test }
- match: {hits.hits.0._source.timestamp: "2019-10-21 00:30:04.828740" }
- match: {hits.hits.0.sort: [1571617804828740000] }

# search_after with the formatted date
- do:
search:
index: test
body:
size: 1
sort: [{ timestamp: desc }]
search_after: ["2019-10-21 08:30:04.828733"]
- match: {hits.total.value: 2 }
- length: {hits.hits: 1 }
- match: {hits.hits.0._index: test }
- match: {hits.hits.0._source.timestamp: "2019-10-21 00:30:04.828740" }
- match: {hits.hits.0.sort: [1571617804828740000] }

4 changes: 2 additions & 2 deletions ...er/src/main/java/org/elasticsearch/index/fielddata/plain/SortedNumericIndexFieldData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ protected boolean sortRequiresCustomComparator() {
@Override
protected XFieldComparatorSource dateComparatorSource(Object missingValue, MultiValueMode sortMode, Nested nested) {
if (numericType == NumericType.DATE_NANOSECONDS) {
// converts date values to nanosecond resolution
// converts date_nanos values to millisecond resolution
return new LongValuesComparatorSource(this, missingValue,
sortMode, nested, dvs -> convertNumeric(dvs, DateUtils::toMilliSeconds));
}
Expand All @@ -115,7 +115,7 @@ protected XFieldComparatorSource dateComparatorSource(Object missingValue, Multi
@Override
protected XFieldComparatorSource dateNanosComparatorSource(Object missingValue, MultiValueMode sortMode, Nested nested) {
if (numericType == NumericType.DATE) {
// converts date_nanos values to millisecond resolution
// converts date values to nanosecond resolution
return new LongValuesComparatorSource(this, missingValue,
sortMode, nested, dvs -> convertNumeric(dvs, DateUtils::toNanoSeconds));
}
Expand Down
1 change: 1 addition & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ public DocValueFormat docValueFormat(@Nullable String format, ZoneId timeZone) {
}
// the resolution here is always set to milliseconds, as aggregations use this formatter mainly and those are always in
// milliseconds. The only special case here is docvalue fields, which are handled somewhere else
// TODO maybe aggs should force millis because lot so of other places want nanos?
nik9000 marked this conversation as resolved.
Show resolved Hide resolved
return new DocValueFormat.DateTime(dateTimeFormatter, timeZone, Resolution.MILLISECONDS);
}
}
Expand Down
5 changes: 5 additions & 0 deletions server/src/main/java/org/elasticsearch/search/DocValueFormat.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,11 @@ public long parseLong(String value, boolean roundUp, LongSupplier now) {
public double parseDouble(String value, boolean roundUp, LongSupplier now) {
return parseLong(value, roundUp, now);
}

@Override
public String toString() {
return "DocValueFormat.DateTime(" + formatter + ", " + timeZone + ", " + resolution + ")";
}
}

DocValueFormat GEOHASH = new DocValueFormat() {
Expand Down
10 changes: 9 additions & 1 deletion server/src/main/java/org/elasticsearch/search/sort/FieldSortBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException {
throw new QueryShardException(context, "we only support AVG, MEDIAN and SUM on number based fields");
}
final SortField field;
boolean isNanosecond = false;
if (numericType != null) {
if (fieldData instanceof IndexNumericFieldData == false) {
throw new QueryShardException(context,
Expand All @@ -348,8 +349,15 @@ public SortFieldAndFormat build(QueryShardContext context) throws IOException {
field = numericFieldData.sortField(resolvedType, missing, localSortMode(), nested, reverse);
} else {
field = fieldData.sortField(missing, localSortMode(), nested, reverse);
if (fieldData instanceof IndexNumericFieldData) {
isNanosecond = ((IndexNumericFieldData) fieldData).getNumericType() == NumericType.DATE_NANOSECONDS;
}
}
DocValueFormat format = fieldType.docValueFormat(null, null);
if (isNanosecond) {
format = DocValueFormat.withNanosecondResolution(format);
}
return new SortFieldAndFormat(field, fieldType.docValueFormat(null, null));
return new SortFieldAndFormat(field, format);
}

public boolean canRewriteToMatchNone() {
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/search/sort/FieldSortBuilderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public class FieldSortBuilderTests extends AbstractSortTestCase<FieldSortBuilder
/**
* {@link #provideMappedFieldType(String)} will return a
*/
private static String MAPPED_STRING_FIELDNAME = "_stringField";
private static final String MAPPED_STRING_FIELDNAME = "_stringField";

@Override
protected FieldSortBuilder createTestItem() {
Expand Down