Skip to content

Commit

Permalink
fix: public urls with computed measures and dependant measures are hi…
Browse files Browse the repository at this point in the history
…dden (#6520)

* Add requiredMeasures to explore fields

* Do not send referenced measures for non-window measures

* Fix windowed measures

* Fix lint+tests

* Refactor filter method

* Change function name
  • Loading branch information
AdityaHegde authored Jan 30, 2025
1 parent 81dbf94 commit 0b87431
Show file tree
Hide file tree
Showing 8 changed files with 96 additions and 168 deletions.
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));
});
return [...measures];
};
Expand Down Expand Up @@ -212,6 +166,5 @@ export const measureSelectors = {

filteredSimpleMeasures,

getFilteredMeasuresAndDimensions,
leaderboardMeasureName,
};
Loading

1 comment on commit 0b87431

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

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

Please sign in to comment.