aminghadersohi commented on code in PR #40617:
URL: https://github.com/apache/superset/pull/40617#discussion_r3342892294
##########
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 `{ 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:
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.
--
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]
