Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: update Datagrid filter to account for reoccuring metadata columns #10046

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
6 changes: 3 additions & 3 deletions webui/react/src/components/ColumnPickerMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { Loadable } from 'hew/utils/loadable';
import React, { ChangeEvent, useCallback, useMemo, useState } from 'react';
import { FixedSizeList as List } from 'react-window';

import { runColumns } from 'pages/FlatRuns/columns';
import { V1ColumnType, V1LocationType } from 'services/api-ts-sdk';
import { ProjectColumn } from 'types';
import { ensureArray } from 'utils/data';
Expand Down Expand Up @@ -66,6 +65,8 @@ interface ColumnTabProps {
onHeatmapSelectionRemove?: (id: string) => void;
}

const KNOWN_BOOLEAN_COLUMNS = ['archived', 'isExpMultitrial', 'parentArchived'];

const ColumnPickerTab: React.FC<ColumnTabProps> = ({
columnState,
compare,
Expand Down Expand Up @@ -174,8 +175,7 @@ const ColumnPickerTab: React.FC<ColumnTabProps> = ({
({ index, style }: { index: number; style: React.CSSProperties }) => {
const col = filteredColumns[index];
const colType =
(runColumns as readonly string[]).includes(col.column) &&
col.type === V1ColumnType.UNSPECIFIED
KNOWN_BOOLEAN_COLUMNS.includes(col.column) && col.type === V1ColumnType.UNSPECIFIED
? 'BOOLEAN'
: removeColumnTypePrefix(col.type);
const getColDisplayName = (col: ProjectColumn) => {
Expand Down
3 changes: 2 additions & 1 deletion webui/react/src/components/FilterForm/TableFilter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import FilterForm from 'components/FilterForm/components/FilterForm';
import { FilterFormStore } from 'components/FilterForm/components/FilterFormStore';
import { FormKind } from 'components/FilterForm/components/type';
import { V1ProjectColumn } from 'services/api-ts-sdk';
import { formatColumnKey } from 'utils/flatRun';

interface Props {
loadableColumns: Loadable<V1ProjectColumn[]>;
Expand All @@ -32,7 +33,7 @@ const TableFilter = ({
onIsOpenFilterChange,
}: Props): JSX.Element => {
const columns: V1ProjectColumn[] = Loadable.getOrElse([], loadableColumns).filter(
(column) => !bannedFilterColumns?.has(column.column),
(column) => !bannedFilterColumns?.has(formatColumnKey(column)),
);
const fieldCount = useObservable(formStore.fieldCount);
const formset = useObservable(formStore.formset);
Expand Down
39 changes: 27 additions & 12 deletions webui/react/src/components/FilterForm/components/FilterField.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import dayjs from 'dayjs';
import Badge from 'hew/Badge';
import Button from 'hew/Button';
import DatePicker, { DatePickerProps } from 'hew/DatePicker';
import Icon from 'hew/Icon';
Expand Down Expand Up @@ -32,6 +33,7 @@ import { getMetadataValues } from 'services/api';
import { V1ColumnType, V1LocationType, V1ProjectColumn } from 'services/api-ts-sdk';
import clusterStore from 'stores/cluster';
import userStore from 'stores/users';
import { formatColumnKey, METADATA_SEPARATOR, removeColumnTypePrefix } from 'utils/flatRun';
import { alphaNumericSorter } from 'utils/sort';

import css from './FilterField.module.scss';
Expand Down Expand Up @@ -69,7 +71,10 @@ const FilterField = ({
}: Props): JSX.Element => {
const users = Loadable.getOrElse([], useObservable(userStore.getUsers()));
const resourcePools = Loadable.getOrElse([], useObservable(clusterStore.resourcePools));
const currentColumn = columns.find((c) => c.column === field.columnName);
const currentColumn = useMemo(
() => columns.find((c) => c.type === field.type && c.column === field.columnName),
[columns, field.columnName, field.type],
);

const columnType = useMemo(() => {
if (field.location === V1LocationType.RUNMETADATA && field.type === V1ColumnType.TEXT) {
Expand All @@ -96,19 +101,18 @@ const FilterField = ({
};

const onChangeColumnName = (value: SelectValue) => {
const prevType = currentColumn?.type;
const newColName = value?.toString() ?? '';
const newCol = columns.find((c) => c.column === newColName);
const prevType = field.type;
const [type, newColName] = (value?.toString() ?? '').split(METADATA_SEPARATOR, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

how come 2 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to ensure/enforce the split to a max of 2 entries

Copy link
Contributor

Choose a reason for hiding this comment

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

i know the meaning. i wonder why its 2 instead of 1

Copy link
Contributor

Choose a reason for hiding this comment

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

if it were 1, it would only output an array with length 1.

const newCol = columns.find((c) => c.column === newColName && type === c.type);
if (newCol) {
Observable.batch(() => {
formStore.setFieldColumnName(field.id, newCol);

if ((SpecialColumnNames as ReadonlyArray<string>).includes(newColName)) {
formStore.setFieldOperator(field.id, Operator.Eq);
updateFieldValue(field.id, null);
} else if (prevType !== newCol?.type) {
} else if (prevType !== newCol.type) {
const defaultOperator: Operator =
AvailableOperators[newCol?.type ?? V1ColumnType.UNSPECIFIED][0];
AvailableOperators[newCol.type ?? V1ColumnType.UNSPECIFIED][0];
formStore.setFieldOperator(field.id, defaultOperator);
updateFieldValue(field.id, null);
}
Expand Down Expand Up @@ -218,6 +222,16 @@ const FilterField = ({
[columnType, field.type, formStore, index, inputOpen, parentId],
);

const getColDisplayName = (col: V1ProjectColumn) => {
const colType = removeColumnTypePrefix(col.type);

return (
<>
{col.displayName || col.column} <Badge text={colType} />
</>
);
};

return (
<div className={css.base} data-test-component="FilterField" ref={(node) => drop(node)}>
<ConjunctionContainer
Expand All @@ -232,12 +246,13 @@ const FilterField = ({
autoFocus
data-test="columnName"
dropdownMatchSelectWidth={300}
options={columns.map((col, idx) => ({
key: `${col.column} ${idx}`,
label: col.displayName || col.column,
value: col.column,
options={columns.map((col) => ({
key: formatColumnKey(col, true),
label: getColDisplayName(col),
title: col.displayName || col.column,
value: formatColumnKey(col, true),
}))}
value={field.columnName}
value={`${field.type}${METADATA_SEPARATOR}${field.columnName}`}
width={'100%'}
onChange={onChangeColumnName}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,8 @@ export class FilterFormStore {
col: Pick<V1ProjectColumn, 'location' | 'type' | 'column'>,
): void {
return this.#updateField(id, (form) => {
if (form.columnName === col.column && form.location === col.location) {
const isSameColumn = form.columnName === col.column && form.type === col.type;
if (isSameColumn && form.location === col.location) {
return form;
}
return {
Expand Down
63 changes: 58 additions & 5 deletions webui/react/src/components/MultiSortMenu.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
import Badge from 'hew/Badge';
import Button from 'hew/Button';
import { DirectionType, Sort, validSort } from 'hew/DataGrid/DataGrid';
import Dropdown, { MenuItem } from 'hew/Dropdown';
import Icon from 'hew/Icon';
import Select, { Option } from 'hew/Select';
import { Loadable } from 'hew/utils/loadable';
import { groupBy, mapValues } from 'lodash';
import { Fragment, useMemo } from 'react';

import { runColumns } from 'pages/FlatRuns/columns';
import { V1ColumnType } from 'services/api-ts-sdk';
import { ProjectColumn } from 'types';
import { removeColumnTypePrefix } from 'utils/flatRun';

import css from './MultiSortMenu.module.scss';

Expand All @@ -25,6 +30,7 @@ interface MultiSortRowProps {
onChange?: (sort: Sort) => void;
onRemove?: () => void;
bannedSortColumns: Set<string>;
typeMap: Record<string, V1ColumnType[]>;
}
interface DirectionOptionsProps {
onChange?: (direction: DirectionType) => void;
Expand All @@ -36,6 +42,7 @@ interface ColumnOptionsProps {
onChange?: (column: string) => void;
value?: string;
bannedSortColumns: Set<string>;
typeMap: Record<string, V1ColumnType[]>;
}

export const optionsByColumnType = {
Expand Down Expand Up @@ -153,6 +160,7 @@ const ColumnOptions: React.FC<ColumnOptionsProps> = ({
columns,
value,
bannedSortColumns,
typeMap,
}) => (
<Select
autoFocus
Expand All @@ -161,10 +169,31 @@ const ColumnOptions: React.FC<ColumnOptionsProps> = ({
loading={Loadable.isNotLoaded(columns)}
options={Loadable.getOrElse([], columns)
.filter((c) => !bannedSortColumns.has(c.column))
.map((c) => ({
label: c.displayName || c.column,
value: c.column,
}))}
.map((c) => {
const badges = typeMap[c.column].map((type) => {
const copy =
(runColumns as readonly string[]).includes(c.column) &&
type === V1ColumnType.UNSPECIFIED
? 'BOOLEAN'
: removeColumnTypePrefix(type);
return (
<Fragment key={type}>
<Badge text={copy} />{' '}
</Fragment>
);
});
const title = c.displayName || c.column;
const label = (
<>
{title} {badges}
</>
);
return {
label,
title,
value: c.column,
};
})}
placeholder="Select column"
value={value}
width="100%"
Expand All @@ -178,6 +207,7 @@ const MultiSortRow: React.FC<MultiSortRowProps> = ({
onChange,
onRemove,
bannedSortColumns,
typeMap,
}) => {
const valueType =
Loadable.getOrElse([], columns).find((c) => c.column === sort.column)?.type ||
Expand All @@ -188,6 +218,7 @@ const MultiSortRow: React.FC<MultiSortRowProps> = ({
<ColumnOptions
bannedSortColumns={bannedSortColumns}
columns={columns}
typeMap={typeMap}
value={sort.column}
onChange={(column) => onChange?.({ ...sort, column })}
/>
Expand Down Expand Up @@ -231,14 +262,35 @@ const MultiSort: React.FC<MultiSortProps> = ({ sorts, columns, onChange, bannedS
window.document.body.dispatchEvent(new Event('mousedown', { bubbles: true }));
onChange?.([EMPTY_SORT]);
};
// replace duplicate columns with a single unspecified column for copy
// reasons. maintain types so we can display in the select
const [mergedColumns, typeMap] = useMemo(() => {
const loadableTuple = columns.map((c) => {
const columnGroups = groupBy(c, 'column');
const fixedColumns = Object.values(columnGroups).flatMap((group) => {
if (group.length > 1) {
return [
{
...group[0],
type: V1ColumnType.UNSPECIFIED,
},
];
}
return group;
});
const typeMap = mapValues(columnGroups, (group) => group.map((column) => column.type));
return [fixedColumns, typeMap] as const;
});
return [loadableTuple.map((l) => l[0]), loadableTuple.map((l) => l[1]).getOrElse({})];
}, [columns]);

return (
<div className={css.base} data-test-component="multiSort">
<div>{SORT_MENU_TITLE}</div>
<div className={css.rows} data-test="rows">
{sorts.map((sort, idx) => {
const seenColumns = sorts.slice(0, idx).map((s) => s.column);
const columnOptions = Loadable.map(columns, (cols) =>
const columnOptions = mergedColumns.map((cols) =>
cols.filter((c) => !seenColumns.includes(c.column)),
);
return (
Expand All @@ -247,6 +299,7 @@ const MultiSort: React.FC<MultiSortProps> = ({ sorts, columns, onChange, bannedS
columns={columnOptions}
key={sort.column || idx}
sort={sort}
typeMap={typeMap}
onChange={makeOnRowChange(idx)}
onRemove={makeOnRowRemove(idx)}
/>
Expand Down
2 changes: 1 addition & 1 deletion webui/react/src/e2e/tests/experimentList.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ test.describe('Experiment List', () => {
async () => {
await expect(
tableFilter.filterForm.filter.filterFields.columnName.selectionItem.pwLocator,
).toHaveText('ID');
).toContainText('ID');
await tableFilter.filterForm.filter.filterFields.operator.selectMenuOption('!=');
},
totalExperiments - 1,
Expand Down
2 changes: 1 addition & 1 deletion webui/react/src/pages/FlatRuns/FlatRuns.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ const FlatRuns: React.FC<Props> = ({ projectId, workspaceId, searchId }) => {
const arrayTypeColumns = projectColumns
.getOrElse([])
.filter((col) => col.type === V1ColumnType.ARRAY)
.map((col) => col.column);
.map((col) => formatColumnKey(col));
return arrayTypeColumns;
}, [projectColumns]);

Expand Down
5 changes: 3 additions & 2 deletions webui/react/src/utils/flatRun.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,9 @@ export const removeColumnTypePrefix = (columnName: V1ColumnType): string => {
return columnName.replace('COLUMN_TYPE_', '');
};

/// wanna know why this separator is used? see https://hpe-aiatscale.atlassian.net/browse/ET-785
export const METADATA_SEPARATOR = '\u241F' as const; // TODO: unify after merging PR 10052
/// we want to use special characters as "separators" to prevent user input to match with such when creating the arbitrary metadata
export const COLUMN_SEPARATOR = '␟';
export const METADATA_SEPARATOR = '\u241F' as const;
thiagodallacqua-hpe marked this conversation as resolved.
Show resolved Hide resolved

export const formatColumnKey = (col: ProjectColumn, required = false): string => {
if (required || col.location === V1LocationType.RUNMETADATA)
Expand Down
Loading