codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394243404
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
+ const rawSeries = sizeSeriesLabel
+ ? allRawSeries.filter(entry => !isSizeSeries(String(entry.name ?? '')))
+ : allRawSeries;
+ // Maps each value series' dimension key to a lookup from primary-axis value
+ // to size value.
+ let sizeLookups: Map<string, Map<DataRecordValue, number>> | undefined;
+ let sizeExtent: [number, number] | undefined;
+ if (sizeSeriesLabel) {
+ let sizeMin = Infinity;
+ let sizeMax = -Infinity;
+ if (sizeIsValueMetric) {
+ rawSeries.forEach(entry => {
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ });
+ } else {
+ sizeLookups = new Map();
+ allRawSeries
+ .filter(entry => isSizeSeries(String(entry.name ?? '')))
+ .forEach(entry => {
+ const name = String(entry.name ?? '');
+ const dimsKey =
+ name === sizeSeriesLabel
+ ? ''
+ : name.slice(sizeSeriesLabel.length + 2);
+ const lookup = new Map<DataRecordValue, number>();
+ (entry.data as DataRecordValue[][]).forEach(datum => {
+ const axisValue = isHorizontal ? datum[1] : datum[0];
+ const sizeValue = isHorizontal ? datum[0] : datum[1];
+ if (typeof sizeValue === 'number' && Number.isFinite(sizeValue)) {
+ lookup.set(axisValue, sizeValue);
+ sizeMin = Math.min(sizeMin, sizeValue);
+ sizeMax = Math.max(sizeMax, sizeValue);
+ }
+ });
+ sizeLookups!.set(dimsKey, lookup);
+ });
+ }
+ if (sizeMin <= sizeMax) {
+ sizeExtent = [sizeMin, sizeMax];
+ }
+ }
+ // Strips the metric label off a series name, leaving the dimension key used
+ // to match a value series with its size series.
+ const getSeriesDimsKey = (name: string): string => {
+ const matchedLabel = valueMetricLabels.find(
+ label => name === label || name.startsWith(`${label}, `),
+ );
+ if (matchedLabel === undefined) {
+ return name;
+ }
+ return name === matchedLabel ? '' : name.slice(matchedLabel.length + 2);
+ };
Review Comment:
**Suggestion:** Dimension-key extraction for matching value series to size
lookups only handles exact metric names and `<metric>, ...` names, but misses
derived names like `<metric>__<offset>...`. In time-comparison charts this
prevents matching value points to their size values, so symbol sizing silently
falls back to fixed marker size for derived series. Normalize/remap derived
metric names (including `__<offset>` forms) before extracting the dimension
key. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Time-comparison scatter dots ignore size metric on shifted series.
- ⚠️ Dot sizes inconsistent between base and time-shifted series.
- ⚠️ Misleads users interpreting size as a comparable measure.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the same ECharts Timeseries scatter configuration as in suggestion 1:
`seriesType =
EchartsTimeseriesSeriesType.Scatter`, a distinct `size` metric, and at least
one value
metric, all defined via `EchartsTimeseriesFormData`
(`src/Timeseries/types.ts:54-82`), and
enable time comparison with one or more offsets so the backend emits derived
series such
as `"metric__1 day ago"` as documented in the comment at
`transformProps.ts:485-488`.
2. When `transformProps` executes, it computes `sizeSeriesLabel`,
`valueMetricLabels`, and
`sizeLookups` (the latter built from size-series in the block at
`transformProps.ts:116-151`) with dimension keys extracted for size-series
as `dimsKey =
name === sizeSeriesLabel ? '' : name.slice(sizeSeriesLabel.length + 2)`
(`transformProps.ts:134-138`), so size lookups are keyed only by the
dimension portion
(e.g. `"US"` or `"US, segment"`), not by the full metric+offset string.
3. Later, in the `rawSeries.forEach(entry => { ... })` loop
(`transformProps.ts:223-235`),
the code tries to retrieve the matching size lookup for each value series
using `const
sizeLookup = sizeLookups?.get(getSeriesDimsKey(entryName));` inside the
symbol-size block
(`transformProps.ts:567-569`). The helper `getSeriesDimsKey` at
`transformProps.ts:417-425` only strips a value metric label when `name ===
label` or
`name.startsWith(\`${label}, \`)`; for time-comparison series named like
`"revenue__1 day
ago, US"` or `"revenue__1 day ago"` (per the comment at
`transformProps.ts:485-488`), no
`valueMetricLabels` element matches, so `matchedLabel` is `undefined` and
`getSeriesDimsKey` returns the entire derived name (e.g. `"revenue__1 day
ago, US"`).
4. Because `sizeLookups` were keyed by dimension-only strings (from
size-series names)
while `getSeriesDimsKey` returns full `"metric__offset, dims"` names for
derived
value-series, `sizeLookups.get(getSeriesDimsKey(entryName))` yields
`undefined` for those
derived series when `sizeIsValueMetric` is `false`. The condition `if
(sizeIsValueMetric
|| sizeLookup)` at `transformProps.ts:569-575` then fails, leaving
`symbolSizeFn`
undefined for derived series; `transformSeries` ultimately uses `symbolSize:
symbolSizeFn
?? markerSize` (`transformers.ts:120`), so time-shifted series fall back to
fixed
`markerSize` and ignore the configured dot-size metric while base
(non-derived) series can
still be sized correctly.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=60f2aa530dea4d8c900f895b55dd4432&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=60f2aa530dea4d8c900f895b55dd4432&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 417:425
**Comment:**
*Logic Error: Dimension-key extraction for matching value series to
size lookups only handles exact metric names and `<metric>, ...` names, but
misses derived names like `<metric>__<offset>...`. In time-comparison charts
this prevents matching value points to their size values, so symbol sizing
silently falls back to fixed marker size for derived series. Normalize/remap
derived metric names (including `__<offset>` forms) before extracting the
dimension key.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=816041b9c5616a646fee8d7d67d0a10c454aaef912e5bd9a185d80ef21e2d645&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=816041b9c5616a646fee8d7d67d0a10c454aaef912e5bd9a185d80ef21e2d645&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -337,6 +342,89 @@ export default function transformProps(
xAxisType,
},
);
+
+ // Dot size by metric (scatter): the size metric's series are excluded from
+ // rendering and instead provide per-point values that scale each marker's
+ // area between minMarkerSize and maxMarkerSize.
+ const sizeMetricLabel =
+ seriesType === EchartsTimeseriesSeriesType.Scatter && size
+ ? getMetricLabel(size)
+ : undefined;
+ const sizeSeriesLabel = isDefined(sizeMetricLabel)
+ ? (verboseMap[sizeMetricLabel!] ?? sizeMetricLabel)
+ : undefined;
+ const valueMetricLabels = ensureIsArray(metrics)
+ .map(getMetricLabel)
+ .map(label => verboseMap[label] ?? label);
+ // When the size metric is also a value metric, the query dedupes them into a
+ // single column, so each point's own value doubles as its size value.
+ const sizeIsValueMetric = isDefined(sizeSeriesLabel)
+ ? valueMetricLabels.includes(sizeSeriesLabel!)
+ : false;
+ const isSizeSeries = (name: string) =>
+ isDefined(sizeSeriesLabel) &&
+ !sizeIsValueMetric &&
+ (name === sizeSeriesLabel || name.startsWith(`${sizeSeriesLabel}, `));
Review Comment:
**Suggestion:** The size-series detection only matches exact metric labels
or the `<metric>, <group>` prefix, but time-comparison series are also emitted
as patterns like `<metric>__<offset>` (and related offset forms). Those derived
size series will not be filtered out, so the size metric can incorrectly render
as visible chart/legend series instead of being used only for dot sizing.
Extend the matcher to recognize time-offset naming patterns before filtering.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Time-comparison scatter charts show extra unintended size-series.
- ⚠️ Legends include confusing size-metric entries for each offset.
- ⚠️ Dot-size feature behaves inconsistently for shifted metrics.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an ECharts Timeseries chart (the plugin uses
`TimeseriesTransformProps`
exported from
`superset-frontend/plugins/plugin-chart-echarts/src/index.ts:55`) with
`seriesType` set to `EchartsTimeseriesSeriesType.Scatter` and a distinct
`size` metric set
in the form data (`EchartsTimeseriesFormData` fields `seriesType` and `size`
are defined
in `src/Timeseries/types.ts:44-79`).
2. Enable time comparison on the same chart by setting `timeCompare` /
`time_compare` in
the form data so that the backend returns time-shifted series;
`transformProps` receives
this in `timeCompare` and `timeShiftColor` (see destructuring in
`transformProps.ts:220-255`), and the comment at `transformProps.ts:485-488`
describes the
resulting time-comparison series naming patterns like `"metric__1 day ago"`
and `"1 day
ago, groupby"`.
3. When `transformProps` runs, it builds `[allRawSeries]` via `extractSeries`
(`transformProps.ts:70-85`), then computes `sizeSeriesLabel` and
`sizeIsValueMetric`
(`transformProps.ts:90-104`). The helper `isSizeSeries` at
`transformProps.ts:364-367`
only treats series as size-series when `name === sizeSeriesLabel` or
`name.startsWith(\`${sizeSeriesLabel}, \`)`, so time-shifted size-series
named according
to the documented patterns (e.g. `"sizeMetric__1 day ago"` or
`"sizeMetric__1 day ago,
US"`) are not recognized as size-series.
4. Because `rawSeries` is built as `sizeSeriesLabel ?
allRawSeries.filter(entry =>
!isSizeSeries(String(entry.name ?? ''))) : allRawSeries`
(`transformProps.ts:105-111`),
those unrecognized time-shifted size-metric series remain in `rawSeries` and
are then
iterated in the `rawSeries.forEach(entry => { ... const transformedSeries =
transformSeries(...); })` loop (`transformProps.ts:223-235`), causing the
size metric's
time-comparison series to be rendered as normal scatter/legend series
instead of being
used solely to drive marker sizes.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a96a8b044b604b6db83f913bd13088c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a96a8b044b604b6db83f913bd13088c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 364:367
**Comment:**
*Logic Error: The size-series detection only matches exact metric
labels or the `<metric>, <group>` prefix, but time-comparison series are also
emitted as patterns like `<metric>__<offset>` (and related offset forms). Those
derived size series will not be filtered out, so the size metric can
incorrectly render as visible chart/legend series instead of being used only
for dot sizing. Extend the matcher to recognize time-offset naming patterns
before filtering.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=0e6bdd5e5bcd23ebdd413ea88fc6d4442e2e7bf7880b69b7579db12fbb577983&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=0e6bdd5e5bcd23ebdd413ea88fc6d4442e2e7bf7880b69b7579db12fbb577983&reaction=dislike'>👎</a>
--
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]
