This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch change/33615-chart-data-query-context in repository https://gitbox.apache.org/repos/asf/superset.git
commit fa1e850f637cb602800073920da6c051caee3314 Author: Eugene Olkhovski <[email protected]> AuthorDate: Mon Sep 14 07:01:22 2026 -0400 fix(charts): green CI + address query_context review follow-ups (#33615) Frontend registry codegen (fixes lint-frontend + pre-commit): - gen-qc-registry.mjs: quote object keys only when required (oxfmt `as-needed` style) so the emitted registry is oxfmt-idempotent and the `--check` and oxfmt hooks stop conflicting (both were failing on the same generated file). - gen-qc-registry.mjs: map Chord and Cartodiagram via explicit overrides. Chord registers its buildQuery lazily (`loadBuildQuery: () => import('./buildQuery')`) and Cartodiagram is constructed with arguments, neither of which the static structural matchers cover; each override path is asserted to exist. Scoped to the two viz types raised in review; generalizing the matchers is a follow-up. - Regenerated registry.generated.ts (also picks up the already-added Butterfly plugin the committed file was stale against). Python fallback builder (query_context_builder.py): - Derive raw-mode tables from `all_columns` (query_mode: raw) instead of classifying them non-derivable, so the bundled raw Table.yaml no longer imports with a null query_context that 400s. - Stop hard-coding row_limit=5000; leave it unset when the chart specifies none so the read path applies the runtime-configured ROW_LIMIT rather than silently truncating imported charts. Added unit tests for both. Co-Authored-By: Claude Opus 4.8 <[email protected]> --- superset-frontend/scripts/gen-qc-registry.mjs | 72 ++++++++++--- .../src/backend-querycontext/registry.generated.ts | 116 +++++++++++---------- superset/commands/chart/query_context_builder.py | 15 ++- .../importers/v1/query_context_builder_test.py | 32 ++++++ 4 files changed, 164 insertions(+), 71 deletions(-) diff --git a/superset-frontend/scripts/gen-qc-registry.mjs b/superset-frontend/scripts/gen-qc-registry.mjs index 3c798c5ace2..f864794c8fa 100644 --- a/superset-frontend/scripts/gen-qc-registry.mjs +++ b/superset-frontend/scripts/gen-qc-registry.mjs @@ -22,13 +22,24 @@ // by entry.ts. Join: buildQuery module <- (index.ts that imports it) -> plugin class // name -> MainPreset `.configure({ key: VizType.X })` -> VizType enum string. // A single builder legitimately maps to several keys (e.g. echarts_timeseries + _line/_bar/...). -import { readFileSync, writeFileSync, readdirSync, statSync, existsSync } from 'node:fs'; +import { + readFileSync, + writeFileSync, + readdirSync, + statSync, + existsSync, +} from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; const FE = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); const PLUGINS = path.join(FE, 'plugins'); -const OUT = path.join(FE, 'src', 'backend-querycontext', 'registry.generated.ts'); +const OUT = path.join( + FE, + 'src', + 'backend-querycontext', + 'registry.generated.ts', +); // --- 1. VizType enum: EnumName -> 'string_value' --- const vizTypeSrc = readFileSync( @@ -36,7 +47,8 @@ const vizTypeSrc = readFileSync( 'utf8', ); const VIZ_ENUM = {}; -for (const m of vizTypeSrc.matchAll(/(\w+)\s*=\s*['"]([\w-]+)['"]/g)) VIZ_ENUM[m[1]] = m[2]; +for (const m of vizTypeSrc.matchAll(/(\w+)\s*=\s*['"]([\w-]+)['"]/g)) + VIZ_ENUM[m[1]] = m[2]; // --- 2. MainPreset: ClassName -> viz string --- const mainPreset = readFileSync( @@ -47,7 +59,9 @@ const mainPreset = readFileSync( // PivotTableChartPluginV2 } from '...'`), then `new PivotTableChartPluginV2()`. Map // each local `new X()` name back to the ORIGINAL package export name the codegen sees. const importOrig = {}; // localName -> package-export name -for (const im of mainPreset.matchAll(/import\s*\{([^}]*)\}\s*from\s*['"][^'"]+['"]/g)) { +for (const im of mainPreset.matchAll( + /import\s*\{([^}]*)\}\s*from\s*['"][^'"]+['"]/g, +)) { for (let spec of im[1].split(',')) { spec = spec.trim(); if (!spec) continue; @@ -79,8 +93,8 @@ function walk(dir, acc = []) { return acc; } const files = walk(PLUGINS); -const indexFiles = files.filter((f) => /[/\\]index\.ts$/.test(f)); -const buildQueryFiles = files.filter((f) => /[/\\]buildQuery\.(ts|js)$/.test(f)); +const indexFiles = files.filter(f => /[/\\]index\.ts$/.test(f)); +const buildQueryFiles = files.filter(f => /[/\\]buildQuery\.(ts|js)$/.test(f)); // resolve an import specifier from a file to an absolute module file (ts/js/index) function resolveImport(fromFile, spec) { @@ -105,7 +119,9 @@ const edges = new Map(); // node -> node it forwards to const key = (name, file) => `${name}@${file}`; for (const idx of indexFiles) { const src = readFileSync(idx, 'utf8'); - for (const m of src.matchAll(/export\s*\{([^}]*)\}\s*from\s*['"]([^'"]+)['"]/g)) { + for (const m of src.matchAll( + /export\s*\{([^}]*)\}\s*from\s*['"]([^'"]+)['"]/g, + )) { const target = resolveImport(idx, m[2]); if (!target) continue; for (let spec of m[1].split(',')) { @@ -149,8 +165,13 @@ for (const bq of buildQueryFiles) { const owningIndexes = new Set(); for (const idx of indexFiles) { const src = readFileSync(idx, 'utf8'); - for (const m of src.matchAll(/import\s+\w+\s+from\s+['"]([^'"]+buildQuery)['"]/g)) { - if (resolveImport(idx, m[1]) === bqAbs && /export\s+default\s+class\s+\w+/.test(src)) + for (const m of src.matchAll( + /import\s+\w+\s+from\s+['"]([^'"]+buildQuery)['"]/g, + )) { + if ( + resolveImport(idx, m[1]) === bqAbs && + /export\s+default\s+class\s+\w+/.test(src) + ) owningIndexes.add(path.resolve(idx)); } } @@ -161,12 +182,34 @@ for (const bq of buildQueryFiles) { if (localCls) aliases.add(localCls[1]); // direct (unrenamed) registration for (const a of aliasesForDefault(idx)) aliases.add(a); // renamed re-exports } - const vizKeys = [...new Set([...aliases].map((c) => CLASS_TO_VIZ[c]).filter(Boolean))]; + const vizKeys = [ + ...new Set([...aliases].map(c => CLASS_TO_VIZ[c]).filter(Boolean)), + ]; if (vizKeys.length === 0) unmapped.push({ bq: path.relative(FE, bq), classes: [...aliases] }); else for (const v of vizKeys) REGISTRY[v] = bqAbs; } +// --- 4b. Explicit overrides for plugins the structural matchers above do not +// cover yet: Chord registers its buildQuery lazily +// (`loadBuildQuery: () => import('./buildQuery')`) rather than as a static +// default import, and Cartodiagram is constructed with arguments +// (`new CartodiagramPlugin({...}).configure(...)`) rather than the empty-paren +// form. Mapped explicitly so this change stays scoped to the two viz types +// raised in review (#33615); generalizing the matchers to every such plugin is +// left as a follow-up. Each path is asserted to exist so a plugin move surfaces +// here instead of silently dropping the entry. +const EXPLICIT_BUILD_QUERY = { + chord: 'plugins/plugin-chart-chord/src/buildQuery.ts', + cartodiagram: 'plugins/plugin-chart-cartodiagram/src/plugin/buildQuery.ts', +}; +for (const [viz, rel] of Object.entries(EXPLICIT_BUILD_QUERY)) { + const abs = path.resolve(FE, rel); + if (!existsSync(abs)) + throw new Error(`EXPLICIT_BUILD_QUERY path missing for '${viz}': ${rel}`); + REGISTRY[viz] = abs; +} + // --- 5. emit registry.generated.ts --- const entries = Object.entries(REGISTRY).sort(([a], [b]) => a.localeCompare(b)); let imports = ''; @@ -185,8 +228,12 @@ for (const [, abs] of entries) { imports += `import ${id} from '${rel}';\n`; } } +// Quote object keys only when required (dashes etc.), matching oxfmt's `as-needed` +// style so the emitted file is oxfmt-idempotent and the `--check` and oxfmt hooks agree. +const propKey = k => (/^[A-Za-z_$][\w$]*$/.test(k) ? k : `'${k}'`); let body = 'export const REGISTRY: Record<string, (fd: any) => any> = {\n'; -for (const [viz, abs] of entries) body += ` '${viz}': ${nameByPath.get(abs)} as (fd: any) => any,\n`; +for (const [viz, abs] of entries) + body += ` ${propKey(viz)}: ${nameByPath.get(abs)} as (fd: any) => any,\n`; body += '};\n\nexport const VIZ_TYPES: string[] = Object.keys(REGISTRY);\n'; const header = `// Licensed to the Apache Software Foundation (ASF) under one @@ -229,5 +276,6 @@ console.log(`buildQuery modules: ${buildQueryFiles.length}`); console.log(`viz keys mapped: ${entries.length}`); console.log(`keys: ${entries.map(([v]) => v).join(', ')}`); console.log(`unmapped buildQuery (no MainPreset key): ${unmapped.length}`); -for (const u of unmapped) console.log(` - ${u.bq} (classes: ${u.classes.join(',') || 'none'})`); +for (const u of unmapped) + console.log(` - ${u.bq} (classes: ${u.classes.join(',') || 'none'})`); console.log(`wrote ${path.relative(FE, OUT)}`); diff --git a/superset-frontend/src/backend-querycontext/registry.generated.ts b/superset-frontend/src/backend-querycontext/registry.generated.ts index 192c6a1d456..17b92227277 100644 --- a/superset-frontend/src/backend-querycontext/registry.generated.ts +++ b/superset-frontend/src/backend-querycontext/registry.generated.ts @@ -21,64 +21,70 @@ import bq2 from '../../plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal import bq3 from '../../plugins/plugin-chart-echarts/src/BoxPlot/buildQuery'; import bq4 from '../../plugins/plugin-chart-echarts/src/Bubble/buildQuery'; import bq5 from '../../plugins/plugin-chart-echarts/src/Bullet/buildQuery'; -import bq6 from '../../plugins/plugin-chart-echarts/src/Timeseries/buildQuery'; -import bq7 from '../../plugins/plugin-chart-echarts/src/Funnel/buildQuery'; -import bq8 from '../../plugins/plugin-chart-echarts/src/Gantt/buildQuery'; -import bq9 from '../../plugins/plugin-chart-echarts/src/Gauge/buildQuery'; -import bq10 from '../../plugins/plugin-chart-echarts/src/Graph/buildQuery'; -import bq11 from '../../plugins/plugin-chart-handlebars/src/plugin/buildQuery'; -import bq12 from '../../plugins/plugin-chart-echarts/src/Heatmap/buildQuery'; -import bq13 from '../../plugins/plugin-chart-echarts/src/Histogram/buildQuery'; -import bq14 from '../../plugins/plugin-chart-echarts/src/MixedTimeseries/buildQuery'; -import bq15 from '../../plugins/plugin-chart-echarts/src/Pie/buildQuery'; -import bq16 from '../../plugins/plugin-chart-pivot-table/src/plugin/buildQuery'; -import bq17 from '../../plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery'; -import bq18 from '../../plugins/plugin-chart-echarts/src/Radar/buildQuery'; -import bq19 from '../../plugins/plugin-chart-echarts/src/Rose/buildQuery'; -import bq20 from '../../plugins/plugin-chart-echarts/src/Sankey/buildQuery'; -import bq21 from '../../plugins/plugin-chart-echarts/src/Sunburst/buildQuery'; -import bq22 from '../../plugins/plugin-chart-table/src/buildQuery'; -import bq23 from '../../plugins/plugin-chart-echarts/src/TimePivot/buildQuery'; -import bq24 from '../../plugins/plugin-chart-echarts/src/Tree/buildQuery'; -import bq25 from '../../plugins/plugin-chart-echarts/src/Treemap/buildQuery'; -import bq26 from '../../plugins/plugin-chart-echarts/src/Waterfall/buildQuery'; -import bq27 from '../../plugins/plugin-chart-word-cloud/src/plugin/buildQuery'; +import bq6 from '../../plugins/plugin-chart-echarts/src/Butterfly/buildQuery'; +import bq7 from '../../plugins/plugin-chart-cartodiagram/src/plugin/buildQuery'; +import bq8 from '../../plugins/plugin-chart-chord/src/buildQuery'; +import bq9 from '../../plugins/plugin-chart-echarts/src/Timeseries/buildQuery'; +import bq10 from '../../plugins/plugin-chart-echarts/src/Funnel/buildQuery'; +import bq11 from '../../plugins/plugin-chart-echarts/src/Gantt/buildQuery'; +import bq12 from '../../plugins/plugin-chart-echarts/src/Gauge/buildQuery'; +import bq13 from '../../plugins/plugin-chart-echarts/src/Graph/buildQuery'; +import bq14 from '../../plugins/plugin-chart-handlebars/src/plugin/buildQuery'; +import bq15 from '../../plugins/plugin-chart-echarts/src/Heatmap/buildQuery'; +import bq16 from '../../plugins/plugin-chart-echarts/src/Histogram/buildQuery'; +import bq17 from '../../plugins/plugin-chart-echarts/src/MixedTimeseries/buildQuery'; +import bq18 from '../../plugins/plugin-chart-echarts/src/Pie/buildQuery'; +import bq19 from '../../plugins/plugin-chart-pivot-table/src/plugin/buildQuery'; +import bq20 from '../../plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/buildQuery'; +import bq21 from '../../plugins/plugin-chart-echarts/src/Radar/buildQuery'; +import bq22 from '../../plugins/plugin-chart-echarts/src/Rose/buildQuery'; +import bq23 from '../../plugins/plugin-chart-echarts/src/Sankey/buildQuery'; +import bq24 from '../../plugins/plugin-chart-echarts/src/Sunburst/buildQuery'; +import bq25 from '../../plugins/plugin-chart-table/src/buildQuery'; +import bq26 from '../../plugins/plugin-chart-echarts/src/TimePivot/buildQuery'; +import bq27 from '../../plugins/plugin-chart-echarts/src/Tree/buildQuery'; +import bq28 from '../../plugins/plugin-chart-echarts/src/Treemap/buildQuery'; +import bq29 from '../../plugins/plugin-chart-echarts/src/Waterfall/buildQuery'; +import bq30 from '../../plugins/plugin-chart-word-cloud/src/plugin/buildQuery'; export const REGISTRY: Record<string, (fd: any) => any> = { 'ag-grid-table': bq0 as (fd: any) => any, - 'big_number': bq1 as (fd: any) => any, - 'big_number_total': bq2 as (fd: any) => any, - 'box_plot': bq3 as (fd: any) => any, - 'bubble_v2': bq4 as (fd: any) => any, - 'bullet': bq5 as (fd: any) => any, - 'echarts_area': bq6 as (fd: any) => any, - 'echarts_timeseries': bq6 as (fd: any) => any, - 'echarts_timeseries_bar': bq6 as (fd: any) => any, - 'echarts_timeseries_line': bq6 as (fd: any) => any, - 'echarts_timeseries_scatter': bq6 as (fd: any) => any, - 'echarts_timeseries_smooth': bq6 as (fd: any) => any, - 'echarts_timeseries_step': bq6 as (fd: any) => any, - 'funnel': bq7 as (fd: any) => any, - 'gantt_chart': bq8 as (fd: any) => any, - 'gauge_chart': bq9 as (fd: any) => any, - 'graph_chart': bq10 as (fd: any) => any, - 'handlebars': bq11 as (fd: any) => any, - 'heatmap_v2': bq12 as (fd: any) => any, - 'histogram_v2': bq13 as (fd: any) => any, - 'mixed_timeseries': bq14 as (fd: any) => any, - 'pie': bq15 as (fd: any) => any, - 'pivot_table_v2': bq16 as (fd: any) => any, - 'pop_kpi': bq17 as (fd: any) => any, - 'radar': bq18 as (fd: any) => any, - 'rose': bq19 as (fd: any) => any, - 'sankey_v2': bq20 as (fd: any) => any, - 'sunburst_v2': bq21 as (fd: any) => any, - 'table': bq22 as (fd: any) => any, - 'time_pivot': bq23 as (fd: any) => any, - 'tree_chart': bq24 as (fd: any) => any, - 'treemap_v2': bq25 as (fd: any) => any, - 'waterfall': bq26 as (fd: any) => any, - 'word_cloud': bq27 as (fd: any) => any, + big_number: bq1 as (fd: any) => any, + big_number_total: bq2 as (fd: any) => any, + box_plot: bq3 as (fd: any) => any, + bubble_v2: bq4 as (fd: any) => any, + bullet: bq5 as (fd: any) => any, + butterfly: bq6 as (fd: any) => any, + cartodiagram: bq7 as (fd: any) => any, + chord: bq8 as (fd: any) => any, + echarts_area: bq9 as (fd: any) => any, + echarts_timeseries: bq9 as (fd: any) => any, + echarts_timeseries_bar: bq9 as (fd: any) => any, + echarts_timeseries_line: bq9 as (fd: any) => any, + echarts_timeseries_scatter: bq9 as (fd: any) => any, + echarts_timeseries_smooth: bq9 as (fd: any) => any, + echarts_timeseries_step: bq9 as (fd: any) => any, + funnel: bq10 as (fd: any) => any, + gantt_chart: bq11 as (fd: any) => any, + gauge_chart: bq12 as (fd: any) => any, + graph_chart: bq13 as (fd: any) => any, + handlebars: bq14 as (fd: any) => any, + heatmap_v2: bq15 as (fd: any) => any, + histogram_v2: bq16 as (fd: any) => any, + mixed_timeseries: bq17 as (fd: any) => any, + pie: bq18 as (fd: any) => any, + pivot_table_v2: bq19 as (fd: any) => any, + pop_kpi: bq20 as (fd: any) => any, + radar: bq21 as (fd: any) => any, + rose: bq22 as (fd: any) => any, + sankey_v2: bq23 as (fd: any) => any, + sunburst_v2: bq24 as (fd: any) => any, + table: bq25 as (fd: any) => any, + time_pivot: bq26 as (fd: any) => any, + tree_chart: bq27 as (fd: any) => any, + treemap_v2: bq28 as (fd: any) => any, + waterfall: bq29 as (fd: any) => any, + word_cloud: bq30 as (fd: any) => any, }; export const VIZ_TYPES: string[] = Object.keys(REGISTRY); diff --git a/superset/commands/chart/query_context_builder.py b/superset/commands/chart/query_context_builder.py index ce377a40ec7..99b3561a5c8 100644 --- a/superset/commands/chart/query_context_builder.py +++ b/superset/commands/chart/query_context_builder.py @@ -47,9 +47,6 @@ from superset.utils.core import split_adhoc_filters_into_base_filters # context that 400s on the data endpoint whenever the V8 bundle is unavailable. _NON_DATASOURCE_VIZ: frozenset[str] = frozenset({"markup", "divider"}) -# Default row limit mirrors the query-object default used across the read path. -_DEFAULT_ROW_LIMIT = 5000 - def _translate_adhoc_filters( adhoc_filters: list[Any] | None, @@ -141,6 +138,12 @@ def build_query_context_config( metrics = [params["metric"]] # `groupby` is the deprecated alias of `columns`. columns = params.get("columns") or params.get("groupby") or [] + # Raw (non-aggregated) tables carry no metrics/groupby and select row-level + # ``all_columns`` instead. Without this the bundled raw ``Table.yaml`` + # (query_mode: raw, empty metrics/groupby, populated all_columns) is + # misclassified non-derivable and its data endpoint 400s (#33615 review). + if not columns and params.get("query_mode") == "raw": + columns = params.get("all_columns") or [] if not metrics and not columns: return None @@ -160,7 +163,11 @@ def build_query_context_config( "metrics": metrics, "orderby": _derive_orderby(params), "annotation_layers": [], - "row_limit": params.get("row_limit", _DEFAULT_ROW_LIMIT), + # Leave ``row_limit`` unset when the chart does not specify one so the + # read path applies the runtime-configured ``ROW_LIMIT`` (50,000 by + # default), rather than baking in a hard-coded cap that would silently + # truncate imported charts to fewer rows than the UI shows (#33615 review). + "row_limit": params.get("row_limit"), "timeseries_limit": 0, "order_desc": params.get("order_desc", True), "url_params": {}, diff --git a/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py b/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py index 002af842e83..668461b3755 100644 --- a/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py +++ b/tests/unit_tests/charts/commands/importers/v1/query_context_builder_test.py @@ -151,6 +151,38 @@ def test_handlebars_is_derivable() -> None: assert query_context["queries"][0]["metrics"] == ["count"] +def test_raw_mode_all_columns_is_derivable() -> None: + """ + A raw-mode table (``query_mode: raw``) carries no metrics/groupby and selects + row-level ``all_columns``. It must be derivable from ``all_columns`` rather + than classified non-derivable — the bundled raw ``Table.yaml`` otherwise kept + a null query_context and 400d on its data endpoint (#33615 review). + """ + params = { + "query_mode": "raw", + "metrics": [], + "groupby": [], + "all_columns": ["gender", "name"], + } + query_context = build_query_context_config(params, "table", 7, "table") + assert query_context is not None + assert query_context["queries"][0]["columns"] == ["gender", "name"] + assert query_context["queries"][0]["metrics"] == [] + + +def test_row_limit_absent_left_unset_for_runtime_default() -> None: + """ + When the chart specifies no ``row_limit`` the payload must leave it unset so + the read path applies the runtime-configured ``ROW_LIMIT``, rather than baking + in a hard-coded cap that silently truncates imported charts (#33615 review). + """ + query_context = build_query_context_config( + {"metrics": ["count"]}, "table", 7, "table" + ) + assert query_context is not None + assert query_context["queries"][0]["row_limit"] is None + + def test_adhoc_simple_filter_translation() -> None: """SIMPLE adhoc filters → simple {col, op, val} filters.""" params = {
