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

fix: public urls with computed measures and dependant measures are hidden #6520

Merged
merged 6 commits into from
Jan 30, 2025
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 @@ -12,7 +12,6 @@
type MetricsViewSpecDimensionV2,
type V1Expression,
type V1MetricsViewAggregationMeasure,
type V1MetricsViewSpec,
type V1TimeRange,
} from "@rilldata/web-common/runtime-client";
import { runtime } from "@rilldata/web-common/runtime-client/runtime-store";
Expand All @@ -36,7 +35,6 @@
export let whereFilter: V1Expression;
export let metricsViewName: string;
export let dimensionThresholdFilters: DimensionThresholdFilter[];
export let metricsView: V1MetricsViewSpec;
export let visibleMeasureNames: string[];
export let timeControlsReady: boolean;
export let dimension: MetricsViewSpecDimensionV2;
Expand Down Expand Up @@ -102,7 +100,6 @@
$: measures = getMeasuresForDimensionTable(
activeMeasureName,
dimensionThresholdFilters,
metricsView,
visibleMeasureNames,
)
.map(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
MetricsViewSpecDimensionV2,
V1Expression,
V1MetricsViewAggregationMeasure,
V1MetricsViewSpec,
V1TimeRange,
} from "@rilldata/web-common/runtime-client";
import {
Expand All @@ -24,7 +23,6 @@
additionalMeasures,
getFiltersForOtherDimensions,
} from "../selectors";
import { getIndependentMeasures } from "../state-managers/selectors/measures";
import {
createAndExpression,
createOrExpression,
Expand Down Expand Up @@ -60,7 +58,6 @@
export let dimensionThresholdFilters: DimensionThresholdFilter[];
export let activeMeasureName: string;
export let metricsViewName: string;
export let metricsView: V1MetricsViewSpec;
export let sortType: SortType;
export let tableWidth: number;
export let sortedAscending: boolean;
Expand Down Expand Up @@ -125,10 +122,7 @@
undefined,
);

