diff --git a/plugins/pivot/src/js/src/PivotUtils.test.ts b/plugins/pivot/src/js/src/PivotUtils.test.ts index 0b0ee62fa..8f82877be 100644 --- a/plugins/pivot/src/js/src/PivotUtils.test.ts +++ b/plugins/pivot/src/js/src/PivotUtils.test.ts @@ -3,7 +3,10 @@ import { TestUtils } from '@deephaven/test-utils'; import { GRAND_TOTALS_GROUP_NAME, makeColumnGroups, + makeSnapshotColumnGroups, + NULL_KEY_TOKEN, ROOT_DEPTH, + TOTALS_GROUP_NAME, } from './PivotUtils'; const { createMockProxy } = TestUtils; @@ -86,4 +89,97 @@ describe('getColumnGroups', () => { }) ); }); + + it('distinguishes a real null key from a rollup total placeholder', () => { + // Two column sources (e.g. Level, AuthenticatedUser) where the second + // source contains a real null value. The "INFO" rollup total and the + // "INFO + null AuthenticatedUser" leaf both carry the key array + // ['INFO', null] and used to collapse to the same name, producing a + // duplicate grid column. + const levelSource = + createMockProxy({ + name: 'Level', + type: 'string', + }); + const userSource = + createMockProxy({ + name: 'AuthenticatedUser', + type: 'string', + }); + const valueSource = + createMockProxy({ + name: 'Timestamp', + type: 'long', + }); + + // c0: INFO rollup total (depth 2), c1: INFO + null user leaf (depth 3), + // c2: INFO + "iris" user leaf (depth 3) + const keysByIndex = [ + ['INFO', null], + ['INFO', null], + ['INFO', 'iris'], + ]; + const depthByIndex = [2, 3, 3]; + const expandedByIndex = [true, false, false]; + + const snapshotColumns = + createMockProxy({ + offset: 0, + count: 3, + totalCount: 3, + getKeys: jest.fn((i: number) => keysByIndex[i]), + getDepth: jest.fn((i: number) => depthByIndex[i]), + isExpanded: jest.fn((i: number) => expandedByIndex[i]), + }); + + const result = makeSnapshotColumnGroups( + snapshotColumns, + [levelSource, userSource], + [valueSource] + ); + + // The INFO total leaf and the null-user leaf must resolve to distinct + // value columns. + expect(result).toContainEqual( + expect.objectContaining({ + name: 'INFO/AuthenticatedUser', + isTotalGroup: true, + displayName: TOTALS_GROUP_NAME, + children: ['INFO/Timestamp'], + }) + ); + expect(result).toContainEqual( + expect.objectContaining({ + name: `INFO/${NULL_KEY_TOKEN}`, + isTotalGroup: false, + children: [`INFO/${NULL_KEY_TOKEN}/Timestamp`], + }) + ); + expect(result).toContainEqual( + expect.objectContaining({ + name: 'INFO/iris', + isTotalGroup: false, + displayName: 'iris', + children: ['INFO/iris/Timestamp'], + }) + ); + + // The top-level INFO group references all three distinct child groups. + expect(result).toContainEqual( + expect.objectContaining({ + name: 'INFO', + children: [ + 'INFO/AuthenticatedUser', + `INFO/${NULL_KEY_TOKEN}`, + 'INFO/iris', + ], + }) + ); + + // No duplicate leaf value column names across all groups. + const leafNames = result.flatMap(g => + g.children.filter(c => c.endsWith('/Timestamp')) + ); + expect(new Set(leafNames).size).toBe(leafNames.length); + }); }); diff --git a/plugins/pivot/src/js/src/PivotUtils.ts b/plugins/pivot/src/js/src/PivotUtils.ts index 1b30e4c7e..5745ef5ab 100644 --- a/plugins/pivot/src/js/src/PivotUtils.ts +++ b/plugins/pivot/src/js/src/PivotUtils.ts @@ -36,6 +36,32 @@ export const GRAND_TOTALS_GROUP_NAME = 'Grand Total'; export const TOTALS_GROUP_NAME = 'Total'; export const ROOT_DEPTH = 2; +/** + * Reserved token used to represent a real `null` value in a generated column or + * group name. A pivot key can be `null` for two different reasons: + * 1. It is a rollup/total placeholder for a grouping level deeper than the + * column's depth (these slots are always `null`). + * 2. It is an actual `null` value present in the source data. + * Both used to collapse to the same generated name, producing duplicate grid + * columns. Encoding real nulls with this token keeps them distinct. + * + * The token contains a `#`, which `encodeURIComponent` always escapes to `%23`. + * Real values are run through `encodeURIComponent` (see {@link encodeKey}), so + * the encoded form of a real value can never contain a raw `#` and therefore + * can never collide with this token — including the literal string `"#NULL"`, + * which encodes to `%23NULL`. + */ +export const NULL_KEY_TOKEN = '#NULL'; + +/** + * Encode a single pivot key segment for use in a generated name. + * Real `null` values are encoded with {@link NULL_KEY_TOKEN} so they remain + * distinct from rollup placeholders. + */ +function encodeKey(key: unknown): string { + return key == null ? NULL_KEY_TOKEN : encodeURIComponent(String(key)); +} + export type SnapshotDimensionKeys = readonly (unknown | null)[]; export type SnapshotDimensionKeyMap = Map; @@ -159,35 +185,50 @@ export function makeGrandTotalColumnName( } /** - * Create a column name for the grid based on the pivot dimension keys and depth + * Create a column name for the grid based on the pivot dimension keys and depth. + * Only the real grouping levels (`depth - 1` of them) are included; the + * remaining keys are rollup placeholders. Real `null` values are preserved via + * {@link NULL_KEY_TOKEN} so they don't collide with rollup placeholders. + * @param keys Column keys + * @param depth Snapshot depth of the column (1-based: grand total = 1) + * @returns Generated column name */ export function makeColumnName( keys: SnapshotDimensionKeys, depth: number ): string { return keys - .slice(0, depth + 1) - .filter(k => k != null) - .map(k => encodeURIComponent(String(k))) + .slice(0, depth - 1) + .map(encodeKey) .join('/'); } /** - * Get the column group name for a specific depth + * Get the column group name for a specific hierarchy level. + * Slots beyond the column's real grouping depth (`snapshotDepth - 1`) are + * rollup/total placeholders and are named after their column source. Slots + * within the real grouping depth use the actual key value (real `null`s are + * encoded with {@link NULL_KEY_TOKEN}). * @param keys Column keys * @param columnSources Column sources - * @param depth Column depth + * @param level Hierarchy level index (0-based) + * @param snapshotDepth Snapshot depth of the column (1-based: grand total = 1) * @returns Column group name */ export function makeColumnGroupName( keys: SnapshotDimensionKeys, columnSources: readonly CorePlusDhType.coreplus.pivot.PivotSource[], - depth: number + level: number, + snapshotDepth: number ): string { + const groupingDepth = snapshotDepth - 1; return keys - .slice(0, depth + 1) - .map((k, i) => (k == null ? columnSources[i].name : k)) - .map(k => encodeURIComponent(String(k))) + .slice(0, level + 1) + .map((k, i) => + i >= groupingDepth + ? encodeURIComponent(columnSources[i].name) + : encodeKey(k) + ) .join('/'); } @@ -398,17 +439,26 @@ export function makeSnapshotColumnGroups( const keys = snapshotColumns.getKeys(c); const depth = snapshotColumns.getDepth(c); const isExpanded = snapshotColumns.isExpanded(c); + // Number of real grouping levels for this column; slots at or beyond this + // index are rollup/total placeholders rather than real (possibly null) keys. + const groupingDepth = depth - 1; columnSources.forEach((_, i) => { - // Join keys, replace nulls with the source name for the current level - const name = makeColumnGroupName(keys, columnSources, i); - const isTotalGroup = keys[i] == null; - const parentKey = i > 0 ? keys[i - 1] : null; - const totalsGroupDisplayName = parentKey == null ? '' : groupName; + const name = makeColumnGroupName(keys, columnSources, i, depth); + // A level is a total only if it is beyond the real grouping depth. A real + // null value (i < groupingDepth) must not be treated as a total. + const isTotalGroup = i >= groupingDepth; + // Only the first total level (directly under a real group) gets the + // "Total(s)" label; deeper nested total levels are left blank. + const totalsGroupDisplayName = + i > 0 && i - 1 < groupingDepth ? groupName : ''; const group = groupMap.get(name) ?? new PivotColumnHeaderGroup({ name, - displayName: isTotalGroup ? totalsGroupDisplayName : keys[i], + displayName: isTotalGroup + ? totalsGroupDisplayName + : formatValue?.(keys[i], columnSources[i].type) ?? + String(keys[i] ?? ''), isTotalGroup, children: [], depth: maxDepth - i, @@ -421,10 +471,10 @@ export function makeSnapshotColumnGroups( i === columnSources.length - 1 ? // The last group contains all value source columns valueSources.map(v => - makeValueSourceColumnName(makeColumnName(keys, depth - 1), v) + makeValueSourceColumnName(makeColumnName(keys, depth), v) ) : // Add the next group in the hierarchy as a child - [makeColumnGroupName(keys, columnSources, i + 1)] + [makeColumnGroupName(keys, columnSources, i + 1, depth)] ); groupMap.set(name, group); });