From ab0e31ba83b0f61423e1be3859e47ac9cac5c71c Mon Sep 17 00:00:00 2001 From: cgu Date: Wed, 3 Feb 2021 13:07:39 -0500 Subject: [PATCH 1/2] Fix sort cell behavior for query results --- .../StatementResultTable.tsx | 130 +++++++++++------- querybook/webapp/hooks/useImmer.ts | 16 +++ querybook/webapp/lib/query-result/detector.ts | 6 +- querybook/webapp/lib/utils/number.ts | 11 +- querybook/webapp/ui/Table/Table.tsx | 27 ++-- 5 files changed, 119 insertions(+), 71 deletions(-) create mode 100644 querybook/webapp/hooks/useImmer.ts diff --git a/querybook/webapp/components/DataDocStatementExecution/StatementResultTable.tsx b/querybook/webapp/components/DataDocStatementExecution/StatementResultTable.tsx index f2ca09da1..0bac94d51 100644 --- a/querybook/webapp/components/DataDocStatementExecution/StatementResultTable.tsx +++ b/querybook/webapp/components/DataDocStatementExecution/StatementResultTable.tsx @@ -1,16 +1,10 @@ -import { produce } from 'immer'; import classNames from 'classnames'; -import React, { - useRef, - useMemo, - useState, - useCallback, - useEffect, -} from 'react'; +import React, { useMemo, useState, useCallback, useEffect } from 'react'; import { useSelector } from 'react-redux'; import styled from 'styled-components'; import { UserSettingsFontSizeToCSSFontSize } from 'const/font'; +import { useImmer } from 'hooks/useImmer'; import { IColumnTransformer } from 'lib/query-result/types'; import { findColumnType } from 'lib/query-result/detector'; import { isNumeric } from 'lib/utils/number'; @@ -76,10 +70,9 @@ export const StatementResultTable: React.FunctionComponent<{ paginate: boolean; maxNumberOfRowsToShow?: number; }> = ({ data, paginate, maxNumberOfRowsToShow = 20, isPreview = false }) => { - const [expandedColumn, setExpandedColumn] = React.useState< - Record - >({}); - const [columnTransformerByIndex, setColumnTransformer] = useState< + const [expandedColumn, toggleExpandedColumn] = useExpandedColumn(); + + const [columnTransformerByIndex, setColumnTransformer] = useImmer< Record >({}); @@ -107,10 +100,9 @@ export const StatementResultTable: React.FunctionComponent<{ ); const setTransformerForColumn = useCallback( (colIndex: number, transformer: IColumnTransformer) => { - setColumnTransformer((old) => ({ - ...old, - [colIndex]: transformer, - })); + setColumnTransformer((old) => { + old[colIndex] = transformer; + }); }, [] ); @@ -120,7 +112,7 @@ export const StatementResultTable: React.FunctionComponent<{ { - if (a == null || a === 'null') { - return -1; - } else if (b == null || b === 'null') { - return 1; - } - const aValue = isNumeric(a) ? Number(a) : String(a); - const bValue = isNumeric(a) ? Number(b) : String(b); - return aValue < bValue ? -1 : aValue > bValue ? 1 : 0; - }} + sortCell={useSortCell(rows)} /> ); }; -function useLastNotNull(value: T): T { - const [s, set] = useState(null); - useEffect(() => { - if (value != null) { - set(value); - } - }, [value]); +function useExpandedColumn() { + const [expandedColumn, setExpandedColumn] = useImmer< + Record + >({}); + const toggleExpandedColumn = useCallback((column: string) => { + setExpandedColumn((old) => { + if (column in old) { + delete old[column]; + } else { + old[column] = true; + } + }); + }, []); + return [expandedColumn, toggleExpandedColumn] as const; +} - return s; +type ColumnSortType = 'string' | 'number'; +function isCellNull(cell: any) { + return cell === 'null' || cell == null; +} + +function useSortCell(rows: string[][]) { + const columnTypeCache: Record = useMemo( + () => ({}), + [rows] + ); + + const sortCell = useCallback( + (colIdx: number, a: any, b: any) => { + if (isCellNull(a)) { + return -1; + } else if (isCellNull(b)) { + return 1; + } + + if (!(colIdx in columnTypeCache)) { + columnTypeCache[colIdx] = getColumnType(rows, colIdx); + } + + const colType = columnTypeCache[colIdx]; + if (colType === 'number') { + a = Number(a); + b = Number(b); + } + return a < b ? -1 : a > b ? 1 : 0; + }, + [rows, columnTypeCache] + ); + return sortCell; +} + +function getColumnType(rows: any[][], colIdx: number): ColumnSortType { + for (const row of rows) { + const cell = row[colIdx]; + if (isCellNull(cell)) { + continue; + } else if (!isNumeric(cell)) { + return 'string'; + } + } + return 'number'; } const StatementResultTableColumn: React.FC<{ column: string; expandedColumn: Record; - setExpandedColumn: (c: Record) => any; + toggleExpandedColumn: (c: string) => any; // These props are for the column info colIndex: number; @@ -217,7 +252,7 @@ const StatementResultTableColumn: React.FC<{ }> = ({ column, expandedColumn, - setExpandedColumn, + toggleExpandedColumn, colIndex, colType, @@ -259,15 +294,7 @@ const StatementResultTableColumn: React.FC<{ e.preventDefault(); e.stopPropagation(); - setExpandedColumn( - produce(expandedColumn, (draft) => { - if (isExpanded) { - delete draft[column]; - } else { - draft[column] = true; - } - }) - ); + toggleExpandedColumn(column); }} /> ); }; + +function useLastNotNull(value: T): T { + const [s, set] = useState(null); + useEffect(() => { + if (value != null) { + set(value); + } + }, [value]); + + return s; +} diff --git a/querybook/webapp/hooks/useImmer.ts b/querybook/webapp/hooks/useImmer.ts new file mode 100644 index 000000000..e3322b396 --- /dev/null +++ b/querybook/webapp/hooks/useImmer.ts @@ -0,0 +1,16 @@ +import produce from 'immer'; +import { useCallback, useState } from 'react'; + +export type SetStateImmer = (f: (draft: S) => void) => void; +export function useImmer( + initialValue: S | (() => S) +): [S, SetStateImmer] { + const [state, setState] = useState(initialValue); + + return [ + state, + useCallback((updater) => { + setState((prevState) => produce(prevState, updater)); + }, []), + ]; +} diff --git a/querybook/webapp/lib/query-result/detector.ts b/querybook/webapp/lib/query-result/detector.ts index fe4f9cb3c..77980a147 100644 --- a/querybook/webapp/lib/query-result/detector.ts +++ b/querybook/webapp/lib/query-result/detector.ts @@ -6,11 +6,7 @@ const columnDetectors: IColumnDetector[] = [ { type: 'string', priority: 0, - checker: (colName: string, values: any[]) => - detectTypeForValues(values, (v) => { - const vType = typeof v; - return vType === 'string'; - }), + checker: (colName: string, values: any[]) => true, }, { type: 'number', diff --git a/querybook/webapp/lib/utils/number.ts b/querybook/webapp/lib/utils/number.ts index 4fd85b871..72cd55357 100644 --- a/querybook/webapp/lib/utils/number.ts +++ b/querybook/webapp/lib/utils/number.ts @@ -75,14 +75,13 @@ export function roundNumberToDecimal(n: number, decimalPlaces: number) { } export function isNumeric(n: any): boolean { - if (typeof n === 'bigint') { - return true; - } - if (typeof n === 'number') { - return !isNaN(n); - } else if (typeof n === 'string') { + if (typeof n === 'string') { // typescript thinks isNaN can only take number return !isNaN(n as any) && !isNaN(parseFloat(n)); + } else if (typeof n === 'number') { + return !isNaN(n); + } else if (typeof n === 'bigint') { + return true; } return false; } diff --git a/querybook/webapp/ui/Table/Table.tsx b/querybook/webapp/ui/Table/Table.tsx index e0bd038f6..02ea450f0 100644 --- a/querybook/webapp/ui/Table/Table.tsx +++ b/querybook/webapp/ui/Table/Table.tsx @@ -28,7 +28,7 @@ export interface ITableProps extends Partial { row: any ) => React.ReactNode; - sortCell?: (cellA: any, cellB: any) => -1 | 0 | 1; + sortCell?: (columnIdx: number, cellA: any, cellB: any) => -1 | 0 | 1; } export class Table extends React.Component { @@ -46,13 +46,23 @@ export class Table extends React.Component { widthObj?: Record, alignObj?: Record ): Column[] { - return columns.map((column: string, columnIndex) => { + return columns.map((column: Column | string, columnIndex: number) => { let formattedColumn: Column; if (typeof column === 'string') { formattedColumn = { Header: titleize(column, '_', ' '), accessor: column, }; + if (widthObj && widthObj[column]) { + formattedColumn.minWidth = widthObj[column]; + } + + if (alignObj && alignObj[column]) { + formattedColumn.style = { + ...formattedColumn.style, + textAlign: alignObj[column], + }; + } } else { formattedColumn = column; } @@ -65,18 +75,7 @@ export class Table extends React.Component { ); } if (sortCell) { - formattedColumn.sortMethod = sortCell; - } - - if (widthObj && widthObj[column]) { - formattedColumn.minWidth = widthObj[column]; - } - - if (alignObj && alignObj[column]) { - formattedColumn.style = { - ...formattedColumn.style, - textAlign: alignObj[column], - }; + formattedColumn.sortMethod = sortCell.bind(null, columnIndex); } return formattedColumn; From 061cf24e7e563c3fd7ed4211c466b33975c59c0e Mon Sep 17 00:00:00 2001 From: cgu Date: Wed, 3 Feb 2021 16:31:46 -0500 Subject: [PATCH 2/2] Resolve comments --- querybook/webapp/ui/Table/Table.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/querybook/webapp/ui/Table/Table.tsx b/querybook/webapp/ui/Table/Table.tsx index 02ea450f0..2e05f73ee 100644 --- a/querybook/webapp/ui/Table/Table.tsx +++ b/querybook/webapp/ui/Table/Table.tsx @@ -59,7 +59,6 @@ export class Table extends React.Component { if (alignObj && alignObj[column]) { formattedColumn.style = { - ...formattedColumn.style, textAlign: alignObj[column], }; }