Skip to content

Commit 5237f3c

Browse files
authored
feat: add median agg for totals table (#2388)
https://deephaven.atlassian.net/browse/DH-18856 Requires this change to jsapi (tested): deephaven/deephaven-core#6700 Median works with Totals Table, but does not work with rollups (not supported in the engine). After discussion with Don, the agg is ignored for rollups and displays a warning to such an effect. A lot of the logic changes exist to support that behavior.
1 parent c4ffce7 commit 5237f3c

6 files changed

Lines changed: 134 additions & 27 deletions

File tree

packages/iris-grid/src/IrisGrid.tsx

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3510,15 +3510,32 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
35103510
/**
35113511
* User added, removed, or changed the order of aggregations, or position
35123512
* @param aggregationSettings The new aggregation settings
3513+
* @param added The aggregations that were added
3514+
* @param removed The aggregations that were removed
35133515
*/
3514-
handleAggregationsChange(aggregationSettings: AggregationSettings): void {
3515-
log.debug('handleAggregationsChange', aggregationSettings);
3516-
3517-
this.startLoading(
3518-
`Aggregating ${aggregationSettings.aggregations
3519-
.map(a => a.operation)
3520-
.join(', ')}...`
3516+
handleAggregationsChange(
3517+
aggregationSettings: AggregationSettings,
3518+
added: AggregationOperation[] = [],
3519+
removed: AggregationOperation[] = []
3520+
): void {
3521+
log.debug('handleAggregationsChange', aggregationSettings, added, removed);
3522+
const { rollupConfig } = this.state;
3523+
const isRollup = (rollupConfig?.columns?.length ?? 0) > 0;
3524+
// Do not start loading if this is rollup and added / removed aggregations are prohibited for rollups
3525+
const changes = [...added, ...removed];
3526+
const shouldStartLoading = !(
3527+
isRollup &&
3528+
changes.length > 0 &&
3529+
changes.every(op => AggregationUtils.isRollupProhibited(op))
35213530
);
3531+
3532+
if (shouldStartLoading) {
3533+
this.startLoading(
3534+
`Aggregating ${aggregationSettings.aggregations
3535+
.map(a => a.operation)
3536+
.join(', ')}...`
3537+
);
3538+
}
35223539
this.setState({ aggregationSettings });
35233540
}
35243541

@@ -3528,8 +3545,16 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
35283545
*/
35293546
handleAggregationChange(aggregation: Aggregation): void {
35303547
log.debug('handleAggregationChange', aggregation);
3548+
const { rollupConfig } = this.state;
3549+
const isRollup = (rollupConfig?.columns?.length ?? 0) > 0;
3550+
// Do not start loading if this is rollup and the aggregation is prohibited for rollups
3551+
const shouldStartLoading = !(
3552+
isRollup && AggregationUtils.isRollupProhibited(aggregation.operation)
3553+
);
35313554

3532-
this.startLoading(`Aggregating ${aggregation.operation}...`);
3555+
if (shouldStartLoading) {
3556+
this.startLoading(`Aggregating ${aggregation.operation}...`);
3557+
}
35333558

35343559
this.setState(({ aggregationSettings }) => ({
35353560
selectedAggregation: aggregation,
@@ -4783,6 +4808,7 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
47834808
isRollup={isRollup}
47844809
onChange={this.handleAggregationsChange}
47854810
onEdit={this.handleAggregationEdit}
4811+
dh={model.dh}
47864812
/>
47874813
);
47884814
case OptionType.AGGREGATION_EDIT:

packages/iris-grid/src/IrisGridUtils.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -828,7 +828,10 @@ class IrisGridUtils {
828828
includeDescriptions = true,
829829
} = config ?? {};
830830
const { aggregations = [] } = aggregationSettings ?? {};
831-
const aggregationColumns = aggregations.map(
831+
const filteredAggregations = aggregations.filter(
832+
({ operation }) => !AggregationUtils.isRollupProhibited(operation)
833+
);
834+
const aggregationColumns = filteredAggregations.map(
832835
({ operation, selected, invert }) =>
833836
AggregationUtils.isRollupOperation(operation)
834837
? []
@@ -842,8 +845,8 @@ class IrisGridUtils {
842845

843846
const aggregationMap = {} as Record<AggregationOperation, string[]>;
844847
// Aggregation columns should show first, add them first
845-
for (let i = 0; i < aggregations.length; i += 1) {
846-
aggregationMap[aggregations[i].operation] = aggregationColumns[i];
848+
for (let i = 0; i < filteredAggregations.length; i += 1) {
849+
aggregationMap[filteredAggregations[i].operation] = aggregationColumns[i];
847850
}
848851

849852
if (showNonAggregatedColumns) {

packages/iris-grid/src/sidebar/aggregations/AggregationOperation.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ enum AggregationOperation {
66
ABS_SUM = 'AbsSum',
77
VAR = 'Var',
88
AVG = 'Avg',
9+
MEDIAN = 'Median',
910
STD = 'Std',
1011
FIRST = 'First',
1112
LAST = 'Last',

packages/iris-grid/src/sidebar/aggregations/AggregationUtils.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ export const SELECTABLE_OPTIONS = [
99
AggregationOperation.MAX,
1010
AggregationOperation.VAR,
1111
AggregationOperation.AVG,
12+
AggregationOperation.MEDIAN,
1213
AggregationOperation.STD,
1314
AggregationOperation.FIRST,
1415
AggregationOperation.LAST,
@@ -31,6 +32,20 @@ export const isRollupOperation = (type: AggregationOperation): boolean => {
3132
}
3233
};
3334

35+
/**
36+
* Check if an operation is not supported for a rollup/table grouping
37+
* @param type The operation to check
38+
* @returns True if this operation is not supported for a rollup/table grouping
39+
*/
40+
export const isRollupProhibited = (type: AggregationOperation): boolean => {
41+
switch (type) {
42+
case AggregationOperation.MEDIAN:
43+
return true;
44+
default:
45+
return false;
46+
}
47+
};
48+
3449
/**
3550
* Check if an operation is valid against the given column type
3651
* @param operationType The operation to check
@@ -48,6 +63,7 @@ export const isValidOperation = (
4863
case AggregationOperation.DISTINCT:
4964
case AggregationOperation.UNIQUE:
5065
return true;
66+
case AggregationOperation.MEDIAN:
5167
case AggregationOperation.MIN:
5268
case AggregationOperation.MAX:
5369
return (
@@ -88,6 +104,7 @@ export const getOperationColumnNames = (
88104
export default {
89105
isValidOperation,
90106
isRollupOperation,
107+
isRollupProhibited,
91108
filterValidColumns,
92109
getOperationColumnNames,
93110
};

packages/iris-grid/src/sidebar/aggregations/Aggregations.test.tsx

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import React from 'react';
22
import { render, type RenderResult, screen } from '@testing-library/react';
33
import userEvent from '@testing-library/user-event';
4+
import type { dh as DhType } from '@deephaven/jsapi-types';
45
import Aggregations, { type Aggregation } from './Aggregations';
56
import AggregationOperation from './AggregationOperation';
67
import { SELECTABLE_OPTIONS } from './AggregationUtils';
@@ -13,6 +14,25 @@ function makeAggregation({
1314
return { operation, selected, invert };
1415
}
1516

17+
const MOCK_DH = {
18+
AggregationOperation: {
19+
SUM: 'SUM',
20+
ABS_SUM: 'ABS_SUM',
21+
MIN: 'MIN',
22+
MAX: 'MAX',
23+
VAR: 'VAR',
24+
AVG: 'AVG',
25+
MEDIAN: 'MEDIAN',
26+
STD: 'STD',
27+
FIRST: 'FIRST',
28+
LAST: 'LAST',
29+
COUNT_DISTINCT: 'COUNT_DISTINCT',
30+
DISTINCT: 'DISTINCT',
31+
COUNT: 'COUNT',
32+
UNIQUE: 'UNIQUE',
33+
},
34+
} as unknown as typeof DhType;
35+
1636
function mountAggregations({
1737
settings = { aggregations: [] as Aggregation[], showOnTop: false },
1838
onChange = jest.fn(),
@@ -25,6 +45,7 @@ function mountAggregations({
2545
onChange={onChange}
2646
onEdit={onEdit}
2747
isRollup={isRollup}
48+
dh={MOCK_DH}
2849
/>
2950
);
3051
}
@@ -37,6 +58,7 @@ it('shows all operations in select when no aggregations selected yet', () => {
3758
expect(screen.getByText('Min')).toBeInTheDocument();
3859
expect(screen.getByText('Max')).toBeInTheDocument();
3960
expect(screen.getByText('Avg')).toBeInTheDocument();
61+
expect(screen.getByText('Median')).toBeInTheDocument();
4062
expect(screen.getByText('Std')).toBeInTheDocument();
4163
expect(screen.getByText('First')).toBeInTheDocument();
4264
expect(screen.getByText('Last')).toBeInTheDocument();
@@ -61,7 +83,9 @@ it('adds an aggregation when clicking the add button', async () => {
6183
aggregations: expect.arrayContaining([
6284
expect.objectContaining({ operation: SELECTABLE_OPTIONS[0] }),
6385
]),
64-
})
86+
}),
87+
expect.arrayContaining([SELECTABLE_OPTIONS[0]]),
88+
expect.arrayContaining([])
6589
);
6690
});
6791

@@ -80,7 +104,9 @@ it('deletes an aggregation when clicking the trash can', async () => {
80104
await user.click(buttons[buttons.length - 1]);
81105

82106
expect(onChange).toHaveBeenCalledWith(
83-
expect.objectContaining({ aggregations: [], showOnTop: false })
107+
expect.objectContaining({ aggregations: [], showOnTop: false }),
108+
expect.arrayContaining([]),
109+
expect.arrayContaining([AggregationOperation.SUM])
84110
);
85111
});
86112

packages/iris-grid/src/sidebar/aggregations/Aggregations.tsx

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ import {
2020
} from '@deephaven/components';
2121
import type { DraggableRenderItemProps, Range } from '@deephaven/components';
2222
import { type ModelIndex } from '@deephaven/grid';
23-
import type AggregationOperation from './AggregationOperation';
23+
import type { dh as DhType } from '@deephaven/jsapi-types';
24+
import AggregationOperation from './AggregationOperation';
2425
import AggregationUtils, { SELECTABLE_OPTIONS } from './AggregationUtils';
2526
import './Aggregations.scss';
2627

@@ -40,36 +41,48 @@ export type AggregationSettings = {
4041
export type AggregationsProps = {
4142
isRollup: boolean;
4243
settings: AggregationSettings;
43-
onChange: (settings: AggregationSettings) => void;
44+
onChange: (
45+
settings: AggregationSettings,
46+
added: AggregationOperation[],
47+
removed: AggregationOperation[]
48+
) => void;
4449
onEdit: (aggregation: Aggregation) => void;
50+
dh: typeof DhType;
4551
};
4652

4753
function Aggregations({
4854
isRollup,
4955
settings,
5056
onChange,
5157
onEdit,
58+
dh,
5259
}: AggregationsProps): JSX.Element {
5360
const { aggregations, showOnTop } = settings;
5461
const options = useMemo(
5562
() =>
5663
SELECTABLE_OPTIONS.filter(
5764
option =>
58-
!aggregations.some(aggregation => aggregation.operation === option)
65+
!aggregations.some(aggregation => aggregation.operation === option) &&
66+
!(
67+
option === AggregationOperation.MEDIAN &&
68+
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
69+
// @ts-ignore - MEDIAN is not defined in older version of Core
70+
dh.AggregationOperation.MEDIAN === undefined
71+
)
5972
),
60-
[aggregations]
73+
[aggregations, dh]
6174
);
6275
const [selectedOperation, setSelectedOperation] = useState(options[0]);
6376
const [selectedRanges, setSelectedRanges] = useState<Range[]>([]);
6477
const changeSettings = useCallback(
65-
changedSettings => {
66-
onChange({ ...settings, ...changedSettings });
78+
(changedSettings, added = [], removed = []) => {
79+
onChange({ ...settings, ...changedSettings }, added, removed);
6780
},
6881
[onChange, settings]
6982
);
7083
const changeAggregations = useCallback(
71-
newAggregations => {
72-
changeSettings({ aggregations: newAggregations });
84+
(newAggregations, added = [], removed = []) => {
85+
changeSettings({ aggregations: newAggregations }, added, removed);
7386
},
7487
[changeSettings]
7588
);
@@ -130,17 +143,29 @@ function Aggregations({
130143
);
131144

132145
const handleAdd = useCallback(() => {
133-
changeAggregations([
134-
...aggregations,
135-
{ operation: selectedOperation, selected: [], invert: true },
136-
]);
146+
changeAggregations(
147+
[
148+
...aggregations,
149+
{ operation: selectedOperation, selected: [], invert: true },
150+
],
151+
[selectedOperation]
152+
);
137153
}, [aggregations, selectedOperation, changeAggregations]);
138154

139155
const handleDeleteClicked = useCallback(
140156
(itemIndex: ModelIndex) => {
141-
changeAggregations(
142-
aggregations.filter((aggregation, index) => index !== itemIndex)
157+
const [keep, remove] = aggregations.reduce(
158+
([keepSoFar, removeSoFar], aggregation, index) => {
159+
if (index === itemIndex) {
160+
removeSoFar.push(aggregation.operation);
161+
} else {
162+
keepSoFar.push(aggregation);
163+
}
164+
return [keepSoFar, removeSoFar];
165+
},
166+
[[], []] as [Aggregation[], AggregationOperation[]]
143167
);
168+
changeAggregations(keep, [], remove);
144169
},
145170
[aggregations, changeAggregations]
146171
);
@@ -191,6 +216,9 @@ function Aggregations({
191216
const isRollupOperation = AggregationUtils.isRollupOperation(
192217
item.operation
193218
);
219+
const isRollupProhibited = AggregationUtils.isRollupProhibited(
220+
item.operation
221+
);
194222
const isEditable = !isClone && !isRollupOperation;
195223
return (
196224
<>
@@ -208,6 +236,12 @@ function Aggregations({
208236
<FontAwesomeIcon icon={dhWarningFilled} /> Requires rollup
209237
</span>
210238
)}
239+
{isRollup && isRollupProhibited && (
240+
<span className="small text-warning">
241+
<FontAwesomeIcon icon={dhWarningFilled} /> Not available on
242+
rollups
243+
</span>
244+
)}
211245
</span>
212246
{DraggableItemList.renderBadge({ text: badgeText })}
213247
{DraggableItemList.renderHandle()}

0 commit comments

Comments
 (0)