Skip to content

Commit abfc8d5

Browse files
mattseddonmattseddon
authored
Remove column merge logic (#4346)
1 parent 214effb commit abfc8d5

File tree

17 files changed

+168
-658
lines changed

17 files changed

+168
-658
lines changed

‎extension/src/experiments/columns/collect/deps.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import {
44
ColumnAccumulator,
55
limitAncestorDepth,
66
mergeAncestors,
7-
mergeValueColumn
7+
collectColumn
88
} from './util'
99
import { buildDepPath } from '../paths'
1010
import { ColumnType } from '../../webview/contract'
@@ -37,7 +37,7 @@ export const collectDeps = (acc: ColumnAccumulator, data: ExpData) => {
3737
(...pathArray: string[]) => buildDepPath(...pathArray)
3838
)
3939

40-
mergeValueColumn(
40+
collectColumn(
4141
acc,
4242
path,
4343
buildDepPath(...limitedDepthAncestors),

‎extension/src/experiments/columns/collect/index.test.ts‎

Lines changed: 1 addition & 222 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
import { collectChanges, collectColumns, collectRelativeMetricsFiles } from '.'
33
import { timestampColumn } from '../constants'
44
import { buildMetricOrParamPath } from '../paths'
5-
import { Column, ColumnType } from '../../webview/contract'
5+
import { ColumnType } from '../../webview/contract'
66
import outputFixture from '../../../test/fixtures/expShow/base/output'
77
import columnsFixture from '../../../test/fixtures/expShow/base/columns'
88
import workspaceChangesFixture from '../../../test/fixtures/expShow/base/workspaceChanges'
@@ -71,180 +71,6 @@ describe('collectColumns', () => {
7171
expect(columns).toStrictEqual([])
7272
})
7373

74-
const exampleBigNumber = 3000000000
75-
76-
const data: ExpShowOutput = generateTestExpShowOutput(
77-
{
78-
params: {
79-
'params.yaml': {
80-
data: { mixedParam: exampleBigNumber }
81-
}
82-
}
83-
},
84-
{
85-
rev: 'branchA',
86-
data: {
87-
params: {
88-
'params.yaml': {
89-
data: { mixedParam: 'string' }
90-
}
91-
}
92-
},
93-
experiments: [
94-
{
95-
params: {
96-
'params.yaml': {
97-
data: { mixedParam: true }
98-
}
99-
}
100-
}
101-
]
102-
},
103-
{
104-
rev: 'branchB',
105-
data: {
106-
params: {
107-
'params.yaml': {
108-
data: { mixedParam: null }
109-
}
110-
}
111-
}
112-
}
113-
)
114-
115-
const columns = collectColumns(data)
116-
117-
const exampleMixedParam = columns.find(
118-
column =>
119-
column.parentPath ===
120-
buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml')
121-
) as Column
122-
123-
it('should correctly identify mixed type params', () => {
124-
expect(exampleMixedParam.types).toStrictEqual([
125-
'number',
126-
'string',
127-
'boolean',
128-
'null'
129-
])
130-
})
131-
132-
it('should correctly identify a number as the highest string length of a mixed param', () => {
133-
expect(exampleMixedParam.maxStringLength).toStrictEqual(10)
134-
})
135-
136-
it('should add the highest and lowest number from the one present', () => {
137-
expect(exampleMixedParam.maxNumber).toStrictEqual(exampleBigNumber)
138-
expect(exampleMixedParam.minNumber).toStrictEqual(exampleBigNumber)
139-
})
140-
141-
it('should find a different minNumber and maxNumber on a mixed param', () => {
142-
const columns = collectColumns(
143-
generateTestExpShowOutput(
144-
{},
145-
{
146-
rev: 'branchA',
147-
data: {
148-
params: {
149-
'params.yaml': {
150-
data: { mixedNumber: null }
151-
}
152-
}
153-
},
154-
experiments: [
155-
{
156-
params: {
157-
'params.yaml': {
158-
data: { mixedNumber: 0 }
159-
}
160-
}
161-
},
162-
{
163-
params: {
164-
'params.yaml': {
165-
data: { mixedNumber: -1 }
166-
}
167-
}
168-
},
169-
{
170-
params: {
171-
'params.yaml': {
172-
data: { mixedNumber: 1 }
173-
}
174-
}
175-
}
176-
]
177-
}
178-
)
179-
)
180-
181-
const mixedParam = columns.find(
182-
column =>
183-
column.path ===
184-
buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml', 'mixedNumber')
185-
) as Column
186-
187-
expect(mixedParam.minNumber).toStrictEqual(-1)
188-
expect(mixedParam.maxNumber).toStrictEqual(1)
189-
})
190-
191-
const numericColumns = collectColumns(
192-
generateTestExpShowOutput(
193-
{},
194-
{
195-
rev: 'branch1',
196-
data: {
197-
params: {
198-
'params.yaml': {
199-
data: { withNumbers: -1, withoutNumbers: 'a' }
200-
}
201-
}
202-
},
203-
experiments: [
204-
{
205-
params: {
206-
'params.yaml': {
207-
data: { withNumbers: 2, withoutNumbers: 'b' }
208-
}
209-
}
210-
},
211-
{
212-
params: {
213-
'params.yaml': {
214-
data: { withNumbers: 'c', withoutNumbers: 'b' }
215-
}
216-
}
217-
}
218-
]
219-
}
220-
)
221-
)
222-
223-
const param = numericColumns.filter(
224-
column => column.type === ColumnType.PARAMS
225-
)
226-
const paramWithNumbers = param.find(p => p.label === 'withNumbers') as Column
227-
const paramWithoutNumbers = param.find(
228-
p => p.label === 'withoutNumbers'
229-
) as Column
230-
231-
it('should not add a maxNumber or minNumber on a param with no numbers', () => {
232-
expect(paramWithoutNumbers.minNumber).toBeUndefined()
233-
expect(paramWithoutNumbers.maxNumber).toBeUndefined()
234-
})
235-
236-
it('should find the min number of -1', () => {
237-
expect(paramWithNumbers.minNumber).toStrictEqual(-1)
238-
})
239-
240-
it('should find the max number of 2', () => {
241-
expect(paramWithNumbers.maxNumber).toStrictEqual(2)
242-
})
243-
244-
it('should find a max string length of two from -1', () => {
245-
expect(paramWithNumbers.maxStringLength).toStrictEqual(2)
246-
})
247-
24874
it('should aggregate multiple different field names', () => {
24975
const columns = collectColumns(
25076
generateTestExpShowOutput(
@@ -349,53 +175,6 @@ describe('collectColumns', () => {
349175
)
350176
])
351177
})
352-
353-
it('should not report types for params and metrics without primitives or children for params and metrics without objects', () => {
354-
const columns = collectColumns(
355-
generateTestExpShowOutput({
356-
params: {
357-
'params.yaml': {
358-
data: {
359-
onlyHasChild: {
360-
onlyHasPrimitive: 1
361-
}
362-
}
363-
}
364-
}
365-
})
366-
)
367-
368-
const objectParam = columns.find(
369-
column =>
370-
column.parentPath ===
371-
buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml')
372-
) as Column
373-
374-
expect(objectParam.label).toStrictEqual('onlyHasChild')
375-
expect(objectParam.types).toBeUndefined()
376-
377-
const primitiveParam = columns.find(
378-
column =>
379-
column.parentPath ===
380-
buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml', 'onlyHasChild')
381-
) as Column
382-
383-
expect(primitiveParam.label).toStrictEqual('onlyHasPrimitive')
384-
expect(primitiveParam.types).toBeDefined()
385-
386-
const onlyHasPrimitiveChild = columns.find(
387-
column =>
388-
column.parentPath ===
389-
buildMetricOrParamPath(
390-
ColumnType.PARAMS,
391-
'params.yaml',
392-
'onlyHasChild',
393-
'onlyHasPrimitive'
394-
)
395-
) as Column
396-
397-
expect(onlyHasPrimitiveChild).toBeUndefined()
398-
})
399178
})
400179

401180
describe('collectChanges', () => {

‎extension/src/experiments/columns/collect/metricsAndParams.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import {
44
ColumnAccumulator,
55
limitAncestorDepth,
66
mergeAncestors,
7-
mergeValueColumn
7+
collectColumn
88
} from './util'
99
import { ColumnType } from '../../webview/contract'
1010
import {
@@ -44,7 +44,7 @@ const collectMetricOrParam = (
4444
(...pathArray: string[]) => buildMetricOrParamPath(type, ...pathArray)
4545
)
4646

47-
mergeValueColumn(
47+
collectColumn(
4848
acc,
4949
buildMetricOrParamPath(
5050
type,

‎extension/src/experiments/columns/collect/util.ts‎

Lines changed: 6 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -99,22 +99,6 @@ const getValueType = (value: Value) => {
9999
return typeof value
100100
}
101101

102-
const setStringLength = (column: Column, stringLength: number) => {
103-
if (!column.maxStringLength || column.maxStringLength < stringLength) {
104-
column.maxStringLength = stringLength
105-
}
106-
}
107-
108-
const mergeNumberIntoColumn = (column: Column, value: number) => {
109-
const { maxNumber, minNumber } = column
110-
if (maxNumber === undefined || maxNumber < value) {
111-
column.maxNumber = value
112-
}
113-
if (minNumber === undefined || minNumber > value) {
114-
column.minNumber = value
115-
}
116-
}
117-
118102
export const mergeAncestors = (
119103
acc: ColumnAccumulator,
120104
type: ColumnType,
@@ -136,60 +120,34 @@ export const mergeAncestors = (
136120
}
137121
}
138122

139-
const mergeValueIntoColumn = (column: Column, value: Value) => {
140-
const valueType = getValueType(value)
141-
if (!column.types) {
142-
column.types = [valueType]
143-
} else if (!column.types.includes(valueType)) {
144-
column.types.push(valueType)
145-
}
146-
147-
setStringLength(column, String(value).length)
148-
149-
if (valueType === 'number') {
150-
mergeNumberIntoColumn(column, value as number)
151-
}
152-
}
153-
154123
const buildValueColumn = (
155124
path: string,
156125
parentPath: string,
157126
pathArray: string[],
158127
label: string,
159128
value: Value
160129
) => {
161-
const valueType = getValueType(value)
162-
163-
const newColumn: Column = {
130+
return {
131+
firstValueType: getValueType(value),
164132
hasChildren: false,
165133
label,
166-
maxStringLength: String(value).length,
167134
parentPath,
168135
path,
169136
pathArray,
170-
type: pathArray[0] as ColumnType,
171-
types: [valueType]
137+
type: pathArray[0] as ColumnType
172138
}
173-
174-
if (valueType === 'number') {
175-
newColumn.maxNumber = value as number
176-
newColumn.minNumber = value as number
177-
}
178-
179-
return newColumn
180139
}
181140

182-
export const mergeValueColumn = (
141+
export const collectColumn = (
183142
acc: ColumnAccumulator,
184143
path: string,
185144
parentPath: string,
186145
pathArray: string[],
187146
label: string,
188147
value: Value
189148
) => {
190-
if (!acc[path]) {
191-
acc[path] = buildValueColumn(path, parentPath, pathArray, label, value)
149+
if (acc[path]) {
192150
return
193151
}
194-
mergeValueIntoColumn(acc[path], value)
152+
acc[path] = buildValueColumn(path, parentPath, pathArray, label, value)
195153
}

‎extension/src/experiments/columns/constants.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@ import { Column, ColumnType } from '../webview/contract'
33
const type = ColumnType.TIMESTAMP
44

55
export const timestampColumn: Column = {
6+
firstValueType: type,
67
hasChildren: false,
78
label: 'Created',
89
path: 'Created',
9-
type,
10-
types: [type]
10+
type
1111
}
1212

1313
export const EXPERIMENT_COLUMN_ID = 'id'

0 commit comments

Comments
 (0)