betodealmeida commented on code in PR #34175: URL: https://github.com/apache/superset/pull/34175#discussion_r2240325165
########## superset-frontend/packages/superset-ui-chart-controls/src/utils/metricColumnFilter.ts: ########## @@ -0,0 +1,102 @@ +/** + * 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 { + QueryFormMetric, + getMetricLabel, + SqlaFormData, +} from '@superset-ui/core'; + +export interface MetricColumnFilterParams { + colname: string; + colnames: string[]; + formData: SqlaFormData; +} + +/** + * Determines if a column should be skipped based on metric filtering logic. + * + * This function implements the logic to skip unprefixed percent metric columns + * if a prefixed version exists, but doesn't skip if it's also a regular metric. + * + * @param params - The parameters for metric column filtering + * @returns true if the column should be skipped, false otherwise + */ +export function shouldSkipMetricColumn({ + colname, + colnames, + formData, +}: MetricColumnFilterParams): boolean { + // Skip unprefixed percent metric columns if a prefixed version exists + // But don't skip if it's also a regular metric + return !!( + colname && + !colname.startsWith('%') && + colnames.includes(`%${colname}`) && + formData.percent_metrics?.some( + (metric: QueryFormMetric) => getMetricLabel(metric) === colname, + ) && + !formData.metrics?.some( + (metric: QueryFormMetric) => getMetricLabel(metric) === colname, + ) + ); Review Comment: It might be clearer here if you rewrite the logic following the comment you left, something like (untested): ```js // Skip unprefixed percent metric columns if a prefixed version exists // But don't skip if it's also a regular metric if (!colname || colname.startsWith('%')) { return false; // Don't skip prefixed columns or invalid names } const hasPrefixedVersion = colnames.includes(`%${colname}`); const isPercent = isPercentMetric(`%${colname}`, formData); const isRegular = isRegularMetric(colname, formData); // Skip if: has prefixed version AND is percent metric AND is NOT regular metric return hasPrefixedVersion && isPercent && !isRegular; ``` But in general it would be better to use the information from the `form_data`, in order to know if there any percent metrics, instead of parsing the format looking for percent symbols. ########## superset-frontend/packages/superset-ui-chart-controls/src/utils/metricColumnFilter.ts: ########## @@ -0,0 +1,102 @@ +/** + * 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 { + QueryFormMetric, + getMetricLabel, + SqlaFormData, +} from '@superset-ui/core'; + +export interface MetricColumnFilterParams { + colname: string; + colnames: string[]; + formData: SqlaFormData; +} + +/** + * Determines if a column should be skipped based on metric filtering logic. + * + * This function implements the logic to skip unprefixed percent metric columns + * if a prefixed version exists, but doesn't skip if it's also a regular metric. + * + * @param params - The parameters for metric column filtering + * @returns true if the column should be skipped, false otherwise + */ +export function shouldSkipMetricColumn({ + colname, + colnames, + formData, +}: MetricColumnFilterParams): boolean { + // Skip unprefixed percent metric columns if a prefixed version exists + // But don't skip if it's also a regular metric + return !!( + colname && + !colname.startsWith('%') && + colnames.includes(`%${colname}`) && + formData.percent_metrics?.some( + (metric: QueryFormMetric) => getMetricLabel(metric) === colname, + ) && + !formData.metrics?.some( + (metric: QueryFormMetric) => getMetricLabel(metric) === colname, + ) + ); +} + +/** + * Determines if a column is a regular metric. + * + * @param colname - The column name to check + * @param formData - The form data containing metrics + * @returns true if the column is a regular metric, false otherwise + */ +export function isRegularMetric( + colname: string, + formData: SqlaFormData, +): boolean { + return !!formData.metrics?.some(metric => getMetricLabel(metric) === colname); +} + +/** + * Determines if a column is a percentage metric. + * + * @param colname: string, + * @param formData - The form data containing percent_metrics + * @returns true if the column is a percentage metric, false otherwise + */ +export function isPercentMetric( + colname: string, + formData: SqlaFormData, +): boolean { + return !!formData.percent_metrics?.some( + (metric: QueryFormMetric) => `%${getMetricLabel(metric)}` === colname, + ); +} + +/** + * Checks if a column is either a regular metric or percentage metric. + * + * @param colname - The column name to check + * @param formData - The form data containing metrics and percent_metrics + * @returns true if the column is a metric (regular or percentage), false otherwise + */ +export function isMetric(colname: string, formData: SqlaFormData): boolean { + return ( + isRegularMetric(colname, formData) || isPercentMetric(colname, formData) + ); +} Review Comment: You don't use this anywhere, better to get rid of it. -- 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]
