-
Notifications
You must be signed in to change notification settings - Fork 917
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.
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
[Fix] VisType switching persistence and selectively show warning #3715
Changes from all commits
226536f
0782d0e
cf6bcb5
2005395
2cd5597
be6cbc4
752a866
6c7997e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,145 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { AggGroupName, AggGroupNames, CreateAggConfigParams } from '../../../../data/common'; | ||
import { Schema } from '../../../../vis_default_editor/public'; | ||
import { getPersistedAggParams } from './get_persisted_agg_params'; | ||
|
||
describe('getPersistedAggParams', () => { | ||
const getSchema = ( | ||
name: string, | ||
group: AggGroupName, | ||
aggFilter: string[] = ['*'], | ||
max: number = Infinity | ||
): Schema => ({ | ||
name, | ||
group, | ||
max, | ||
min: 0, | ||
aggFilter, | ||
defaults: [], | ||
editor: true, | ||
params: [], | ||
title: name, | ||
}); | ||
|
||
test('Should return the same aggConfigParams when the new vis type schemas have the same schemas as the existing schemas', () => { | ||
const aggConfigParams: CreateAggConfigParams[] = [ | ||
{ type: 'avg', schema: 'm1' }, | ||
{ type: 'avg', schema: 'm2' }, | ||
{ type: 'avg', schema: 'b2' }, | ||
{ type: 'avg', schema: 'b2' }, | ||
]; | ||
|
||
const schemas = [ | ||
getSchema('m1', AggGroupNames.Metrics), | ||
getSchema('m2', AggGroupNames.Metrics), | ||
getSchema('b2', AggGroupNames.Buckets), | ||
]; | ||
|
||
const persistResult = getPersistedAggParams(aggConfigParams, schemas, schemas); | ||
|
||
expect(persistResult).toEqual(aggConfigParams); | ||
}); | ||
|
||
test('Should select the next compatible schema when aggConfigParam schema exists but exceeds the max count of the schema', () => { | ||
const aggConfigParams: CreateAggConfigParams[] = [ | ||
{ type: 'avg', schema: 'm1' }, | ||
{ type: 'avg', schema: 'm1' }, | ||
{ type: 'avg', schema: 'b2' }, | ||
{ type: 'avg', schema: 'b2' }, | ||
]; | ||
|
||
const schemas = [ | ||
getSchema('m1', AggGroupNames.Metrics, ['avg'], 1), | ||
getSchema('m2', AggGroupNames.Metrics), | ||
getSchema('b2', AggGroupNames.Buckets), | ||
]; | ||
|
||
const persistResult = getPersistedAggParams(aggConfigParams, schemas, schemas); | ||
|
||
const expected: CreateAggConfigParams[] = [...aggConfigParams]; | ||
expected[1].schema = 'm2'; | ||
expect(persistResult).toEqual(expected); | ||
}); | ||
|
||
test('Should select the next compatible schema when aggConfigParam schema exists but does not match the group in the new schema', () => { | ||
const aggConfigParams: CreateAggConfigParams[] = [ | ||
{ type: 'avg', schema: 'm1' }, | ||
{ type: 'avg', schema: 'm1' }, | ||
{ type: 'avg', schema: 'b1' }, | ||
{ type: 'avg', schema: 'b1' }, | ||
]; | ||
|
||
const oldSchemas = [ | ||
getSchema('m1', AggGroupNames.Metrics), | ||
getSchema('b1', AggGroupNames.Buckets), | ||
]; | ||
|
||
const newSchemas = [ | ||
getSchema('m1', AggGroupNames.Buckets), | ||
getSchema('m2', AggGroupNames.Metrics), | ||
getSchema('b1', AggGroupNames.Buckets), | ||
]; | ||
|
||
const persistResult = getPersistedAggParams(aggConfigParams, oldSchemas, newSchemas); | ||
|
||
const expected: CreateAggConfigParams[] = [...aggConfigParams]; | ||
expected[0].schema = 'm2'; | ||
expected[1].schema = 'm2'; | ||
expect(persistResult).toEqual(expected); | ||
}); | ||
|
||
test('Should select the next compatible schema with the correct aggfilters', () => { | ||
const aggConfigParams: CreateAggConfigParams[] = [ | ||
{ type: 'count', schema: 'm1' }, | ||
{ type: 'avg', schema: 'm1' }, | ||
{ type: 'avg', schema: 'b1' }, | ||
{ type: 'avg', schema: 'b1' }, | ||
]; | ||
|
||
const oldSchemas = [ | ||
getSchema('m1', AggGroupNames.Metrics), | ||
getSchema('b1', AggGroupNames.Buckets), | ||
]; | ||
|
||
const newSchemas = [ | ||
getSchema('m2', AggGroupNames.Metrics, ['count']), | ||
getSchema('m3', AggGroupNames.Metrics, ['!count']), | ||
getSchema('b1', AggGroupNames.Buckets), | ||
]; | ||
|
||
const persistResult = getPersistedAggParams(aggConfigParams, oldSchemas, newSchemas); | ||
|
||
const expected: CreateAggConfigParams[] = [...aggConfigParams]; | ||
expected[0].schema = 'm2'; | ||
expected[1].schema = 'm3'; | ||
expect(persistResult).toEqual(expected); | ||
}); | ||
|
||
test('Should drop aggConfigParam when no compatible schema is found', () => { | ||
const aggConfigParams: CreateAggConfigParams[] = [ | ||
{ type: 'avg', schema: 'm1' }, | ||
{ type: 'avg', schema: 'm1' }, | ||
{ type: 'avg', schema: 'b1' }, | ||
{ type: 'avg', schema: 'b1' }, | ||
]; | ||
|
||
const oldSchemas = [ | ||
getSchema('m1', AggGroupNames.Metrics), | ||
getSchema('b1', AggGroupNames.Buckets), | ||
]; | ||
|
||
const newSchemas = [ | ||
getSchema('m2', AggGroupNames.Metrics, ['count']), | ||
getSchema('m3', AggGroupNames.Metrics, ['!count'], 1), | ||
getSchema('b1', AggGroupNames.Buckets), | ||
]; | ||
|
||
const persistResult = getPersistedAggParams(aggConfigParams, oldSchemas, newSchemas); | ||
|
||
expect(persistResult.length).toBe(3); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
/* | ||
* Copyright OpenSearch Contributors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
import { CreateAggConfigParams, propFilter } from '../../../../data/common'; | ||
import { Schema } from '../../../../vis_default_editor/public'; | ||
|
||
const filterByType = propFilter('type'); | ||
|
||
export const getPersistedAggParams = ( | ||
aggConfigParams: CreateAggConfigParams[], | ||
oldVisSchemas: Schema[] = [], | ||
newVisSchemas: Schema[] = [] | ||
): CreateAggConfigParams[] => { | ||
const updatedAggConfigParams: CreateAggConfigParams[] = []; | ||
const newVisSchemaCounts: Record<string, number> = newVisSchemas.reduce((acc, schema: Schema) => { | ||
acc[schema.name] = schema.max; | ||
return acc; | ||
}, {}); | ||
|
||
// For each aggConfigParam, check if a compatible schema exists in the new visualization type | ||
aggConfigParams.forEach((aggConfigParam) => { | ||
const currentSchema = oldVisSchemas.find((schema: Schema) => { | ||
return schema.name === aggConfigParam.schema; | ||
}); | ||
|
||
// see if a matching schma exists in the new visualization type | ||
const matchingSchema = newVisSchemas.find((schema: Schema) => { | ||
return schema.name === aggConfigParam.schema; | ||
}); | ||
|
||
// if the matching schema is same as the current schema, add the aggConfigParam to the updatedAggConfigParams | ||
if ( | ||
isSchemaEqual(matchingSchema, currentSchema) && | ||
newVisSchemaCounts[matchingSchema!.name] > 0 | ||
) { | ||
updatedAggConfigParams.push(aggConfigParam); | ||
newVisSchemaCounts[matchingSchema!.name] -= 1; | ||
return; | ||
} | ||
|
||
// if a matching schema does not exist, check if a compatible schema exists | ||
for (const schema of newVisSchemas) { | ||
// Check if the schema group is the same | ||
if (schema.group !== currentSchema!.group) continue; | ||
|
||
const compatibleSchema = filterByType([aggConfigParam], schema.aggFilter).length !== 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain more what this line does? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the main reason for the change. So each schema today can define a list of agg types it supports. Its not a simple lift and it can sometimes be something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, make sense |
||
|
||
if (compatibleSchema && newVisSchemaCounts[schema.name] > 0) { | ||
updatedAggConfigParams.push({ | ||
...aggConfigParam, | ||
schema: schema.name, | ||
}); | ||
newVisSchemaCounts[schema.name] -= 1; | ||
break; | ||
} | ||
} | ||
}); | ||
|
||
return updatedAggConfigParams; | ||
}; | ||
|
||
function isSchemaEqual(schema1?: Schema, schema2?: Schema) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think the naming There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No this is common. Having an undefined check in the beginning of a function does not change the purpose of the function. The decision to return false instead of throwing an error however is subjective since the main purpose of the function is to check if two schemas are equal. If either or both is undefined, for the purposes of this check, that's correct. |
||
// Check if schema1 and schema2 exist | ||
if (!schema1 || !schema2) return false; | ||
|
||
if (schema1.name !== schema2.name) return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that line needed? Seemed like if schema1 and schema2 already exists, they should already have same schema.name since they are both found by using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah its a little redundant, but are you okay with it going through though? All the tests have passed and making this change would require all the tests to pass again. |
||
if (schema1.group !== schema2.group) return false; | ||
|
||
// Check if aggFilter is the same | ||
if (schema1.aggFilter.length !== schema2.aggFilter.length) return false; | ||
for (let i = 0; i < schema1.aggFilter.length; i++) { | ||
if (schema1.aggFilter[i] !== schema2.aggFilter[i]) return false; | ||
} | ||
|
||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo schma --> schema