[Lens] Better defaults for top values odering (#97099) (#97443)

Co-authored-by: Joe Reuter <johannes.reuter@elastic.co>

x-pack/plugins/lens/public/indexpattern_datasource/indexpattern_suggestions.test.tsx

@@ -965,7 +965,19 @@ describe('IndexPattern Data Source suggestions', () => {
             currentLayer: {
               ...initialState.layers.currentLayer,
               columns: {
-                cola: initialState.layers.currentLayer.columns.cola,
+                cola: {
+                  dataType: 'string',
+                  isBucketed: true,
+                  sourceField: 'source',
+                  label: 'values of source',
+                  customLabel: true,
+                  operationType: 'terms',
+                  params: {
+                    orderBy: { type: 'alphabetical', fallback: false },
+                    orderDirection: 'asc',
+                    size: 5,
+                  },
+                },
               },
               columnOrder: ['cola'],
             },

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts

@@ -179,7 +179,7 @@ interface BaseOperationDefinitionProps<C extends BaseIndexPatternColumn> {
     columns: Record<string, IndexPatternColumn>
   ) => string;
   /**
-   * This function is called if another column in the same layer changed or got removed.
+   * This function is called if another column in the same layer changed or got added/removed.
    * Can be used to update references to other columns (e.g. for sorting).
    * Based on the current column and the other updated columns, this function has to
    * return an updated column. If not implemented, the `id` function is used instead.

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx

@@ -57,7 +57,9 @@ export interface TermsIndexPatternColumn extends FieldBasedIndexPatternColumn {
   operationType: 'terms';
   params: {
     size: number;
-    orderBy: { type: 'alphabetical' } | { type: 'column'; columnId: string };
+    // if order is alphabetical, the `fallback` flag indicates whether it became alphabetical because there wasn't
+    // another option or whether the user explicitly chose to make it alphabetical.
+    orderBy: { type: 'alphabetical'; fallback?: boolean } | { type: 'column'; columnId: string };
     orderDirection: 'asc' | 'desc';
     otherBucket?: boolean;
     missingBucket?: boolean;
@@ -123,7 +125,7 @@ export const termsOperation: OperationDefinition<TermsIndexPatternColumn, 'field
               type: 'column',
               columnId: existingMetricColumn,
             }
-          : { type: 'alphabetical' },
+          : { type: 'alphabetical', fallback: true },
         orderDirection: existingMetricColumn ? 'desc' : 'asc',
         otherBucket: !indexPattern.hasRestrictions,
         missingBucket: false,
@@ -168,16 +170,29 @@ export const termsOperation: OperationDefinition<TermsIndexPatternColumn, 'field
   onOtherColumnChanged: (layer, thisColumnId, changedColumnId) => {
     const columns = layer.columns;
     const currentColumn = columns[thisColumnId] as TermsIndexPatternColumn;
-    if (currentColumn.params.orderBy.type === 'column') {
+    if (currentColumn.params.orderBy.type === 'column' || currentColumn.params.orderBy.fallback) {
       // check whether the column is still there and still a metric
-      const columnSortedBy = columns[currentColumn.params.orderBy.columnId];
-      if (!columnSortedBy || !isSortableByColumn(layer, changedColumnId)) {
+      const columnSortedBy =
+        currentColumn.params.orderBy.type === 'column'
+          ? columns[currentColumn.params.orderBy.columnId]
+          : undefined;
+      if (
+        !columnSortedBy ||
+        (currentColumn.params.orderBy.type === 'column' &&
+          !isSortableByColumn(layer, currentColumn.params.orderBy.columnId))
+      ) {
+        // check whether we can find another metric column to sort by
+        const existingMetricColumn = Object.entries(layer.columns)
+          .filter(([columnId]) => isSortableByColumn(layer, columnId))
+          .map(([id]) => id)[0];
         return {
           ...currentColumn,
           params: {
             ...currentColumn.params,
-            orderBy: { type: 'alphabetical' },
-            orderDirection: 'asc',
+            orderBy: existingMetricColumn
+              ? { type: 'column', columnId: existingMetricColumn }
+              : { type: 'alphabetical', fallback: true },
+            orderDirection: existingMetricColumn ? 'desc' : 'asc',
           },
         };
       }
@@ -197,7 +212,7 @@ export const termsOperation: OperationDefinition<TermsIndexPatternColumn, 'field
 
     function fromValue(value: string): TermsIndexPatternColumn['params']['orderBy'] {
       if (value === 'alphabetical') {
-        return { type: 'alphabetical' };
+        return { type: 'alphabetical', fallback: false };
       }
       const parts = value.split(SEPARATOR);
       return {
@@ -271,16 +286,23 @@ export const termsOperation: OperationDefinition<TermsIndexPatternColumn, 'field
             data-test-subj="indexPattern-terms-orderBy"
             options={orderOptions}
             value={toValue(currentColumn.params.orderBy)}
-            onChange={(e: React.ChangeEvent<HTMLSelectElement>) =>
+            onChange={(e: React.ChangeEvent<HTMLSelectElement>) => {
+              const newOrderByValue = fromValue(e.target.value);
+              const updatedLayer = updateColumnParam({
+                layer,
+                columnId,
+                paramName: 'orderBy',
+                value: newOrderByValue,
+              });
               updateLayer(
                 updateColumnParam({
-                  layer,
+                  layer: updatedLayer,
                   columnId,
-                  paramName: 'orderBy',
-                  value: fromValue(e.target.value),
+                  paramName: 'orderDirection',
+                  value: newOrderByValue.type === 'alphabetical' ? 'asc' : 'desc',
                 })
-              )
-            }
+              );
+            }}
             aria-label={i18n.translate('xpack.lens.indexPattern.terms.orderBy', {
               defaultMessage: 'Rank by',
             })}

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/terms.test.tsx

@@ -358,7 +358,7 @@ describe('terms', () => {
         },
       });
       expect(termsColumn.params).toEqual(
-        expect.objectContaining({ orderBy: { type: 'alphabetical' } })
+        expect.objectContaining({ orderBy: { type: 'alphabetical', fallback: true } })
       );
     });
 
@@ -469,7 +469,7 @@ describe('terms', () => {
       );
       expect(updatedColumn.params).toEqual(
         expect.objectContaining({
-          orderBy: { type: 'alphabetical' },
+          orderBy: { type: 'alphabetical', fallback: true },
         })
       );
     });
@@ -516,7 +516,7 @@ describe('terms', () => {
       );
       expect(updatedColumn.params).toEqual(
         expect.objectContaining({
-          orderBy: { type: 'alphabetical' },
+          orderBy: { type: 'alphabetical', fallback: true },
         })
       );
     });
@@ -548,7 +548,7 @@ describe('terms', () => {
       );
       expect(termsColumn.params).toEqual(
         expect.objectContaining({
-          orderBy: { type: 'alphabetical' },
+          orderBy: { type: 'alphabetical', fallback: true },
         })
       );
     });
@@ -592,7 +592,81 @@ describe('terms', () => {
       );
       expect(termsColumn.params).toEqual(
         expect.objectContaining({
-          orderBy: { type: 'alphabetical' },
+          orderBy: { type: 'alphabetical', fallback: true },
+        })
+      );
+    });
+
+    it('should set order to ascending if falling back to alphabetical', () => {
+      const termsColumn = termsOperation.onOtherColumnChanged!(
+        {
+          columns: {
+            col2: {
+              label: 'Top value of category',
+              dataType: 'string',
+              isBucketed: true,
+
+              // Private
+              operationType: 'terms',
+              params: {
+                orderBy: { type: 'column', columnId: 'col1' },
+                size: 3,
+                orderDirection: 'desc',
+              },
+              sourceField: 'category',
+            },
+          },
+          columnOrder: [],
+          indexPatternId: '',
+        },
+        'col2',
+        'col1'
+      );
+      expect(termsColumn.params).toEqual(
+        expect.objectContaining({
+          orderDirection: 'asc',
+        })
+      );
+    });
+
+    it('should switch back to descending metric sorting if alphabetical sorting was applied as fallback', () => {
+      const initialColumn: TermsIndexPatternColumn = {
+        label: 'Top value of category',
+        dataType: 'string',
+        isBucketed: true,
+
+        // Private
+        operationType: 'terms',
+        params: {
+          orderBy: { type: 'alphabetical', fallback: true },
+          size: 3,
+          orderDirection: 'asc',
+        },
+        sourceField: 'category',
+      };
+      const updatedColumn = termsOperation.onOtherColumnChanged!(
+        {
+          indexPatternId: '',
+          columnOrder: [],
+          columns: {
+            col2: initialColumn,
+            col1: {
+              label: 'Count',
+              dataType: 'number',
+              isBucketed: false,
+              sourceField: 'Records',
+              operationType: 'count',
+            },
+          },
+        },
+        'col2',
+        'col1'
+      );
+
+      expect(updatedColumn.params).toEqual(
+        expect.objectContaining({
+          orderBy: { type: 'column', columnId: 'col1' },
+          orderDirection: 'desc',
         })
       );
     });
@@ -774,6 +848,7 @@ describe('terms', () => {
                 type: 'column',
                 columnId: 'col2',
               },
+              orderDirection: 'desc',
             },
           },
         },

x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.test.ts

@@ -352,6 +352,50 @@ describe('state_helpers', () => {
       ).toEqual(expect.objectContaining({ columnOrder: ['col1', 'col2'] }));
     });
 
+    it('should call onOtherColumn changed on existing columns', () => {
+      expect(
+        insertNewColumn({
+          layer: {
+            indexPatternId: '1',
+            columnOrder: ['col1'],
+            columns: {
+              col1: {
+                label: 'Top values of source',
+                dataType: 'string',
+                isBucketed: true,
+
+                // Private
+                operationType: 'terms',
+                sourceField: 'source',
+                params: {
+                  orderBy: { type: 'alphabetical', fallback: true },
+                  orderDirection: 'asc',
+                  size: 5,
+                },
+              },
+            },
+          },
+          columnId: 'col2',
+          indexPattern,
+          op: 'sum',
+          field: indexPattern.fields[2],
+          visualizationGroups: [],
+        })
+      ).toEqual(
+        expect.objectContaining({
+          columns: expect.objectContaining({
+            col1: expect.objectContaining({
+              params: {
+                orderBy: { columnId: 'col2', type: 'column' },
+                orderDirection: 'desc',
+                size: 5,
+              },
+            }),
+          }),
+        })
+      );
+    });
+
     it('should allow multiple metrics', () => {
       expect(
         insertNewColumn({
@@ -908,7 +952,11 @@ describe('state_helpers', () => {
           columns: {
             col1: {
               ...termsColumn,
-              params: { orderBy: { type: 'alphabetical' }, orderDirection: 'asc', size: 5 },
+              params: {
+                orderBy: { type: 'alphabetical', fallback: true },
+                orderDirection: 'asc',
+                size: 5,
+              },
             },
             id1: expect.objectContaining({
               dataType: 'number',
@@ -1624,7 +1672,7 @@ describe('state_helpers', () => {
             ...termsColumn,
             params: {
               ...termsColumn.params,
-              orderBy: { type: 'alphabetical' },
+              orderBy: { type: 'alphabetical', fallback: true },
               orderDirection: 'asc',
             },
           },

x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts

@@ -714,7 +714,11 @@ function addBucket(
     columns: { ...layer.columns, [addedColumnId]: column },
     columnOrder: updatedColumnOrder,
   };
-  return { ...tempLayer, columnOrder: getColumnOrder(tempLayer) };
+  return {
+    ...tempLayer,
+    columns: adjustColumnReferencesForChangedColumn(tempLayer, addedColumnId),
+    columnOrder: getColumnOrder(tempLayer),
+  };
 }
 
 export function reorderByGroups(
@@ -766,7 +770,11 @@ function addMetric(
       [addedColumnId]: column,
     },
   };
-  return { ...tempLayer, columnOrder: getColumnOrder(tempLayer) };
+  return {
+    ...tempLayer,
+    columnOrder: getColumnOrder(tempLayer),
+    columns: adjustColumnReferencesForChangedColumn(tempLayer, addedColumnId),
+  };
 }
 
 export function getMetricOperationTypes(field: IndexPatternField) {

x-pack/test/functional/apps/lens/chart_data.ts

@@ -37,19 +37,19 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
     });
 
     const expectedData = [
-      { x: '0.53.251.53', y: 4624.75 },
-      { x: '0.108.3.2', y: 7359.41 },
-      { x: '0.209.80.244', y: 6169.9 },
-      { x: '0.228.1.71', y: 7092.8 },
-      { x: '0.254.91.215', y: 3835.58 },
-      { x: '__other__', y: 5727.24 },
+      { x: '97.220.3.248', y: 19755 },
+      { x: '169.228.188.120', y: 18994 },
+      { x: '78.83.247.30', y: 17246 },
+      { x: '226.82.228.233', y: 15687 },
+      { x: '93.28.27.24', y: 15614.33 },
+      { x: '__other__', y: 5722.77 },
     ];
 
     function assertMatchesExpectedData(state: DebugState) {
       expect(
         state.bars![0].bars.map((bar) => ({
           x: bar.x,
-          y: Math.round(bar.y * 100) / 100,
+          y: Math.floor(bar.y * 100) / 100,
         }))
       ).to.eql(expectedData);
     }
@@ -94,7 +94,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       expect(terms.map((term) => (term === 'Other' ? '__other__' : term))).to.eql(
         expectedData.map(({ x }) => x)
       );
-      expect(values.map((value) => Math.round(100 * Number(value.replace(',', ''))) / 100)).to.eql(
+      expect(values.map((value) => Math.floor(100 * Number(value.replace(',', ''))) / 100)).to.eql(
         expectedData.map(({ y }) => y)
       );
     });