Re: [PR] fix(echarts): drop the sort metric's time-shift column from timeseries series [superset]

2026-09-05 Thread via GitHub 


rusackas commented on code in PR #43175:
URL: https://github.com/apache/superset/pull/43175#discussion_r3943101831


##
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##
@@ -367,9 +367,20 @@ export default function transformProps(
   // metric. extraMetricLabels must be mapped the same way, or a sort-only
   // metric with a verbose_name set would silently fail to match here (and in
   // extractSeries below, which has the same requirement).
-  const extraMetricLabels = extractExtraMetrics(chartProps.rawFormData)
-.map(getMetricLabel)
-.map(label => verboseMap[label] ?? label);
+  const rawExtraMetricLabels = extractExtraMetrics(chartProps.rawFormData).map(
+getMetricLabel,
+  );
+  const timeCompareOffsets = ensureIsArray(timeCompare).map(String);
+  const extraMetricLabels = [
+...rawExtraMetricLabels.map(label => verboseMap[label] ?? label),
+// Time comparison emits a derived column per query metric and sort-only
+// metrics are part of the query, so the sort metric's shifted column has
+// to be excluded as well. Those columns keep the raw label with an
+// `__` suffix rather than the verbose name.
+...rawExtraMetricLabels.flatMap(label =>
+  timeCompareOffsets.map(offset => `${label}__${offset}`),
+),

Review Comment:
   Agreed the forecast variants (`__yhat` and friends) slip through here, but I 
think that is a pre-existing gap rather than something this PR introduced... 
`master` does not exclude `sort_metric__yhat` either, since the match is exact 
on the label. I would rather not scope-creep this one, so a follow-up seems 
fine unless @bikash-barnwal wants to fold it in.



-- 
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]

Re: [PR] fix(echarts): drop the sort metric's time-shift column from timeseries series [superset]

2026-08-26 Thread via GitHub 


rusackas commented on PR #43175:
URL: https://github.com/apache/superset/pull/43175#issuecomment-5434995505

   Other than the screenshots, code is looking good to me. Happy to stamp this 
if you can provide them for the record/review process.


-- 
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]

Re: [PR] fix(echarts): drop the sort metric's time-shift column from timeseries series [superset]

2026-08-17 Thread via GitHub 


sadpandajoe commented on PR #43175:
URL: https://github.com/apache/superset/pull/43175#issuecomment-5318108354

   @bikash-barnwal can you add before and after screenshots or videos into the 
PR description? 


-- 
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]

Re: [PR] fix(echarts): drop the sort metric's time-shift column from timeseries series [superset]

2026-08-14 Thread via GitHub 


bito-code-review[bot] commented on PR #43175:
URL: https://github.com/apache/superset/pull/43175#issuecomment-5299118416

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a 
very large PR).
   
   
   
   Bito Usage Guide
   
   
   **Commands**
 
   Type the following command in the pull request comment and save the comment.
 
   - `/review` - Manually triggers a full AI review.
   
   - `/pause` - Pauses automatic reviews on this pull request.
   - `/resume` - Resumes automatic reviews.
   - `/resolve` - Marks all Bito-posted review comments as resolved.
   - `/abort` - Cancels all in-progress reviews.
   
 
   Refer to the https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands";
 target="_blank" rel="noopener noreferrer">documentation for additional 
commands.
 
   **Configuration**
   
   This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" 
rel="noopener noreferrer">here or contact your Bito workspace admin at 
[email protected].
 
   **Documentation & Help**
   - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance";
 target="_blank" rel="noopener noreferrer">Customize agent settings
   - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules";
 target="_blank" rel="noopener noreferrer">Review rules
   - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; 
target="_blank" rel="noopener noreferrer">General documentation
   - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; 
target="_blank" rel="noopener noreferrer">FAQ
   
   
   


-- 
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]

Re: [PR] fix(echarts): drop the sort metric's time-shift column from timeseries series [superset]

