Re: [PR] fix(echarts): normalize BigInt metric values before summing/dividing in stacked charts (#36401) [superset]

["sadpandajoe (via GitHub)" <gi...@apache.org>]

sadpandajoe commented on code in PR #42594:
URL: https://github.com/apache/superset/pull/42594#discussion_r3740966960


##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -434,7 +434,14 @@ export function extractDataTotalValues(
           return prev;
         }
         const value = datum[curr] || 0;
-        return prev + (value as number);
+        // Query results with integers beyond Number.MAX_SAFE_INTEGER are
+        // parsed as native BigInt (see
+        // packages/superset-ui-core/src/connection/callApi/parseResponse.ts).
+        // Normalize to Number before summing so BigInt and Number values
+        // can be combined without throwing (see #36401).
+        const numericValue =
+          typeof value === 'bigint' ? Number(value) : (value as number);

Review Comment:
   Normalization here only covers stacked totals, but the transform then calls `sortAndFilterSeries` with the default `SortSeriesType.Sum`. In the issue's two-row `a` series, one value is parsed as `bigint` and the other as `number`, so Lodash's `sumBy` still throws before `extractSeries` reaches the new Expand conversion; could we normalize metric values before the default series-sort aggregation and cover that two-row path?



##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -699,7 +706,17 @@ export function extractSeries(
           stack === StackControlsValue.Expand &&
           totalStackedValue !== undefined
         ) {
-          value = ((value || 0) as number) / totalStackedValue;
+          // Query results with integers beyond Number.MAX_SAFE_INTEGER are
+          // parsed as native BigInt (see
+          // packages/superset-ui-core/src/connection/callApi/parseResponse.ts).
+          // totalStackedValue is always a Number (extractDataTotalValues
+          // normalizes it), so dividing a raw BigInt datum value by it
+          // throws; normalize to Number first (see #36401).
+          const numericValue =

Review Comment:
   The per-datum conversion is limited to `Expand`, so `Stream` still emits the raw `bigint`; `getBaselineSeriesForStream` then evaluates `0.5 * delta` and throws before ECharts renders. Could we normalize stacked metric values before the mode-specific branch and add a Stream transform case that asserts numeric series output?



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org