Skip to content

Commit edb965f

Browse files
dgodinez-dhmofojed
andauthored
fix: DH-17831: Fix partition table losing quick filters (#2682)
- partition tables were losing there quick filters - needed to rebuild the filters on table change for partition tables --------- Co-authored-by: Mike Bender <mikebender@deephaven.io>
1 parent 5028cf1 commit edb965f

4 files changed

Lines changed: 121 additions & 1 deletion

File tree

packages/iris-grid/src/IrisGrid.test.tsx

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,18 @@ import {
1111
import IrisGrid from './IrisGrid';
1212
import IrisGridTestUtils from './IrisGridTestUtils';
1313
import type IrisGridProxyModel from './IrisGridProxyModel';
14+
import { isPartitionedGridModel } from './PartitionedGridModel';
1415

1516
jest.mock('@deephaven/grid', () => ({
1617
...jest.requireActual('@deephaven/grid'),
1718
isExpandableColumnGridModel: jest.fn(),
1819
}));
1920

21+
jest.mock('./PartitionedGridModel', () => ({
22+
...jest.requireActual('./PartitionedGridModel'),
23+
isPartitionedGridModel: jest.fn(() => false),
24+
}));
25+
2026
const { asMock } = TestUtils;
2127

2228
const VIEW_SIZE = 500;
@@ -592,6 +598,29 @@ describe('handleResizeAllColumns', () => {
592598
});
593599
});
594600

