korbit-ai[bot] commented on code in PR #32264:
URL: https://github.com/apache/superset/pull/32264#discussion_r1956568398
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,150 @@
+// @ts-nocheck
+
+import { format as d3Format } from 'd3-format';
+import NumberFormatter from '../NumberFormatter';
+import NumberFormats from '../NumberFormats';
+
+const float2PointFormatter = d3Format(`.2~f`);
+const float4PointFormatter = d3Format(`.4~f`);
+
+const bytesSILabels = ['Bytes', 'kB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB'];
+const bytesIECLabels = [
+ 'Bytes',
+ 'KiB',
+ 'MiB',
+ 'GiB',
+ 'TiB',
+ 'PiB',
+ 'EiB',
+ 'ZiB',
+];
+const byterateSILabels = [
+ 'Bytes/s',
+ 'kB/s',
+ 'MB/s',
+ 'GB/s',
+ 'TB/s',
+ 'PB/s',
+ 'EB/s',
+ 'ZB/s',
+];
+const byterateIECLabels = [
+ 'Bytes/s',
+ 'KiB/s',
+ 'MiB/s',
+ 'GiB/s',
+ 'TiB/s',
+ 'PiB/s',
+ 'EiB/s',
+ 'ZiB/s',
+];
+
+const formatterCache = new Map<number, Function>();
+
+function siFormatter(decimals: number) {
+ try {
+ if (!formatterCache.has(decimals)) {
+ formatterCache.set(decimals, d3Format(`.${decimals}s`));
+ }
+ return formatterCache.get(decimals);
+ } catch (error) {
+ throw new Error(
+ `Failed to create SI formatter for ${decimals} decimals:
${error.message}`,
+ );
+ }
+}
+
+function formatValue(
+ value: number,
+ labels: any,
+ base: number,
+ decimals: number,
+) {
+ if (value === 0) {
+ const formatted = `0 ${labels[0]}`;
+ return formatted;
+ }
+
+ const absoluteValue = Math.abs(value);
+ if (absoluteValue >= 1000) {
+ const i = Math.min(
+ Math.floor(Math.log(absoluteValue) / Math.log(base)),
+ labels.length - 1,
+ );
+ const parsedVal = parseFloat(
+ (absoluteValue / Math.pow(base, i)).toFixed(decimals),
+ );
+ return `${value < 0 ? '-' : ''}${parsedVal} ${labels[i]}`;
+ }
+ if (absoluteValue >= 1) {
+ const formattedVal = `${float2PointFormatter(value)} ${labels[0]}`;
+ return formattedVal;
+ }
+ if (absoluteValue >= 0.001) {
+ return `${float4PointFormatter(value)} ${labels[0]}`;
+ }
+
+ if (absoluteValue > 0.000001) {
+ return `${siFormatter(value * 1000000)}ยต ${labels[0]}`;
+ }
Review Comment:
### Invalid siFormatter Parameter for Microsecond Values <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The siFormatter function is called with 'value * 1000000' instead of the
expected decimals parameter, which will cause incorrect formatting for
microsecond values.
###### Why this matters
This will attempt to create a formatter with potentially millions of decimal
places instead of using the intended decimals parameter, leading to incorrect
formatting and possible performance issues.
###### Suggested change โ *Feature Preview*
Modify the code to use the decimals parameter correctly:
```typescript
if (absoluteValue > 0.000001) {
return `${siFormatter(decimals)(value * 1000000)}ยต ${labels[0]}`;
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aa90e161-0944-4c4f-be1e-11182b2a1db5?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:a83904b9-02a5-4161-b0a4-91c7c22233b3 -->
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,150 @@
+// @ts-nocheck
+
+import { format as d3Format } from 'd3-format';
+import NumberFormatter from '../NumberFormatter';
+import NumberFormats from '../NumberFormats';
+
+const float2PointFormatter = d3Format(`.2~f`);
+const float4PointFormatter = d3Format(`.4~f`);
+
+const bytesSILabels = ['Bytes', 'kB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB'];
+const bytesIECLabels = [
+ 'Bytes',
+ 'KiB',
+ 'MiB',
+ 'GiB',
+ 'TiB',
+ 'PiB',
+ 'EiB',
+ 'ZiB',
+];
+const byterateSILabels = [
+ 'Bytes/s',
+ 'kB/s',
+ 'MB/s',
+ 'GB/s',
+ 'TB/s',
+ 'PB/s',
+ 'EB/s',
+ 'ZB/s',
+];
+const byterateIECLabels = [
+ 'Bytes/s',
+ 'KiB/s',
+ 'MiB/s',
+ 'GiB/s',
+ 'TiB/s',
+ 'PiB/s',
+ 'EiB/s',
+ 'ZiB/s',
+];
+
+const formatterCache = new Map<number, Function>();
Review Comment:
### Unbounded Cache Growth <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The formatter cache is unbounded and grows with each unique decimal value
used, potentially leading to memory leaks.
###### Why this matters
Without an upper bound on cache size, the application could consume
excessive memory if many different decimal values are used over time.
###### Suggested change โ *Feature Preview*
Implement a size-limited LRU cache or set a maximum cache size with eviction
policy:
```typescript
const MAX_CACHE_SIZE = 100;
const formatterCache = new Map<number, Function>();
function siFormatter(decimals: number) {
if (formatterCache.size >= MAX_CACHE_SIZE) {
const firstKey = formatterCache.keys().next().value;
formatterCache.delete(firstKey);
}
// ... rest of the function
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e7fef1f6-d066-4298-8772-1f988047ebeb?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:5a978e68-3d5e-4283-81da-2d04acfce612 -->
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,150 @@
+// @ts-nocheck
+
+import { format as d3Format } from 'd3-format';
+import NumberFormatter from '../NumberFormatter';
+import NumberFormats from '../NumberFormats';
+
+const float2PointFormatter = d3Format(`.2~f`);
+const float4PointFormatter = d3Format(`.4~f`);
+
+const bytesSILabels = ['Bytes', 'kB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB'];
+const bytesIECLabels = [
+ 'Bytes',
+ 'KiB',
+ 'MiB',
+ 'GiB',
+ 'TiB',
+ 'PiB',
+ 'EiB',
+ 'ZiB',
+];
+const byterateSILabels = [
+ 'Bytes/s',
+ 'kB/s',
+ 'MB/s',
+ 'GB/s',
+ 'TB/s',
+ 'PB/s',
+ 'EB/s',
+ 'ZB/s',
+];
+const byterateIECLabels = [
+ 'Bytes/s',
+ 'KiB/s',
+ 'MiB/s',
+ 'GiB/s',
+ 'TiB/s',
+ 'PiB/s',
+ 'EiB/s',
+ 'ZiB/s',
+];
+
+const formatterCache = new Map<number, Function>();
+
+function siFormatter(decimals: number) {
+ try {
+ if (!formatterCache.has(decimals)) {
+ formatterCache.set(decimals, d3Format(`.${decimals}s`));
+ }
+ return formatterCache.get(decimals);
+ } catch (error) {
+ throw new Error(
+ `Failed to create SI formatter for ${decimals} decimals:
${error.message}`,
+ );
+ }
Review Comment:
### Stack trace lost in error re-throw <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error handling re-throws the error with a new message but loses the
original error stack trace.
###### Why this matters
Loss of the original stack trace makes debugging more difficult as it
removes valuable context about where the error originated.
###### Suggested change โ *Feature Preview*
Use the Error cause option to preserve the original error:
```typescript
try {
if (!formatterCache.has(decimals)) {
formatterCache.set(decimals, d3Format(`.${decimals}s`));
}
return formatterCache.get(decimals);
} catch (error) {
throw new Error(
`Failed to create SI formatter for ${decimals} decimals:
${error.message}`,
{ cause: error }
);
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d0d702bd-e884-4eb1-8b1c-18c4d0799d07?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:91eb2918-61d2-43d2-adb4-ace5aa8e35f6 -->
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,150 @@
+// @ts-nocheck
+
+import { format as d3Format } from 'd3-format';
+import NumberFormatter from '../NumberFormatter';
+import NumberFormats from '../NumberFormats';
+
+const float2PointFormatter = d3Format(`.2~f`);
+const float4PointFormatter = d3Format(`.4~f`);
+
+const bytesSILabels = ['Bytes', 'kB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB'];
+const bytesIECLabels = [
+ 'Bytes',
+ 'KiB',
+ 'MiB',
+ 'GiB',
+ 'TiB',
+ 'PiB',
+ 'EiB',
+ 'ZiB',
+];
+const byterateSILabels = [
+ 'Bytes/s',
+ 'kB/s',
+ 'MB/s',
+ 'GB/s',
+ 'TB/s',
+ 'PB/s',
+ 'EB/s',
+ 'ZB/s',
+];
+const byterateIECLabels = [
+ 'Bytes/s',
+ 'KiB/s',
+ 'MiB/s',
+ 'GiB/s',
+ 'TiB/s',
+ 'PiB/s',
+ 'EiB/s',
+ 'ZiB/s',
+];
+
+const formatterCache = new Map<number, Function>();
+
+function siFormatter(decimals: number) {
+ try {
+ if (!formatterCache.has(decimals)) {
+ formatterCache.set(decimals, d3Format(`.${decimals}s`));
+ }
+ return formatterCache.get(decimals);
+ } catch (error) {
+ throw new Error(
+ `Failed to create SI formatter for ${decimals} decimals:
${error.message}`,
+ );
+ }
+}
+
+function formatValue(
+ value: number,
+ labels: any,
Review Comment:
### Missing Type Safety for Labels Parameter <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The formatValue function accepts 'labels' parameter as 'any' type, which
could lead to runtime errors if invalid label arrays are provided.
###### Why this matters
Without proper type checking, the function might attempt to access array
indices that don't exist or process non-string labels, potentially causing
crashes.
###### Suggested change โ *Feature Preview*
Add proper typing for the labels parameter:
```typescript
function formatValue(
value: number,
labels: string[],
base: number,
decimals: number,
)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f718653f-a150-405d-bdd2-a0d3b6b3599a?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:b0eeff82-3cf2-4cd9-b3b6-fdd3be380c8d -->
--
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]
