aminghadersohi commented on code in PR #40617:
URL: https://github.com/apache/superset/pull/40617#discussion_r3342894536
##########
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
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.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Histogram/buildQuery.test.ts:
##########
@@ -0,0 +1,81 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import buildQuery from '../../src/Histogram/buildQuery';
+import { HistogramFormData } from '../../src/Histogram/types';
+
+const baseFormData: HistogramFormData = {
+ datasource: '5__table',
+ granularity_sqla: 'ds',
+ column: 'price',
+ groupby: [],
+ bins: 10,
+ viz_type: 'histogram',
+ cumulative: false,
+ normalize: false,
+ sliceId: 1,
+ showLegend: false,
+ showValue: false,
+ xAxisFormat: '',
+ xAxisTitle: '',
+ yAxisFormat: '',
+ yAxisTitle: '',
+};
+
+test('should build query with column and no metrics', () => {
+ const queryContext = buildQuery(baseFormData);
+ const [query] = queryContext.queries;
+ expect(query.columns).toContain('price');
+ expect(query.metrics).toBeUndefined();
+});
+
+test('should include groupby columns in query columns', () => {
+ const queryContext = buildQuery({ ...baseFormData, groupby: ['category'] });
+ const [query] = queryContext.queries;
+ expect(query.columns).toEqual(['category', 'price']);
+});
+
+test('Regression for #30330: HAVING-clause metric filters require aggregation
in the query', () => {
+ /**
+ * buildQuery unconditionally sets metrics: undefined, which means any
+ * HAVING-clause adhoc_filter produces SQL with a HAVING clause but no
+ * GROUP BY or aggregated metric — invalid SQL that most databases reject.
+ *
+ * The fix should preserve (or synthesise) a metric so the HAVING clause
+ * has an aggregated value to filter on. This test asserts that desired
+ * behaviour: when a HAVING adhoc_filter is present, query.metrics must
+ * not be undefined or empty.
+ */
+ const formDataWithHavingFilter: HistogramFormData = {
+ ...baseFormData,
+ adhoc_filters: [
+ {
+ clause: 'HAVING',
+ expressionType: 'SQL',
+ sqlExpression: 'COUNT(*) > 5',
+ },
+ ],
+ };
+ const queryContext = buildQuery(formDataWithHavingFilter);
+ const [query] = queryContext.queries;
+
+ // 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);
Review Comment:
**NIT — `as unknown[]` cast loses type safety; prefer `.toHaveLength()`**
```ts
// 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:
```ts
expect(query.metrics?.[0]).toMatchObject({ expressionType: 'SQL', label:
'COUNT(*)' });
```
--
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]
