Fix sort on nanosecond date fields with missing values #74760

cbuescher · 2021-06-30T14:47:27Z

For missing values on date fields we use Long.MIN_VALUE by default. This is okay
when the resolution of the field is milliseconds. For nanoseconds though,
negative values can lead to IllegalArgumentExpections when we try to format them
internally.
This change fixes this by explicitely setting the minimum value to 0L (which
corresponds to 1970-01-01T00:00:00.000 for nanosecond resolution) when no other
explicit missing value is defined and the target numeric type is a nanosecond
type (this is true for nanosecond fields and when "numeric_type" is explicitely
set). This way we correct the behaviour for single typed indices and cases where
we are sorting across multiple indices with mixed "date" and "date_nanos" type
where "numeric_type" is set in the sort definition.

Closes #73763

For missing values on date fields we use Long.MIN_VALUE by default. This is okay when the resolution of the field is milliseconds. For nanoseconds though, negative values can lead to IllegalArgumentExpections when we try to format them internally. This change fixes this by explicitely setting the minimum value to 0L (which corresponds to 1970-01-01T00:00:00.000 for nanosecond resolution) when no other explicit missing value is defined and the target numeric type is a nanosecond type (this is true for nanosecond fields and when "numeric_type" is explicitely set). This way we correct the behaviour for single typed indices and cases where we are sorting across multiple indices with mixed "date" and "date_nanos" type where "numeric_type" is set in the sort definition. Closes elastic#73763

elasticmachine · 2021-06-30T14:47:30Z

Pinging @elastic/es-search (Team:Search)

cbuescher · 2021-06-30T16:14:04Z

@elasticmachine update branch

jimczi

I left some comments.

jimczi · 2021-07-05T11:41:35Z

server/src/main/java/org/elasticsearch/index/fielddata/IndexNumericFieldData.java

+                final boolean min = sortMissingFirst(missingValue) ^ reverse;
+                missingValue = min ? 0L : Long.MAX_VALUE;
+            }
+        }


This logic could move to IndexNumericFieldData#missingObject ? IndexFieldData#missingObject doesn't need to be final, we need to pick the value depending on the targetNumericType.

I looked into this a bit and one problem I couldn't solve with this is that "missingObject" is part of XFieldComparatorSource, not IndexFieldData. That probably woudl mean overwriting it in special versions of LongValuesComparatorSource, and for the things I tried it wasn't always clear how to ther the "targetNumericType" from the sort (e.g. reduceType() returns SortField.Type that doesn't even differentiate between Long, Data and Datenanos like IndexNumericFieldData.NumericType does). Maybe I'm missing something but from what I tried this current location seems less complex to me.

The targetNumericType could be added to the LongValuesSource and checked in missingObject ? It's not really about complexity here, missingObject is called in several contexts so it needs a fix.

jimczi · 2021-07-05T11:45:25Z

server/src/internalClusterTest/java/org/elasticsearch/search/sort/FieldSortIT.java

+                SearchResponse searchResponse = client().prepareSearch(index)
+                    .addSort(SortBuilders.fieldSort("mydate").order(SortOrder.ASC).setFormat(format).setNumericType("date_nanos"))
+                    .get();
+                assertHitsInOrder(searchResponse, new String[] { "1", "2", "4", "5", "3", "6" });


A tiebreaker should be used to ensure that 3 is always sorted before 6 (same value otherwise).

jimczi · 2021-07-05T11:45:36Z

server/src/internalClusterTest/java/org/elasticsearch/search/sort/FieldSortIT.java

+                            .setNumericType("date_nanos")
+                    )
+                    .get();
+                assertHitsInOrder(searchResponse, new String[] { "3", "6", "1", "2", "4", "5" });


cbuescher · 2021-07-05T15:08:23Z

@jimczi thanks for the review, I improved the tests and left a comment regarding moving the implementation logic elsewhere and the problems I encountered. If you have other suggestions please let me know, otherwise I hope its also okay if we leave it where it is.

cbuescher · 2021-07-06T15:22:33Z

@jimczi I pushed another commit that adresses your latest comment, should be ready for another look I think

jimczi

I left some minor comments, LGTM otherwise

jimczi · 2021-07-06T17:32:45Z

server/src/main/java/org/elasticsearch/index/fielddata/IndexNumericFieldData.java

+        @Nullable Object missingValue,
+        MultiValueMode sortMode,
+        Nested nested,
+        NumericType targetNumericType


The targetNumericType is implicit here ?

jimczi · 2021-07-06T17:33:17Z

server/src/main/java/org/elasticsearch/index/fielddata/IndexNumericFieldData.java

+        @Nullable Object missingValue,
+        MultiValueMode sortMode,
+        Nested nested,
+        NumericType targetNumericType


The targetNumericType is implicit here, no need to add it.

jimczi · 2021-07-06T17:34:24Z

.../main/java/org/elasticsearch/index/fielddata/fieldcomparator/LongValuesComparatorSource.java

