[Lens] Fix sorting on table when using Last value on date field (elas…

…tic#177288)

## Summary

Fixes elastic#175659

The bug was due to the format returned from the `Last value` of a date
field, which was the ISO string type, but the sorting criteria was
assuming a number format for dates.

I've revisited the `kbn-sort-predicates` logic for dates to support now
both numeric and ISO string format, with dedicated test suite.

<img width="449" alt="Screenshot 2024-02-20 at 13 45 07"
src="https://github.com/elastic/kibana/assets/924948/091e2a9d-70d1-44e1-b42b-c396df0fa1ab">
<img width="434" alt="Screenshot 2024-02-20 at 13 45 01"
src="https://github.com/elastic/kibana/assets/924948/5d6ac269-96b9-4ea9-80a9-e8db8934e00b">


Also, it was an opportunity to uniform the logic for multi-values
comparisons and document it in the package `README`.

### Checklist

Delete any items that are not applicable to this PR.

- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Stratoula Kalafateli <[email protected]>

packages/kbn-sort-predicates/README.md

@@ -6,10 +6,11 @@ This package contains a flexible sorting function who supports the following typ
 * number
 * version
 * ip addresses (both IPv4 and IPv6) - handles `Others`/strings correcly in this case
-* dates
+* dates (both as number or ISO string)
 * ranges open and closed (number type only for now)
 * null and undefined (always sorted as last entries, no matter the direction)
 * any multi-value version of the types above (version excluded)
+  * for multi-values with different length it wins the first non-zero comparison (see note at the bottom)
 
 The function is intended to use with objects and to simplify the usage with sorting by a specific column/field.
 The functions has been extracted from Lens datatable where it was originally used.
@@ -71,4 +72,34 @@ return <EuiDataGrid
         onSort: () => { ... }
     }}
 />;
-```
+```
+
+### Multi-value notes
+
+In this section there's some more details about multi-value comparison algorithm used in this package.
+For multi-values of the same length, the first non-zero comparison wins (ASC example):
+
+```
+a: [5, 7]
+b: [1]
+```
+
+`b` comes before `a` as `1 < 5`.
+
+With this other set of data:
+
+```
+a: [1, 2, 3, 5]
+b: [1, 2, 3, 3]
+```
+
+`a` comes before `b` as the first 3 comparisons will return 0, while the last one (`5 > 3`) returns -1.
+
+In case of arrays of different length, the `undefined` value is used for the shortest multi-value:
+
+```
+a; [1, 2]
+b: [1]
+```
+
+In this case `b` wins as on the second comparison `undefined < 2`.

packages/kbn-sort-predicates/src/sorting.test.ts

