bito-code-review[bot] commented on code in PR #44550:
URL: https://github.com/apache/superset/pull/44550#discussion_r4144478782
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -64,6 +68,73 @@ describe('Timeseries buildQuery', () => {
expect(query.metrics).toEqual(['bar', 'baz']);
});
+ test('should query the sort-only limit metric with dimensions so its pivoted
columns can order the axis', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ metrics: ['count'],
+ x_axis: 'genre',
+ groupby: ['platform'],
+ timeseries_limit_metric: 'na_sales',
+ x_axis_sort: 'na_sales',
+ x_axis_sort_asc: false,
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['count', 'na_sales']);
+ const pivot = (query.post_processing || []).find(
+ op => op?.operation === 'pivot',
+ );
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate pivot lookup idiom</b></div>
<div id="fix">
This diff locates the pivot op two different ways: line 83 hand-rolls
`op?.operation === 'pivot'` while line 128 uses the imported
`isPostProcessingPivot` guard (same predicate, `PostProcessing.ts:297`). Using
the guard in both keeps one idiom for the same lookup in this file and drops
the duplicated predicate.
</div>
</div>
<small><i>Code Review Run #6f5296</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -64,6 +68,73 @@ describe('Timeseries buildQuery', () => {
expect(query.metrics).toEqual(['bar', 'baz']);
});
+ test('should query the sort-only limit metric with dimensions so its pivoted
columns can order the axis', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ metrics: ['count'],
+ x_axis: 'genre',
+ groupby: ['platform'],
+ timeseries_limit_metric: 'na_sales',
+ x_axis_sort: 'na_sales',
+ x_axis_sort_asc: false,
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['count', 'na_sales']);
+ const pivot = (query.post_processing || []).find(
+ op => op?.operation === 'pivot',
+ );
+ expect(pivot?.options).toMatchObject({
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing assertion before find deref</b></div>
<div id="fix">
`pivot` comes from `.find()` (lines 83-85) and is dereferenced here via
optional chaining without a preceding `expect(pivot).toBeDefined()`. If the
pivot op ever goes missing, `toMatchObject` reports `Received: undefined`
instead of pinpointing the missing pivot. BITO rule 15727 asks for the explicit
assertion first.
</div>
</div>
<small><i>Code Review Run #6f5296</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -64,6 +68,73 @@ describe('Timeseries buildQuery', () => {
expect(query.metrics).toEqual(['bar', 'baz']);
});
+ test('should query the sort-only limit metric with dimensions so its pivoted
columns can order the axis', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ metrics: ['count'],
+ x_axis: 'genre',
+ groupby: ['platform'],
+ timeseries_limit_metric: 'na_sales',
+ x_axis_sort: 'na_sales',
+ x_axis_sort_asc: false,
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['count', 'na_sales']);
+ const pivot = (query.post_processing || []).find(
+ op => op?.operation === 'pivot',
+ );
+ expect(pivot?.options).toMatchObject({
+ index: ['genre'],
+ columns: ['platform'],
+ aggregates: {
+ count: { operator: 'mean' },
+ na_sales: { operator: 'mean' },
+ },
+ });
+ // With dimensions the chart sorts the pivoted rows itself.
+ expect(
+ (query.post_processing || []).map(op => op?.operation),
+ ).not.toContain('sort');
+ });
+
+ test('should not query the limit metric with dimensions when the axis is
sorted by a series aggregate', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ metrics: ['count'],
+ x_axis: 'genre',
+ groupby: ['platform'],
+ timeseries_limit_metric: 'na_sales',
+ x_axis_sort: 'sum',
+ x_axis_sort_asc: false,
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['count']);
+ });
+
+ test('should keep the sort-only metric through the pivot with time
comparison', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ metrics: ['count'],
+ x_axis: 'genre',
+ groupby: ['platform'],
+ timeseries_limit_metric: 'na_sales',
+ x_axis_sort: 'na_sales',
+ x_axis_sort_asc: false,
+ comparison_type: 'values',
+ time_compare: ['1 week ago'],
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['count', 'na_sales']);
+ const pivot = (query.post_processing || []).find(isPostProcessingPivot);
+ // The sort metric survives the pivot under its base label only; its
+ // time-shifted column is dropped along with the rest.
+ expect(Object.keys(pivot?.options.aggregates ?? {}).sort()).toEqual([
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing assertion before find deref</b></div>
<div id="fix">
`pivot` comes from `.find(isPostProcessingPivot)` (line 128) and
`pivot?.options.aggregates` is read here without a preceding
`expect(pivot).toBeDefined()`. If the pivot op is ever dropped, the failure
shows an empty-key array rather than naming the missing pivot. BITO rule 15727
asks for the explicit assertion first.
</div>
</div>
<small><i>Code Review Run #6f5296</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/xAxisSortWithDimensions.test.ts:
##########
@@ -0,0 +1,348 @@
+/**
+ * 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 {
+ ChartDataResponseResult,
+ DataRecord,
+ QueryFormMetric,
+} from '@superset-ui/core';
+import { SortSeriesType } from '@superset-ui/chart-controls';
+import transformProps from '../../src/Timeseries/transformProps';
+import { DEFAULT_FORM_DATA } from '../../src/Timeseries/constants';
+import {
+ EchartsTimeseriesChartProps,
+ EchartsTimeseriesFormData,
+ EchartsTimeseriesSeriesType,
+} from '../../src/Timeseries/types';
+import { createEchartsTimeseriesTestChartProps } from '../helpers';
+
+// A bar chart of COUNT(*) per genre, split by platform, with the axis sorted
+// by a metric that is neither the value metric nor a dimension (#34352).
+// The rows are what the backend returns for that query: the value metric's
+// columns lose their metric label (`truncate_metric`), the sort-only metric
+// keeps it, and `label_map` records the structure behind each column.
+const naSales: QueryFormMetric = {
+ expressionType: 'SIMPLE',
+ aggregate: 'SUM',
+ column: { column_name: 'na_sales' },
+ label: 'SUM(na_sales)',
+};
+
+const rows: DataRecord[] = [
+ {
+ genre: 'Action',
+ PS4: 30,
+ XOne: 20,
+ 'SUM(na_sales), PS4': 5,
+ 'SUM(na_sales), XOne': 3,
+ },
+ {
+ genre: 'Puzzle',
+ PS4: 5,
+ XOne: 5,
+ 'SUM(na_sales), PS4': 20,
+ 'SUM(na_sales), XOne': 10,
+ },
+ {
+ genre: 'Sports',
+ PS4: 40,
+ XOne: 10,
+ 'SUM(na_sales), PS4': 1,
+ 'SUM(na_sales), XOne': 1,
+ },
+];
+
+const labelMap = {
+ genre: ['genre'],
+ PS4: ['PS4'],
+ XOne: ['XOne'],
+ 'SUM(na_sales), PS4': ['SUM(na_sales)', 'PS4'],
+ 'SUM(na_sales), XOne': ['SUM(na_sales)', 'XOne'],
+};
+
+const queriesData = [
+ {
+ annotation_data: null,
+ cache_key: null,
+ cache_timeout: null,
+ cached_dttm: null,
+ queried_dttm: null,
+ data: rows,
+ colnames: [],
+ coltypes: [],
+ error: null,
+ is_cached: false,
+ query: '',
+ rowcount: rows.length,
+ sql_rowcount: rows.length,
+ stacktrace: null,
+ status: 'success',
+ from_dttm: null,
+ to_dttm: null,
+ label_map: labelMap,
+ } as unknown as ChartDataResponseResult,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Double assertion bypasses types</b></div>
<div id="fix">
The `as unknown as ChartDataResponseResult` double assertion silently
disables type checking for the fixture. `ChartDataResponseResult` already
declares `label_map` and `from_dttm`, so a correctly typed literal compiles
without the cast, and the cast can mask future fixture drift from the real
query-response contract. Prefer typing the object as `ChartDataResponseResult`
directly.
</div>
</div>
<small><i>Code Review Run #6f5296</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/xAxisSortWithDimensions.test.ts:
##########
@@ -0,0 +1,348 @@
+/**
+ * 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 {
+ ChartDataResponseResult,
+ DataRecord,
+ QueryFormMetric,
+} from '@superset-ui/core';
+import { SortSeriesType } from '@superset-ui/chart-controls';
+import transformProps from '../../src/Timeseries/transformProps';
+import { DEFAULT_FORM_DATA } from '../../src/Timeseries/constants';
+import {
+ EchartsTimeseriesChartProps,
+ EchartsTimeseriesFormData,
+ EchartsTimeseriesSeriesType,
+} from '../../src/Timeseries/types';
+import { createEchartsTimeseriesTestChartProps } from '../helpers';
+
+// A bar chart of COUNT(*) per genre, split by platform, with the axis sorted
+// by a metric that is neither the value metric nor a dimension (#34352).
+// The rows are what the backend returns for that query: the value metric's
+// columns lose their metric label (`truncate_metric`), the sort-only metric
+// keeps it, and `label_map` records the structure behind each column.
+const naSales: QueryFormMetric = {
+ expressionType: 'SIMPLE',
+ aggregate: 'SUM',
+ column: { column_name: 'na_sales' },
+ label: 'SUM(na_sales)',
+};
+
+const rows: DataRecord[] = [
+ {
+ genre: 'Action',
+ PS4: 30,
+ XOne: 20,
+ 'SUM(na_sales), PS4': 5,
+ 'SUM(na_sales), XOne': 3,
+ },
+ {
+ genre: 'Puzzle',
+ PS4: 5,
+ XOne: 5,
+ 'SUM(na_sales), PS4': 20,
+ 'SUM(na_sales), XOne': 10,
+ },
+ {
+ genre: 'Sports',
+ PS4: 40,
+ XOne: 10,
+ 'SUM(na_sales), PS4': 1,
+ 'SUM(na_sales), XOne': 1,
+ },
+];
+
+const labelMap = {
+ genre: ['genre'],
+ PS4: ['PS4'],
+ XOne: ['XOne'],
+ 'SUM(na_sales), PS4': ['SUM(na_sales)', 'PS4'],
+ 'SUM(na_sales), XOne': ['SUM(na_sales)', 'XOne'],
+};
+
+const queriesData = [
+ {
+ annotation_data: null,
+ cache_key: null,
+ cache_timeout: null,
+ cached_dttm: null,
+ queried_dttm: null,
+ data: rows,
+ colnames: [],
+ coltypes: [],
+ error: null,
+ is_cached: false,
+ query: '',
+ rowcount: rows.length,
+ sql_rowcount: rows.length,
+ stacktrace: null,
+ status: 'success',
+ from_dttm: null,
+ to_dttm: null,
+ label_map: labelMap,
+ } as unknown as ChartDataResponseResult,
+];
+
+type Series = {
+ name: string;
+ data: [string, number][];
+ label: {
+ formatter: (params: {
+ value: [string, number];
+ dataIndex: number;
+ seriesIndex: number;
+ }) => string;
+ };
+};
+
+// transformProps reads camelCase keys from `formData` and snake_case keys
+// from `rawFormData`; the test helper hands the same object to both.
+function transform(
+ overrides: Record<string, unknown>,
+ verboseMap = {},
+ data = rows,
+) {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Helper missing return type</b></div>
<div id="fix">
The `transform` helper returns an inferred object shape; the org rule on
explicit return type hints for test functions/helpers asks this to be
annotated. The shape is stable (`series`, `seriesNames`, `axisOrder`,
`stackedTotals`) and cheap to name, which documents the helper's contract and
catches accidental shape changes.
</div>
</div>
<small><i>Code Review Run #6f5296</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/xAxisSortWithDimensions.test.ts:
##########
@@ -0,0 +1,348 @@
+/**
+ * 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 {
+ ChartDataResponseResult,
+ DataRecord,
+ QueryFormMetric,
+} from '@superset-ui/core';
+import { SortSeriesType } from '@superset-ui/chart-controls';
+import transformProps from '../../src/Timeseries/transformProps';
+import { DEFAULT_FORM_DATA } from '../../src/Timeseries/constants';
+import {
+ EchartsTimeseriesChartProps,
+ EchartsTimeseriesFormData,
+ EchartsTimeseriesSeriesType,
+} from '../../src/Timeseries/types';
+import { createEchartsTimeseriesTestChartProps } from '../helpers';
+
+// A bar chart of COUNT(*) per genre, split by platform, with the axis sorted
+// by a metric that is neither the value metric nor a dimension (#34352).
+// The rows are what the backend returns for that query: the value metric's
+// columns lose their metric label (`truncate_metric`), the sort-only metric
+// keeps it, and `label_map` records the structure behind each column.
+const naSales: QueryFormMetric = {
+ expressionType: 'SIMPLE',
+ aggregate: 'SUM',
+ column: { column_name: 'na_sales' },
+ label: 'SUM(na_sales)',
+};
+
+const rows: DataRecord[] = [
+ {
+ genre: 'Action',
+ PS4: 30,
+ XOne: 20,
+ 'SUM(na_sales), PS4': 5,
+ 'SUM(na_sales), XOne': 3,
+ },
+ {
+ genre: 'Puzzle',
+ PS4: 5,
+ XOne: 5,
+ 'SUM(na_sales), PS4': 20,
+ 'SUM(na_sales), XOne': 10,
+ },
+ {
+ genre: 'Sports',
+ PS4: 40,
+ XOne: 10,
+ 'SUM(na_sales), PS4': 1,
+ 'SUM(na_sales), XOne': 1,
+ },
+];
+
+const labelMap = {
+ genre: ['genre'],
+ PS4: ['PS4'],
+ XOne: ['XOne'],
+ 'SUM(na_sales), PS4': ['SUM(na_sales)', 'PS4'],
+ 'SUM(na_sales), XOne': ['SUM(na_sales)', 'XOne'],
+};
+
+const queriesData = [
+ {
+ annotation_data: null,
+ cache_key: null,
+ cache_timeout: null,
+ cached_dttm: null,
+ queried_dttm: null,
+ data: rows,
+ colnames: [],
+ coltypes: [],
+ error: null,
+ is_cached: false,
+ query: '',
+ rowcount: rows.length,
+ sql_rowcount: rows.length,
+ stacktrace: null,
+ status: 'success',
+ from_dttm: null,
+ to_dttm: null,
+ label_map: labelMap,
+ } as unknown as ChartDataResponseResult,
+];
+
+type Series = {
+ name: string;
+ data: [string, number][];
+ label: {
+ formatter: (params: {
+ value: [string, number];
+ dataIndex: number;
+ seriesIndex: number;
+ }) => string;
+ };
+};
+
+// transformProps reads camelCase keys from `formData` and snake_case keys
+// from `rawFormData`; the test helper hands the same object to both.
+function transform(
+ overrides: Record<string, unknown>,
+ verboseMap = {},
+ data = rows,
+) {
+ const chartProps = createEchartsTimeseriesTestChartProps<
+ EchartsTimeseriesFormData,
+ EchartsTimeseriesChartProps
+ >({
+ defaultFormData: DEFAULT_FORM_DATA,
+ defaultVizType: 'echarts_timeseries_bar',
+ formData: {
+ colorScheme: 'bnbColors',
+ seriesType: EchartsTimeseriesSeriesType.Bar,
+ x_axis: 'genre',
+ xAxis: 'genre',
+ metrics: ['count'],
+ groupby: ['platform'],
+ timeseries_limit_metric: naSales,
+ truncate_metric: true,
+ stack: true,
+ showValue: true,
+ onlyTotal: true,
+ ...overrides,
+ } as Partial<EchartsTimeseriesFormData>,
+ queriesData:
+ data === rows
+ ? queriesData
+ : [{ ...queriesData[0], data } as ChartDataResponseResult],
+ datasource: { verboseMap },
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as Series[];
+ return {
+ series,
+ // Legend order is a separate control; only the set of series matters here.
+ seriesNames: series.map(({ name }) => name).sort(),
+ axisOrder: series[0].data.map(([genre]) => genre),
+ // With `onlyTotal` the top series' label carries the stacked total.
+ stackedTotals: series
+ .flatMap((entry, seriesIndex) =>
+ entry.data.map((value, dataIndex) =>
+ entry.label.formatter({ value, dataIndex, seriesIndex }),
+ ),
+ )
+ .filter(label => label !== ''),
+ };
+}
+
+test('sorts the axis by the sort-only metric across its pivoted columns', ()
=> {
+ const { axisOrder, seriesNames, stackedTotals } = transform({
+ x_axis_sort: 'SUM(na_sales)',
+ xAxisSort: 'SUM(na_sales)',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ });
+ // SUM(na_sales) totals: Puzzle 30, Action 8, Sports 2. Sorting by the value
+ // metric would put Action/Sports first, sorting by name would put Puzzle
+ // second.
+ expect(axisOrder).toEqual(['Puzzle', 'Action', 'Sports']);
+ expect(seriesNames).toEqual(['PS4', 'XOne']);
+ // COUNT(*) totals only, in the sorted order.
+ expect(stackedTotals).toEqual(['10', '50', '50']);
+});
+
+test('sorts the axis by the sort-only metric ascending', () => {
+ const { axisOrder } = transform({
+ x_axis_sort: 'SUM(na_sales)',
+ xAxisSort: 'SUM(na_sales)',
+ x_axis_sort_asc: true,
+ xAxisSortAsc: true,
+ });
+ expect(axisOrder).toEqual(['Sports', 'Action', 'Puzzle']);
+});
+
+test('sorts by the sort-only metric when it has a verbose name', () => {
+ const { axisOrder, seriesNames } = transform(
+ {
+ x_axis_sort: 'SUM(na_sales)',
+ xAxisSort: 'SUM(na_sales)',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ },
+ { 'SUM(na_sales)': 'NA Sales' },
+ );
+ expect(axisOrder).toEqual(['Puzzle', 'Action', 'Sports']);
+ expect(seriesNames).toEqual(['PS4', 'XOne']);
+});
+
+test('sorts the axis by the x-axis column with dimensions set', () => {
+ expect(
+ transform({
+ x_axis_sort: 'genre',
+ xAxisSort: 'genre',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ }).axisOrder,
+ ).toEqual(['Sports', 'Puzzle', 'Action']);
+ expect(
+ transform({
+ x_axis_sort: 'genre',
+ xAxisSort: 'genre',
+ x_axis_sort_asc: true,
+ xAxisSortAsc: true,
+ }).axisOrder,
+ ).toEqual(['Action', 'Puzzle', 'Sports']);
+});
+
+test('still sorts the axis by a series aggregate with dimensions set', () => {
+ // An aggregate sort does not query the sort-only metric, so its columns
+ // are not in the response.
+ const valueRows = rows.map(({ genre, PS4, XOne }) => ({ genre, PS4, XOne }));
+ const { axisOrder, seriesNames } = transform(
+ {
+ x_axis_sort: SortSeriesType.Max,
+ xAxisSort: SortSeriesType.Max,
+ x_axis_sort_asc: true,
+ xAxisSortAsc: true,
+ },
+ {},
+ valueRows,
+ );
+ // Max per row: Puzzle 5, Action 30, Sports 40.
+ expect(axisOrder).toEqual(['Puzzle', 'Action', 'Sports']);
+ expect(seriesNames).toEqual(['PS4', 'XOne']);
+});
+
+test('leaves the query order alone when the sort field is not in the data', ()
=> {
+ const { axisOrder } = transform({
+ x_axis_sort: 'jp_sales',
+ xAxisSort: 'jp_sales',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ });
+ expect(axisOrder).toEqual(['Action', 'Puzzle', 'Sports']);
+});
+
+test('sorts the axis by the sort-only metric with a time comparison', () => {
+ // With a time comparison the backend also returns the shifted value
+ // metric, which `renameOperator` relabels to the bare offset, and
+ // `label_map` leads those entries with the offset. The sort-only metric
+ // is pivoted like the value metric; its own shifted column is dropped by
+ // the pivot, so it never reaches the response.
+ const comparisonRows: DataRecord[] = [
+ {
+ genre: 'Action',
+ 'count, PS4': 30,
+ 'count, XOne': 20,
+ '1 year ago, PS4': 25,
+ '1 year ago, XOne': 15,
+ 'SUM(na_sales), PS4': 5,
+ 'SUM(na_sales), XOne': 3,
+ },
+ {
+ genre: 'Puzzle',
+ 'count, PS4': 5,
+ 'count, XOne': 5,
+ '1 year ago, PS4': 4,
+ '1 year ago, XOne': 6,
+ 'SUM(na_sales), PS4': 20,
+ 'SUM(na_sales), XOne': 10,
+ },
+ {
+ genre: 'Sports',
+ 'count, PS4': 40,
+ 'count, XOne': 10,
+ '1 year ago, PS4': 35,
+ '1 year ago, XOne': 12,
+ 'SUM(na_sales), PS4': 1,
+ 'SUM(na_sales), XOne': 1,
+ },
+ ];
+ const comparisonLabelMap = {
+ genre: ['genre'],
+ count: ['count'],
+ 'SUM(na_sales)': ['SUM(na_sales)'],
+ 'count, PS4': ['count', 'PS4'],
+ 'count, XOne': ['count', 'XOne'],
+ '1 year ago, PS4': ['1 year ago', 'PS4'],
+ '1 year ago, XOne': ['1 year ago', 'XOne'],
+ 'SUM(na_sales), PS4': ['SUM(na_sales)', 'PS4'],
+ 'SUM(na_sales), XOne': ['SUM(na_sales)', 'XOne'],
+ };
+ const chartProps = createEchartsTimeseriesTestChartProps<
+ EchartsTimeseriesFormData,
+ EchartsTimeseriesChartProps
+ >({
+ defaultFormData: DEFAULT_FORM_DATA,
+ defaultVizType: 'echarts_timeseries_bar',
+ formData: {
+ colorScheme: 'bnbColors',
+ seriesType: EchartsTimeseriesSeriesType.Bar,
+ x_axis: 'genre',
+ xAxis: 'genre',
+ metrics: ['count'],
+ groupby: ['platform'],
+ timeseries_limit_metric: naSales,
+ truncate_metric: true,
+ comparison_type: 'values',
+ comparisonType: 'values',
+ time_compare: ['1 year ago'],
+ timeCompare: ['1 year ago'],
+ x_axis_sort: 'SUM(na_sales)',
+ xAxisSort: 'SUM(na_sales)',
+ x_axis_sort_asc: false,
+ xAxisSortAsc: false,
+ } as Partial<EchartsTimeseriesFormData>,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: comparisonRows,
+ label_map: comparisonLabelMap,
+ } as unknown as ChartDataResponseResult,
+ ],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Repeated double assertion cast</b></div>
<div id="fix">
Same double assertion as line 98, repeated for the time-comparison fixture:
`as unknown as ChartDataResponseResult` discards shape checking for
`comparisonRows`/`comparisonLabelMap`. The spread base already satisfies the
interface, so typing the object directly keeps the fixture honest. Consider
extracting one typed fixture builder instead of repeating the cast.
</div>
</div>
<small><i>Code Review Run #6f5296</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
