korbit-ai[bot] commented on code in PR #32264:
URL: https://github.com/apache/superset/pull/32264#discussion_r1956523344
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,179 @@
+// @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 bitrateSILabels = [
+ 'bits/s',
+ 'kb/s',
+ 'Mb/s',
+ 'Gb/s',
+ 'Tb/s',
+ 'Pb/s',
+ 'Eb/s',
+ 'Zb/s',
+];
+const bitrateIECLabels = [
+ 'bits/s',
+ 'Kib/s',
+ 'Mib/s',
+ 'Gib/s',
+ 'Tib/s',
+ 'Pib/s',
+ 'Eib/s',
+ 'Zib/s',
+];
+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',
+];
+
+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.floor(Math.log(absoluteValue) / Math.log(base));
+ 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]}`;
+ }
+
+ const siFormatter = d3Format(`.${decimals}s`);
+ if (absoluteValue > 0.000001) {
+ return `${siFormatter(value * 1000000)}ยต ${labels[0]}`;
+ }
+ return `${siFormatter(value)} ${labels[0]}`;
+}
+
+function formatBytesSI(value: number, decimals: number) {
+ return formatValue(value, bytesSILabels, 1000, decimals);
+}
+
+function formatBytesIEC(value: number, decimals: number) {
+ return formatValue(value, bytesIECLabels, 1024, decimals);
+}
+
+function formatBitrateSI(value: number, decimals: number) {
+ return formatValue(value, bitrateSILabels, 1000, decimals);
+}
+
+function formatBitrateIEC(value: number, decimals: number) {
+ return formatValue(value, bitrateIECLabels, 1024, decimals);
+}
+
+function formatByterateSI(value: number, decimals: number) {
+ return formatValue(value, byterateSILabels, 1000, decimals);
+}
+
+function formatByterateIEC(value: number, decimals: number) {
+ return formatValue(value, byterateIECLabels, 1024, decimals);
+}
+
+export default function createNetworkNumberFormatter(
+ config: {
+ description?: string;
+ n?: number;
+ id?: string;
+ label?: string;
+ } = {},
+) {
+ const { description, n = 3, id, label } = config;
+
+ switch (id) {
+ case NumberFormats.BYTES_IEC:
+ return new NumberFormatter({
+ description,
+ formatFunc: value => formatBytesIEC(value, n),
+ id,
+ label: label ?? 'Bytes IEC Formatter',
+ });
+ break;
Review Comment:
### Redundant Break After Return <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Unnecessary break statements after return in switch cases
###### Why this matters
The code will never reach these break statements since the return statement
already exits the function, which could confuse developers about the control
flow.
###### Suggested change โ *Feature Preview*
Remove all break statements that follow return statements in the switch
cases.
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/acae8c4c-4295-4257-b4dd-a542291f2251?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:992abd63-970f-4aed-92f0-99e81a16c286 -->
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,179 @@
+// @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 bitrateSILabels = [
+ 'bits/s',
+ 'kb/s',
+ 'Mb/s',
+ 'Gb/s',
+ 'Tb/s',
+ 'Pb/s',
+ 'Eb/s',
+ 'Zb/s',
+];
+const bitrateIECLabels = [
+ 'bits/s',
+ 'Kib/s',
+ 'Mib/s',
+ 'Gib/s',
+ 'Tib/s',
+ 'Pib/s',
+ 'Eib/s',
+ 'Zib/s',
+];
+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',
+];
+
+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.floor(Math.log(absoluteValue) / Math.log(base));
+ 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]}`;
+ }
+
+ const siFormatter = d3Format(`.${decimals}s`);
Review Comment:
### Uncached D3 Formatter Creation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
D3 formatter is being created on every function call instead of being
memoized or cached.
###### Why this matters
Creating new D3 formatters is an expensive operation that could impact
performance when processing large datasets or high-frequency updates.
###### Suggested change โ *Feature Preview*
Create a cache of formatters using a Map or memoization:
```typescript
const formatterCache = new Map<number, Function>();
function getSIFormatter(decimals: number) {
if (!formatterCache.has(decimals)) {
formatterCache.set(decimals, d3Format(`.${decimals}s`));
}
return formatterCache.get(decimals);
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1b082094-f3ab-4bbe-a441-d4b3d0003647?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:61ea57bf-ed7c-4468-973c-3eee7a261ac2 -->
##########
superset-frontend/src/explore/controls.jsx:
##########
@@ -87,6 +87,12 @@ export const D3_FORMAT_OPTIONS = [
['$,.2f', '$,.2f (12345.432 => $12,345.43)'],
['DURATION', t('Duration in ms (66000 => 1m 6s)')],
['DURATION_SUB', t('Duration in ms (100.40008 => 100ms 400ยตs 80ns)')],
+ ['BYTES_SI', 'Bytes in SI (kB, MB)'],
+ ['BYTES_IEC', 'Bytes in IEC (kiB, MiB)'],
+ ['BITRATE_SI', 'Bitrate in SI (kb/s, Mb/s)'],
+ ['BYTERATE_SI', 'Byterate in SI (kB/s, MB/s'],
+ ['BITRATE_IEC', 'Bitrate in IEC (kib/s, Mib/s'],
+ ['BYTERATE_IEC', 'Byterate in IEC (kiB/s, MiB/s'],
Review Comment:
### Format description inconsistencies <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Inconsistent notation in format descriptions. Some entries are missing
closing parentheses and use inconsistent capitalization for units (kib vs kiB).
###### Why this matters
Inconsistent documentation makes it harder for users to understand the exact
format they'll get when selecting these options.
###### Suggested change โ *Feature Preview*
['BYTES_SI', 'Bytes in SI (kB, MB)'],
['BYTES_IEC', 'Bytes in IEC (KiB, MiB)'],
['BITRATE_SI', 'Bitrate in SI (kb/s, Mb/s)'],
['BYTERATE_SI', 'Byterate in SI (kB/s, MB/s)'],
['BITRATE_IEC', 'Bitrate in IEC (Kib/s, Mib/s)'],
['BYTERATE_IEC', 'Byterate in IEC (KiB/s, MiB/s)'],
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/61bd31f5-5cd5-4679-afe3-086452f904c8?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:16ce5bf9-28cd-42d4-bc5c-c9e6a53e7d8d -->
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,179 @@
+// @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 bitrateSILabels = [
+ 'bits/s',
+ 'kb/s',
+ 'Mb/s',
+ 'Gb/s',
+ 'Tb/s',
+ 'Pb/s',
+ 'Eb/s',
+ 'Zb/s',
+];
+const bitrateIECLabels = [
+ 'bits/s',
+ 'Kib/s',
+ 'Mib/s',
+ 'Gib/s',
+ 'Tib/s',
+ 'Pib/s',
+ 'Eib/s',
+ 'Zib/s',
+];
+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',
+];
+
+function formatValue(
+ value: number,
+ labels: any,
+ base: number,
+ decimals: number,
+) {
+ if (value === 0) {
+ const formatted = `0 + ${labels[0]}`;
Review Comment:
### Incorrect Zero Value Formatting <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The formatValue function adds an unexpected '+' symbol when formatting zero
values.
###### Why this matters
This will display values like '0 + Bytes' instead of the expected '0 Bytes',
making the output inconsistent with standard network data formatting
conventions.
###### Suggested change โ *Feature Preview*
Remove the '+' symbol from the zero value formatting:
```typescript
const formatted = `0 ${labels[0]}`;
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5043988a-69ed-4c46-bcc8-0660e269f7cb?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:6a3bb2cf-7cfb-4f63-baab-0ccc67d7a839 -->
##########
superset-frontend/packages/superset-ui-core/src/number-format/NumberFormats.ts:
##########
@@ -54,6 +54,13 @@ const SMART_NUMBER = 'SMART_NUMBER';
const SMART_NUMBER_SIGNED = 'SMART_NUMBER_SIGNED';
const OVER_MAX_HIDDEN = 'OVER_MAX_HIDDEN';
+const BYTES_SI = 'BYTES_SI';
+const BYTES_IEC = 'BYTES_IEC';
+const BITRATE_SI = 'BITRATE_SI';
+const BYTERATE_SI = 'BYTERATE_SI';
+const BITRATE_IEC = 'BITRATE_IEC';
+const BYTERATE_IEC = 'BYTERATE_IEC';
Review Comment:
### Invalid format patterns for network data formatting <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The new format strings are defined as simple string literals matching their
constant names, rather than actual d3-format patterns that would enable the
intended network data formatting functionality.
###### Why this matters
These format strings won't actually format numbers into SI/IEC byte or bit
rate representations when used, leading to failed number formatting in the
application.
###### Suggested change โ *Feature Preview*
Replace the string literals with proper d3-format patterns that support byte
and bit rate formatting. For example:
```typescript
const BYTES_SI = '.2s';
const BYTES_IEC = '.2sb';
const BITRATE_SI = '.2s/s';
const BYTERATE_SI = '.2sb/s';
const BITRATE_IEC = '.2sb/s';
const BYTERATE_IEC = '.2sb/s';
```
Note: The exact format patterns should be verified with the d3-format
documentation to ensure they match the intended formatting requirements.
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/03bdbcfd-ae35-4897-9edd-b6e683ebae87?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:834277ce-46f0-4355-a29d-a73c2d7c19f4 -->
--
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-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
