Re: [PR] fix(big number with trendline):Make aggregation optional. [superset]

via GitHub Mon, 12 May 2025 02:17:56 -0700


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



##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.ts:
##########
@@ -52,15 +52,19 @@ export default function buildQuery(formData: QueryFormData) 
{
     {
       ...baseQueryObject,
       columns: [
-        ...(isXAxisSet(formData)
-          ? ensureIsArray(getXAxisColumn(formData))
+        ...(formData.aggregation
+          ? isXAxisSet(formData)
+            ? ensureIsArray(getXAxisColumn(formData))
+            : []
           : []),
       ],
-      ...(isXAxisSet(formData) ? {} : { is_timeseries: true }),
-      post_processing: [
-        pivotOperator(formData, baseQueryObject),
-        aggregationOperator(formData, baseQueryObject),
-      ],
+      is_timeseries: !!formData.aggregation,

Review Comment:
   ### Incorrect Time Series Flag Logic <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The is_timeseries flag is incorrectly tied to the presence of aggregation, 
when it should be based on whether an X-axis is set (as in the first query 
object).
   
   ###### Why this matters
   This could cause incorrect data processing for time series data when no 
aggregation is selected, potentially leading to visualization errors or data 
misrepresentation.
   
   ###### Suggested change ∙ *Feature Preview*
   The is_timeseries flag should maintain the same logic as the first query 
object:
   ```typescript
         ...(isXAxisSet(formData) ? {} : { is_timeseries: true }),
   ```
   
   
   ###### 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/526a4bc5-4e0c-409a-8916-83a32dae1c19/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/526a4bc5-4e0c-409a-8916-83a32dae1c19?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/526a4bc5-4e0c-409a-8916-83a32dae1c19?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/526a4bc5-4e0c-409a-8916-83a32dae1c19?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/526a4bc5-4e0c-409a-8916-83a32dae1c19)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:75dc8d09-d748-4e67-bf96-cce6394a188d -->
   
   
   [](75dc8d09-d748-4e67-bf96-cce6394a188d)



##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/buildQuery.ts:
##########
@@ -52,15 +52,19 @@
     {
       ...baseQueryObject,
       columns: [
-        ...(isXAxisSet(formData)
-          ? ensureIsArray(getXAxisColumn(formData))
+        ...(formData.aggregation
+          ? isXAxisSet(formData)
+            ? ensureIsArray(getXAxisColumn(formData))
+            : []
           : []),
       ],
-      ...(isXAxisSet(formData) ? {} : { is_timeseries: true }),
-      post_processing: [
-        pivotOperator(formData, baseQueryObject),
-        aggregationOperator(formData, baseQueryObject),
-      ],
+      is_timeseries: !!formData.aggregation,
+      post_processing: formData.aggregation
+        ? [
+            pivotOperator(formData, baseQueryObject),
+            aggregationOperator(formData, baseQueryObject),
+          ]
+        : [],

Review Comment:
   ### Missing Essential Post-Processing <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   When aggregation is disabled, all post-processing operations are skipped, 
including pivoting which might still be necessary for proper data 
transformation.
   
   ###### Why this matters
   This could result in incorrectly structured data when pivoting is required 
but aggregation is not selected, leading to visualization failures.
   
   ###### Suggested change ∙ *Feature Preview*
   Separate the pivot operation from the aggregation condition:
   ```typescript
         post_processing: [
           pivotOperator(formData, baseQueryObject),
           ...(formData.aggregation ? [aggregationOperator(formData, 
baseQueryObject)] : []),
         ],
   ```
   
   
   ###### 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/6f1fe1b8-933f-4b7d-aad1-60a025293765/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f1fe1b8-933f-4b7d-aad1-60a025293765?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/6f1fe1b8-933f-4b7d-aad1-60a025293765?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/6f1fe1b8-933f-4b7d-aad1-60a025293765?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f1fe1b8-933f-4b7d-aad1-60a025293765)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:94b9b312-ed06-4d0d-9c84-4ddc8b957ff8 -->
   
   
   [](94b9b312-ed06-4d0d-9c84-4ddc8b957ff8)



-- 
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(big number with trendline):Make aggregation optional. [superset]

Reply via email to