601+
describe('handleTableChanged', () => {
602+
it('calls rebuildFilters for a partitioned model with partition mode', () => {
603+
const component = makeComponent();
604+
component.rebuildFilters = jest.fn();
605+
// Set partitionConfig while isPartitionedGridModel is still false so
606+
// IrisGridModelUpdater does not attempt to apply it to the regular model
607+
act(() => {
608+
component.setState({
609+
partitionConfig: { mode: 'partition', partitions: [] },
610+
});
611+
});
612+
// Mock true only for the handleTableChanged call; deps are unchanged so
613+
// IrisGridModelUpdater's updatePartitionConfig won't re-fire during the
614+
// movedColumns setState triggered inside handleTableChanged
615+
asMock(isPartitionedGridModel).mockReturnValue(true);
616+
act(() => {
617+
component.handleTableChanged();
618+
});
619+
asMock(isPartitionedGridModel).mockReturnValue(false);
620+
expect(component.rebuildFilters).toHaveBeenCalled();
621+
});
622+
});
623+
595624
describe('Advanced Filter', () => {
596625
it.each([
597626
{ columnIndex: -1, expectedVisibility: false },

packages/iris-grid/src/IrisGrid.tsx

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3389,6 +3389,11 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
33893389
const { model } = this.props;
33903390
// movedColumns reset triggers metricCalculator update in the Grid component
33913391
this.setState({ movedColumns: model.initialMovedColumns });
3392+
// For partitioned tables, we want to rebuild filters on table change to ensure filters are applied to the new partition
3393+
const { partitionConfig } = this.state;
3394+
if (isPartitionedGridModel(model) && partitionConfig?.mode !== 'keys') {
3395+
this.rebuildFilters();
3396+
}
33923397
}
33933398

33943399
handleViewChanged(metrics?: GridMetrics): void {

packages/iris-grid/src/IrisGridProxyModel.test.ts

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import dh from '@deephaven/jsapi-shim';
22
import type IrisGridModel from './IrisGridModel';
3-
import type IrisGridProxyModel from './IrisGridProxyModel';
3+
import IrisGridProxyModel from './IrisGridProxyModel';
44
import IrisGridTestUtils from './IrisGridTestUtils';
5+
import { type PartitionConfig } from './PartitionedGridModel';
56

67
const irisGridTestUtils = new IrisGridTestUtils(dh);
78

@@ -136,6 +137,77 @@ describe('IrisGridProxyModel', () => {
136137
expect(testUnderlyingSetter).toHaveBeenCalledTimes(1);
137138
});
138139

140+
describe('set partitionConfig', () => {
141+
const mockPartitionedTable = {
142+
getMergedTable: jest.fn(() =>
143+
Promise.resolve(irisGridTestUtils.makeTable())
144+
),
145+
getKeyTable: jest.fn(() =>
146+
Promise.resolve(irisGridTestUtils.makeTable())
147+
),
148+
getKeys: jest.fn(() => Promise.resolve([])),
149+
getTable: jest.fn(() => Promise.resolve(irisGridTestUtils.makeTable())),
150+
keyColumns: [],
151+
columns: [],
152+
close: jest.fn(),
153+
addEventListener: jest.fn(),
154+
removeEventListener: jest.fn(),
155+
};
156+
157+
let setNextModelSpy: jest.SpyInstance;
158+
159+
beforeEach(() => {
160+
setNextModelSpy = jest
161+
.spyOn(IrisGridProxyModel.prototype, 'setNextModel')
162+
.mockImplementation(() => null);
163+
});
164+
165+
afterEach(() => {
166+
setNextModelSpy.mockRestore();
167+
});
168+
169+
function makePartitionedModel() {
170+
return irisGridTestUtils.makeModel(
171+
mockPartitionedTable as unknown as Parameters<
172+
typeof irisGridTestUtils.makeModel
173+
>[0]
174+
);
175+
}
176+
177+
it('does not swap model when the same config reference is set again', () => {
178+
const model = makePartitionedModel();
179+
const config: PartitionConfig = { mode: 'partition', partitions: ['a'] };
180+
model.partitionConfig = config;
181+
setNextModelSpy.mockClear();
182+
model.partitionConfig = config;
183+
expect(setNextModelSpy).not.toHaveBeenCalled();
184+
});
185+
186+
it('does not swap model when a structurally equal config is set', () => {
187+
const model = makePartitionedModel();
188+
model.partitionConfig = { mode: 'partition', partitions: ['a'] };
189+
setNextModelSpy.mockClear();
190+
model.partitionConfig = { mode: 'partition', partitions: ['a'] };
191+
expect(setNextModelSpy).not.toHaveBeenCalled();
192+
});
193+
194+
it('swaps model when mode changes', () => {
195+
const model = makePartitionedModel();
196+
model.partitionConfig = { mode: 'partition', partitions: ['a'] };
197+
setNextModelSpy.mockClear();
198+
model.partitionConfig = { mode: 'merged', partitions: ['a'] };
199+
expect(setNextModelSpy).toHaveBeenCalled();
200+
});
201+
202+
it('swaps model when partition values change', () => {
203+
const model = makePartitionedModel();
204+
model.partitionConfig = { mode: 'partition', partitions: ['a'] };
205+
setNextModelSpy.mockClear();
206+
model.partitionConfig = { mode: 'partition', partitions: ['b'] };
207+
expect(setNextModelSpy).toHaveBeenCalled();
208+
});
209+
});
210+
139211
// Functions must be set on prototype
140212
test('Proxies functions when necessary', () => {
141213
const testFn = jest.fn();

packages/iris-grid/src/IrisGridProxyModel.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,20 @@ class IrisGridProxyModel extends IrisGridModel implements PartitionedGridModel {
314314
if (!this.isPartitionRequired) {
315315
throw new Error('Partitions are not available');
316316
}
317+
// Skip if config hasn't changed to avoid unnecessary model swaps (e.g. on remount)
318+
if (
319+
this.partition === partitionConfig ||
320+
(this.partition != null &&
321+
partitionConfig != null &&
322+
this.partition.mode === partitionConfig.mode &&
323+
this.partition.partitions.length ===
324+
partitionConfig.partitions.length &&
325+
this.partition.partitions.every(
326+
(p, i) => p === partitionConfig.partitions[i]
327+
))
328+
) {
329+
return;
330+
}
317331
log.debug('set partitionConfig', partitionConfig);
318332
this.partition = partitionConfig;
319333

0 commit comments

Comments
 (0)