korbit-ai[bot] commented on code in PR #33269:
URL: https://github.com/apache/superset/pull/33269#discussion_r2065071259
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/renameOperator.ts:
##########
@@ -37,50 +38,60 @@
);
const { truncate_metric } = formData;
const xAxisLabel = getXAxisLabel(formData);
+ const isTimeComparisonValue = isTimeComparison(formData, queryObject);
+
// remove or rename top level of column name(metric name) in the MultiIndex
when
- // 1) only 1 metric
+ // 1) at least 1 metric
// 2) dimension exist
// 3) xAxis exist
- // 4) time comparison exist, and comparison type is "actual values"
- // 5) truncate_metric in form_data and truncate_metric is true
+ // 4) truncate_metric in form_data and truncate_metric is true
if (
- metrics.length === 1 &&
+ metrics.length > 0 &&
columns.length > 0 &&
xAxisLabel &&
- !(
- // todo: we should provide an approach to handle derived metrics
- (
- isTimeComparison(formData, queryObject) &&
- [
- ComparisonType.Difference,
- ComparisonType.Ratio,
- ComparisonType.Percentage,
- ].includes(formData.comparison_type)
- )
- ) &&
truncate_metric !== undefined &&
!!truncate_metric
) {
const renamePairs: [string, string | null][] = [];
-
if (
// "actual values" will add derived metric.
// we will rename the "metric" from the metricWithOffset label
// for example: "count__1 year ago" => "1 year ago"
- isTimeComparison(formData, queryObject) &&
- formData.comparison_type === ComparisonType.Values
+ isTimeComparisonValue
) {
const metricOffsetMap = getMetricOffsetsMap(formData, queryObject);
const timeOffsets = ensureIsArray(formData.time_compare);
- [...metricOffsetMap.keys()].forEach(metricWithOffset => {
- const offsetLabel = timeOffsets.find(offset =>
- metricWithOffset.includes(offset),
- );
- renamePairs.push([metricWithOffset, offsetLabel]);
- });
+ [...metricOffsetMap.entries()].forEach(
Review Comment:
### Inefficient Map Iteration <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using array spread with entries() followed by forEach creates unnecessary
intermediate array allocation.
###### Why this matters
This pattern creates a temporary array in memory just to iterate over the
entries, which is inefficient especially for large maps.
###### Suggested change ∙ *Feature Preview*
Use direct iteration over the Map entries to avoid temporary array
allocation:
```typescript
for (const [metricWithOffset, metricOnly] of metricOffsetMap) {
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f59aa482-66de-496a-9c73-bfd33d4a9bc4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f59aa482-66de-496a-9c73-bfd33d4a9bc4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f59aa482-66de-496a-9c73-bfd33d4a9bc4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f59aa482-66de-496a-9c73-bfd33d4a9bc4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f59aa482-66de-496a-9c73-bfd33d4a9bc4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:31de7941-2af9-4a66-9918-1f8998da38c5 -->
[](31de7941-2af9-4a66-9918-1f8998da38c5)
##########
superset-frontend/packages/superset-ui-chart-controls/src/operators/renameOperator.ts:
##########
@@ -37,50 +38,60 @@ export const renameOperator:
PostProcessingFactory<PostProcessingRename> = (
);
const { truncate_metric } = formData;
const xAxisLabel = getXAxisLabel(formData);
+ const isTimeComparisonValue = isTimeComparison(formData, queryObject);
+
// remove or rename top level of column name(metric name) in the MultiIndex
when
- // 1) only 1 metric
+ // 1) at least 1 metric
// 2) dimension exist
// 3) xAxis exist
- // 4) time comparison exist, and comparison type is "actual values"
- // 5) truncate_metric in form_data and truncate_metric is true
+ // 4) truncate_metric in form_data and truncate_metric is true
if (
- metrics.length === 1 &&
+ metrics.length > 0 &&
columns.length > 0 &&
xAxisLabel &&
- !(
- // todo: we should provide an approach to handle derived metrics
- (
- isTimeComparison(formData, queryObject) &&
- [
- ComparisonType.Difference,
- ComparisonType.Ratio,
- ComparisonType.Percentage,
- ].includes(formData.comparison_type)
- )
- ) &&
truncate_metric !== undefined &&
!!truncate_metric
) {
const renamePairs: [string, string | null][] = [];
-
if (
// "actual values" will add derived metric.
// we will rename the "metric" from the metricWithOffset label
// for example: "count__1 year ago" => "1 year ago"
- isTimeComparison(formData, queryObject) &&
- formData.comparison_type === ComparisonType.Values
+ isTimeComparisonValue
) {
const metricOffsetMap = getMetricOffsetsMap(formData, queryObject);
const timeOffsets = ensureIsArray(formData.time_compare);
- [...metricOffsetMap.keys()].forEach(metricWithOffset => {
- const offsetLabel = timeOffsets.find(offset =>
- metricWithOffset.includes(offset),
- );
- renamePairs.push([metricWithOffset, offsetLabel]);
- });
+ [...metricOffsetMap.entries()].forEach(
+ ([metricWithOffset, metricOnly]) => {
+ const offsetLabel = timeOffsets.find(offset =>
+ metricWithOffset.includes(offset),
+ );
Review Comment:
### Unhandled undefined from find() <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The find() operation could return undefined when no offset matches, but this
case is not handled.
###### Why this matters
If no matching offset is found, undefined will be used in string
concatenation later, potentially leading to unexpected output in the
visualization labels.
###### Suggested change ∙ *Feature Preview*
```typescript
const offsetLabel = timeOffsets.find(offset =>
metricWithOffset.includes(offset)
) ?? 'Unknown offset';
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69d1817a-8bf8-469d-adb9-8b1d9bd82506/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69d1817a-8bf8-469d-adb9-8b1d9bd82506?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69d1817a-8bf8-469d-adb9-8b1d9bd82506?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69d1817a-8bf8-469d-adb9-8b1d9bd82506?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/69d1817a-8bf8-469d-adb9-8b1d9bd82506)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a718e61e-d4b9-4ffb-bb28-92f95e229eb7 -->
[](a718e61e-d4b9-4ffb-bb28-92f95e229eb7)
--
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]
