Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
2 changes: 1 addition & 1 deletion src/components/QueryResultTable/QueryResultTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ import type {Column, Settings} from '@gravity-ui/react-data-table';
import type {ColumnType, KeyValueRow} from '../../types/api/query';
import {cn} from '../../utils/cn';
import {DEFAULT_TABLE_SETTINGS} from '../../utils/constants';
import {getColumnWidth} from '../../utils/getColumnWidth';
import {getColumnType, prepareQueryResponse} from '../../utils/query';
import {isNumeric} from '../../utils/utils';
import type {ResizeableDataTableProps} from '../ResizeableDataTable/ResizeableDataTable';
import {ResizeableDataTable} from '../ResizeableDataTable/ResizeableDataTable';

import {Cell} from './Cell';
import i18n from './i18n';
import {getColumnWidth} from './utils/getColumnWidth';

import './QueryResultTable.scss';

Expand Down
37 changes: 0 additions & 37 deletions src/components/QueryResultTable/utils/getColumnWidth.test.ts

This file was deleted.

21 changes: 0 additions & 21 deletions src/components/QueryResultTable/utils/getColumnWidth.ts

This file was deleted.

10 changes: 5 additions & 5 deletions src/containers/Tenant/Schema/SchemaViewer/SchemaViewer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,20 @@ export const SchemaViewer = ({type, path, tenantName, extended = false}: SchemaV

const columns = React.useMemo(() => {
if (isViewType(type)) {
return getViewColumns();
return getViewColumns(tableData);
}
if (isExternalTableType(type)) {
return getExternalTableColumns();
return getExternalTableColumns(tableData);
}
if (isColumnEntityType(type)) {
return getColumnTableColumns();
return getColumnTableColumns(tableData);
}
if (isRowTableType(type)) {
return getRowTableColumns(extended, hasAutoIncrement, hasDefaultValue);
return getRowTableColumns(tableData, extended, hasAutoIncrement, hasDefaultValue);
}

return [];
}, [type, extended, hasAutoIncrement, hasDefaultValue]);
}, [type, extended, hasAutoIncrement, hasDefaultValue, tableData]);

