korbit-ai[bot] commented on code in PR #35967: URL: https://github.com/apache/superset/pull/35967#discussion_r2488729827
########## superset-frontend/packages/superset-ui-core/src/number-format/factories/createIndianNumberFormatter.ts: ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + format as d3Format, + formatLocale, + FormatLocaleDefinition, +} from 'd3-format'; +import NumberFormatter from '../NumberFormatter'; + +export default function createIndianNumberFormatter( + config: { + description?: string; + signed?: boolean; + id?: string; + label?: string; + locale?: FormatLocaleDefinition; + } = {}, +) { + const { description, signed = false, id, label, locale } = config; + + const local = locale ? formatLocale(locale) : null; + const fmt = (spec: string) => (local ? local.format(spec) : d3Format(spec)); + + const formatAbbrev = (value: number) => { + const abs = Math.abs(value); + if (abs >= 100) return fmt(',d')(value); Review Comment: ### Integer format applied to floating-point values <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The format specifier ',d' is used for integer formatting but applied to a floating-point value that may have decimal places. ###### Why this matters This will truncate decimal values without proper rounding, potentially causing data loss and incorrect display of numbers like 123.7 showing as 123 instead of 124. ###### Suggested change ∙ *Feature Preview* Change the format specifier to handle floating-point values properly: ```typescript if (abs >= 100) return fmt(',.0f')(value); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/218dc7a8-619d-4b83-8b22-b569ac7d22b4/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/218dc7a8-619d-4b83-8b22-b569ac7d22b4?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/218dc7a8-619d-4b83-8b22-b569ac7d22b4?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/218dc7a8-619d-4b83-8b22-b569ac7d22b4?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/218dc7a8-619d-4b83-8b22-b569ac7d22b4) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:c9f7c0fb-106d-4c31-98fa-f0e6daa0b4a6 --> [](c9f7c0fb-106d-4c31-98fa-f0e6daa0b4a6) ########## superset-frontend/src/setup/setupFormatters.ts: ########## @@ -93,6 +94,44 @@ export default function setupFormatters( createMemoryFormatter({ binary: true, transfer: true }), ); + // Override default smart number to use Indian units (L/CR) + getNumberFormatterRegistry() + .registerValue( Review Comment: ### Duplicate Indian Number Formatter Configurations <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code contains repeated configuration blocks for Indian number formatters with similar parameters, violating the DRY principle. ###### Why this matters Duplicated configuration leads to maintenance overhead and increased risk of inconsistencies when changes are needed across all Indian number formatter configurations. ###### Suggested change ∙ *Feature Preview* Extract the common configuration into a factory function: ```typescript const createIndianFormatterConfig = (id: string, signed = false) => ({ locale: getNumberFormatterRegistry().d3Format, id, label: `Adaptive (Indian${signed ? ', signed' : ''})`, signed, }); getNumberFormatterRegistry() .registerValue(NumberFormats.SMART_NUMBER, createIndianNumberFormatter(createIndianFormatterConfig(NumberFormats.SMART_NUMBER))) .registerValue(NumberFormats.SMART_NUMBER_SIGNED, createIndianNumberFormatter(createIndianFormatterConfig(NumberFormats.SMART_NUMBER_SIGNED, true))) // ... etc ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7773d172-6d20-44ee-a467-5b3ef15789c4/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7773d172-6d20-44ee-a467-5b3ef15789c4?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7773d172-6d20-44ee-a467-5b3ef15789c4?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7773d172-6d20-44ee-a467-5b3ef15789c4?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7773d172-6d20-44ee-a467-5b3ef15789c4) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:9f3b6d7f-ab35-4489-bfc1-71a6c8cc7ce5 --> [](9f3b6d7f-ab35-4489-bfc1-71a6c8cc7ce5) ########## superset-frontend/packages/superset-ui-core/src/number-format/factories/createIndianNumberFormatter.ts: ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + format as d3Format, + formatLocale, + FormatLocaleDefinition, +} from 'd3-format'; +import NumberFormatter from '../NumberFormatter'; + +export default function createIndianNumberFormatter( + config: { + description?: string; + signed?: boolean; + id?: string; + label?: string; + locale?: FormatLocaleDefinition; + } = {}, +) { + const { description, signed = false, id, label, locale } = config; + + const local = locale ? formatLocale(locale) : null; + const fmt = (spec: string) => (local ? local.format(spec) : d3Format(spec)); + + const formatAbbrev = (value: number) => { + const abs = Math.abs(value); + if (abs >= 100) return fmt(',d')(value); + if (abs >= 10) return fmt(',.1~f')(value); + return fmt(',.2~f')(value); + }; + + const formatValue = (value: number) => { + if (value === 0) return '0'; + const abs = Math.abs(value); + if (abs >= 10000000) { + const inCr = value / 10000000; + return `${formatAbbrev(inCr)}CR`; + } + if (abs >= 100000) { + const inLakh = value / 100000; + return `${formatAbbrev(inLakh)}L`; + } Review Comment: ### Magic Numbers for Indian Number System Thresholds <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Magic numbers (10000000, 100000) are used directly in the code without being defined as named constants. ###### Why this matters Using magic numbers makes the code harder to maintain and understand. If these Indian number system thresholds need to be modified, they would need to be changed in multiple places. ###### Suggested change ∙ *Feature Preview* ```typescript const CRORE = 10000000; const LAKH = 100000; const formatValue = (value: number) => { if (value === 0) return '0'; const abs = Math.abs(value); if (abs >= CRORE) { const inCr = value / CRORE; return `${formatAbbrev(inCr)}CR`; } if (abs >= LAKH) { const inLakh = value / LAKH; return `${formatAbbrev(inLakh)}L`; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfc67880-596a-49da-a262-bb7fb7ff452d/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfc67880-596a-49da-a262-bb7fb7ff452d?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfc67880-596a-49da-a262-bb7fb7ff452d?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfc67880-596a-49da-a262-bb7fb7ff452d?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dfc67880-596a-49da-a262-bb7fb7ff452d) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:b9b5d905-1897-4298-a880-a7de3345a85a --> [](b9b5d905-1897-4298-a880-a7de3345a85a) ########## superset-frontend/packages/superset-ui-core/src/number-format/factories/createIndianNumberFormatter.ts: ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + format as d3Format, + formatLocale, + FormatLocaleDefinition, +} from 'd3-format'; +import NumberFormatter from '../NumberFormatter'; + +export default function createIndianNumberFormatter( + config: { + description?: string; + signed?: boolean; + id?: string; + label?: string; + locale?: FormatLocaleDefinition; + } = {}, +) { + const { description, signed = false, id, label, locale } = config; + + const local = locale ? formatLocale(locale) : null; + const fmt = (spec: string) => (local ? local.format(spec) : d3Format(spec)); + + const formatAbbrev = (value: number) => { + const abs = Math.abs(value); Review Comment: ### Redundant Math.abs() calculations <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Math.abs() is called multiple times on the same value within formatValue function - once at the beginning and again within formatAbbrev when called. ###### Why this matters This creates redundant computations since Math.abs() is called twice for values >= 10000000 and >= 100000, leading to unnecessary CPU cycles in a formatting function that may be called frequently. ###### Suggested change ∙ *Feature Preview* Pass the already computed `abs` value to formatAbbrev instead of recalculating it: ```typescript const formatAbbrev = (value: number, absValue?: number) => { const abs = absValue ?? Math.abs(value); if (abs >= 100) return fmt(',d')(value); if (abs >= 10) return fmt(',.1~f')(value); return fmt(',.2~f')(value); }; // Then call it as: return `${formatAbbrev(inCr, Math.abs(inCr))}CR`; return `${formatAbbrev(inLakh, Math.abs(inLakh))}L`; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8a77d872-7719-48e7-8044-c8a8fcedbb91/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8a77d872-7719-48e7-8044-c8a8fcedbb91?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8a77d872-7719-48e7-8044-c8a8fcedbb91?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8a77d872-7719-48e7-8044-c8a8fcedbb91?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8a77d872-7719-48e7-8044-c8a8fcedbb91) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:a7863cc8-c177-49bd-8886-2d0ae67e9373 --> [](a7863cc8-c177-49bd-8886-2d0ae67e9373) ########## superset-frontend/packages/superset-ui-core/src/number-format/factories/createIndianNumberFormatter.ts: ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + format as d3Format, + formatLocale, + FormatLocaleDefinition, +} from 'd3-format'; +import NumberFormatter from '../NumberFormatter'; + +export default function createIndianNumberFormatter( + config: { + description?: string; + signed?: boolean; + id?: string; + label?: string; + locale?: FormatLocaleDefinition; + } = {}, +) { + const { description, signed = false, id, label, locale } = config; + + const local = locale ? formatLocale(locale) : null; + const fmt = (spec: string) => (local ? local.format(spec) : d3Format(spec)); + + const formatAbbrev = (value: number) => { + const abs = Math.abs(value); + if (abs >= 100) return fmt(',d')(value); + if (abs >= 10) return fmt(',.1~f')(value); + return fmt(',.2~f')(value); + }; + + const formatValue = (value: number) => { + if (value === 0) return '0'; + const abs = Math.abs(value); + if (abs >= 10000000) { + const inCr = value / 10000000; + return `${formatAbbrev(inCr)}CR`; + } + if (abs >= 100000) { + const inLakh = value / 100000; + return `${formatAbbrev(inLakh)}L`; + } + if (abs >= 1) { + return fmt(',.2~f')(value); + } + if (abs >= 0.001) { + return fmt('.4~f')(value); + } + return d3Format('.3~s')(value); Review Comment: ### Locale-unaware fallback formatting <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The fallback formatting uses d3Format directly instead of the configured locale-aware formatter. ###### Why this matters This bypasses any custom locale configuration, causing inconsistent number formatting when a locale is specified, potentially showing numbers in different formats within the same output. ###### Suggested change ∙ *Feature Preview* Use the locale-aware formatter consistently: ```typescript return fmt('.3~s')(value); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ca07d26d-6a95-49ac-b425-6aebbed4b1d3/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ca07d26d-6a95-49ac-b425-6aebbed4b1d3?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ca07d26d-6a95-49ac-b425-6aebbed4b1d3?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ca07d26d-6a95-49ac-b425-6aebbed4b1d3?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ca07d26d-6a95-49ac-b425-6aebbed4b1d3) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:9513071d-f23e-4ad1-91e4-9be4f67eb7d2 --> [](9513071d-f23e-4ad1-91e4-9be4f67eb7d2) ########## superset-frontend/src/setup/setupFormatters.ts: ########## @@ -93,6 +94,44 @@ export default function setupFormatters( createMemoryFormatter({ binary: true, transfer: true }), ); + // Override default smart number to use Indian units (L/CR) + getNumberFormatterRegistry() + .registerValue( + NumberFormats.SMART_NUMBER, + createIndianNumberFormatter({ + locale: getNumberFormatterRegistry().d3Format, Review Comment: ### Redundant registry property access <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The d3Format property is accessed multiple times within the same function call chain, causing redundant registry lookups. ###### Why this matters Each call to getNumberFormatterRegistry() may involve object lookups or function calls, and accessing the d3Format property repeatedly creates unnecessary overhead when the same value could be cached and reused. ###### Suggested change ∙ *Feature Preview* Cache the d3Format value in a variable at the beginning of the Indian number formatter registration block: ```typescript const d3Format = getNumberFormatterRegistry().d3Format; // Then use d3Format instead of getNumberFormatterRegistry().d3Format in all four registerValue calls ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/24bb66e3-411a-4806-8345-f10ec08ff1bf/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/24bb66e3-411a-4806-8345-f10ec08ff1bf?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/24bb66e3-411a-4806-8345-f10ec08ff1bf?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/24bb66e3-411a-4806-8345-f10ec08ff1bf?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/24bb66e3-411a-4806-8345-f10ec08ff1bf) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:72709f5c-0ee4-445a-b113-0b02adf8b91b --> [](72709f5c-0ee4-445a-b113-0b02adf8b91b) ########## superset-frontend/src/setup/setupFormatters.ts: ########## @@ -93,6 +94,44 @@ export default function setupFormatters( createMemoryFormatter({ binary: true, transfer: true }), ); + // Override default smart number to use Indian units (L/CR) + getNumberFormatterRegistry() + .registerValue( + NumberFormats.SMART_NUMBER, + createIndianNumberFormatter({ + locale: getNumberFormatterRegistry().d3Format, + id: NumberFormats.SMART_NUMBER, + label: 'Adaptive (Indian)', + }), + ) Review Comment: ### Global override of number formatter breaks internationalization <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code overrides the default SMART_NUMBER formatter globally without any conditional logic or configuration option, forcing all users to use Indian number formatting regardless of their locale or preference. ###### Why this matters This breaks internationalization and user experience for non-Indian users who expect standard number formatting. Users in other regions will see unexpected number formats (L/CR instead of K/M/B) which could cause confusion and misinterpretation of data. ###### Suggested change ∙ *Feature Preview* Add conditional logic to only apply Indian formatting when appropriate, such as: ```typescript if (shouldUseIndianFormatting()) { getNumberFormatterRegistry() .registerValue( NumberFormats.SMART_NUMBER, createIndianNumberFormatter({ locale: getNumberFormatterRegistry().d3Format, id: NumberFormats.SMART_NUMBER, label: 'Adaptive (Indian)', }), ); } ``` Or provide both options and let users choose via configuration. ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c21ce4e-8ec5-48a8-9471-483ea14b3a08/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c21ce4e-8ec5-48a8-9471-483ea14b3a08?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c21ce4e-8ec5-48a8-9471-483ea14b3a08?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c21ce4e-8ec5-48a8-9471-483ea14b3a08?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7c21ce4e-8ec5-48a8-9471-483ea14b3a08) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:e5b8e027-ce56-4207-bd2a-53ac4228a231 --> [](e5b8e027-ce56-4207-bd2a-53ac4228a231) -- 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]
