test(histogram): metric filters require aggregation in buildQuery (#30330) by rusackas · Pull Request #40617 · apache/superset

rusackas · 2026-06-01T23:02:17Z

SUMMARY

This is a test-only PR opened as a TDD-style validation of issue #30330.

#30330 reports that the Histogram chart fails with a database error when a "filter by metric" (HAVING clause) is added. rusackas confirmed the bug is still live on master.

Root cause: buildQuery.ts unconditionally sets metrics: undefined. Any HAVING-clause adhoc_filter therefore produces SQL with a HAVING clause but no aggregated metric or GROUP BY — invalid SQL that Druid (and most databases) reject.

This PR adds a buildQuery test with three cases:

should build query with column and no metrics — baseline: histogram sends no metrics (passes today).
should include groupby columns in query columns — groupby passthrough (passes today).
Regression for #30330 — asserts that when a HAVING adhoc_filter is present, query.metrics is defined and non-empty. Fails today (metrics is always undefined).

How to interpret CI

CI red → bug is live. Fix target: buildQuery.ts — detect HAVING adhoc_filters and preserve or synthesise a COUNT(*) metric so the HAVING clause has an aggregated value to filter on.
CI green → bug fixed; merge closes Histogram do not support filter by metric #30330 and locks in the regression guard.

TESTING INSTRUCTIONS

cd superset-frontend
npm run test -- plugins/plugin-chart-echarts/test/Histogram/buildQuery.test.ts --no-coverage

ADDITIONAL INFORMATION

Has associated issue: closes Histogram do not support filter by metric #30330
Required feature flags: N/A
Changes UI: No
Includes DB Migration: No
Introduces new feature or API: No

🤖 Generated with Claude Code

…0330) Regression for #30330: the histogram buildQuery unconditionally sets metrics: undefined. When a user adds a HAVING-clause "filter by metric", the generated SQL has a HAVING clause but no aggregated metric or GROUP BY — invalid SQL that most databases (including Druid) reject. This test asserts the desired behaviour: when HAVING adhoc_filters are present the query must include at least one metric so the HAVING clause has an aggregated value to filter on. Fix target: superset-frontend/plugins/plugin-chart-echarts/src/Histogram/buildQuery.ts — detect HAVING filters and preserve (or synthesise) metrics accordingly. Closes #30330 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

bito-code-review · 2026-06-01T23:02:28Z

Code Review Agent Run #384085

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: b7e3f65..b7e3f65
- superset-frontend/plugins/plugin-chart-echarts/test/Histogram/buildQuery.test.ts
Files skipped - 0
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

… present Closes #30330 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

netlify · 2026-06-01T23:57:38Z

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	`2efbdd4`
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a1e1c01ece217000851a459
😎 Deploy Preview	https://deploy-preview-40617--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

codecov · 2026-06-02T00:06:27Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.95%. Comparing base (1523d79) to head (2efbdd4).
⚠️ Report is 18 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #40617   +/-   ##
=======================================
  Coverage   63.94%   63.95%           
=======================================
  Files        2658     2658           
  Lines      143011   143015    +4     
  Branches    32866    32869    +3     
=======================================
+ Hits        91453    91461    +8     
+ Misses      49995    49991    -4     
  Partials     1563     1563