$: measures = getIndependentMeasures(
metricsView,
additionalMeasures(activeMeasureName, dimensionThresholdFilters),
)
$: measures = additionalMeasures(activeMeasureName, dimensionThresholdFilters)
.map(
(n) =>
({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import { getStateManagers } from "@rilldata/web-common/features/dashboards/state-managers/state-managers";
import type {
V1Expression,
V1MetricsViewSpec,
V1TimeRange,
} from "@rilldata/web-common/runtime-client";
import { runtime } from "@rilldata/web-common/runtime-client/runtime-store";
Expand All @@ -22,7 +21,6 @@
export let comparisonTimeRange: V1TimeRange | undefined;
export let timeControlsReady: boolean;
export let activeMeasureName: string;
export let metricsView: V1MetricsViewSpec;

const StateManagers = getStateManagers();
const {
Expand Down Expand Up @@ -108,7 +106,6 @@
isSummableMeasure={$isSummableMeasure}
{parentElement}
{suppressTooltip}
{metricsView}
{timeControlsReady}
selectedValues={$selectedDimensionValues(dimension.name)}
isBeingCompared={$isBeingComparedReadable(dimension.name)}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,14 @@
import { getIndependentMeasures } from "@rilldata/web-common/features/dashboards/state-managers/selectors/measures";
import type { V1MetricsViewSpec } from "@rilldata/web-common/runtime-client";
import { additionalMeasures } from "../../selectors";
import type { DimensionThresholdFilter } from "../../stores/metrics-explorer-entity";

export function getMeasuresForDimensionTable(
activeMeasureName: string,
dimensionThresholdFilters: DimensionThresholdFilter[],
metricsView: V1MetricsViewSpec | undefined,
visibleMeasureNames: string[],
) {
const allMeasures = new Set([
...visibleMeasureNames,
...additionalMeasures(activeMeasureName, dimensionThresholdFilters),
]);
return getIndependentMeasures(metricsView ?? {}, [...allMeasures]);
return [...allMeasures];
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getFilteredMeasuresAndDimensions } from "@rilldata/web-common/features/dashboards/state-managers/selectors/measures";
import { removeSomeAdvancedMeasures } from "@rilldata/web-common/features/dashboards/state-managers/selectors/measures";
import type { MetricsExplorerEntity } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity";
import {
type V1MetricsViewSpec,
Expand All @@ -13,15 +13,7 @@ describe("measures selectors", () => {
title: "with unspecified grains, selected DAY",
measures: ["mes", "mes_time_no_grain"],
timeGrain: V1TimeGrain.TIME_GRAIN_DAY,
expected: {
measures: ["mes", "mes_time_no_grain"],
dimensions: [
{
name: "time",
timeGrain: V1TimeGrain.TIME_GRAIN_UNSPECIFIED,
},
],
},
expectedMeasures: ["mes", "mes_time_no_grain"],
},
{
title: "with unspecified and specified grains, selected DAY",
Expand All @@ -32,15 +24,7 @@ describe("measures selectors", () => {
"mes_time_week_grain",
],
timeGrain: V1TimeGrain.TIME_GRAIN_DAY,
expected: {
measures: ["mes", "mes_time_no_grain", "mes_time_day_grain"],
dimensions: [
{
name: "time",
timeGrain: V1TimeGrain.TIME_GRAIN_DAY,
},
],
},
expectedMeasures: ["mes", "mes_time_no_grain", "mes_time_day_grain"],
},
{
title: "with unspecified and specified grains, selected WEEK",
Expand All @@ -51,15 +35,7 @@ describe("measures selectors", () => {
"mes_time_week_grain",
],
timeGrain: V1TimeGrain.TIME_GRAIN_WEEK,
expected: {
measures: ["mes", "mes_time_no_grain", "mes_time_week_grain"],
dimensions: [
{
name: "time",
timeGrain: V1TimeGrain.TIME_GRAIN_WEEK,
},
],
},
expectedMeasures: ["mes", "mes_time_no_grain", "mes_time_week_grain"],
},
{
title: "with unspecified and specified grains, selected MONTH",
Expand All @@ -70,15 +46,13 @@ describe("measures selectors", () => {
"mes_time_week_grain",
],
timeGrain: V1TimeGrain.TIME_GRAIN_MONTH,
expected: {
measures: ["mes", "mes_time_no_grain"],
dimensions: [
{
name: "time",
timeGrain: V1TimeGrain.TIME_GRAIN_UNSPECIFIED,
},
],
},
expectedMeasures: ["mes", "mes_time_no_grain"],
},
{
title: "with window measure and select it",
measures: ["mes", "window_mes"],
timeGrain: V1TimeGrain.TIME_GRAIN_MONTH,
expectedMeasures: ["mes", "window_mes"],
},
];
const MetricsView: V1MetricsViewSpec = {
Expand Down Expand Up @@ -114,20 +88,44 @@ describe("measures selectors", () => {
},
],
},
{
name: "window_mes",
window: {
partition: true,
},
},
],
};
for (const { title, measures, timeGrain, expected } of TestCases) {
for (const { title, measures, timeGrain, expectedMeasures } of TestCases) {
it(title, () => {
expect(
getFilteredMeasuresAndDimensions({
dashboard: {
removeSomeAdvancedMeasures(
{
selectedTimeRange: {
interval: timeGrain,
},
} as MetricsExplorerEntity,
})(MetricsView, measures),
).toEqual(expected);
MetricsView,
measures,
true,
),
).toEqual(expectedMeasures);
});
}

it("with window measure and do not select it", () => {
expect(
removeSomeAdvancedMeasures(
{
selectedTimeRange: {
interval: V1TimeGrain.TIME_GRAIN_UNSPECIFIED,
},
} as MetricsExplorerEntity,
MetricsView,
["mes", "window_mes"],
false,
),
).toEqual(["mes"]);
});
});
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { MetricsExplorerEntity } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity";
import {
type MetricsViewSpecDimensionSelector,
MetricsViewSpecMeasureType,
type MetricsViewSpecMeasureV2,
type V1MetricsViewSpec,
Expand Down Expand Up @@ -82,95 +82,49 @@ export const filteredSimpleMeasures = ({
};

/**
* Selects measure valid for current dashboard selections.
* Also includes additional dimensions needed for any advanced measures.
* Selects measure valid for current dashboard selections. We filter out advanced measures that are,
* 1. Of type MEASURE_TYPE_TIME_COMPARISON.
* 2. Dependent on a time dimension with a defined grain and not equal to the current selected grain.
* 3. Window measures if includeWindowMeasures=false. Right now totals query does not support these.
*/
export const getFilteredMeasuresAndDimensions = ({
dashboard,
}: Pick<DashboardDataSources, "dashboard">) => {
return (
metricsViewSpec: V1MetricsViewSpec,
measureNames: string[],
): {
measures: string[];
dimensions: MetricsViewSpecDimensionSelector[];
} => {
const dimensions = new Map<string, V1TimeGrain>();
const measures = new Set<string>();
measureNames.forEach((measureName) => {
const measure = metricsViewSpec.measures?.find(
(m) => m.name === measureName,
);
if (
!measure ||
measure.type === MetricsViewSpecMeasureType.MEASURE_TYPE_TIME_COMPARISON
// TODO: we need to send a single query for this support
// (measure.type ===
// MetricsViewSpecMeasureType.MEASURE_TYPE_TIME_COMPARISON &&
// (!dashboard.showTimeComparison ||
// !dashboard.selectedComparisonTimeRange))
)
return;

let skipMeasure = false;
measure.requiredDimensions?.forEach((reqDim) => {
if (
reqDim.timeGrain !== V1TimeGrain.TIME_GRAIN_UNSPECIFIED &&
reqDim.timeGrain !== dashboard.selectedTimeRange?.interval
) {
// filter out measures with dependant dimensions not matching the selected grain
skipMeasure = true;
return;
}
if (!reqDim.name) return;

const existingEntry = dimensions.get(reqDim.name);
if (existingEntry) {
if (existingEntry === V1TimeGrain.TIME_GRAIN_UNSPECIFIED) {
dimensions.set(
reqDim.name,
reqDim.timeGrain ?? V1TimeGrain.TIME_GRAIN_UNSPECIFIED,
);
} else {
// mismatching measures are requested
skipMeasure = true;
}
return;
}

dimensions.set(
reqDim.name,
reqDim.timeGrain ?? V1TimeGrain.TIME_GRAIN_UNSPECIFIED,
);
});
if (skipMeasure) return;
measures.add(measureName);
measure.referencedMeasures?.filter((refMes) => measures.add(refMes));
});
return {
measures: [...measures],
dimensions: [...dimensions.entries()].map(([name, timeGrain]) => ({
name,
timeGrain,
})),
};
};
};

export const getIndependentMeasures = (
export const removeSomeAdvancedMeasures = (
exploreState: MetricsExplorerEntity,
metricsViewSpec: V1MetricsViewSpec,
measureNames: string[],
includeWindowMeasures: boolean,
) => {
const measures = new Set<string>();
measureNames.forEach((measureName) => {
const measure = metricsViewSpec.measures?.find(
(m) => m.name === measureName,
);
// temporary check for window measures until the PR to move to AggregationApi is merged
if (!measure || measure.requiredDimensions?.length || !!measure.window)
if (
!measure ||
measure.type ===
MetricsViewSpecMeasureType.MEASURE_TYPE_TIME_COMPARISON ||
(!includeWindowMeasures && measure.window)
// TODO: we need to send a single query for this support
// (measure.type ===
// MetricsViewSpecMeasureType.MEASURE_TYPE_TIME_COMPARISON &&
// (!dashboard.showTimeComparison ||
// !dashboard.selectedComparisonTimeRange))
)
return;

let skipMeasure = false;
measure.requiredDimensions?.forEach((reqDim) => {
if (
reqDim.timeGrain !== V1TimeGrain.TIME_GRAIN_UNSPECIFIED &&
reqDim.timeGrain !== exploreState.selectedTimeRange?.interval
) {
// filter out measures with dependant dimensions not matching the selected grain
skipMeasure = true;
return;
}
});
if (skipMeasure) return;

measures.add(measureName);
measure.referencedMeasures?.filter((refMes) => measures.add(refMes));
});
Comment on lines 96 to 128
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this should be a measures.filter() statement

Copy link
Collaborator Author

@AdityaHegde AdityaHegde Jan 30, 2025

Choose a reason for hiding this comment

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

We dont need referencedMeasures anymore. That was the big fix in the PR. The earlier code should have been a forEach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, GitHub's UI made this comment confusIng... I meant the whole code block that's highlighted. At the top-level, this code is initializing an empty set const measures = new Set(), then building up an array. Rather, I'd expect the input measureNames to be filtered like measureNames.filter(...).

Copy link
Collaborator Author

@AdityaHegde AdityaHegde Jan 30, 2025

Choose a reason for hiding this comment

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

Ah it was adding referencedMeasures before so it was not a filter. Will change in a follow up PR.

return [...measures];
};
Expand Down Expand Up @@ -212,6 +166,5 @@ export const measureSelectors = {

filteredSimpleMeasures,

getFilteredMeasuresAndDimensions,
leaderboardMeasureName,
};
Loading
Loading