codeant-ai-for-open-source[bot] commented on PR #36890: URL: https://github.com/apache/superset/pull/36890#issuecomment-3705297093
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36890/files#diff-098b90b49fea31de015c89847d895a4154d1bdd5161d0e8c5d647467630db05bR82-R86'><strong>Metrics overwrite</strong></a><br>The new code always constructs a `metrics` array (via processMetricsArray) even when `point_radius_fixed.type` is not 'metric'. This can unintentionally clear or overwrite existing metrics in `baseQueryObject.metrics` for queries that depend on other metrics. Consider preserving existing metrics unless the radius is metric-based.<br> - [ ] <a href='https://github.com/apache/superset/pull/36890/files#diff-098b90b49fea31de015c89847d895a4154d1bdd5161d0e8c5d647467630db05bR92-R94'><strong>OrderBy logic</strong></a><br>`orderby` is set to the metric string when radius is metric-based, otherwise it falls back to `baseQueryObject.orderby`. Ensure the metric string used for orderby matches the metric key used in `metrics` (labels vs expressions) and that using a stringified numeric value won't produce incorrect orderby entries.<br> - [ ] <a href='https://github.com/apache/superset/pull/36890/files#diff-c63de53cf185c55bf589249201ca47dda2cfca92451e274120b56db3e84f005cR136-R146'><strong>Possible Bug</strong></a><br>The new implementation only returns a metric label when `metric.type === 'metric'` and `metric.value` is a string. In real form data `metric.value` can be an object (an ad-hoc metric or saved metric object). Those cases will be ignored and return undefined, which may cause missing metricLabels downstream or unexpected behavior. Consider handling object metric values as valid metrics.<br> - [ ] <a href='https://github.com/apache/superset/pull/36890/files#diff-0a74132f13aaf3cd0faad6f78a560bb9994ce1462fade7eef4e7aee2a4a071d2R47-R55'><strong>Metric misclassification</strong></a><br>The new logic treats string `point_radius_fixed` values as metric keys (the branch that assigns metricKey for strings). If a fixed point size is provided as a string (e.g., "1000" from the fixed-size UI), it may still be treated as a metric and cause the query/tooltip code to look up a non-existent metric field. Ensure only metric-typed values are considered metrics.<br> - [ ] <a href='https://github.com/apache/superset/pull/36890/files#diff-0a74132f13aaf3cd0faad6f78a560bb9994ce1462fade7eef4e7aee2a4a071d2R57-R76'><strong>VerboseMap lookup for non-string metric keys</strong></a><br>The verboseMap lookup uses `verboseMap?.[metricKey]` but `metricKey` can be an object (an adhoc metric) which will not index the map correctly. This can result in falling back to getMetricLabel unnecessarily or producing an incorrect label. Normalize the key used for lookup when `metricKey` is an object (for example use its `value` or `label`) before indexing `verboseMap`.<br> </td></tr> </table> -- 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]
