piyushraj001 commented on issue #44079: URL: https://github.com/apache/superset/issues/44079#issuecomment-5903921051
<p dir="auto" data-start="47" data-end="286" class="PDq2pG_selectionAnchorContainer">The linked PR <a data-start="61" data-end="123" class="decorated-link" rel="noopener" target="_new" href="https://github.com/lilyvc/superset/pull/4";>lilyvc/superset#4</a> and my upstream PR <a data-start="143" data-end="198" rel="noopener" target="_new" class="decorated-link" href="https://github.com/apache/superset/pull/44044";>#44044</a> both identify the same parse-time issue and make the same change in <code data-start="267" data-end="285">parseResponse.ts</code>:<span aria-hidden="true" class="PDq2pG_selectionAnchor"></span></p> <pre class="overflow-visible! px-0!" data-start="288" data-end="359"><div class="relative w-full mt-4 mb-1"><div class=""><div class="contents"><div class="border border-token-border-light border-radius-3xl corner-superellipse/1.1 rounded-3xl"><div class="relative h-full w-full border-radius-3xl bg-(--code-block-surface) corner-superellipse/1.1 overflow-clip rounded-3xl [--code-block-surface:var(--bg-elevated-secondary)] dark:[--code-block-surface:var(--composer-surface-primary)] lxnfua_clipPathFallback"><div class="pointer-events-none absolute inset-x-4 top-12 bottom-4"><div class="pointer-events-none sticky z-40 shrink-0 z-1!"><div class="sticky bg-token-border-light"></div></div></div><div class="relative"><div class="h-full min-h-0 min-w-0"><div class="h-full min-h-0 min-w-0"><div class=""><div class="relative"><div class=""><div class="relative z-0 flex max-w-full"><div id="code-block-viewer" dir="ltr" class="q9tKkq_viewer cm-editor z-10 light:cm-light dark:cm-light flex h-fu ll w-full flex-col items-stretch ͼd ͼr"><div class="cm-scroller"><pre class="cm-content q9tKkq_readonly m-0"><code><span>- return BigInt(value.toFixed()); + return value.toFixed();</span></code></pre></div></div></div></div></div></div></div></div><div class=""><div class=""></div></div></div></div></div></div></div></div></pre> <p dir="auto" data-start="361" data-end="604">This addresses the parse-time source of the crash by keeping out-of-range integers as decimal strings instead of creating native <code data-start="490" data-end="498">bigint</code> values. The resulting values remain JSON-serializable while preserving their full decimal representation.</p> <p dir="auto" data-start="606" data-end="845">However, changing <code data-start="624" data-end="642">parseResponse.ts</code> alone is not sufficient for all downstream consumers. Once large integers are represented as decimal strings, several consumers that previously expected numeric values also need to handle these strings:</p> <div dir="auto" class="group TyagGW_tableContainer TyagGW_tableContainerWithTableOfContents"><div tabindex="-1" class="TyagGW_tableWrapper flex flex-col-reverse w-fit"> Consumer | What breaks without the downstream handling -- | -- formatValue.ts (table chart) | The formatter receives a decimal string instead of a number series.ts (ECharts) | Numeric aggregation can operate incorrectly on the string representation TableChart.tsx (bar-in-cell) | String values can be excluded from numeric range calculations Select/utils.tsx (filter dropdown) | Numeric strings can be sorted lexicographically rather than numerically </div></div> <p dir="auto" data-start="1335" data-end="1547">PR <a data-start="1338" data-end="1393" rel="noopener" target="_new" class="decorated-link" href="https://github.com/apache/superset/pull/44044";>#44044</a> already contains the same <code data-start="1420" data-end="1438">parseResponse.ts</code> change, along with the downstream handling for these cases and regression tests covering the relevant paths.</p> <p dir="auto" data-start="1549" data-end="1744">The author of <code data-start="1563" data-end="1582">lilyvc/superset#4</code> also notes in the PR description that the PR addresses the parse-time source and that the formatting/rendering layer is handled separately by upstream PR #44044.</p> <p dir="auto" data-start="1746" data-end="1916">So the overlap appears to be specifically the <code data-start="1792" data-end="1810">parseResponse.ts</code> fix, while #44044 additionally addresses the downstream consumers of the resulting decimal-string values.</p> <p dir="auto" data-start="1918" data-end="1985" data-is-last-node="" data-is-only-node="">Happy to clarify any part of the layered fix if helpful for review.</p>The linked PR [lilyvc/superset#4](https://github.com/lilyvc/superset/pull/4) and my upstream PR [#44044](https://github.com/apache/superset/pull/44044) both identify the same parse-time issue and make the same change in parseResponse.ts: - return BigInt(value.toFixed()); + return value.toFixed(); This addresses the parse-time source of the crash by keeping out-of-range integers as decimal strings instead of creating native bigint values. The resulting values remain JSON-serializable while preserving their full decimal representation. However, changing parseResponse.ts alone is not sufficient for all downstream consumers. Once large integers are represented as decimal strings, several consumers that previously expected numeric values also need to handle these strings: Consumer What breaks without the downstream handling formatValue.ts (table chart) The formatter receives a decimal string instead of a number series.ts (ECharts) Numeric aggregation can operate incorrectly on the string representation TableChart.tsx (bar-in-cell) String values can be excluded from numeric range calculations Select/utils.tsx (filter dropdown) Numeric strings can be sorted lexicographically rather than numerically PR [#44044](https://github.com/apache/superset/pull/44044) already contains the same parseResponse.ts change, along with the downstream handling for these cases and regression tests covering the relevant paths. The author of lilyvc/superset#4 also notes in the PR description that the PR addresses the parse-time source and that the formatting/rendering layer is handled separately by upstream PR #44044. So the overlap appears to be specifically the parseResponse.ts fix, while #44044 additionally addresses the downstream consumers of the resulting decimal-string values. Happy to clarify any part of the layered fix if helpful for review. -- 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]
