aminghadersohi commented on code in PR #40617:
URL: https://github.com/apache/superset/pull/40617#discussion_r3342890413
##########
superset-frontend/plugins/plugin-chart-echarts/src/Histogram/buildQuery.ts:
##########
@@ -21,13 +21,24 @@ import { histogramOperator } from
'@superset-ui/chart-controls';
import { HistogramFormData } from './types';
export default function buildQuery(formData: HistogramFormData) {
- const { column, groupby = [] } = formData;
+ const { column, groupby = [], adhoc_filters } = formData;
+ const hasHavingFilter = (adhoc_filters ?? []).some(
+ (filter: { clause?: string }) => filter.clause === 'HAVING',
Review Comment:
**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:
1. Makes `clause` optional (adds `?`) when `BaseAdhocFilter.clause` is the
required union `'WHERE' | 'HAVING'`.
2. 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:
```ts
(filter: AdhocFilter) => filter.clause === 'HAVING',
```
`AdhocFilter` is already re-exported from `@superset-ui/core` — no new
import needed.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Histogram/buildQuery.ts:
##########
@@ -21,13 +21,24 @@ import { histogramOperator } from
'@superset-ui/chart-controls';
import { HistogramFormData } from './types';
export default function buildQuery(formData: HistogramFormData) {
- const { column, groupby = [] } = formData;
+ const { column, groupby = [], adhoc_filters } = formData;
+ const hasHavingFilter = (adhoc_filters ?? []).some(
Review Comment:
**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.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Histogram/buildQuery.ts:
##########
@@ -21,13 +21,24 @@ import { histogramOperator } from
'@superset-ui/chart-controls';
import { HistogramFormData } from './types';
export default function buildQuery(formData: HistogramFormData) {
- const { column, groupby = [] } = formData;
+ const { column, groupby = [], adhoc_filters } = formData;
Review Comment:
**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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
