korbit-ai[bot] commented on code in PR #32264:
URL: https://github.com/apache/superset/pull/32264#discussion_r1956550646
##########
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'],
Review Comment:
### Missing Closing Parenthesis <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Missing closing parenthesis in the display string for BYTERATE_SI format
option.
###### Why this matters
This syntax error could cause display issues in the UI and make the format
option description appear incomplete.
###### Suggested change โ *Feature Preview*
Add the missing closing parenthesis:
```typescript
[NumberFormats.BYTERATE_SI, 'Byterate in SI (kB/s, MB/s)'],
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bad56b8d-a7ac-4ae6-b806-c235dbcd4b66?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:e1be477a-17e9-42a3-aa37-1e818b68ae78 -->
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,141 @@
+// @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) {
+ if (!formatterCache.has(decimals)) {
+ formatterCache.set(decimals, d3Format(`.${decimals}s`));
+ }
+ return formatterCache.get(decimals);
+}
+
+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),
+ );
Review Comment:
### Missing bounds check for large values <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The index calculation for large values doesn't handle the case where the
calculated index exceeds the available labels array length.
###### Why this matters
When handling extremely large numbers, this could cause an array
out-of-bounds error as it might try to access a label that doesn't exist.
###### Suggested change โ *Feature Preview*
```typescript
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),
);
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05a2e208-bc7e-4f6e-b0e6-e4001b5ceb67?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:cd7106be-4d37-43d4-8b64-5026f1b8c3ee -->
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,141 @@
+// @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) {
+ if (!formatterCache.has(decimals)) {
+ formatterCache.set(decimals, d3Format(`.${decimals}s`));
+ }
+ return formatterCache.get(decimals);
+}
+
+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]}`;
+ }
+
+ if (absoluteValue > 0.000001) {
+ return `${siFormatter(value * 1000000)}ยต ${labels[0]}`;
+ }
+ return `${siFormatter(value)} ${labels[0]}`;
Review Comment:
### Inconsistent small number formatting <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The fallback case for very small numbers uses siFormatter directly with the
value, which could produce inconsistent formatting.
###### Why this matters
Values smaller than 0.000001 might be displayed with scientific notation,
breaking the consistency of the formatting scheme.
###### Suggested change โ *Feature Preview*
```typescript
return `${float4PointFormatter(value)} ${labels[0]}`;
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/01107c53-f6b8-4892-8e68-53eadd54e1eb?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:bec919bc-8554-4d59-bb6b-aed0a0eb40af -->
##########
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:
### Missing Closing Parentheses <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Missing closing parentheses in the format descriptions.
###### Why this matters
Syntax errors in documentation can confuse users and appear unprofessional.
###### Suggested change โ *Feature Preview*
[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/a6a1d710-7290-470c-9a37-e4c3a2a41196?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:6cef60c1-c26e-4360-9816-1534447c5d7b -->
##########
superset-frontend/packages/superset-ui-core/src/number-format/factories/createNetworkNumberFormatter.ts:
##########
@@ -0,0 +1,141 @@
+// @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) {
+ if (!formatterCache.has(decimals)) {
+ formatterCache.set(decimals, d3Format(`.${decimals}s`));
+ }
+ return formatterCache.get(decimals);
Review Comment:
### Uncaught d3Format Error <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The siFormatter function doesn't handle potential errors from d3Format which
could throw if decimals is invalid.
###### Why this matters
If d3Format throws an error due to invalid decimals, it will propagate up
and potentially crash the application. The error would lack context about where
it originated.
###### Suggested change โ *Feature Preview*
```typescript
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}`);
}
}
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/132bed3e-7078-4cce-b418-2f6daa523a1b?suggestedFixEnabled=true)
๐ฌ Chat with Korbit by mentioning @korbit-ai.
</sub>
<!--- korbi internal id:1653f862-e0ae-49e3-bae1-e7541cbeacc1 -->
--
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]
