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.

Sign up for GitHub

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

Refactor system table filter and sorter #518

Closed
wants to merge 6 commits into from
Closed

Refactor system table filter and sorter #518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c07f0cc
move task and split filter to table header
Nov 16, 2022
6e2ced9
move task and split filter to table header
Nov 16, 2022
5bee0b1
move sorting to table header
Nov 16, 2022
b4947b2
move sorting to table header
Nov 16, 2022
2914cda
add inline comment
Nov 16, 2022
2a5817b
search by system and dataset name
Nov 17, 2022
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: 4 additions & 2 deletions backend/src/impl/db_utils/system_db_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,13 @@ def find_systems(
if ids:
search_conditions.append({"_id": {"$in": [ObjectId(_id) for _id in ids]}})
if system_name:
search_conditions.append({"system_name": {"$regex": rf"^{system_name}.*"}})
search_conditions.append({"system_name": {"$regex": rf"{system_name}.*"}})
Copy link
Member

Choose a reason for hiding this comment

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

I think these would require scanning the entire database which may not scale. Could you please confirm?

if task:
search_conditions.append({"task": task})
if dataset_name:
search_conditions.append({"dataset.dataset_name": dataset_name})
search_conditions.append(
{"dataset.dataset_name": {"$regex": rf"{dataset_name}.*"}}
)
if subdataset_name:
search_conditions.append({"dataset.sub_dataset": subdataset_name})
if source_language:
Expand Down
72 changes: 72 additions & 0 deletions frontend/src/components/SystemsTable/SystemTableContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ import { SystemModel } from "../../models";
import { DeleteOutlined, EditOutlined } from "@ant-design/icons";
import { PrivateIcon, useUser } from "..";
import { generateLeaderboardURL } from "../../utils";
import type { FilterValue, SorterResult } from "antd/es/table/interface";
import type { TablePaginationConfig } from "antd/es/table";
import { FilterUpdate, SystemFilter } from "./SystemFilter";
import { TaskCategory } from "../../clients/openapi";
const { Text } = Typography;

interface Props {
Expand All @@ -30,6 +34,9 @@ interface Props {
setSelectedSystemIDs: React.Dispatch<React.SetStateAction<string[]>>;
onActiveSystemChange: (ids: string[]) => void;
showEditDrawer: (systemIDToEdit: string) => void;
onFilterChange: (value: FilterUpdate) => void;
filterValue: SystemFilter;
taskCategories: TaskCategory[];
}

export function SystemTableContent({
Expand All @@ -44,6 +51,9 @@ export function SystemTableContent({
setSelectedSystemIDs,
onActiveSystemChange,
showEditDrawer,
onFilterChange,
filterValue,
taskCategories,
}: Props) {
const { userInfo } = useUser();
const metricColumns: ColumnsType<SystemModel> = metricNames.map((metric) => ({
Expand All @@ -52,6 +62,7 @@ export function SystemTableContent({
width: 135,
ellipsis: true,
align: "center",
sorter: true,
}));

function showSystemAnalysis(systemID: string) {
Expand All @@ -69,6 +80,11 @@ export function SystemTableContent({
}
}
}
const taskFilterList = taskCategories.flatMap((category) => {
return category.tasks.map((task) => {
return { text: task.name, value: task.name };
});
});

const columns: ColumnsType<SystemModel> = [
{
Expand Down Expand Up @@ -100,6 +116,9 @@ export function SystemTableContent({
fixed: "left",
title: "Task",
render: (value) => <Tag style={{ whiteSpace: "normal" }}>{value}</Tag>,
filters: taskFilterList,
filterMultiple: false,
filteredValue: filterValue.task ? [filterValue.task] : null,
},
{
dataIndex: "dataset_name",
Expand Down Expand Up @@ -132,6 +151,13 @@ export function SystemTableContent({
title: "Dataset Split",
fixed: "left",
align: "center",
filterMultiple: false,
filters: [
{ text: "train", value: "train" },
{ text: "validation", value: "validation" },
{ text: "test", value: "test" },
],
filteredValue: filterValue.split ? [filterValue.split] : null,
},
{
dataIndex: "source_language",
Expand All @@ -158,6 +184,7 @@ export function SystemTableContent({
render: (_, record) => record.created_at.format("MM/DD/YYYY HH:mm"),
width: 130,
align: "center",
sorter: true,
Copy link
Member

Choose a reason for hiding this comment

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

I think the sorters also need to be controlled by filterValue similar to the filters. This would allow the table to react to URL changes and show the default sort field.

},
{
dataIndex: "system_tags",
Expand Down Expand Up @@ -220,6 +247,50 @@ export function SystemTableContent({
},
};

const handleTableChange = (
Copy link
Member

Choose a reason for hiding this comment

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

Instead of implementing all the table change logic in one function, I think it'll be more readable if it is implemented for each column where the filter is defined so you don't have to write these if-else statements to match filter names. There does not seem to be an onSort() so we probably have to write sorter logic in this function though. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lyuyangh, I try to use the onFilter property in ColumnsType definition to call the onFilterChange function to update the filtered system table, but I met some problems.

  • onFilter: (value, record) => boolean: reseting the filter doesn't set the value to be null. onFilter is not called.
  • <Table onChange={onChange} /> must have a onChange function so that the column's onFilter can be called.

Therefore, in PR #528 , I still keep the handleTableChange function since Table's onChange property must have value and filters can get the reset null value directly.

_pagination: TablePaginationConfig,
filters: Record<string, FilterValue | null>,
sorter: SorterResult<SystemModel> | SorterResult<SystemModel>[]
) => {
// Handle filter change
for (const k in filters) {
if (
k === "dataset.split" &&
filters[k]?.toString() !== filterValue.split
) {
onFilterChange({ split: filters[k]?.toString() });
} else if (k === "task" && filters[k]?.toString() !== filterValue.task) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like filters[k] can sometimes be an array so this does not work properly. I would suggest to avoid toString() as a type cast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now the filters only allow one filtered value, so filters[k] is of type FilterValue | null. FilterValue is defined as React.Key | boolean, but the split is of type string | undefined.

In PR #528 , I try to separate the null/undefined and Key/string cases, but I don't find a way to avoid type casting.

onFilterChange({ task: filters[k]?.toString() });
}
}
// Handle sorter change
// Only one sorted column allowed now
if (!(sorter instanceof Array)) {
Copy link
Member

@lyuyangh lyuyangh Nov 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It took me a while to understand how sort columns and directions are determined. I think something like this would be easier to read:

if (!(sorter instanceof Array)) {
      if (sorter.column?.title == null || sorter.order == null) {
        // user cleared sorter, set to default
       // if not currently using the default sorter
            onFilterChange({ sortField: "created_at", sortDir: "desc" });
      } else {
        // user modified sorter, apply sorter to filterValue
        let sortFieldName = sorter.column.title;
        if (sortFieldName === "Created At") sortFieldName = "created_at";
        if (typeof sortFieldName !== "string") {
          console.error(`sortFieldName=${sortFieldName} is not a string`);
        } else {
          onFilterChange({
            sortField: sortFieldName,
            sortDir: sorter.order === "descend" ? "desc" : "asc",
          });
        }
      }
    }

I put the default sorter before the more complex sorter logic. The if statement also handles null checks so we don't have to do it in the else block. I think you added toString() to make tsc happy because column.title is a union of many types. Using toString() forces the value to be a string which may hide bugs though so I think using a type check is the safer thing to do.

const sortFieldName =
sorter.column === undefined
? undefined
: sorter.column.title === "Created At"
? "created_at"
: sorter.column.title?.toString();
if (
sortFieldName !== undefined &&
(sortFieldName !== filterValue.sortField ||
!sorter.order?.toString().startsWith(filterValue.sortDir))
) {
onFilterChange({
sortField: sortFieldName,
sortDir: sorter.order?.toString() === "descend" ? "desc" : "asc",
});
} else if (
sortFieldName === undefined &&
filterValue.sortField !== "created_at"
) {
// Default sorted column
onFilterChange({ sortField: "created_at", sortDir: "desc" });
}
}
};

return (
<div>
<Table
Expand All @@ -237,6 +308,7 @@ export function SystemTableContent({
onChange: (newPage, newPageSize) =>
onPageChange(newPage - 1, newPageSize),
}}
onChange={handleTableChange}
sticky={false}
loading={loading}
scroll={{ x: 100 }}
Expand Down
79 changes: 25 additions & 54 deletions frontend/src/components/SystemsTable/SystemTableTools.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,8 @@ import {
Popconfirm,
Radio,
} from "antd";
import { TaskSelect } from "..";
import { TaskCategory } from "../../clients/openapi";
import {
ArrowDownOutlined,
ArrowUpOutlined,
WarningOutlined,
} from "@ant-design/icons";
import { WarningOutlined } from "@ant-design/icons";
import { SystemModel } from "../../models";
import { LoginState, useUser } from "../useUser";
import { backendClient, parseBackendError } from "../../clients";
Expand Down Expand Up @@ -195,6 +190,24 @@ export function SystemTableTools({
);
}

const handleSearch = (query: string) => {
Copy link
Member

@lyuyangh lyuyangh Nov 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this sends out a request even if the query is incorrect. Also, it sends requests for each character when the user is entering the first part of the query (e.g. "dataset:"). This will increase the load of the backend and it also makes the frontend less responsive because the requests are in sequence.

let systemQuery = undefined;
let datasetQuery = undefined;
const segments = query.split(";");
segments.forEach((seg) => {
const components = seg.split(":");
const property = components[0].trim();
if (property === "system" && components.length > 1) {
systemQuery =
components[1]?.trim().length > 0 ? components[1].trim() : undefined;
} else if (property === "dataset" && components.length > 0) {
datasetQuery =
components[1]?.trim().length > 0 ? components[1].trim() : undefined;
}
});
onChange({ name: systemQuery, dataset: datasetQuery });
};

return (
<div style={{ width: "100%" }}>
<Space style={{ width: "fit-content", float: "left" }}>
Expand All @@ -205,55 +218,13 @@ export function SystemTableTools({
</Space>
<Space style={{ width: "fit-content", float: "right" }}>
{mineVsAllSystemsToggle}
<Select
options={["test", "validation", "train", "all"].map((opt) => ({
value: opt,
label: opt,
}))}
value={value.split || undefined}
placeholder="Dataset split"
onChange={(value) => onChange({ split: value })}
style={{ minWidth: "120px" }}
/>
<TaskSelect
taskCategories={taskCategories}
allowClear
value={value.task || undefined}
onChange={(value) => onChange({ task: value || "" })}
placeholder="All Tasks"
style={{ minWidth: "150px" }}
/>
<div style={{ display: "flex", flexDirection: "row" }}>
<Select
options={[
...metricOptions.map((opt) => ({ value: opt, label: opt })),
{ value: "created_at", label: "Created At" },
]}
value={value.sortField}
onChange={(value) => onChange({ sortField: value })}
style={{ minWidth: "120px" }}
<Tooltip title="Search by system name and/or dataset name. Query format is 'system: target name; dataset: target name'.">
<Input.Search
placeholder="Search by system/dataset name"
onChange={(e) => handleSearch(e.target.value)}
style={{ minWidth: "300px" }}
/>
<Tooltip title="Click to change sort direction">
<Button
icon={
value.sortDir === "asc" ? (
<ArrowUpOutlined />
) : (
<ArrowDownOutlined />
)
}
onClick={() =>
onChange({ sortDir: value.sortDir === "asc" ? "desc" : "asc" })
}
/>
</Tooltip>
</div>

<Input.Search
placeholder="Search by system name"
value={value.name}
onChange={(e) => onChange({ name: e.target.value })}
/>
</Tooltip>

<Select
mode="tags"
Expand Down
3 changes: 3 additions & 0 deletions frontend/src/components/SystemsTable/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,9 @@ export function SystemsTable() {
setSelectedSystemIDs={setSelectedSystemIDs}
onActiveSystemChange={onActiveSystemChange}
showEditDrawer={showEditDrawer}
onFilterChange={onFilterChange}
filterValue={filters}
taskCategories={taskCategories}
/>
<AnalysisDrawer
systems={systems.filter((sys) =>
Expand Down