korbit-ai[bot] commented on code in PR #32264:
URL: https://github.com/apache/superset/pull/32264#discussion_r1956579047
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,155 @@
+// @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 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;
Review Comment:
### Unsafe cache key access <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
No error handling for the case when the cache is empty but trying to access
the first key.
###### Why this matters
Accessing .value on an empty iterator's next() result will return undefined,
potentially causing errors in subsequent operations.
###### Suggested change ∙ *Feature Preview*
Add a safety check before accessing the cache:
```typescript
if (formatterCache.size >= MAX_CACHE_SIZE) {
const iterator = formatterCache.keys();
const first = iterator.next();
if (!first.done) {
formatterCache.delete(first.value);
}
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e4889a3c-4e6c-4d07-badb-e9eade5c8174?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:cc5e03b1-58ad-4245-b089-a57c90b79dc0 -->
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/D3Formatting.ts:
##########
@@ -60,6 +60,10 @@ export const D3_FORMAT_OPTIONS: [string, string][] = [
['DURATION_COL', t('Duration in ms (10500 => 0:10.5)')],
['MEMORY_DECIMAL', t('Memory in bytes - decimal (1024B => 1.024kB)')],
['MEMORY_BINARY', t('Memory in bytes - binary (1024B => 1KiB)')],
+ [NumberFormats.BYTES_SI, 'Bytes in SI (kB, MB)'],
+ [NumberFormats.BYTES_IEC, 'Bytes in IEC (kiB, MiB)'],
+ [NumberFormats.BYTERATE_SI, 'Byterate in SI (kB/s, MB/s)'],
+ [NumberFormats.BYTERATE_IEC, 'Byterate in IEC (kiB/s, MiB/s)'],
Review Comment:
### Incorrect Unit Notation in Format Descriptions <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The unit notation in the description strings is inconsistent with standard
SI and IEC conventions. 'kB' should be 'KB' for SI, and 'kiB' should be 'KiB'
for IEC.
###### Why this matters
Incorrect unit notation can mislead users and cause confusion when
interpreting data transfer rates, potentially leading to miscalculations in
network-related analyses.
###### Suggested change ∙ *Feature Preview*
```typescript
[NumberFormats.BYTES_SI, 'Bytes in SI (KB, MB)'],
[NumberFormats.BYTES_IEC, 'Bytes in IEC (KiB, MiB)'],
[NumberFormats.BYTERATE_SI, 'Byterate in SI (KB/s, MB/s)'],
[NumberFormats.BYTERATE_IEC, 'Byterate in IEC (KiB/s, MiB/s)'],
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/19cc3b93-20e5-44d1-a2d2-22379809ddea?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:3c9d630e-b541-4164-997c-8fd0be7829f8 -->
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,155 @@
+// @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 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);
+ }
+
+ 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 },
+ );
+ }
+}
+
+function formatValue(
+ value: number,
+ labels: string[],
+ 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]}`;
Review Comment:
### Inefficient Small Number Formatting Strategy <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Repeated computation of float4PointFormatter for very small values (<0.001)
and as a fallback, when a single format strategy could be used.
###### Why this matters
Redundant formatter calls and string operations increase CPU cycles
unnecessarily when handling small numbers.
###### Suggested change ∙ *Feature Preview*
Consolidate the small number formatting logic into a single case using a
consistent strategy. For example:
```typescript
if (absoluteValue < 0.001) {
return `${siFormatter(decimals)(value)} ${labels[0]}`;
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32b94687-16ef-45cf-b06a-4bc49316e18b?suggestedFixEnabled=true)
💬 Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:12456ccf-c913-44cd-960f-7d551e47a3a4 -->
--
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]
