diff --git a/web-common/src/features/components/charts/builder.ts b/web-common/src/features/components/charts/builder.ts index c009179def30..ffd9d76fb490 100644 --- a/web-common/src/features/components/charts/builder.ts +++ b/web-common/src/features/components/charts/builder.ts @@ -569,6 +569,11 @@ export function createCartesianMultiValueTooltipChannel( return multiValueTooltipChannel; } +/** + * Hover target layer shared by the cartesian charts. `xField` is the raw x + * field name: it is escaped for the encoding here and used as is to look up + * the chart's domain values, which are keyed by raw names. + */ export function buildHoverRuleLayer(args: { xField?: string; defaultTooltip: TooltipValue[]; @@ -595,6 +600,7 @@ export function buildHoverRuleLayer(args: { isDarkMode = false, isInteractive = false, } = args; + const sanitizedXField = xField ? sanitizeValueForVega(xField) : undefined; const params: SelectionParameter[] = [ { @@ -632,7 +638,7 @@ export function buildHoverRuleLayer(args: { }, encoding: { x: { - field: xField, + field: sanitizedXField, ...(xBand !== undefined ? { bandPosition: xBand } : {}), ...(xSort && xField ? { sort: domainValues?.[xField] } : {}), }, diff --git a/web-common/src/features/components/charts/cartesian/area/spec.spec.ts b/web-common/src/features/components/charts/cartesian/area/spec.spec.ts new file mode 100644 index 000000000000..c036876905b3 --- /dev/null +++ b/web-common/src/features/components/charts/cartesian/area/spec.spec.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "vitest"; +import type { CartesianChartSpec } from "../CartesianChartProvider"; +import { at, chartData } from "../test-fixtures"; +import { generateVLAreaChartSpec } from "./spec"; + +describe("generateVLAreaChartSpec with a dotted color dimension", () => { + it("escapes the dimension in the hover pivot", () => { + // Nested Druid columns surface as dimensions whose names contain dots, + // which Vega would otherwise read as a nested field path. + const dimension = "query.context.metaData.type"; + const data = chartData(); + data.fields[dimension] = { name: dimension, displayName: "Query Type" }; + data.domainValues = { + ...data.domainValues, + [dimension]: ["Scheduled Report", "Other"], + }; + + const config: CartesianChartSpec = { + metrics_view: "druid_requests", + x: { field: "__time", type: "temporal" }, + y: { field: "count", type: "quantitative", zeroBasedOrigin: true }, + color: { field: dimension, type: "nominal" }, + }; + const spec = generateVLAreaChartSpec(config, data); + expect(at(spec, "layer.1.transform.0.pivot")).toBe( + "query\\.context\\.metaData\\.type", + ); + expect(at(spec, "layer.0.encoding.color.field")).toBe( + "query\\.context\\.metaData\\.type", + ); + }); +}); + +describe("generateVLAreaChartSpec with a dotted x dimension", () => { + it("escapes the x field once in the hover layer and keeps the custom sort", () => { + // The hover helper escapes the x field itself. Passing it pre-escaped + // escaped the backslash a second time, so the hover rule pointed at a + // field that does not exist and tooltips never showed. + const dimension = "event.time"; + const data = chartData(); + data.fields[dimension] = { name: dimension, displayName: "Event Time" }; + data.domainValues = { [dimension]: ["Other", "Scheduled Report"] }; + + const spec = generateVLAreaChartSpec( + { + metrics_view: "druid_requests", + x: { field: dimension, type: "nominal", sort: "-y" }, + y: { field: "count", type: "quantitative", zeroBasedOrigin: true }, + }, + data, + ); + expect(at(spec, "layer.1.encoding.x.field")).toBe("event\\.time"); + expect(at(spec, "layer.1.encoding.x.sort")).toEqual([ + "Other", + "Scheduled Report", + ]); + }); +}); diff --git a/web-common/src/features/components/charts/cartesian/area/spec.ts b/web-common/src/features/components/charts/cartesian/area/spec.ts index bc25534d3eab..85f081972381 100644 --- a/web-common/src/features/components/charts/cartesian/area/spec.ts +++ b/web-common/src/features/components/charts/cartesian/area/spec.ts @@ -68,7 +68,7 @@ export function generateVLAreaChartSpec( layer: inner, }, buildHoverRuleLayer({ - xField: sanitizedXField, + xField: config.x?.field, domainValues: data.domainValues, defaultTooltip: defaultTooltipChannel, multiValueTooltipChannel, @@ -82,7 +82,7 @@ export function generateVLAreaChartSpec( colorField && multiValueTooltipChannel?.length ? { - field: colorField, + field: sanitizeValueForVega(colorField), value: sanitizedYField, groupby: [sanitizedXField], } diff --git a/web-common/src/features/components/charts/cartesian/bar-chart/spec.spec.ts b/web-common/src/features/components/charts/cartesian/bar-chart/spec.spec.ts index 9824815b7185..ea11b340247a 100644 --- a/web-common/src/features/components/charts/cartesian/bar-chart/spec.spec.ts +++ b/web-common/src/features/components/charts/cartesian/bar-chart/spec.spec.ts @@ -193,3 +193,50 @@ describe("generateVLStackedBarChartSpec orientation", () => { expect(() => compile(spec as TopLevelSpec)).not.toThrow(); }); }); + +describe("generateVLBarChartSpec with a dotted color dimension", () => { + it("escapes the dimension in the hover pivot and the xOffset", () => { + // Nested Druid columns surface as dimensions whose names contain dots, + // which Vega would otherwise read as a nested field path. + const dimension = "query.context.metaData.type"; + const data = chartData(); + data.fields[dimension] = { name: dimension, displayName: "Query Type" }; + data.domainValues = { + ...data.domainValues, + [dimension]: ["Scheduled Report", "Other"], + }; + + const spec = generateVLBarChartSpec( + { ...base, color: { field: dimension, type: "nominal" } }, + data, + ); + expect(at(spec, "layer.0.transform.0.pivot")).toBe( + "query\\.context\\.metaData\\.type", + ); + expect(at(spec, "layer.1.encoding.xOffset.field")).toBe( + "query\\.context\\.metaData\\.type", + ); + }); +}); + +describe("generateVLBarChartSpec with a dotted x dimension", () => { + it("keeps the custom x sort values for a dotted x dimension", () => { + // The hover layer looks the sort values up by the raw name. + const dimension = "query.context.metaData.type"; + const data = chartData(); + data.fields[dimension] = { name: dimension, displayName: "Query Type" }; + data.domainValues = { [dimension]: ["Other", "Scheduled Report"] }; + + const spec = generateVLBarChartSpec( + { ...base, x: { field: dimension, type: "nominal", sort: "-y" } }, + data, + ); + expect(at(spec, "layer.0.encoding.x.field")).toBe( + "query\\.context\\.metaData\\.type", + ); + expect(at(spec, "layer.0.encoding.x.sort")).toEqual([ + "Other", + "Scheduled Report", + ]); + }); +}); diff --git a/web-common/src/features/components/charts/cartesian/bar-chart/spec.ts b/web-common/src/features/components/charts/cartesian/bar-chart/spec.ts index c9f58b4eda3d..8a24791a749c 100644 --- a/web-common/src/features/components/charts/cartesian/bar-chart/spec.ts +++ b/web-common/src/features/components/charts/cartesian/bar-chart/spec.ts @@ -70,7 +70,7 @@ export function generateVLBarChartSpec( const isInteractive = !!config.isInteractive && !horizontal; const hoverRuleLayer = buildHoverRuleLayer({ - xField: sanitizedXField, + xField: config.x?.field, domainValues: data.domainValues, isBarMark: true, defaultTooltip: defaultTooltipChannel, @@ -104,7 +104,7 @@ export function generateVLBarChartSpec( !hasComparison ? { xOffset: { - field: config.color.field, + field: sanitizeValueForVega(config.color.field), title: data.fields[config.color.field]?.displayName || config.color.field, diff --git a/web-common/src/features/components/charts/cartesian/line-chart/spec.ts b/web-common/src/features/components/charts/cartesian/line-chart/spec.ts index 635a498266f0..0c7a1198417a 100644 --- a/web-common/src/features/components/charts/cartesian/line-chart/spec.ts +++ b/web-common/src/features/components/charts/cartesian/line-chart/spec.ts @@ -58,7 +58,7 @@ export function generateVLLineChartSpec( const hasComparison = data.hasComparison; const hoverRuleLayer = buildHoverRuleLayer({ - xField: sanitizedXField, + xField: config.x?.field, domainValues: data.domainValues, defaultTooltip: defaultTooltipChannel, multiValueTooltipChannel, diff --git a/web-common/src/features/components/charts/cartesian/multi-metric-chart.spec.ts b/web-common/src/features/components/charts/cartesian/multi-metric-chart.spec.ts index d6c81ff33b6e..5439b84dc414 100644 --- a/web-common/src/features/components/charts/cartesian/multi-metric-chart.spec.ts +++ b/web-common/src/features/components/charts/cartesian/multi-metric-chart.spec.ts @@ -97,3 +97,32 @@ describe("generateVLMultiMetricChartSpec orientation", () => { } }); }); + +describe("generateVLMultiMetricChartSpec with dotted measure names", () => { + it("escapes the folded measures and keeps the raw names as output columns", () => { + const measures = ["post.count", "comment.count"]; + const data = chartData(); + for (const measure of measures) { + data.fields[measure] = { name: measure, displayName: measure }; + } + + const spec = generateVLMultiMetricChartSpec( + { + ...base, + y: { + field: measures[0], + fields: measures, + type: "quantitative", + zeroBasedOrigin: true, + }, + }, + data, + "grouped_bar", + ); + expect(at(spec, "transform.0.fold")).toEqual([ + "post\\.count", + "comment\\.count", + ]); + expect(at(spec, "transform.0.as")).toEqual(["Measure", "value"]); + }); +}); diff --git a/web-common/src/features/components/charts/cartesian/multi-metric-chart.ts b/web-common/src/features/components/charts/cartesian/multi-metric-chart.ts index d2202ab7820f..2a139fae9b58 100644 --- a/web-common/src/features/components/charts/cartesian/multi-metric-chart.ts +++ b/web-common/src/features/components/charts/cartesian/multi-metric-chart.ts @@ -81,7 +81,7 @@ export function generateVLMultiMetricChartSpec( const transforms: Transform[] = [ { - fold: fieldsToFold, + fold: fieldsToFold.map((field) => sanitizeValueForVega(field)), as: [measureField, valueField], }, ]; @@ -242,7 +242,7 @@ export function generateVLMultiMetricChartSpec( } const hoverRuleLayer = buildHoverRuleLayer({ - xField, + xField: config.x?.field, defaultTooltip: [], multiValueTooltipChannel, primaryColor: data.theme.primary, diff --git a/web-common/src/features/components/charts/cartesian/stacked-bar/default.spec.ts b/web-common/src/features/components/charts/cartesian/stacked-bar/default.spec.ts new file mode 100644 index 000000000000..75958f7f14be --- /dev/null +++ b/web-common/src/features/components/charts/cartesian/stacked-bar/default.spec.ts @@ -0,0 +1,100 @@ +import { sanitizeFieldName } from "@rilldata/web-common/components/vega/util"; +import { ColorWithComparisonField } from "@rilldata/web-common/features/components/charts/comparison-builder"; +import type { Scene } from "vega"; +import type { TopLevelSpec } from "vega-lite"; +import { describe, expect, it } from "vitest"; +import type { CartesianChartSpec } from "../CartesianChartProvider"; +import { at, chartData, stubCanvasContext } from "../test-fixtures"; +import { generateVLStackedBarChartSpec } from "./default"; + +// A nested Druid column surfaces as a dimension whose name contains dots. +// Vega reads an unescaped dotted field as a nested lookup, so every place the +// spec names the dimension has to escape it or the chart renders nothing. +const dimension = "query.context.metaData.type"; +const escapedDimension = "query\\.context\\.metaData\\.type"; +const values = ["Scheduled Report", "Other"]; + +const config: CartesianChartSpec = { + metrics_view: "druid_requests", + x: { field: "__time", type: "temporal" }, + y: { field: "count", type: "quantitative", zeroBasedOrigin: true }, + color: { field: dimension, type: "nominal", values }, +}; + +function data(hasComparison = false) { + const result = chartData({ hasComparison }); + result.fields = { + __time: { name: "__time", displayName: "Time" }, + count: { name: "count", displayName: "Count" }, + [dimension]: { name: dimension, displayName: "Query Type" }, + }; + result.domainValues = { [dimension]: values }; + return result; +} + +describe("generateVLStackedBarChartSpec with a dotted color dimension", () => { + it("escapes the dimension in the pivot and color encodings", () => { + const spec = generateVLStackedBarChartSpec(config, data()); + expect(at(spec, "layer.0.transform.0.pivot")).toBe(escapedDimension); + expect(at(spec, "layer.1.encoding.color.field")).toBe(escapedDimension); + }); + + it("pivots on the synthetic color field in comparison mode", () => { + const spec = generateVLStackedBarChartSpec(config, data(true)); + expect(at(spec, "layer.0.transform.0.pivot")).toBe( + ColorWithComparisonField, + ); + }); + + it("renders a bar per row when rows are keyed by the dotted dimension", async () => { + stubCanvasContext(); + const { compile } = await import("vega-lite"); + const vega = await import("vega"); + // The app registers a formatter per measure for tooltips (see Chart.svelte). + vega.expressionFunction(sanitizeFieldName("count"), (value: unknown) => + String(value), + ); + + const spec = generateVLStackedBarChartSpec(config, data()); + const rows = [ + { __time: "2026-10-27T00:00:00Z", count: 2, [dimension]: values[0] }, + { __time: "2026-10-27T00:00:00Z", count: 3, [dimension]: values[1] }, + { __time: "2026-10-27T01:00:00Z", count: 4, [dimension]: values[0] }, + { __time: "2026-10-27T01:00:00Z", count: 1, [dimension]: values[1] }, + ]; + + // Vega reports dataflow failures through its logger instead of throwing. + const errors: unknown[] = []; + const view = new vega.View(vega.parse(compile(spec as TopLevelSpec).spec), { + renderer: "none", + }) + .logger( + vega.logger(vega.Error, "error", (_method, _level, args) => { + errors.push(...Array.from(args as ArrayLike)); + }), + ) + .width(400) + .height(200); + view.data("metrics-view", rows); + await view.runAsync(); + + expect(errors).toEqual([]); + // At runtime `scenegraph()` returns the Scenegraph wrapper, not the root mark. + const { root } = view.scenegraph() as unknown as { root: Scene }; + expect(countBars(root)).toBe(rows.length); + }); +}); + +/** Counts the painted bar marks, skipping the transparent hover targets. */ +function countBars(mark: Scene): number { + const own = + mark.marktype === "rect" + ? mark.items.filter( + (bar: { fill?: string; height?: number }) => + bar.fill !== "transparent" && (bar.height ?? 0) > 0, + ).length + : 0; + return mark.items + .flatMap((group) => group.items ?? []) + .reduce((sum, child) => sum + countBars(child), own); +} diff --git a/web-common/src/features/components/charts/cartesian/stacked-bar/default.ts b/web-common/src/features/components/charts/cartesian/stacked-bar/default.ts index 5bff9588ab63..26094614783d 100644 --- a/web-common/src/features/components/charts/cartesian/stacked-bar/default.ts +++ b/web-common/src/features/components/charts/cartesian/stacked-bar/default.ts @@ -67,7 +67,7 @@ export function generateVLStackedBarChartSpec( const isInteractive = !!config.isInteractive && !horizontal; const hoverRuleLayer = buildHoverRuleLayer({ - xField: sanitizedXField, + xField: config.x?.field, domainValues: data.domainValues, isBarMark: true, defaultTooltip: defaultTooltipChannel, diff --git a/web-common/src/features/components/charts/cartesian/stacked-bar/normalized.spec.ts b/web-common/src/features/components/charts/cartesian/stacked-bar/normalized.spec.ts index 9aa82802cc30..301652128973 100644 --- a/web-common/src/features/components/charts/cartesian/stacked-bar/normalized.spec.ts +++ b/web-common/src/features/components/charts/cartesian/stacked-bar/normalized.spec.ts @@ -1,7 +1,12 @@ import type { TopLevelSpec } from "vega-lite"; import { describe, expect, it } from "vitest"; import type { CartesianChartSpec } from "../CartesianChartProvider"; -import { at, chartData, stubCanvasContext } from "../test-fixtures"; +import { + at, + chartData, + stubCanvasContext, + vegaFieldReferences, +} from "../test-fixtures"; import { generateVLStackedBarNormalizedSpec } from "./normalized"; const base: CartesianChartSpec = { @@ -57,3 +62,25 @@ describe("generateVLStackedBarNormalizedSpec orientation", () => { expect(() => compile(spec as TopLevelSpec)).not.toThrow(); }); }); + +describe("generateVLStackedBarNormalizedSpec with a dotted x dimension", () => { + it("escapes the dimension in the percentage groupby and the hover encoding", () => { + // Nested Druid columns surface as dimensions whose names contain dots, + // which Vega would otherwise read as a nested field path. + const dimension = "query.context.metaData.type"; + const data = chartData({ colorValues: ["us", "eu"] }); + data.fields[dimension] = { name: dimension, displayName: "Query Type" }; + + const spec = generateVLStackedBarNormalizedSpec( + { ...base, x: { field: dimension, type: "nominal" } }, + data, + ); + expect(at(spec, "transform.0.groupby")).toEqual([ + "query\\.context\\.metaData\\.type", + ]); + expect(at(spec, "layer.0.encoding.x.field")).toBe( + "query\\.context\\.metaData\\.type", + ); + expect(vegaFieldReferences(spec)).not.toContain(dimension); + }); +}); diff --git a/web-common/src/features/components/charts/cartesian/stacked-bar/normalized.ts b/web-common/src/features/components/charts/cartesian/stacked-bar/normalized.ts index edd6e4e11442..ace86f5a6ab2 100644 --- a/web-common/src/features/components/charts/cartesian/stacked-bar/normalized.ts +++ b/web-common/src/features/components/charts/cartesian/stacked-bar/normalized.ts @@ -70,7 +70,7 @@ export function generateVLStackedBarNormalizedSpec( // Add a transform to calculate the percentage // When in comparison mode, include the measure_key in groupby // so each period (current vs comparison) totals to 100% independently - const groupbyFields = config.x?.field ? [config.x.field] : []; + const groupbyFields = config.x?.field ? [sanitizedXField] : []; if (hasComparison) { groupbyFields.push(MeasureKeyField); } @@ -80,7 +80,7 @@ export function generateVLStackedBarNormalizedSpec( joinaggregate: [ { op: "sum", - field: yField, + field: sanitizedYField, as: "total", }, ], @@ -150,7 +150,7 @@ export function generateVLStackedBarNormalizedSpec( }; const hoverRuleLayer = buildHoverRuleLayer({ - xField: sanitizedXField, + xField: config.x?.field, domainValues: data.domainValues, isBarMark: true, defaultTooltip: baseEncoding.tooltip as TooltipValue[], diff --git a/web-common/src/features/components/charts/cartesian/test-fixtures.ts b/web-common/src/features/components/charts/cartesian/test-fixtures.ts index 209e78b196eb..a88a6fe40200 100644 --- a/web-common/src/features/components/charts/cartesian/test-fixtures.ts +++ b/web-common/src/features/components/charts/cartesian/test-fixtures.ts @@ -52,3 +52,34 @@ export function at(spec: unknown, path: string): unknown { return (value as Record)[key]; }, spec); } + +/** + * Collects every name a generated spec hands to Vega as a field reference: + * the values of `field` and `pivot` keys and the entries of `fold` and + * `groupby` arrays. Vega resolves these as field paths, so a test can assert + * that a dotted name only reaches Vega in its escaped form. + */ +export function vegaFieldReferences(spec: unknown): string[] { + const refs: string[] = []; + const visit = (node: unknown) => { + if (Array.isArray(node)) { + node.forEach(visit); + return; + } + if (node === null || typeof node !== "object") return; + for (const [key, value] of Object.entries(node)) { + if ((key === "field" || key === "pivot") && typeof value === "string") { + refs.push(value); + } else if ( + (key === "fold" || key === "groupby") && + Array.isArray(value) + ) { + refs.push(...value.filter((v): v is string => typeof v === "string")); + } else { + visit(value); + } + } + }; + visit(spec); + return refs; +} diff --git a/web-common/src/features/components/charts/cartesian/util.spec.ts b/web-common/src/features/components/charts/cartesian/util.spec.ts new file mode 100644 index 000000000000..73e0ce549b0a --- /dev/null +++ b/web-common/src/features/components/charts/cartesian/util.spec.ts @@ -0,0 +1,37 @@ +import { ColorWithComparisonField } from "@rilldata/web-common/features/components/charts/comparison-builder"; +import { describe, expect, it } from "vitest"; +import { createVegaTransformPivotConfig } from "./util"; + +describe("createVegaTransformPivotConfig", () => { + it("escapes a dotted color dimension so Vega reads it as one field", () => { + const pivot = createVegaTransformPivotConfig( + "__time", + "count", + "query.context.metaData.type", + false, + true, + ); + expect(pivot).toEqual({ + field: "query\\.context\\.metaData\\.type", + value: "count", + groupby: ["__time"], + }); + }); + + it("pivots on the synthetic color field in comparison mode", () => { + const pivot = createVegaTransformPivotConfig( + "__time", + "count", + "query.context.metaData.type", + true, + true, + ); + expect(pivot?.field).toBe(ColorWithComparisonField); + }); + + it("returns no pivot without a multi-value tooltip", () => { + expect( + createVegaTransformPivotConfig("__time", "count", "region", false, false), + ).toBeUndefined(); + }); +}); diff --git a/web-common/src/features/components/charts/cartesian/util.ts b/web-common/src/features/components/charts/cartesian/util.ts index f49c1f0d50bc..b6e1c7100899 100644 --- a/web-common/src/features/components/charts/cartesian/util.ts +++ b/web-common/src/features/components/charts/cartesian/util.ts @@ -1,3 +1,4 @@ +import { sanitizeValueForVega } from "@rilldata/web-common/components/vega/util"; import { ColorWithComparisonField, MeasureKeyField, @@ -6,6 +7,12 @@ import { /** * Determines the pivot configuration for the bar chart based on the presence of * x/y fields, color fields, and comparison mode. + * + * `xField` and `yField` are already escaped Vega field references. `colorField` + * is the raw dimension name and is escaped here: Vega resolves the pivot field + * as a field path, so a name with dots such as `query.context.metaData.type` + * would otherwise be read as a nested lookup and throw inside the dataflow, + * leaving the whole chart blank. */ export function createVegaTransformPivotConfig( xField: string | undefined, @@ -22,7 +29,9 @@ export function createVegaTransformPivotConfig( if (colorField) { return { // Use color_with_comparison field when in comparison mode to include both current and previous values - field: hasComparison ? ColorWithComparisonField : colorField, + field: hasComparison + ? ColorWithComparisonField + : sanitizeValueForVega(colorField), value: yField, groupby: [xField], }; diff --git a/web-common/src/features/components/charts/circular/pie.spec.ts b/web-common/src/features/components/charts/circular/pie.spec.ts new file mode 100644 index 000000000000..028dbc29b179 --- /dev/null +++ b/web-common/src/features/components/charts/circular/pie.spec.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from "vitest"; +import { chartData, vegaFieldReferences } from "../cartesian/test-fixtures"; +import type { CircularChartSpec } from "./CircularChartProvider"; +import { TOTAL_DOMAIN_KEY } from "./constants"; +import { generateVLPieChartSpec } from "./pie"; + +describe("generateVLPieChartSpec with dotted field names", () => { + it("escapes the measure in the slice label text encoding", () => { + // Nested Druid columns surface as fields whose names contain dots, which + // Vega would otherwise read as nested field paths. + const measure = "stats.count"; + const dimension = "query.context.metaData.type"; + const config: CircularChartSpec = { + metrics_view: "druid_requests", + measure: { field: measure, type: "quantitative" }, + color: { field: dimension, type: "nominal" }, + labels: { show: true, format: "value" }, + }; + const data = chartData(); + data.fields = { + [measure]: { name: measure, displayName: "Count" }, + [dimension]: { name: dimension, displayName: "Query Type" }, + }; + data.domainValues = { + [TOTAL_DOMAIN_KEY]: [100], + [dimension]: ["Scheduled Report", "Other"], + }; + + const spec = generateVLPieChartSpec(config, data); + const refs = vegaFieldReferences(spec); + expect(refs).toContain("stats\\.count"); + expect(refs).not.toContain(measure); + expect(refs).not.toContain(dimension); + }); +}); diff --git a/web-common/src/features/components/charts/circular/pie.ts b/web-common/src/features/components/charts/circular/pie.ts index a796f9cffb58..cde8474245cf 100644 --- a/web-common/src/features/components/charts/circular/pie.ts +++ b/web-common/src/features/components/charts/circular/pie.ts @@ -217,7 +217,7 @@ export function generateVLPieChartSpec( }; } else { labelEncoding.text = { - field: measureFieldRaw, + field: sanitizeValueForVega(measureFieldRaw), type: "quantitative", formatType: sanitizeFieldName(measureFieldRaw), ...(measureMetaData && diff --git a/web-common/src/features/components/charts/combo/spec.spec.ts b/web-common/src/features/components/charts/combo/spec.spec.ts new file mode 100644 index 000000000000..d907dd105024 --- /dev/null +++ b/web-common/src/features/components/charts/combo/spec.spec.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from "vitest"; +import { at, chartData } from "../cartesian/test-fixtures"; +import type { ComboChartSpec } from "./ComboChartProvider"; +import { generateVLComboChartSpec } from "./spec"; + +describe("generateVLComboChartSpec with dotted field names", () => { + const dimension = "event.time"; + const measure = "stats.count"; + + function data() { + const result = chartData(); + result.fields[dimension] = { name: dimension, displayName: "Event Time" }; + result.fields[measure] = { name: measure, displayName: "Count" }; + result.domainValues = { [dimension]: ["Other", "Scheduled Report"] }; + return result; + } + + const config: ComboChartSpec = { + metrics_view: "druid_requests", + x: { field: dimension, type: "nominal", sort: "-y" }, + y1: { field: measure, type: "quantitative", mark: "bar" }, + y2: { field: "comment_count", type: "quantitative", mark: "line" }, + }; + + it("escapes the x field once in the hover layer and keeps the custom sort", () => { + // The hover helper escapes the x field itself; a pre-escaped name would + // be escaped twice and the hover rule would reference a missing field. + const spec = generateVLComboChartSpec(config, data()); + expect(at(spec, "layer.0.encoding.x.field")).toBe("event\\.time"); + expect(at(spec, "layer.0.encoding.x.sort")).toEqual([ + "Other", + "Scheduled Report", + ]); + }); + + it("escapes the measures handed to fold and filters on the raw names", () => { + // Vega emits the raw measure name as the fold key, so the per-measure + // filters and the color domain keep the raw names. + const spec = generateVLComboChartSpec(config, data()); + expect(at(spec, "transform.0.fold")).toEqual([ + "stats\\.count", + "comment_count", + ]); + expect(at(spec, "layer.1.transform.0.filter")).toBe( + "datum['Measure'] === 'stats.count'", + ); + expect(at(spec, "layer.1.encoding.color.scale.domain")).toEqual([ + "stats.count", + "comment_count", + ]); + }); +}); diff --git a/web-common/src/features/components/charts/combo/spec.ts b/web-common/src/features/components/charts/combo/spec.ts index f32c9436aba8..7c264d55c9e4 100644 --- a/web-common/src/features/components/charts/combo/spec.ts +++ b/web-common/src/features/components/charts/combo/spec.ts @@ -49,7 +49,6 @@ export function generateVLComboChartSpec( const spec = createMultiLayerBaseSpec(); const vegaConfig = createConfigWithLegend(config, config.color); - const xField = sanitizeValueForVega(config.x?.field); const y1MarkType = config.y1?.mark || "bar"; const y2MarkType = config.y2?.mark || "line"; @@ -93,7 +92,7 @@ export function generateVLComboChartSpec( // Transform data to long format for legend spec.transform = [ { - fold: measures, + fold: measures.map((measure) => sanitizeValueForVega(measure)), as: [measureField, valueField], }, ]; @@ -106,7 +105,7 @@ export function generateVLComboChartSpec( // Add hover rule layer layers.push( buildHoverRuleLayer({ - xField, + xField: config.x?.field, domainValues: data.domainValues, isBarMark: y1MarkType === "bar" || y2MarkType === "bar", defaultTooltip: defaultTooltipChannel, diff --git a/web-common/src/features/components/charts/comparison-builder.spec.ts b/web-common/src/features/components/charts/comparison-builder.spec.ts new file mode 100644 index 000000000000..e2266d77df23 --- /dev/null +++ b/web-common/src/features/components/charts/comparison-builder.spec.ts @@ -0,0 +1,20 @@ +import { ComparisonDeltaPreviousSuffix } from "@rilldata/web-common/features/dashboards/filters/measure-filters/measure-filter-entry"; +import { describe, expect, it } from "vitest"; +import { + createComparisonTransforms, + MeasureKeyField, +} from "./comparison-builder"; + +describe("createComparisonTransforms", () => { + it("escapes the folded fields and keeps the raw measure name as the output column", () => { + const [fold] = createComparisonTransforms( + "__time", + "stats.count", + "query.context.metaData.type", + ); + expect(fold).toEqual({ + fold: ["stats\\.count", `stats\\.count${ComparisonDeltaPreviousSuffix}`], + as: [MeasureKeyField, "stats.count"], + }); + }); +}); diff --git a/web-common/src/features/components/charts/comparison-builder.ts b/web-common/src/features/components/charts/comparison-builder.ts index f6e283fc5967..5e5ac34e4fab 100644 --- a/web-common/src/features/components/charts/comparison-builder.ts +++ b/web-common/src/features/components/charts/comparison-builder.ts @@ -1,3 +1,4 @@ +import { sanitizeValueForVega } from "@rilldata/web-common/components/vega/util"; import { ComparisonDeltaPreviousSuffix } from "@rilldata/web-common/features/dashboards/filters/measure-filters/measure-filter-entry"; import type { Field, @@ -25,10 +26,10 @@ export const ComparisonMarkOpacity = 0.4; * The data structure has current and previous values as separate measure fields: * e.g., "total_bids" (current) and "total_bids_prev" (comparison). * - * Field names are passed raw (not pre-escaped) on purpose. `fold.as` stores - * its second element as a literal field name, while downstream consumers - * such as `pivot.value` resolve field-path escapes — pre-escaping here makes - * those two views of the same field disagree (e.g. `"x \\| y"` vs `"x | y"`). + * Field names arrive raw. The `fold` inputs are Vega field references, so + * they are escaped here. `fold.as` stores literal names, so it keeps the raw + * measure name, which is what an escaped reference such as `pivot.value` + * resolves to; Vega also emits the raw name as the `measure_key` value. * Embedded Vega expression literals escape `\` and `'` via `escapeVegaString`. */ export function createComparisonTransforms( @@ -45,7 +46,10 @@ export function createComparisonTransforms( // Fold the current and previous measure fields into the original measure // column, with a `measure_key` flag indicating which period each row is. transforms.push({ - fold: [measureField, previousMeasureField], + fold: [ + sanitizeValueForVega(measureField), + sanitizeValueForVega(previousMeasureField), + ], as: [MeasureKeyField, measureField], }); diff --git a/web-common/src/features/components/charts/funnel/spec.spec.ts b/web-common/src/features/components/charts/funnel/spec.spec.ts new file mode 100644 index 000000000000..a204718b1c39 --- /dev/null +++ b/web-common/src/features/components/charts/funnel/spec.spec.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; +import { at, chartData, vegaFieldReferences } from "../cartesian/test-fixtures"; +import type { FunnelChartSpec } from "./FunnelChartProvider"; +import { generateVLFunnelChartSpec } from "./spec"; + +// Nested Druid columns surface as fields whose names contain dots, which +// Vega would otherwise read as nested field paths. +const stage = "query.context.metaData.type"; +const measure = "stats.count"; + +function data() { + const result = chartData(); + result.fields = { + [stage]: { name: stage, displayName: "Query Type" }, + [measure]: { name: measure, displayName: "Count" }, + "other.count": { name: "other.count", displayName: "Other Count" }, + }; + result.domainValues = { [stage]: ["Scheduled Report", "Other"] }; + return result; +} + +describe("generateVLFunnelChartSpec with dotted field names", () => { + it("escapes the stage and measure everywhere Vega resolves them as fields", () => { + const config: FunnelChartSpec = { + metrics_view: "druid_requests", + stage: { field: stage, type: "nominal" }, + measure: { field: measure, type: "quantitative" }, + color: "stage", + }; + const spec = generateVLFunnelChartSpec(config, data()); + + expect(at(spec, "layer.0.encoding.color.field")).toBe( + "query\\.context\\.metaData\\.type", + ); + const refs = vegaFieldReferences(spec); + expect(refs).toContain("stats\\.count"); + expect(refs).not.toContain(stage); + expect(refs).not.toContain(measure); + }); + + it("escapes the folded measures of a multi-measure funnel", () => { + const config: FunnelChartSpec = { + metrics_view: "druid_requests", + breakdownMode: "measures", + measure: { + field: measure, + fields: [measure, "other.count"], + type: "quantitative", + }, + color: "name", + }; + const spec = generateVLFunnelChartSpec(config, data()); + + expect(at(spec, "transform.0.fold")).toEqual([ + "stats\\.count", + "other\\.count", + ]); + expect(at(spec, "transform.0.as")).toEqual(["Measure", "value"]); + }); +}); diff --git a/web-common/src/features/components/charts/funnel/spec.ts b/web-common/src/features/components/charts/funnel/spec.ts index 3f91f11a55b0..001db77d90e8 100644 --- a/web-common/src/features/components/charts/funnel/spec.ts +++ b/web-common/src/features/components/charts/funnel/spec.ts @@ -29,7 +29,9 @@ function createModeTransforms( window: [{ op: "row_number", as: "funnel_rank" }], sort: [ { - field: isMultiMeasure ? "value" : measureField!, + field: isMultiMeasure + ? "value" + : sanitizeValueForVega(measureField!), order: "descending", }, ], @@ -98,8 +100,16 @@ function createPercentageTransforms( { calculate: positionExpr, as: "funnel_position" }, { window: [ - { op: "first_value", field: measureField!, as: "top_value" }, - { op: "lag", field: measureField!, as: "prev_value" }, + { + op: "first_value", + field: sanitizeValueForVega(measureField!), + as: "top_value", + }, + { + op: "lag", + field: sanitizeValueForVega(measureField!), + as: "prev_value", + }, ], sort: [{ field: "funnel_position", order: "ascending" }], }, @@ -109,8 +119,16 @@ function createPercentageTransforms( // row is the top of the funnel and lag returns the immediately prior stage. transforms.push({ window: [ - { op: "first_value", field: measureField!, as: "top_value" }, - { op: "lag", field: measureField!, as: "prev_value" }, + { + op: "first_value", + field: sanitizeValueForVega(measureField!), + as: "top_value", + }, + { + op: "lag", + field: sanitizeValueForVega(measureField!), + as: "prev_value", + }, ], }); } @@ -155,6 +173,13 @@ export function generateVLFunnelChartSpec( const textColor = data.isDarkMode ? "#eeeeee" : "#353535"; const isMultiMeasure = config.breakdownMode === "measures"; + // Vega field references for the configured measure and stage. + const measureRef = config.measure?.field + ? sanitizeValueForVega(config.measure.field) + : undefined; + const stageRef = config.stage?.field + ? sanitizeValueForVega(config.stage.field) + : undefined; const measureDisplayNames: Record = {}; if (isMultiMeasure && config.measure?.field) { @@ -232,7 +257,7 @@ export function generateVLFunnelChartSpec( // Transform data for multi-measure funnel transforms.push( { - fold: measures, + fold: measures.map((measure) => sanitizeValueForVega(measure)), as: ["Measure", "value"], }, { @@ -300,7 +325,7 @@ export function generateVLFunnelChartSpec( }, }, color: { - field: colorField, + field: colorField ? sanitizeValueForVega(colorField) : undefined, type: (colorType === "value" ? "nominal" : colorType) as | "quantitative" | "ordinal" @@ -364,7 +389,7 @@ export function generateVLFunnelChartSpec( stack: "center", }, text: { - field: isMultiMeasure ? "value" : config.measure?.field, + field: isMultiMeasure ? "value" : measureRef, type: isMultiMeasure ? "quantitative" : config.measure?.type === "value" @@ -397,7 +422,7 @@ export function generateVLFunnelChartSpec( type: "nominal", } : { - field: config.stage?.field, + field: stageRef, type: "nominal", }, }, diff --git a/web-common/src/features/components/charts/heatmap/spec.spec.ts b/web-common/src/features/components/charts/heatmap/spec.spec.ts new file mode 100644 index 000000000000..6758c937b58a --- /dev/null +++ b/web-common/src/features/components/charts/heatmap/spec.spec.ts @@ -0,0 +1,41 @@ +import { describe, expect, it } from "vitest"; +import { at, chartData, vegaFieldReferences } from "../cartesian/test-fixtures"; +import type { HeatmapChartSpec } from "./HeatmapChartProvider"; +import { generateVLHeatmapSpec } from "./spec"; + +describe("generateVLHeatmapSpec with dotted field names", () => { + it("escapes the measure in the data label text encoding", () => { + const measure = "stats.count"; + const config: HeatmapChartSpec = { + metrics_view: "druid_requests", + x: { field: "query.dataSource.name", type: "nominal" }, + y: { field: "query.context.metaData.type", type: "nominal" }, + color: { field: measure, type: "quantitative" }, + show_data_labels: true, + }; + const data = chartData(); + data.fields = { + "query.dataSource.name": { + name: "query.dataSource.name", + displayName: "Data Source", + }, + "query.context.metaData.type": { + name: "query.context.metaData.type", + displayName: "Query Type", + }, + [measure]: { name: measure, displayName: "Count" }, + }; + data.domainValues = {}; + + const spec = generateVLHeatmapSpec(config, data); + expect(at(spec, "layer.1.encoding.text.field")).toBe("stats\\.count"); + const refs = vegaFieldReferences(spec); + for (const name of [ + measure, + "query.dataSource.name", + "query.context.metaData.type", + ]) { + expect(refs).not.toContain(name); + } + }); +}); diff --git a/web-common/src/features/components/charts/heatmap/spec.ts b/web-common/src/features/components/charts/heatmap/spec.ts index 41df5731ae6c..b79428138cb6 100644 --- a/web-common/src/features/components/charts/heatmap/spec.ts +++ b/web-common/src/features/components/charts/heatmap/spec.ts @@ -116,7 +116,9 @@ export function generateVLHeatmapSpec( }, text: { - field: config.color?.field ? config.color.field : undefined, + field: config.color?.field + ? sanitizeValueForVega(config.color.field) + : undefined, type: config.color?.type === "value" ? "nominal"