@@ -46,27 +46,107 @@ function testSorting({
 }
 
 describe('Data sorting criteria', () => {
-  describe('Numeric values', () => {
+  describe('Date values', () => {
     for (const direction of ['asc', 'desc'] as const) {
-      it(`should provide the number criteria of numeric values (${direction})`, () => {
+      it(`should provide the date criteria for date values (${direction})`, () => {
+        const now = Date.now();
         testSorting({
-          input: [7, 6, 5, -Infinity, Infinity],
-          output: [-Infinity, 5, 6, 7, Infinity],
+          input: [now, now - 150000, 0],
+          output: [0, now - 150000, now],
           direction,
-          type: 'number',
+          type: 'date',
         });
       });
 
-      it(`should provide the number criteria for date values (${direction})`, () => {
+      it(`should provide the date criteria for array date values (${direction})`, () => {
         const now = Date.now();
         testSorting({
-          input: [now, 0, now - 150000],
-          output: [0, now - 150000, now],
+          input: [now, [0, now], [0], now - 150000],
+          output: [[0], [0, now], now - 150000, now],
           direction,
           type: 'date',
         });
       });
 
+      it(`should provide the date criteria for ISO string date values (${direction})`, () => {
+        const now = new Date(Date.now()).toISOString();
+        const beforeNow = new Date(Date.now() - 150000).toISOString();
+        const originString = new Date(0).toISOString();
+        testSorting({
+          input: [now, beforeNow, originString],
+          output: [originString, beforeNow, now],
+          direction,
+          type: 'date',
+        });
+      });
+
+      it(`should provide the date criteria for array ISO string date values (${direction})`, () => {
+        const now = new Date(Date.now()).toISOString();
+        const beforeNow = new Date(Date.now() - 150000).toISOString();
+        const originString = new Date(0).toISOString();
+        testSorting({
+          input: [now, [originString, now], [originString], beforeNow],
+          output: [[originString], [originString, now], beforeNow, now],
+          direction,
+          type: 'date',
+        });
+      });
+
+      it(`should provide the date criteria for date values (${direction})`, () => {
+        const now = Date.now();
+        const originString = new Date(0).toISOString();
+        testSorting({
+          input: [now, now - 150000, originString],
+          output: [originString, now - 150000, now],
+          direction,
+          type: 'date',
+        });
+      });
+
+      it(`should provide the date criteria for array date values of mixed types (${direction})`, () => {
+        const now = Date.now();
+        const beforeNow = Date.now() - 150000;
+        const originString = new Date(0).toISOString();
+        testSorting({
+          input: [now, [originString, now], [originString], beforeNow],
+          output: [[originString], [originString, now], beforeNow, now],
+          direction,
+          type: 'date',
+        });
+      });
+    }
+
+    it(`should sort undefined and null to the end`, () => {
+      const now = new Date(Date.now()).toISOString();
+      const beforeNow = new Date(Date.now() - 150000).toISOString();
+      testSorting({
+        input: [null, now, 0, undefined, null, beforeNow],
+        output: [0, beforeNow, now, null, undefined, null],
+        direction: 'asc',
+        type: 'date',
+        reverseOutput: false,
+      });
+
+      testSorting({
+        input: [null, now, 0, undefined, null, beforeNow],
+        output: [now, beforeNow, 0, null, undefined, null],
+        direction: 'desc',
+        type: 'date',
+        reverseOutput: false,
+      });
+    });
+  });
+  describe('Numeric values', () => {
+    for (const direction of ['asc', 'desc'] as const) {
+      it(`should provide the number criteria of numeric values (${direction})`, () => {
+        testSorting({
+          input: [7, 6, 5, -Infinity, Infinity],
+          output: [-Infinity, 5, 6, 7, Infinity],
+          direction,
+          type: 'number',
+        });
+      });
+
       it(`should provide the number criteria of array numeric values (${direction})`, () => {
         testSorting({
           input: [7, [7, 1], [7, 2], [6], -Infinity, Infinity],
@@ -76,13 +156,12 @@ describe('Data sorting criteria', () => {
         });
       });
 
-      it(`should provide the number criteria for array date values (${direction})`, () => {
-        const now = Date.now();
+      it(`should provide the number criteria of array numeric values (${direction})`, () => {
         testSorting({
-          input: [now, [0, now], [0], now - 150000],
-          output: [[0], [0, now], now - 150000, now],
+          input: [7, [0, 7], [0], 1],
+          output: [[0], [0, 7], 1, 7],
           direction,
-          type: 'date',
+          type: 'number',
         });
       });
     }

packages/kbn-sort-predicates/src/sorting.ts

@@ -10,6 +10,7 @@ import versionCompare from 'compare-versions';
 import valid from 'semver/functions/valid';
 import ipaddr, { type IPv4, type IPv6 } from 'ipaddr.js';
 import { FieldFormat } from '@kbn/field-formats-plugin/common';
+import moment from 'moment';
 
 type CompareFn<T extends unknown> = (
   v1: T | undefined,
@@ -18,18 +19,47 @@ type CompareFn<T extends unknown> = (
   formatter: FieldFormat
 ) => number;
 
-const numberCompare: CompareFn<number> = (v1, v2) => (v1 ?? -Infinity) - (v2 ?? -Infinity);
+// Catches null, undefined and NaN
+const isInvalidValue = (value: unknown): value is null | undefined =>
+  value == null || (typeof value === 'number' && Number.isNaN(value));
 
-const stringComparison: CompareFn<string> = (v1, v2, _, formatter) => {
-  const aString = formatter.convert(v1);
-  const bString = formatter.convert(v2);
-  if (v1 == null) {
+const handleInvalidValues = (v1: unknown, v2: unknown) => {
+  if (isInvalidValue(v1)) {
+    if (isInvalidValue(v2)) {
+      return 0;
+    }
+    return -1;
+  }
+  if (isInvalidValue(v2)) {
+    return 1;
+  }
+  return undefined;
+};
+
+const numberCompare: CompareFn<number> = (v1, v2) => handleInvalidValues(v1, v2) ?? v1! - v2!;
+
+const dateCompare: CompareFn<number | string> = (v1, v2) => {
+  const mV1 = moment(v1);
+  const mV2 = moment(v2);
+  if (!mV1.isValid() || isInvalidValue(v1)) {
+    if (!mV2.isValid() || isInvalidValue(v2)) {
+      return 0;
+    }
     return -1;
   }
-  if (v2 == null) {
+  if (!mV2.isValid() || isInvalidValue(v2)) {
     return 1;
   }
-  return aString.localeCompare(bString);
+  if (mV1.isSame(mV2)) {
+    return 0;
+  }
+  return mV1.isBefore(mV2) ? -1 : 1;
+};
+
+const stringComparison: CompareFn<string> = (v1, v2, _, formatter) => {
+  const aString = formatter.convert(v1);
+  const bString = formatter.convert(v2);
+  return handleInvalidValues(v1, v2) ?? aString.localeCompare(bString);
 };
 
 // The maximum length of a IP is 39 chars for a IPv6
@@ -89,7 +119,7 @@ function isIPv6Address(ip: IPv4 | IPv6): ip is IPv6 {
 }
 
 function getSafeIpAddress(ip: string | undefined, directionFactor: number) {
-  if (ip == null || !ipaddr.isValid(ip)) {
+  if (isInvalidValue(ip) || !ipaddr.isValid(ip)) {
     // if ip is null, then it's a part of an array ip value
     // therefore the comparison might be between a single value [ipA, undefined] vs multiple values ip [ipA, ipB]
     // set in this case -1 for the undefined of the former to force it to be always before
@@ -177,17 +207,18 @@ function getUndefinedHandler(
   ) => {
     const valueA = rowA[sortBy];
     const valueB = rowB[sortBy];
-    if (valueA != null && valueB != null && !Number.isNaN(valueA) && !Number.isNaN(valueB)) {
-      return sortingCriteria(rowA, rowB, direction);
-    }
+    // do not use the utility above as null at root level is handled differently
+    // than null/undefined within an array type
     if (valueA == null || Number.isNaN(valueA)) {
+      if (valueB == null || Number.isNaN(valueB)) {
+        return 0;
+      }
       return 1;
     }
     if (valueB == null || Number.isNaN(valueB)) {
       return -1;
     }
-
-    return 0;
+    return sortingCriteria(rowA, rowB, direction);
   };
 }
 
@@ -198,7 +229,10 @@ export function getSortingCriteria(
 ) {
   const arrayValueHandler = createArrayValuesHandler(sortBy, formatter);
 
-  if (['number', 'date'].includes(type || '')) {
+  if (type === 'date') {
+    return getUndefinedHandler(sortBy, arrayValueHandler(dateCompare));
+  }
+  if (type === 'number') {
     return getUndefinedHandler(sortBy, arrayValueHandler(numberCompare));
   }
   // this is a custom type, and can safely assume the gte and lt fields are all numbers or undefined