bito-code-review[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394640685
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -46,6 +46,24 @@ describe('Timeseries buildQuery', () => {
expect(query.orderby).toEqual([['bar', false]]);
});
+ test('should include the scatter dot size metric in query metrics', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'qux',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz', 'qux']);
+ });
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion is based on the observation that `buildQuery.ts`
does not explicitly handle the `size` property. However, if
`extractQueryFields` in the core logic correctly aliases `size` to `metrics`
before `buildQueryObject` is called, the test will indeed pass as expected. You
may consider marking this suggestion as resolved if the data flow is handled by
the core utility as you described.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts:
##########
@@ -46,6 +46,24 @@ describe('Timeseries buildQuery', () => {
expect(query.orderby).toEqual([['bar', false]]);
});
+ test('should include the scatter dot size metric in query metrics', () => {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'qux',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz', 'qux']);
+ });
+
+ test('should dedupe the dot size metric when it is also a value metric', ()
=> {
+ const queryContext = buildQuery({
+ ...formData,
+ size: 'bar',
+ });
+ const [query] = queryContext.queries;
+ expect(query.metrics).toEqual(['bar', 'baz']);
+ });
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding the deduplication logic is noted. Since the
implementation relies on `extractQueryFields` and `removeDuplicates` to handle
the metric mapping and deduplication, the test correctly validates the expected
behavior of the `buildQuery` function without requiring additional changes in
`buildQuery.ts`.
**superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/buildQuery.test.ts**
```
test('should dedupe the dot size metric when it is also a value metric', ()
=> {
const queryContext = buildQuery({
...formData,
size: 'bar',
});
const [query] = queryContext.queries;
expect(query.metrics).toEqual(['bar', 'baz']);
});
```
--
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]
