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.

Sign up for GitHub

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

Jump to bottom

Fix pie rows settings include filtered out values #47879

Merged
merged 8 commits into from
Sep 13, 2024
Merged

Fix pie rows settings include filtered out values #47879

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3b6f46c
dedupe a bit
alxnddr Sep 11, 2024
4c28427
remove pie row settings for values not from dataset
alxnddr Sep 11, 2024
f9fec2a
remove unnecessary filtering
alxnddr Sep 11, 2024
8c11154
series -> rows
alxnddr Sep 11, 2024
0465f22
extend spec
alxnddr Sep 11, 2024
fb8dad3
feedback
alxnddr Sep 12, 2024
f2207cd
Merge branch 'master' into fix-pie-rows-settings-include-filtered-out…
alxnddr Sep 13, 2024
96a3b49
prettier
alxnddr Sep 13, 2024
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
Next Next commit
dedupe a bit
  • Loading branch information
@alxnddr
alxnddr committed Sep 11, 2024
commit 3b6f46c85681b901e32f3d50a371dc2211320af6
4 changes: 2 additions & 2 deletions frontend/src/metabase/visualizations/echarts/pie/model/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import type {
PieSliceData,
} from "./types";

function getColDescs(
export function getPieColumns(
rawSeries: RawSeries,
settings: ComputedVisualizationSettings,
): PieColumnDescriptors {
Expand Down Expand Up @@ -71,7 +71,7 @@ export function getPieChartModel(
data: { rows: dataRows },
},
] = rawSeries;
const colDescs = getColDescs(rawSeries, settings);
const colDescs = getPieColumns(rawSeries, settings);

const rowIndiciesByKey = new Map<string | number, number>();
dataRows.forEach((row, index) => {
Expand Down
59 changes: 26 additions & 33 deletions frontend/src/metabase/visualizations/shared/settings/pie.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { getColorsForValues } from "metabase/lib/colors/charts";
import { NULL_DISPLAY_VALUE } from "metabase/lib/constants";
import { checkNotNull, checkNumber, isNumber } from "metabase/lib/types";
import { SLICE_THRESHOLD } from "metabase/visualizations/echarts/pie/constants";
import { getPieColumns } from "metabase/visualizations/echarts/pie/model";
import type { PieRow } from "metabase/visualizations/echarts/pie/model/types";
import type { ShowWarning } from "metabase/visualizations/echarts/types";
import { getNumberOr } from "metabase/visualizations/lib/settings/row-values";
Expand Down Expand Up @@ -148,23 +149,18 @@ export function getPieRows(
) {
const [
{
data: { cols, rows: dataRows },
data: { rows: dataRows },
},
] = rawSeries;

const { metricDesc, dimensionDesc } = getPieColumns(rawSeries, settings);

const getColumnSettings = settings["column"];
if (!getColumnSettings) {
throw Error("`settings.column` is undefined");
}

const dimensionCol = cols.find(c => c.name === settings["pie.dimension"]);
if (dimensionCol == null) {
throw Error(
`Could not find column based on "pie.dimension setting with value ${settings["pie.dimension"]}`,
);
}

const dimensionColSettings = getColumnSettings(dimensionCol);
const dimensionColSettings = getColumnSettings(dimensionDesc.column);

const formatDimensionValue = (value: RowValue) => {
if (value == null) {
Expand All @@ -174,13 +170,6 @@ export function getPieRows(
return formatter(value, dimensionColSettings);
};

const dimensionIndex = cols.findIndex(
col => col.name === settings["pie.dimension"],
);
const metricIndex = cols.findIndex(
col => col.name === settings["pie.metric"],
);

let colors = getColors(rawSeries, settings);
// `pie.colors` is a legacy setting used by old questions for their
// colors. We'll still read it to preserve those color selections, but
Expand All @@ -193,32 +182,36 @@ export function getPieRows(

const savedPieKeys = savedPieRows.map(pieRow => pieRow.key);

const keyToSavedPieRow = new Map<PieRow["key"], PieRow>();
savedPieRows.map(pieRow => keyToSavedPieRow.set(pieRow.key, pieRow));
const keyToSavedPieRow = new Map<PieRow["key"], PieRow>(
savedPieRows.map(pieRow => [pieRow.key, pieRow]),
);

const currentDataRows = getAggregatedRows(
dataRows,
dimensionIndex,
metricIndex,
dimensionDesc.index,
metricDesc.index,
);

const keyToCurrentDataRow = new Map<PieRow["key"], RowValues>();
const currentDataKeys = currentDataRows.map(dataRow => {
const key = getKeyFromDimensionValue(dataRow[dimensionIndex]);
keyToCurrentDataRow.set(key, dataRow);

return key;
});
const keyToCurrentDataRow = new Map<PieRow["key"], RowValues>(
currentDataRows.map(dataRow => [
getKeyFromDimensionValue(dataRow[dimensionDesc.index]),
dataRow,
]),
);
const currentDataKeys = Array.from(keyToCurrentDataRow.keys());

const removed = _.difference(savedPieKeys, currentDataKeys);

let newPieRows: PieRow[] = [];
// Case 1: Auto sorted, sort existing and new rows together
if (settings["pie.sort_rows"]) {
const sortedCurrentDataRows = getSortedRows(currentDataRows, metricIndex);
const sortedCurrentDataRows = getSortedRows(
currentDataRows,
metricDesc.index,
);

newPieRows = sortedCurrentDataRows.map(dataRow => {
const dimensionValue = dataRow[dimensionIndex];
const dimensionValue = dataRow[dimensionDesc.index];
const key = getKeyFromDimensionValue(dimensionValue);
// Historically we have used the dimension value in the `pie.colors`
// setting instead of the key computed above. For compatibility with
Expand Down Expand Up @@ -278,11 +271,11 @@ export function getPieRows(

return dataRow;
});
const sortedAddedRows = getSortedRows(addedRows, metricIndex);
const sortedAddedRows = getSortedRows(addedRows, metricDesc.index);

newPieRows.push(
...sortedAddedRows.map(addedDataRow => {
const dimensionValue = addedDataRow[dimensionIndex];
const dimensionValue = addedDataRow[dimensionDesc.index];

const color = Color(colors[String(dimensionValue)]).hex();
const key = getKeyFromDimensionValue(dimensionValue);
Expand Down Expand Up @@ -323,7 +316,7 @@ export function getPieRows(
return (
currTotal +
checkNumber(
checkNotNull(keyToCurrentDataRow.get(pieRow.key))[metricIndex],
checkNotNull(keyToCurrentDataRow.get(pieRow.key))[metricDesc.index],
)
);
}, 0);
Expand All @@ -335,7 +328,7 @@ export function getPieRows(
}

const metricValue = checkNumber(
checkNotNull(keyToCurrentDataRow.get(pieRow.key))[metricIndex],
checkNotNull(keyToCurrentDataRow.get(pieRow.key))[metricDesc.index],
);
const normalizedPercentage = metricValue / total;

Expand Down