2026-08-14 Thread via GitHub 


codecov[bot] commented on PR #43175:
URL: https://github.com/apache/superset/pull/43175#issuecomment-5298013026

   ## 
[Codecov](https://app.codecov.io/gh/apache/superset/pull/43175?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 66.70%. Comparing base 
([`c068a8c`](https://app.codecov.io/gh/apache/superset/commit/c068a8c09cf355e7fe493a7867bf33644d34ea39?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
 to head 
([`3c699d6`](https://app.codecov.io/gh/apache/superset/commit/3c699d62acde5895f0f7f5489ca29453fd7cf5db?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 7 commits behind head on master.
   
   Additional details and impacted files
   
   
   
   ```diff
   @@   Coverage Diff   @@
   ##   master   #43175   +/-   ##
   ===
 Coverage   66.70%   66.70%   
   ===
 Files2872 2872   
 Lines  163447   163449+2 
 Branches3772737727   
   ===
   + Hits   109024   109026+2 
 Misses  5229352293   
 Partials 2130 2130   
   ```
   
   | 
[Flag](https://app.codecov.io/gh/apache/superset/pull/43175/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Coverage Δ | |
   |---|---|---|
   | 
[javascript](https://app.codecov.io/gh/apache/superset/pull/43175/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `73.76% <100.00%> (+<0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click 
here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment)
 to find out more.
   
   
   [:umbrella: View full report in Codecov by 
Harness](https://app.codecov.io/gh/apache/superset/pull/43175?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   
   :loudspeaker: Have feedback on the report? [Share it 
here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:rocket: New features to boost your workflow: 
   
   - :snowflake: [Test 
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, 
report on failures, and find test suite problems.
   - :package: [JS Bundle 
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save 
yourself from yourself by tracking and limiting bundle sizes in JS merges.
   


-- 
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]

Re: [PR] fix(echarts): drop the sort metric's time-shift column from timeseries series [superset]

2026-08-14 Thread via GitHub 


bikash-barnwal commented on PR #43175:
URL: https://github.com/apache/superset/pull/43175#issuecomment-5297796533

   This also closes #43068 (stacked **Only Total** counting the sort-only 
metric's time-comparison column), which turns out to be the same one-line 
mechanism rather than a separate bug.
   
   `extraMetricLabels` feeds two consumers. Besides `sortAndFilterSeries`, it 
builds `excludedKeys` in `extractDataTotalValues`:
   
   ```ts
   const excludedKeys = new Set([xAxisCol, ...(extraMetricLabels ?? [])]);
   ```
   
   #42881 excluded the sort-only metric by exact name, so `SortMetric__1 year 
ago` matched nothing and was summed into the total — and into the 
`thresholdValues` derived from it, which is why the percentage-threshold cutoff 
drifted by the same amount. Adding the `__` entries fixes 
the total for free.
   
   Pushed a regression test pinning that: it fails on master (the total label 
reads `1032` instead of `32`) and passes with this change. Full file is 71/71 
green.


-- 
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]

Re: [PR] fix(echarts): drop the sort metric's time-shift column from timeseries series [superset]

2026-08-14 Thread via GitHub 


codeant-ai-for-open-source[bot] commented on code in PR #43175:
URL: https://github.com/apache/superset/pull/43175#discussion_r3786467231


##
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##
@@ -367,9 +367,20 @@ export default function transformProps(
   // metric. extraMetricLabels must be mapped the same way, or a sort-only
   // metric with a verbose_name set would silently fail to match here (and in
   // extractSeries below, which has the same requirement).
-  const extraMetricLabels = extractExtraMetrics(chartProps.rawFormData)
-.map(getMetricLabel)
-.map(label => verboseMap[label] ?? label);
+  const rawExtraMetricLabels = extractExtraMetrics(chartProps.rawFormData).map(
+getMetricLabel,
+  );
+  const timeCompareOffsets = ensureIsArray(timeCompare).map(String);
+  const extraMetricLabels = [
+...rawExtraMetricLabels.map(label => verboseMap[label] ?? label),
+// Time comparison emits a derived column per query metric and sort-only
+// metrics are part of the query, so the sort metric's shifted column has
+// to be excluded as well. Those columns keep the raw label with an
+// `__` suffix rather than the verbose name.
+...rawExtraMetricLabels.flatMap(label =>
+  timeCompareOffsets.map(offset => `${label}__${offset}`),
+),

Review Comment:
   **Suggestion:** The exclusion only includes the exact shifted column name, 
so when forecasting is enabled the sort-only metric's derived forecast columns 
(for example `sort_metric__1 year ago__yhat`, `__yhat_lower`, or 
`__yhat_upper`) are not excluded. `rebaseForecastDatum` preserves those shifted 
forecast names, allowing them to be included in stacked totals and rendered as 
extra series. Exclude the forecast-derived variants for each shifted sort 
metric as well. [incomplete implementation]
   
   
   Severity Level: Major ⚠️
   
   ```mdx
   - ❌ Forecasted sort-only columns appear in chart legends.
   - ❌ Stacked totals include unrequested forecast series.
   - ⚠️ Forecasting and time comparison produce misleading visuals.
   ```
   
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5ed77319ea124270924ca1f04d88767c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5ed77319ea124270924ca1f04d88767c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   Prompt for AI Agent 🤖 
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
   **Line:** 380:382
   **Comment:**
*Incomplete Implementation: The exclusion only includes the exact 
shifted column name, so when forecasting is enabled the sort-only metric's 
derived forecast columns (for example `sort_metric__1 year ago__yhat`, 
`__yhat_lower`, or `__yhat_upper`) are not excluded. `rebaseForecastDatum` 
preserves those shifted forecast names, allowing them to be included in stacked 
totals and rendered as extra series. Exclude the forecast-derived variants for 
each shifted sort metric as well.
   
   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
   ```
   
   👍
 | 👎



-- 
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]

Re: [PR] fix(echarts): drop the sort metric's time-shift column from timeseries series [superset]

2026-08-14 Thread via GitHub 


bito-code-review[bot] commented on PR #43175:
URL: https://github.com/apache/superset/pull/43175#issuecomment-5297117118

   
   The flagged issue is correct. When forecasting is enabled, the current 
implementation only excludes the base shifted column name, leaving 
forecast-derived variants (e.g., `__yhat`, `__yhat_lower`, `__yhat_upper`) to 
be incorrectly included in stacked totals and chart legends.
   
   To resolve this, you should extend the exclusion logic to also filter out 
these forecast-derived suffixes for each shifted sort metric. In 
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`,
 you can update the `extraMetricLabels` generation to include these variants in 
the exclusion list:
   
   ```typescript
   ...rawExtraMetricLabels.flatMap(label =>
 timeCompareOffsets.flatMap(offset => [
   `${label}__${offset}`,
   `${label}__${offset}__yhat`,
   `${label}__${offset}__yhat_lower`,
   `${label}__${offset}__yhat_upper`,
 ]),
   ),
   ```
   
   This ensures that all forecast-related columns associated with the sort-only 
metric are properly excluded.
   
   
**superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts**
   ```
   ...rawExtraMetricLabels.flatMap(label =>
 timeCompareOffsets.flatMap(offset => [
   `${label}__${offset}`,
   `${label}__${offset}__yhat`,
   `${label}__${offset}__yhat_lower`,
   `${label}__${offset}__yhat_upper`,
 ]),
   ),
   ```


-- 
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]

Re: [PR] fix(echarts): drop the sort metric's time-shift column from timeseries series [superset]

2026-08-14 Thread via GitHub 


bito-code-review[bot] commented on PR #43175:
URL: https://github.com/apache/superset/pull/43175#issuecomment-5297050322

   AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a 
very large PR).


-- 
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]