mikebridge commented on code in PR #44909:
URL: https://github.com/apache/superset/pull/44909#discussion_r4209084025
##########
superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts:
##########
@@ -59,9 +60,17 @@ export default function buildQueryContext(
if (isXAxisSet(formData)) {
queries = queries.map(query => normalizeTimeColumn(formData, query));
}
+ const datasource = new DatasourceKey(formData.datasource).toObject();
+ if (datasource.type === DatasourceType.SemanticView) {
+ queries = queries.map(query =>
+ (query.series_limit ?? 0) > 0 && !query.series_columns?.length
+ ? { ...query, series_limit: 0 }
Review Comment:
Good catch. The backend infers series columns from `columns` for a
timeseries query with metrics, so the earlier frontend guard was suppressing a
real GroupLimit.
Could we use this narrower guard? Commit 23dc4685 preserves positive limits
for that inferred-series shape while still clearing genuinely no-series
queries. I added red-first Rose, Partition, Horizon and TimeTable tests, plus
the empty/no-metrics cases; the focused and broader suites are green.
##########
superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts:
##########
@@ -19,6 +19,41 @@
import { buildQueryContext, VizType } from '@superset-ui/core';
import * as queryModule from '../../src/query/normalizeTimeColumn';
+test('semantic queries omit a positive limit without final series columns', ()
=> {
+ const formData = {
+ datasource: '5__semantic_view',
+ viz_type: VizType.Table,
+ metrics: ['revenue'],
+ series_limit: 2,
+ };
+ const context = buildQueryContext(formData, baseQuery => [
+ { ...baseQuery, series_columns: [] },
+ ]);
+
+ expect(context.queries[0].series_limit).toBe(0);
+ expect(context.form_data?.series_limit).toBe(2);
+ expect(
+ buildQueryContext({ ...formData, datasource: '5__table' }, baseQuery => [
+ { ...baseQuery, series_columns: [] },
+ ]).queries[0].series_limit,
+ ).toBe(2);
+ expect(
+ buildQueryContext(formData, baseQuery => [
+ { ...baseQuery, series_columns: ['product'] },
+ ]).queries[0].series_limit,
+ ).toBe(2);
+ expect(
+ buildQueryContext(formData, baseQuery => [
+ { ...baseQuery, is_timeseries: true, metrics: [], series_columns: [] },
Review Comment:
Good catch. Would you mind checking the [follow-up
commit](https://github.com/apache/superset/commit/5815a31a828c949e3887fde319bdcea8c026d3d0)?
The empty-metrics case now keeps `columns` nonempty. Removing the metrics
conjunct makes that case fail (expected 0, received 2), and restoring the
unchanged production source makes all seven test cases pass.
--
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]
