Skip to content
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

feat: blind sorting option for vislib #813

Merged
merged 2 commits into from
Sep 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ export const computeSeriesDomainsSelector = createCachedSelector(
customYDomainsByGroupId,
deselectedDataSeries,
settingsSpec.xDomain,
// @ts-ignore blind sort option for vislib
settingsSpec.enableVislibSeriesSort,
);
return domains;
},
Expand Down
4 changes: 4 additions & 0 deletions src/chart_types/xy_chart/state/utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ function getLastValues(formattedDataSeries: {
* @param customYDomainsByGroupId custom Y domains grouped by GroupId
* @param customXDomain if specified in <Settings />, the custom X domain
* @param deselectedDataSeries is optional; if not supplied,
* @param customXDomain is optional; if not supplied,
* @param enableVislibSeriesSort is optional; if not specified in <Settings />,
* then all series will be factored into computations. Otherwise, selectedDataSeries
* is used to restrict the computation for just the selected series
* @returns `SeriesDomainsAndData`
Expand All @@ -202,10 +204,12 @@ export function computeSeriesDomains(
customYDomainsByGroupId: Map<GroupId, YDomainRange> = new Map(),
deselectedDataSeries: SeriesIdentifier[] = [],
customXDomain?: DomainRange | Domain,
enableVislibSeriesSort?: boolean,
): SeriesDomainsAndData {
const { dataSeriesBySpecId, xValues, seriesCollection, fallbackScale } = getDataSeriesBySpecId(
seriesSpecs,
deselectedDataSeries,
enableVislibSeriesSort,
);

// compute the x domain merging any custom domain
Expand Down
172 changes: 118 additions & 54 deletions src/chart_types/xy_chart/utils/series.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,76 +111,138 @@ export function getSeriesIndex(series: SeriesIdentifier[], target: SeriesIdentif
* `y` values and `mark` values are casted to number or null.
* @internal
*/
export function splitSeriesDataByAccessors({
id: specId,
data,
xAccessor,
yAccessors,
y0Accessors,
markSizeAccessor,
splitSeriesAccessors = [],
}: Pick<
BasicSeriesSpec,
'id' | 'data' | 'xAccessor' | 'yAccessors' | 'y0Accessors' | 'splitSeriesAccessors' | 'markSizeAccessor'
>): {
export function splitSeriesDataByAccessors(
{
id: specId,
data,
xAccessor,
yAccessors,
y0Accessors,
markSizeAccessor,
splitSeriesAccessors = [],
}: Pick<
BasicSeriesSpec,
'id' | 'data' | 'xAccessor' | 'yAccessors' | 'y0Accessors' | 'splitSeriesAccessors' | 'markSizeAccessor'
>,
enableVislibSeriesSort = false,
): {
dataSeries: Map<SeriesKey, DataSeries>;
xValues: Array<string | number>;
} {
const dataSeries = new Map<SeriesKey, DataSeries>();
const xValues: Array<string | number> = [];
const nonNumericValues: any[] = [];

data.forEach((datum) => {
const splitAccessors = getSplitAccessors(datum, splitSeriesAccessors);
// if splitSeriesAccessors are defined we should have at least one split value to include datum
if (splitSeriesAccessors.length > 0 && splitAccessors.size < 1) {
return;
}

// skip if the datum is not an object or null
if (typeof datum !== 'object' || datum === null) {
return null;
}
if (enableVislibSeriesSort) {
/*
* This logic is mostly duplicated from below but is a temporary fix before
* https://github.com/elastic/elastic-charts/issues/795 is completed to allow sorting
* The difference from below is that it loops through all the yAsccessors before the data.
*/
yAccessors.forEach((accessor, index) => {
data.forEach((datum) => {
const splitAccessors = getSplitAccessors(datum, splitSeriesAccessors);
// if splitSeriesAccessors are defined we should have at least one split value to include datum
if (splitSeriesAccessors.length > 0 && splitAccessors.size < 1) {
return;
}

const x = getAccessorValue(datum, xAccessor);
// skip if the datum is not an object or null
if (typeof datum !== 'object' || datum === null) {
return;
}

// skip if the x value is not a string or a number
if (typeof x !== 'string' && typeof x !== 'number') {
return null;
}
const x = getAccessorValue(datum, xAccessor);

xValues.push(x);
// skip if the x value is not a string or a number
if (typeof x !== 'string' && typeof x !== 'number') {
return;
}

yAccessors.forEach((accessor, index) => {
const cleanedDatum = extractYandMarkFromDatum(
datum,
accessor,
nonNumericValues,
y0Accessors && y0Accessors[index],
markSizeAccessor,
);
const seriesKeys = [...splitAccessors.values(), accessor];
const seriesKey = getSeriesKey({
specId,
yAccessor: accessor,
splitAccessors,
});
const newDatum = { x, ...cleanedDatum };
const series = dataSeries.get(seriesKey);
if (series) {
series.data.push(newDatum);
} else {
dataSeries.set(seriesKey, {
xValues.push(x);

const cleanedDatum = extractYandMarkFromDatum(
datum,
accessor,
nonNumericValues,
y0Accessors && y0Accessors[index],
markSizeAccessor,
);
const seriesKeys = [...splitAccessors.values(), accessor];
const seriesKey = getSeriesKey({
specId,
yAccessor: accessor,
splitAccessors,
data: [newDatum],
key: seriesKey,
seriesKeys,
});
const newDatum = { x, ...cleanedDatum };
const series = dataSeries.get(seriesKey);
if (series) {
series.data.push(newDatum);
} else {
dataSeries.set(seriesKey, {
specId,
yAccessor: accessor,
splitAccessors,
data: [newDatum],
key: seriesKey,
seriesKeys,
});
}
});
});
} else {
data.forEach((datum) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a level of code duplication in the branch, it'd be OK to distinguish between the cases in a more fine grained way

const splitAccessors = getSplitAccessors(datum, splitSeriesAccessors);
// if splitSeriesAccessors are defined we should have at least one split value to include datum
if (splitSeriesAccessors.length > 0 && splitAccessors.size < 1) {
return;
}

// skip if the datum is not an object or null
if (typeof datum !== 'object' || datum === null) {
return;
}

const x = getAccessorValue(datum, xAccessor);

// skip if the x value is not a string or a number
if (typeof x !== 'string' && typeof x !== 'number') {
return;
}

xValues.push(x);

yAccessors.forEach((accessor, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DRY approach would also favor code review as now it's a bit hard to contrast how the added logic deviates from the preexisting version

const cleanedDatum = extractYandMarkFromDatum(
datum,
accessor,
nonNumericValues,
y0Accessors && y0Accessors[index],
markSizeAccessor,
);
const seriesKeys = [...splitAccessors.values(), accessor];
const seriesKey = getSeriesKey({
specId,
yAccessor: accessor,
splitAccessors,
});
const newDatum = { x, ...cleanedDatum };
const series = dataSeries.get(seriesKey);
if (series) {
series.data.push(newDatum);
} else {
dataSeries.set(seriesKey, {
specId,
yAccessor: accessor,
splitAccessors,
data: [newDatum],
key: seriesKey,
seriesKeys,
});
}
});
});
});
}

if (nonNumericValues.length > 0) {
Logger.warn(
Expand Down Expand Up @@ -350,11 +412,13 @@ function getDataSeriesBySpecGroup(
*
* @param seriesSpecs the map for all the series spec
* @param deselectedDataSeries the array of deselected/hidden data series
* @param enableVislibSeriesSort is optional; if not specified in <Settings />,
* @internal
*/
export function getDataSeriesBySpecId(
seriesSpecs: BasicSeriesSpec[],
deselectedDataSeries: SeriesIdentifier[] = [],
enableVislibSeriesSort?: boolean,
): {
dataSeriesBySpecId: Map<SpecId, DataSeries[]>;
seriesCollection: Map<SeriesKey, SeriesCollectionValue>;
Expand All @@ -377,7 +441,7 @@ export function getDataSeriesBySpecId(
isOrdinalScale = true;
}

const { dataSeries, xValues } = splitSeriesDataByAccessors(spec);
const { dataSeries, xValues } = splitSeriesDataByAccessors(spec, enableVislibSeriesSort);

// filter deleselected dataseries
let filteredDataSeries: DataSeries[] = [...dataSeries.values()];
Expand Down