if (loading || isViewSchemaLoading) {
return <TableSkeleton />;
Expand Down
39 changes: 32 additions & 7 deletions src/containers/Tenant/Schema/SchemaViewer/columns.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import DataTable from '@gravity-ui/react-data-table';

import {getColumnWidth} from '../../../../utils/getColumnWidth';

import i18n from './i18n';
import type {SchemaColumn, SchemaData} from './types';

Expand Down Expand Up @@ -108,16 +110,37 @@ const compressionColumn: SchemaColumn = {
render: ({row}) => row.columnCodec,
};

export function getViewColumns(): SchemaColumn[] {
return [nameColumn, typeColumn];
const WIDTH_PREDICTION_ROWS_COUNT = 100;

function normalizeColumns(columns: SchemaColumn[], data?: SchemaData[]) {
if (!data) {
return columns;
}
const dataSlice = data.slice(0, WIDTH_PREDICTION_ROWS_COUNT);
return columns.map((column) => {
return {
...column,
width: getColumnWidth({
data: dataSlice,
name: column.name,
header: typeof column.header === 'string' ? column.header : undefined,
sortable: column.sortable === undefined,
Copy link
Member

Choose a reason for hiding this comment

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

sortable could be true, I'd just pass column.sortable here

}),
};
});
}

export function getViewColumns(data?: SchemaData[]): SchemaColumn[] {
return normalizeColumns([nameColumn, typeColumn], data);
}
export function getExternalTableColumns(): SchemaColumn[] {
return [idColumn, nameColumn, typeColumn, notNullColumn];
export function getExternalTableColumns(data?: SchemaData[]): SchemaColumn[] {
return normalizeColumns([idColumn, nameColumn, typeColumn, notNullColumn], data);
}
export function getColumnTableColumns(): SchemaColumn[] {
return [idColumn, nameColumn, typeColumn, notNullColumn];
export function getColumnTableColumns(data?: SchemaData[]): SchemaColumn[] {
return normalizeColumns([idColumn, nameColumn, typeColumn, notNullColumn], data);
}
export function getRowTableColumns(
data: SchemaData[] | undefined,
extended: boolean,
hasAutoIncrement: boolean,
hasDefaultValue: boolean,
Expand All @@ -136,5 +159,7 @@ export function getRowTableColumns(
rowTableColumns.push(autoIncrementColumn);
}

return rowTableColumns;
console.log(normalizeColumns(rowTableColumns, data));
Copy link
Member

Choose a reason for hiding this comment

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

console.log


return normalizeColumns(rowTableColumns, data);
}
93 changes: 93 additions & 0 deletions src/utils/__test__/getColumnWidth.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import {
HEADER_PADDING,
MAX_COLUMN_WIDTH,
PIXELS_PER_CHARACTER,
SORT_ICON_PADDING,
getColumnWidth,
} from '../getColumnWidth';

describe('getColumnWidth', () => {
it('returns minimum width for empty data', () => {
const result = getColumnWidth({data: [], name: 'test'});
expect(result).toBe(HEADER_PADDING + 'test'.length * PIXELS_PER_CHARACTER);
});

it('calculates correct width for string columns', () => {
const data = [{test: 'short'}, {test: 'medium length'}, {test: 'this is a longer string'}];
const result = getColumnWidth({data, name: 'test'});
expect(result).toBe(
HEADER_PADDING + 'this is a longer string'.length * PIXELS_PER_CHARACTER,
);
});

it('calculates correct width for columns with sorting', () => {
const result = getColumnWidth({data: [], name: 'test', sortable: true});
expect(result).toBe(
HEADER_PADDING + SORT_ICON_PADDING + 'test'.length * PIXELS_PER_CHARACTER,
);
});

it('calculates correct width for columns with header', () => {
const result = getColumnWidth({data: [], name: 'test', header: 'a'});
expect(result).toBe(HEADER_PADDING + 'a'.length * PIXELS_PER_CHARACTER);
});

it('returns MAX_COLUMN_WIDTH when calculated width exceeds it', () => {
const data = [{test: 'a'.repeat(100)}];
const result = getColumnWidth({data, name: 'test'});
expect(result).toBe(MAX_COLUMN_WIDTH);
});

it('handles undefined data correctly', () => {
const result = getColumnWidth({name: 'test'});
expect(result).toBe(HEADER_PADDING + 'test'.length * PIXELS_PER_CHARACTER);
});

it('handles missing values in data correctly', () => {
const data = [{test: 'short'}, {}, {test: 'longer string'}];
const result = getColumnWidth({data, name: 'test'});
expect(result).toBe(HEADER_PADDING + 'longer string'.length * PIXELS_PER_CHARACTER);
});

it('uses column name length when all values are shorter', () => {
const data = [{longColumnName: 'a'}, {longColumnName: 'bb'}];
const result = getColumnWidth({data, name: 'longColumnName'});
expect(result).toBe(HEADER_PADDING + 'longColumnName'.length * PIXELS_PER_CHARACTER);
});

it('handles null values in data correctly', () => {
const data = [{test: 'a'}, {test: null}];
const result = getColumnWidth({data, name: 'test'});
expect(result).toBe(HEADER_PADDING + 'test'.length * PIXELS_PER_CHARACTER);
});

it('handles undefined values in data correctly', () => {
const data = [{test: 'a'}, {test: undefined}];
const result = getColumnWidth({data, name: 'test'});
expect(result).toBe(HEADER_PADDING + 'test'.length * PIXELS_PER_CHARACTER);
});

it('handles empty string values in data correctly', () => {
const data = [{test: 'short'}, {test: ''}, {test: 'longer string'}];
const result = getColumnWidth({data, name: 'test'});
expect(result).toBe(HEADER_PADDING + 'longer string'.length * PIXELS_PER_CHARACTER);
});

it('handles an array of numbers correctly', () => {
const data = [{test: 1}, {test: 123}, {test: 12345}];
const result = getColumnWidth({data, name: 'test'});
expect(result).toBe(HEADER_PADDING + '12345'.length * PIXELS_PER_CHARACTER);
});

it('handles an array of mixed data types correctly', () => {
const data = [{test: 'short'}, {test: 123}, {test: null}, {test: 'longer string'}];
const result = getColumnWidth({data, name: 'test'});
expect(result).toBe(HEADER_PADDING + 'longer string'.length * PIXELS_PER_CHARACTER);
});

it('handles empty name correctly', () => {
const data = [{test: 'test'}];
const result = getColumnWidth({data, name: ''});
expect(result).toBe(HEADER_PADDING);
});
});
46 changes: 46 additions & 0 deletions src/utils/getColumnWidth.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import type {SchemaData} from '../containers/Tenant/Schema/SchemaViewer/types';
import type {KeyValueRow} from '../types/api/query';

export const MAX_COLUMN_WIDTH = 600;
export const HEADER_PADDING = 20;
export const SORT_ICON_PADDING = 18;
export const PIXELS_PER_CHARACTER = 10;

export function getColumnWidth<T extends KeyValueRow | SchemaData>({
data,
name,
header,
sortable,
}: {
data?: T[];
name: string;
header?: string;
sortable?: boolean;
}) {
const sortPadding = sortable ? SORT_ICON_PADDING : 0;
let maxColumnContentLength = typeof header === 'string' ? header.length : name.length;

if (data) {
for (const row of data) {
let cellLength = 0;
if (hasProperty(row, name) && row[name]) {
cellLength = String(row[name]).length;
}

maxColumnContentLength = Math.max(maxColumnContentLength, cellLength);

if (
maxColumnContentLength * PIXELS_PER_CHARACTER + HEADER_PADDING >=
MAX_COLUMN_WIDTH
Comment on lines +32 to +34
Copy link
Member

Choose a reason for hiding this comment

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

sortPadding is not used here

) {
return MAX_COLUMN_WIDTH;
}
}
}

return maxColumnContentLength * PIXELS_PER_CHARACTER + HEADER_PADDING + sortPadding;
}

function hasProperty(obj: KeyValueRow | SchemaData, key: string): obj is KeyValueRow {
return key in obj;
}
Copy link
Member

Choose a reason for hiding this comment

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

This type guard actually implicitly do as KeyValueRow to any passed Object type value without any real check. Although it have no influence right now, it's generally not very good practice - it's harder to find error here than in explicit as type, since you expect checks in type guards. And it's less error proof in case we want to make some changes.

You can do following to get rid of any assertions:

  1. Set type T to Record<string, unknown> (getColumnWidth<Record<string, unknown>>)
  2. Convert SchemaData from interface to type so there will be no Index signature for type 'string' is missing error

This will allow to get rid of hasProperty helper and also will make this function more general purpose, since it won't be with specific types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Genius! I've have had so much pain with these types yesterday.

Copy link
Member

Choose a reason for hiding this comment

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

I found this difference between types and interfaces myself only yesterday. Here the issue, that you could read: microsoft/TypeScript#15300, and the most explanatory comment about it: microsoft/TypeScript#15300 (comment)

Loading