korbit-ai[bot] commented on code in PR #33348: URL: https://github.com/apache/superset/pull/33348#discussion_r2072307159
########## superset-frontend/src/explore/components/controls/NumberControl/index.tsx: ########## @@ -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 { styled } from '@superset-ui/core'; +import { InputNumber } from 'src/components/Input'; +import ControlHeader, { ControlHeaderProps } from '../../ControlHeader'; + +type NumberValueType = number | undefined; + +export interface NumberControlProps extends ControlHeaderProps { + onChange?: (value: NumberValueType) => void; + value?: NumberValueType; + label?: string; + description?: string; + min?: number; + max?: number; + step?: number; + placeholder?: string; + disabled?: boolean; +} + +const FullWidthDiv = styled.div` + width: 100%; +`; + +const FullWidthInputNumber = styled(InputNumber)` + width: 100%; +`; + +function parseValue(value: string | number | null | undefined) { + if (value === null || value === undefined || value === '') { + return undefined; + } + return Number(value); +} + +export default function NumberControl({ + min, + max, + step, + placeholder, + value, + onChange, + disabled, + ...rest +}: NumberControlProps) { + return ( + <FullWidthDiv> + <ControlHeader {...rest} /> + <FullWidthInputNumber + min={min} + max={max} + step={step} + placeholder={placeholder} + value={value} + onChange={value => onChange?.(parseValue(value))} + disabled={disabled} + aria-label={rest.label} + /> Review Comment: ### Missing min/max validation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The component doesn't validate that max is greater than min when both are provided, which could create an unusable threshold input. ###### Why this matters Users won't be able to input values if max is less than min, breaking the threshold functionality. ###### Suggested change ∙ *Feature Preview* Add validation check and adjust max if needed: ```typescript export default function NumberControl({ min, max, step, placeholder, value, onChange, disabled, ...rest }: NumberControlProps) { const validatedMax = max != null && min != null ? Math.max(max, min) : max; return ( <FullWidthDiv> <ControlHeader {...rest} /> <FullWidthInputNumber min={min} max={validatedMax} step={step} placeholder={placeholder} value={value} onChange={value => onChange?.(parseValue(value))} disabled={disabled} aria-label={rest.label} /> </FullWidthDiv> ); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/52c16c42-ba56-4663-9f37-8f6532f2ddd4/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/52c16c42-ba56-4663-9f37-8f6532f2ddd4?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/52c16c42-ba56-4663-9f37-8f6532f2ddd4?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/52c16c42-ba56-4663-9f37-8f6532f2ddd4?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/52c16c42-ba56-4663-9f37-8f6532f2ddd4) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:79916287-6388-4829-af94-3c3b5cea2931 --> [](79916287-6388-4829-af94-3c3b5cea2931) ########## superset-frontend/plugins/plugin-chart-echarts/src/Pie/buildQuery.ts: ########## @@ -16,14 +16,30 @@ * specific language governing permissions and limitations * under the License. */ -import { buildQueryContext, QueryFormData } from '@superset-ui/core'; +import { + buildQueryContext, + getMetricLabel, + QueryFormData, +} from '@superset-ui/core'; +import { getContributionLabel } from './utils'; export default function buildQuery(formData: QueryFormData) { const { metric, sort_by_metric } = formData; + const metricLabel = getMetricLabel(metric); + return buildQueryContext(formData, baseQueryObject => [ { ...baseQueryObject, ...(sort_by_metric && { orderby: [[metric, false]] }), + post_processing: [ + { + operation: 'contribution', + options: { + columns: [metricLabel], + rename_columns: [getContributionLabel(metricLabel)], + }, + }, + ], }, ]); } Review Comment: ### Function with Multiple Responsibilities <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The buildQuery function handles multiple responsibilities: query construction, sorting logic, and post-processing configuration. This violates the Single Responsibility Principle. ###### Why this matters Having multiple responsibilities in a single function makes it harder to maintain, test, and modify individual aspects of the query building process independently. ###### Suggested change ∙ *Feature Preview* Split the function into smaller, focused functions: ```typescript const buildSortingConfig = (metric: string, sortByMetric: boolean) => sortByMetric ? { orderby: [[metric, false]] } : {}; const buildPostProcessingConfig = (metricLabel: string) => ({ post_processing: [{ operation: 'contribution', options: { columns: [metricLabel], rename_columns: [getContributionLabel(metricLabel)] } }] }); export default function buildQuery(formData: QueryFormData) { const { metric, sort_by_metric } = formData; const metricLabel = getMetricLabel(metric); return buildQueryContext(formData, baseQueryObject => [{ ...baseQueryObject, ...buildSortingConfig(metric, sort_by_metric), ...buildPostProcessingConfig(metricLabel) }]); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b253641-be7a-4a0b-aa42-dabbd651a9e2/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b253641-be7a-4a0b-aa42-dabbd651a9e2?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b253641-be7a-4a0b-aa42-dabbd651a9e2?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b253641-be7a-4a0b-aa42-dabbd651a9e2?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b253641-be7a-4a0b-aa42-dabbd651a9e2) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:95b63b48-b984-4510-aef3-eb6c88920025 --> [](95b63b48-b984-4510-aef3-eb6c88920025) ########## superset-frontend/src/explore/components/controls/NumberControl/index.tsx: ########## @@ -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 { styled } from '@superset-ui/core'; +import { InputNumber } from 'src/components/Input'; +import ControlHeader, { ControlHeaderProps } from '../../ControlHeader'; + +type NumberValueType = number | undefined; + +export interface NumberControlProps extends ControlHeaderProps { + onChange?: (value: NumberValueType) => void; + value?: NumberValueType; + label?: string; + description?: string; + min?: number; + max?: number; + step?: number; + placeholder?: string; + disabled?: boolean; +} + +const FullWidthDiv = styled.div` + width: 100%; +`; + +const FullWidthInputNumber = styled(InputNumber)` + width: 100%; +`; + +function parseValue(value: string | number | null | undefined) { + if (value === null || value === undefined || value === '') { + return undefined; + } + return Number(value); +} Review Comment: ### Missing NaN value validation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The parseValue function doesn't handle invalid number conversions, which could lead to NaN values being silently passed through. ###### Why this matters When invalid numeric strings are converted using Number(), it returns NaN. This could cause unexpected behavior in the pie chart threshold calculations. ###### Suggested change ∙ *Feature Preview* Add NaN check to return undefined for invalid numbers: ```typescript function parseValue(value: string | number | null | undefined) { if (value === null || value === undefined || value === '') { return undefined; } const num = Number(value); return Number.isNaN(num) ? undefined : num; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/270132db-5412-4a54-80d9-11bb02d7ca86/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/270132db-5412-4a54-80d9-11bb02d7ca86?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/270132db-5412-4a54-80d9-11bb02d7ca86?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/270132db-5412-4a54-80d9-11bb02d7ca86?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/270132db-5412-4a54-80d9-11bb02d7ca86) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:f9d2b4b8-740f-4d7e-abd1-5f953728801b --> [](f9d2b4b8-740f-4d7e-abd1-5f953728801b) ########## superset-frontend/plugins/plugin-chart-echarts/src/Pie/types.ts: ########## @@ -82,9 +83,20 @@ export const DEFAULT_FORM_DATA: EchartsPieFormData = { showLabelsThreshold: 5, dateFormat: 'smart_date', roseType: null, + thresholdForOther: 0, }; export type PieChartTransformedProps = BaseTransformedProps<EchartsPieFormData> & ContextMenuTransformedProps & CrossFilterTransformedProps; + +export interface PieChartDataItem { + name: string; + value: number; + itemStyle: { + color: string; + opacity: number; + }; + [key: string]: unknown; +} Review Comment: ### Over-permissive interface definition <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The PieChartDataItem interface includes an index signature [key: string]: unknown which makes the interface overly permissive and violates Interface Segregation Principle. ###### Why this matters Using an index signature allows adding any string-keyed properties with unknown types, reducing type safety and making the interface harder to maintain. This can lead to runtime errors and makes refactoring more difficult. ###### Suggested change ∙ *Feature Preview* ```typescript export interface PieChartDataItem { name: string; value: number; itemStyle: { color: string; opacity: number; }; // Add specific optional properties if needed instead of [key: string]: unknown } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/694a2057-4dd7-4241-bf1c-7ca854c88ff2/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/694a2057-4dd7-4241-bf1c-7ca854c88ff2?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/694a2057-4dd7-4241-bf1c-7ca854c88ff2?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/694a2057-4dd7-4241-bf1c-7ca854c88ff2?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/694a2057-4dd7-4241-bf1c-7ca854c88ff2) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:50bbdd57-5c3c-48ca-9d54-40581c65f9ab --> [](50bbdd57-5c3c-48ca-9d54-40581c65f9ab) ########## superset-frontend/plugins/plugin-chart-echarts/src/Pie/buildQuery.ts: ########## @@ -16,14 +16,30 @@ * specific language governing permissions and limitations * under the License. */ -import { buildQueryContext, QueryFormData } from '@superset-ui/core'; +import { + buildQueryContext, + getMetricLabel, + QueryFormData, +} from '@superset-ui/core'; +import { getContributionLabel } from './utils'; export default function buildQuery(formData: QueryFormData) { const { metric, sort_by_metric } = formData; + const metricLabel = getMetricLabel(metric); + return buildQueryContext(formData, baseQueryObject => [ { ...baseQueryObject, ...(sort_by_metric && { orderby: [[metric, false]] }), + post_processing: [ + { + operation: 'contribution', + options: { + columns: [metricLabel], + rename_columns: [getContributionLabel(metricLabel)], + }, + }, + ], Review Comment: ### Inefficient Post-Processing of Large Datasets <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The contribution calculation is performed as a post-processing step on the entire dataset, which could be inefficient for large datasets. ###### Why this matters Processing the entire dataset after retrieval can cause unnecessary memory usage and CPU overhead on the client side, especially when dealing with large datasets. ###### Suggested change ∙ *Feature Preview* Consider pushing the contribution calculation to the database level by modifying the SQL query. This can be done by using database-specific window functions or aggregate functions to calculate percentages directly in the query. Example structure: ```typescript return buildQueryContext(formData, baseQueryObject => [ { ...baseQueryObject, metrics: [ { aggregate: 'RATIO_TO_REPORT', column: metric, label: getContributionLabel(metricLabel) } ] } ]); ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/abdd3977-7acd-43ce-95e9-84b5317ad33f/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/abdd3977-7acd-43ce-95e9-84b5317ad33f?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/abdd3977-7acd-43ce-95e9-84b5317ad33f?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/abdd3977-7acd-43ce-95e9-84b5317ad33f?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/abdd3977-7acd-43ce-95e9-84b5317ad33f) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:56467f41-ae9e-4559-a94f-fede0fd05f02 --> [](56467f41-ae9e-4559-a94f-fede0fd05f02) -- 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
