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

Jump to bottom

TablePanel: Add support for Count calculation per column or per entire dataset #58134

Merged
merged 10 commits into from Nov 28, 2022
Merged

TablePanel: Add support for Count calculation per column or per entire dataset #58134

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fc371ae
WIP
mdvictor Nov 3, 2022
af6d441
TablePanel: Add support for Count calculation per column or per entir…
mdvictor Nov 3, 2022
aca5c2b
refactor
mdvictor Nov 3, 2022
a3951f8
refactor
mdvictor Nov 4, 2022
f8eb71e
Merge branch 'main' into mdvictor/table-footer-count
mdvictor Nov 6, 2022
480930f
refactor + fixes
mdvictor Nov 9, 2022
d5cce99
refactor + tests
mdvictor Nov 10, 2022
bf6329f
Merge branch 'main' into mdvictor/table-footer-count
mdvictor Nov 10, 2022
ba8802f
Merge branch 'main' into mdvictor/table-footer-count
mdvictor Nov 24, 2022
1c6a30b
Docs and cue model fix
mdvictor Nov 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion .betterer.results
View file
Open in desktop
Expand Up @@ -1502,7 +1502,8 @@ exports[`better eslint`] = {
],
"packages/grafana-ui/src/components/Table/FooterRow.tsx:5381": [
[0, 0, 0, "Do not use any type assertions.", "0"],
[0, 0, 0, "Unexpected any. Specify a different type.", "1"]
[0, 0, 0, "Unexpected any. Specify a different type.", "1"],
[0, 0, 0, "Do not use any type assertions.", "2"]
],
"packages/grafana-ui/src/components/Table/HeaderRow.tsx:5381": [
[0, 0, 0, "Unexpected any. Specify a different type.", "0"]
Expand Down
2 changes: 1 addition & 1 deletion packages/grafana-data/src/transformations/fieldReducer.ts
View file
Open in desktop
Expand Up @@ -291,7 +291,7 @@ export function doStandardCalcs(field: Field, ignoreNulls: boolean, nullAsZero:
} as FieldCalcs;

const data = field.values;
calcs.count = data.length;
calcs.count = ignoreNulls ? data.length : data.toArray().filter((val) => val != null).length;

const isNumberField = field.type === FieldType.number || FieldType.time;

Expand Down
13 changes: 9 additions & 4 deletions packages/grafana-ui/src/components/Table/FooterRow.tsx
View file
Open in desktop
Expand Up @@ -14,11 +14,12 @@ export interface FooterRowProps {
footerGroups: HeaderGroup[];
footerValues: FooterItem[];
isPaginationVisible: boolean;
isCountAllSet: boolean;
height: number;
}

export const FooterRow = (props: FooterRowProps) => {
const { totalColumnsWidth, footerGroups, height, isPaginationVisible } = props;
const { totalColumnsWidth, footerGroups, height, isPaginationVisible, isCountAllSet } = props;
const e2eSelectorsTable = selectors.components.Panels.Visualization.Table;
const tableStyles = useStyles2(getTableStyles);

Expand All @@ -41,7 +42,7 @@ export const FooterRow = (props: FooterRowProps) => {
style={height ? { height: `${height}px` } : undefined}
>
{footerGroup.headers.map((column: ColumnInstance, index: number) =>
renderFooterCell(column, tableStyles, height)
isCountAllSet && index > 0 ? EmptyCell : renderFooterCell(column, tableStyles, height, isCountAllSet)
)}
</div>
);
Expand All @@ -50,7 +51,7 @@ export const FooterRow = (props: FooterRowProps) => {
);
};

function renderFooterCell(column: ColumnInstance, tableStyles: TableStyles, height?: number) {
function renderFooterCell(column: ColumnInstance, tableStyles: TableStyles, height?: number, isCountAllSet?: boolean) {
const footerProps = column.getHeaderProps();

if (!footerProps) {
Expand All @@ -71,10 +72,14 @@ function renderFooterCell(column: ColumnInstance, tableStyles: TableStyles, heig
);
}

export function getFooterValue(index: number, footerValues?: FooterItem[]) {
export function getFooterValue(index: number, footerValues?: FooterItem[], isCountAllSet?: boolean) {
if (footerValues === undefined) {
return EmptyCell;
}

if (isCountAllSet) {
return FooterCell({ value: [{ Count: footerValues[index] as string }] });
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we properly narrow the type down, rather than just hoping for the best that the value is a string?

We want to avoid adding more exceptions to .betterer.results that we only have to go in and clean up later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, rename this file to .js and remove all typescript and be prepared to justify the decision 😅

Copy link
Contributor

@joshhunt joshhunt Nov 3, 2022 •
edited

Choose a reason for hiding this comment

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

Is the footerValues property really potentially an array of arrays (of an objects)? Or is the wrong type being used here?

At the moment it looks like if this function is called incorrectly it can lead to a runtime crash as react tries to render an object. I see maybe two approaches to fix this directly here:

Throw an exception directly if the input doesn't match our expectations. This is the current behaviour of the code, but it's being honest about it's potential to crash

  if (isCountAllSet) {
    const count = footerValues[index];
    if (typeof count !== 'string') {
      throw new Error("didn't except a string!");
    }

    return FooterCell({ value: [{ Count: count }] });
  }

Or, all .toString() on the value if it's not a string. This can still (silently) give you something undesirable like [object Object] being printed, but at least grafana doesn't crash.

  if (isCountAllSet) {
    const count = footerValues[index];
    if (!count) {
      return EmptyCell;
    }

    const value = typeof count === 'string' ? count : count?.toString();
    return FooterCell({ value: [{ Count: value }] });
  }

}

return FooterCell({ value: footerValues[index] });
}
31 changes: 22 additions & 9 deletions packages/grafana-ui/src/components/Table/Table.tsx
View file
Open in desktop
Expand Up @@ -177,8 +177,8 @@ export const Table = memo((props: Props) => {

// React-table column definitions
const memoizedColumns = useMemo(
() => getColumns(data, width, columnMinWidth, footerItems),
[data, width, columnMinWidth, footerItems]
() => getColumns(data, width, columnMinWidth, footerItems, footerOptions?.countAll),
[data, width, columnMinWidth, footerItems, footerOptions]
);

// Internal react table state reducer
Expand Down Expand Up @@ -232,14 +232,26 @@ export const Table = memo((props: Props) => {
}

if (footerOptions.show) {
setFooterItems(
getFooterItems(
headerGroups[0].headers as unknown as Array<{ field: Field }>,
createFooterCalculationValues(rows),
footerOptions,
theme
)
const footerItems = getFooterItems(
headerGroups[0].headers as unknown as Array<{ field: Field }>,
createFooterCalculationValues(rows),
footerOptions,
theme
);

if (footerOptions.countAll && footerItems) {
const maxCount = footerItems.reduce((max, item) => {
if (typeof item === 'string' && !isNaN(+item)) {
return Math.max(max, +item);
}
return max;
}, 0);

const footerItemsCountAll: FooterItem[] = [maxCount.toString()];
setFooterItems(footerItemsCountAll);
} else {
setFooterItems(footerItems);
}
} else {
setFooterItems(undefined);
}
Expand Down Expand Up @@ -381,6 +393,7 @@ export const Table = memo((props: Props) => {
<FooterRow
height={footerHeight}
isPaginationVisible={Boolean(enablePagination)}
isCountAllSet={Boolean(props.footerOptions?.countAll)}
footerValues={footerItems}
footerGroups={footerGroups}
totalColumnsWidth={totalColumnsWidth}
Expand Down
1 change: 1 addition & 0 deletions packages/grafana-ui/src/components/Table/types.ts
View file
Open in desktop
Expand Up @@ -50,4 +50,5 @@ export interface TableFooterCalc {
reducer: string[]; // actually 1 value
fields?: string[];
enablePagination?: boolean;
countAll?: boolean;
}
5 changes: 3 additions & 2 deletions packages/grafana-ui/src/components/Table/utils.ts
View file
Open in desktop
Expand Up @@ -61,7 +61,8 @@ export function getColumns(
data: DataFrame,
availableWidth: number,
columnMinWidth: number,
footerValues?: FooterItem[]
footerValues?: FooterItem[],
isCountAllSet?: boolean
): GrafanaTableColumn[] {
const columns: GrafanaTableColumn[] = [];
let fieldCountWithoutWidth = 0;
Expand Down Expand Up @@ -104,7 +105,7 @@ export function getColumns(
minWidth: fieldTableOptions.minWidth ?? columnMinWidth,
filter: memoizeOne(filterByValue(field)),
justifyContent: getTextAlign(field),
Footer: getFooterValue(fieldIndex, footerValues),
Footer: getFooterValue(fieldIndex, footerValues, isCountAllSet),
});
}

Expand Down
1 change: 1 addition & 0 deletions public/app/plugins/panel/table/models.gen.ts
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@ export const defaultPanelOptions: PanelOptions = {
footer: {
zoltanbedi marked this conversation as resolved.
Show resolved Hide resolved
show: false,
reducer: [],
countAll: false,
},
};

Expand Down
7 changes: 7 additions & 0 deletions public/app/plugins/panel/table/module.tsx
View file
Open in desktop
Expand Up @@ -131,6 +131,13 @@ export const plugin = new PanelPlugin<PanelOptions, TableFieldOptions>(TablePane
defaultValue: [ReducerID.sum],
showIf: (cfg) => cfg.footer?.show,
})
.addBooleanSwitch({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good! I'm thinking we should add a description here and maybe change the name of this option as well to make it more clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And I'd say we might to reverse the language, like "Count Per Field" with a description along the lines of "Count the non-empty results of each field separately"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From this I understand we also reverse the functionality. E.g: We show a count for all fields in one column and when the "Count Per Field" switch is active we show a count for each field. This would change all existing table panels for the users. Is this something we'd want?

path: 'footer.countAll',
category: [footerCategory],
name: 'Count all data',
defaultValue: defaultPanelOptions.footer?.countAll,
showIf: (cfg) => cfg.footer?.reducer?.length === 1 && cfg.footer?.reducer[0] === ReducerID.count,
})
.addMultiSelect({
path: 'footer.fields',
category: [footerCategory],
Expand Down