+            // special case to prevent negative values that would cause invalid nanosecond ranges
+            if (sortMissingFirst(missingValue) || sortMissingLast(missingValue)) {
+                final boolean min = sortMissingFirst(missingValue) ^ reversed;
+                return min ? 0L : Long.MAX_VALUE;


We should use DateUtils#MAX_NANOSECOND_INSTANT as the max

We need the converted nanoseconds value of that Instant though I think, which should be the same as Long.MAX_VALUE? I introduced another constant (8c6df48#diff-09ba5f62fc8de8923f01213874ef48657920b2cc1428ff4f27f5f06169ee8f1aR207) in my last commit to not have to do that coversion every time, just if you want to check.

cbuescher · 2021-07-07T12:09:20Z

@elasticmachine update branch

For missing values on date fields we use Long.MIN_VALUE by default. This is okay when the resolution of the field is milliseconds. For nanoseconds though, negative values can lead to IllegalArgumentExpections when we try to format them internally. This change fixes this by explicitely setting the minimum value to 0L (which corresponds to 1970-01-01T00:00:00.000 for nanosecond resolution) when no other explicit missing value is defined and the target numeric type is a nanosecond type (this is true for nanosecond fields and when "numeric_type" is explicitely set). This way we correct the behaviour for single typed indices and cases where we are sorting across multiple indices with mixed "date" and "date_nanos" type where "numeric_type" is set in the sort definition. Closes elastic#73763

elasticsearchmachine · 2021-07-07T14:33:24Z

💚 Backport successful

Status	Branch	Result
✅	7.14
✅	7.x

#75064) For missing values on date fields we use Long.MIN_VALUE by default. This is okay when the resolution of the field is milliseconds. For nanoseconds though, negative values can lead to IllegalArgumentExpections when we try to format them internally. This change fixes this by explicitely setting the minimum value to 0L (which corresponds to 1970-01-01T00:00:00.000 for nanosecond resolution) when no other explicit missing value is defined and the target numeric type is a nanosecond type (this is true for nanosecond fields and when "numeric_type" is explicitely set). This way we correct the behaviour for single typed indices and cases where we are sorting across multiple indices with mixed "date" and "date_nanos" type where "numeric_type" is set in the sort definition. Closes #73763

…#75065) For missing values on date fields we use Long.MIN_VALUE by default. This is okay when the resolution of the field is milliseconds. For nanoseconds though, negative values can lead to IllegalArgumentExpections when we try to format them internally. This change fixes this by explicitely setting the minimum value to 0L (which corresponds to 1970-01-01T00:00:00.000 for nanosecond resolution) when no other explicit missing value is defined and the target numeric type is a nanosecond type (this is true for nanosecond fields and when "numeric_type" is explicitely set). This way we correct the behaviour for single typed indices and cases where we are sorting across multiple indices with mixed "date" and "date_nanos" type where "numeric_type" is set in the sort definition. Closes #73763

cbuescher added >bug :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.14.0 labels Jun 30, 2021

elasticmachine added the Team:Search Meta label for search team label Jun 30, 2021

mark-vieira added v7.15.0 and removed v7.14.0 labels Jun 30, 2021

Merge branch 'master' into fix-73763

20a9c44

cbuescher added the v7.14.1 label Jul 1, 2021

jimczi requested changes Jul 5, 2021

View reviewed changes

add tiebreaker to tests

c690836

Christoph Büscher added 4 commits July 5, 2021 17:37

Merge branch 'master' into fix-73763

ab95f0b

Merge branch 'master' into fix-73763

6ad0f23

move logic to LongValuesComparator#missingObject

97cbefa

checkstyle

9037428

jimczi approved these changes Jul 6, 2021

View reviewed changes

iter

8c6df48

Merge branch 'master' into fix-73763

56eccbb

cbuescher merged commit bf952a4 into elastic:master Jul 7, 2021

cbuescher added backport pending auto-backport Automatically create backport pull requests when merged labels Jul 7, 2021

This was referenced Jul 7, 2021

[7.14] Fix sort on nanosecond date fields with missing values (#74760) #75064

Merged

[7.x] Fix sort on nanosecond date fields with missing values (#74760) #75065

Merged

cbuescher added v7.14.0 and removed v7.14.1 labels Jul 7, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

joegallo removed the backport pending label Sep 22, 2021

hanjongho mentioned this pull request Nov 3, 2024

Fix sort on date fields with missing values (#73763) #116099

Open

henningandersen mentioned this pull request Nov 11, 2024

Fix sort on date fields with missing values (#73763) #116534

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sort on nanosecond date fields with missing values #74760

Fix sort on nanosecond date fields with missing values #74760

cbuescher commented Jun 30, 2021

elasticmachine commented Jun 30, 2021

cbuescher commented Jun 30, 2021

jimczi left a comment

jimczi Jul 5, 2021

cbuescher Jul 5, 2021 •

edited

Loading

jimczi Jul 6, 2021

jimczi Jul 5, 2021

jimczi Jul 5, 2021

cbuescher commented Jul 5, 2021

cbuescher commented Jul 6, 2021

jimczi left a comment

jimczi Jul 6, 2021

jimczi Jul 6, 2021

jimczi Jul 6, 2021

cbuescher Jul 7, 2021

cbuescher commented Jul 7, 2021

elasticsearchmachine commented Jul 7, 2021

Fix sort on nanosecond date fields with missing values #74760

Fix sort on nanosecond date fields with missing values #74760

Conversation

cbuescher commented Jun 30, 2021

elasticmachine commented Jun 30, 2021

cbuescher commented Jun 30, 2021

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Jul 5, 2021

cbuescher commented Jul 6, 2021

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Jul 7, 2021

elasticsearchmachine commented Jul 7, 2021

💚 Backport successful

cbuescher Jul 5, 2021 •

edited

Loading