Flag	Coverage Δ
javascript	`67.38% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bito-code-review · 2026-06-02T02:58:51Z

Code Review Agent Run #89cdb2

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: b7e3f65..2efbdd4
- superset-frontend/plugins/plugin-chart-echarts/src/Histogram/buildQuery.ts
Files skipped - 0
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

aminghadersohi

CI all-green. Full scan ran — findings below as inline comments. Second opinion not needed (2 TypeScript files, ~95 lines, no security/migrations/multi-tenant code).

aminghadersohi · 2026-06-02T16:43:13Z

-  const { column, groupby = [] } = formData;
+  const { column, groupby = [], adhoc_filters } = formData;
+  const hasHavingFilter = (adhoc_filters ?? []).some(
+    (filter: { clause?: string }) => filter.clause === 'HAVING',


**MEDIUM — inline type should use **

HistogramFormData extends QueryFormData which already types adhoc_filters?: AdhocFilter[] | null via BaseFormData. TypeScript can infer filter: AdhocFilter in the .some() callback without an explicit annotation. The inline annotation introduces two problems:

Makes clause optional (adds ?) when BaseAdhocFilter.clause is the required union 'WHERE' | 'HAVING'.

Widens the element type from the literal union to string, losing narrowing.

Fix — remove the annotation and let TypeScript infer, or use the proper type:

(filter: AdhocFilter) => filter.clause === 'HAVING',

AdhocFilter is already re-exported from @superset-ui/core — no new import needed.

aminghadersohi · 2026-06-02T16:43:13Z

 export default function buildQuery(formData: HistogramFormData) {
-  const { column, groupby = [] } = formData;
+  const { column, groupby = [], adhoc_filters } = formData;
+  const hasHavingFilter = (adhoc_filters ?? []).some(


MEDIUM — extra_form_data.adhoc_filters HAVING filters not detected

adhoc_filters here is formData.adhoc_filters only. HAVING filters injected by dashboard native filter boxes arrive via extra_form_data.adhoc_filters, which buildQueryContext merges into the built query — but those filters are invisible to this check. If a dashboard box injects a HAVING clause, hasHavingFilter stays false, metrics stays undefined, and the invalid-SQL path from #30330 fires.

This is a niche path (HAVING filters via dashboard boxes are uncommon), so not a hard blocker. Worth a follow-up issue or an inline comment scoping the fix to the direct-filter path.

aminghadersohi · 2026-06-02T16:43:14Z


 export default function buildQuery(formData: HistogramFormData) {
-  const { column, groupby = [] } = formData;
+  const { column, groupby = [], adhoc_filters } = formData;


MEDIUM — PR title and description say test-only but the diff includes production code

The PR title uses the Conventional Commits test type (= tests only) and the description opens with "This is a test-only PR", but this file has +13/−2 lines of production logic — the actual fix. The description does go on to describe the fix approach, so intent is clear, but the title will mislead the Conventional Commits changelog tooling and reviewers scanning git log.

Suggested title: fix(histogram): synthesise COUNT(*) metric when HAVING filter is present (#30330)

Not a merge blocker — the code is correct — but worth fixing before merge.

aminghadersohi · 2026-06-02T16:43:28Z

-  const { column, groupby = [] } = formData;
+  const { column, groupby = [], adhoc_filters } = formData;
+  const hasHavingFilter = (adhoc_filters ?? []).some(
+    (filter: { clause?: string }) => filter.clause === 'HAVING',


MEDIUM — inline type { clause?: string } should use AdhocFilter

HistogramFormData extends QueryFormData which already types adhoc_filters?: AdhocFilter[] | null via BaseFormData. TypeScript can infer filter: AdhocFilter in the .some() callback without an explicit annotation. The inline annotation introduces two problems:

Makes clause optional (adds ?) when BaseAdhocFilter.clause is the required union 'WHERE' | 'HAVING'.

Widens the element type from the literal union to string, losing narrowing.

Fix — remove the annotation and let TypeScript infer, or use the proper type:

(filter: AdhocFilter) => filter.clause === 'HAVING',

AdhocFilter is already re-exported from @superset-ui/core — no new import needed.

aminghadersohi · 2026-06-02T16:43:37Z

 export default function buildQuery(formData: HistogramFormData) {
-  const { column, groupby = [] } = formData;
+  const { column, groupby = [], adhoc_filters } = formData;
+  const hasHavingFilter = (adhoc_filters ?? []).some(


MEDIUM — extra_form_data.adhoc_filters HAVING filters not detected

adhoc_filters here is formData.adhoc_filters only. HAVING filters injected by dashboard native filter boxes arrive via extra_form_data.adhoc_filters, which buildQueryContext merges into the built query — but those filters are invisible to this check. If a dashboard box injects a HAVING clause, hasHavingFilter stays false, metrics stays undefined, and the invalid-SQL path from #30330 fires.

This is a niche path (HAVING filters via dashboard boxes are uncommon), so not a hard blocker. Worth a follow-up issue or an inline comment scoping the fix to the direct-filter path.

aminghadersohi · 2026-06-02T16:43:46Z


 export default function buildQuery(formData: HistogramFormData) {
-  const { column, groupby = [] } = formData;
+  const { column, groupby = [], adhoc_filters } = formData;


MEDIUM — PR title and description say "test-only" but the diff includes production code

The PR title uses the Conventional Commits test type (= tests only) and the description opens with "This is a test-only PR", but this file has +13/−2 lines of production logic — the actual fix. The description does go on to describe the fix approach, so intent is clear, but the title will mislead changelog tooling and reviewers scanning git log.

Suggested title: fix(histogram): synthesise COUNT(*) metric when HAVING filter is present (#30330)

Not a merge blocker — the code is correct — but worth fixing before merge.

aminghadersohi · 2026-06-02T16:43:53Z

+  // HAVING filters without aggregation produce invalid SQL.
+  // The query must include at least one metric when HAVING filters are present.
+  expect(query.metrics).toBeDefined();
+  expect((query.metrics as unknown[]).length).toBeGreaterThan(0);


NIT — as unknown[] cast loses type safety; prefer .toHaveLength()

// before expect((query.metrics as unknown[]).length).toBeGreaterThan(0); // after expect(query.metrics).toHaveLength(1);

toHaveLength works directly on arrays, produces a better failure message, and avoids the unsafe cast. If you also want to lock down the synthesised metric shape:

expect(query.metrics?.[0]).toMatchObject({ expressionType: 'SQL', label: 'COUNT(*)' });

aminghadersohi · 2026-06-02T16:44:08Z

+  expect(query.columns).toEqual(['category', 'price']);
+});
+
+test('Regression for #30330: HAVING-clause metric filters require aggregation in the query', () => {


NIT — test name doesn't follow the 'should ...' convention used by the other two tests

Tests 1 and 2 use 'should ...' names. Consider:

test('should include COUNT(*) metric when HAVING filter is present', () => {

Keep the #30330 reference and the bug explanation in the block comment, where they add context without cluttering the Jest output.

aminghadersohi · 2026-06-02T16:44:17Z

+  const queryContext = buildQuery({ ...baseFormData, groupby: ['category'] });
+  const [query] = queryContext.queries;
+  expect(query.columns).toEqual(['category', 'price']);
+});


NIT — missing complementary WHERE-only filter test

There's no test confirming that a WHERE-only filter leaves metrics as undefined. This is the direct complement to the regression test and guards against a future refactor accidentally triggering on WHERE filters:

test('should not inject metrics for WHERE-only filters', () => { const formData = { ...baseFormData, adhoc_filters: [ { clause: 'WHERE', expressionType: 'SQL', sqlExpression: 'price > 10' }, ], }; const [query] = buildQuery(formData as HistogramFormData).queries; expect(query.metrics).toBeUndefined(); });

pull-request-size Bot added the size/M label Jun 1, 2026

dosubot Bot added the viz:charts:histogram Related to the Histogram chart label Jun 1, 2026

github-actions Bot added the plugins label Jun 1, 2026

fix(histogram): include aggregation metric when HAVING-clause filters…

2efbdd4

… present Closes #30330 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

rusackas requested review from aminghadersohi, Copilot and michael-s-molina June 2, 2026 16:14

Copilot started reviewing on behalf of rusackas June 2, 2026 16:14 View session

Copilot AI reviewed Jun 2, 2026

View reviewed changes

aminghadersohi reviewed Jun 2, 2026

View reviewed changes

aminghadersohi approved these changes Jun 2, 2026

View reviewed changes

sadpandajoe merged commit 24422c8 into master Jun 2, 2026
67 checks passed

sadpandajoe deleted the tdd/issue-30330-histogram-metric-filter branch June 2, 2026 17:49

Conversation

rusackas commented Jun 1, 2026

SUMMARY

How to interpret CI

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

bito-code-review Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #384085

Uh oh!

netlify Bot commented Jun 1, 2026

✅ Deploy Preview for superset-docs-preview ready!

Uh oh!

codecov Bot commented Jun 2, 2026

Codecov Report

Uh oh!

bito-code-review Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #89cdb2

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

aminghadersohi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bito-code-review Bot commented Jun 1, 2026 •

edited

Loading

bito-code-review Bot commented Jun 2, 2026 •

edited

Loading