Re: [PR] fix(Table chart): fix percentage metric column [superset]

Tue, 15 Jul 2025 06:44:15 -0700 


korbit-ai[bot] commented on code in PR #34175:
URL: https://github.com/apache/superset/pull/34175#discussion_r2207521524


##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -590,14 +590,30 @@ const config: ControlPanelConfig = {
                     )
                     .forEach((colname, index) => {
                       if (
-                        explore.form_data.metrics?.some(
-                          metric => getMetricLabel(metric) === colname,
-                        ) ||
+                        colname &&
+                        !colname.startsWith('%') &&
+                        colnames.includes(`%${colname}`) &&
                         explore.form_data.percent_metrics?.some(
                           (metric: QueryFormMetric) =>
                             getMetricLabel(metric) === colname,
+                        ) &&
+                        !explore.form_data.metrics?.some(
+                          (metric: QueryFormMetric) =>
+                            getMetricLabel(metric) === colname,
                         )
                       ) {
+                        return;
+                      }

Review Comment:
   ### Incorrect percentage metric filtering logic <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The condition to skip processing certain percentage metrics is incorrect. It 
will skip processing metrics that are in percent_metrics but not in metrics, 
which is the opposite of what should happen. The metric should be skipped if 
it's in both metrics and percent_metrics to avoid duplicate handling.
   
   
   ###### Why this matters
   This will cause percentage metrics to be incorrectly filtered out, leading 
to missing columns in the table visualization and incorrect data presentation.
   
   ###### Suggested change ∙ *Feature Preview*
   Modify the condition to skip when the metric appears in both metrics and 
percent_metrics:
   ```typescript
   if (
     colname &&
     !colname.startsWith('%') &&
     colnames.includes(`%${colname}`) &&
     explore.form_data.percent_metrics?.some(
       (metric: QueryFormMetric) =>
         getMetricLabel(metric) === colname,
     ) &&
     explore.form_data.metrics?.some(
       (metric: QueryFormMetric) =>
         getMetricLabel(metric) === colname,
     )
   ) {
     return;
   }
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/564bb273-ae5f-47e7-9100-d66236b8c3b6/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/564bb273-ae5f-47e7-9100-d66236b8c3b6?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/564bb273-ae5f-47e7-9100-d66236b8c3b6?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/564bb273-ae5f-47e7-9100-d66236b8c3b6?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/564bb273-ae5f-47e7-9100-d66236b8c3b6)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:e848678e-6d56-493e-adee-584e4d8c9141 -->
   
   
   [](e848678e-6d56-493e-adee-584e4d8c9141)



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

Reply via email to