codeant-ai-for-open-source[bot] commented on PR #36416: URL: https://github.com/apache/superset/pull/36416#issuecomment-3609650217
## 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/36416/files#diff-fda2a3496e8791d837d7a931dbb9d00e4fdcaf6e88447ed9a2c10d383d5c7fb5R141-R143'><strong>Intl.RangeError risk</strong></a><br>Calls to `getCurrencySymbol(this.currency)` use the `currency.symbol` value directly as the Intl `currency` code. If that value is invalid (not a valid ISO 4217 code) `Intl.NumberFormat` may throw a RangeError. The non-AUTO path has no try/catch or validation and can crash at runtime.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-fda2a3496e8791d837d7a931dbb9d00e4fdcaf6e88447ed9a2c10d383d5c7fb5R33-R37'><strong>Null/undefined value handling</strong></a><br>The formatter interface allows `value` to be null/undefined but the implementation (`format`) expects a `number` and immediately passes it to a number formatter. If callers pass null/undefined the formatter may throw or produce invalid output. The implementation should guard/coerce `value` before formatting.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-d9ffcff287c482c6212389c0c2674187f9dd37ab1b5392ae197b41e3d2a373c2R218-R224'><strong>Detected currency validation</strong></a><br>`detectedCurrency` is used if truthy, but backend could return unexpected values (arrays, 'MIXED', null, lowercase, etc.). The frontend should validate the detected value (e.g., ensure it's a single ISO currency code) and fall back to neutral formatting for mixed/invalid results to avoid incorrect currency formatting.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-d9ffcff287c482c6212389c0c2674187f9dd37ab1b5392ae197b41e3d2a373c2R286-R306'><strong>Currency object shape</strong></a><br>The code sets `resolvedCurrency = { ...currency, symbol: detectedCurrency }`. Ensure that `CurrencyFormatter` accepts `symbol` being an ISO currency code string here; if the `Currency` type expects other keys (e.g., `code`, `locale`, or `symbol` with different semantics), this can produce incorrect formatting or runtime errors. Confirm the type/shape and mapping between detected value and CurrencyFormatter expectations.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-2672bc046f1a56f67dd661b1ace7c639da51ad78c4e442b96445c60924621c4eR281-R313'><strong>CurrencyFormatter type mismatch</strong></a><br>The new code passes `resolvedCurrency` into `CurrencyFormatter` (as `currency`) and into `buildCustomFormatters`. Verify `resolvedCurrency` shape matches what these APIs expect (string currency code vs. currency object with symbol/code). If shapes differ, formatting could be incorrect or runtime errors may appear. Also ensure `resolveAutoCurrency` returns consistent structure for primary and secondary series.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-2672bc046f1a56f67dd661b1ace7c639da51ad78c4e442b96445c60924621c4eR314-R331'><strong>buildCustomFormatters signature change</strong></a><br>`buildCustomFormatters` is now called with extra params (`resolvedCurrency`, `data1`, `currencyCodeColumn`). Ensure the function signature and behaviour have been updated accordingly and that these are used only when available to avoid undefined propagation.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-d9ffcff287c482c6212389c0c2674187f9dd37ab1b5392ae197b41e3d2a373c2R200-R320'><strong>Memoization staleness</strong></a><br>processColumns is memoized with a custom equality function (`isEqualColumns`). New inputs (dataset `currencyCodeColumn` and the backend `detected_currency`) affect the returned column formatters but may not be considered by the current memoization comparator, causing stale formatter/column outputs when the detected currency changes. Verify the memoization comparator covers these new fields or remove/adjust it so changes to detection trigger recomputation.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-f81d43cccc89e454b69515e1485808fb3f0406ef4ec085bebd438ced87a6fb2fR260-R328'><strong>Formatting behavior / assumption</strong></a><br>The AUTO currency handling depends on `currencyFormat` having `symbol === 'AUTO'` and the aggregator exposing currencies per-cell. Verify the UI/backend contract: ensure that when `currencyFormat.symbol` is `'AUTO'` the dataset actually provides `currencyCodeColumn`, that aggregators are instrumented to collect currencies, and that `detectedCurrency` fallback logic uses the intended server-provided value. Confirm behavior for mixed/null currency values.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-00b3a3d5ef7bab1dac800b90eeae358d199d6b34c6390fab7ef6aaa6303403f8R75-R76'><strong>Possible Bug</strong></a><br>The query object uses the key `"filter"` but Superset's Query object typically expects `"filters"` (plural). If the underlying `datasource.query` expects `"filters"`, the query will ignore provided filters and may return wrong results (or a full table scan). Confirm the expected query object schema and make the key consistent with consumers.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-359940b6f6470257ed49fa39b82361ccb50a826407072043843a36a0ed6ce4e9R490-R494'><strong>Assumption about `currency_format` shape</strong></a><br>`_detect_currency` expects `self.form_data["currency_format"]` to be a dict with a `symbol` key. If `currency_format` has a different shape (e.g. a string or a legacy format), the AUTO check may silently skip detection. Validate or normalize `currency_format` shape before checking `symbol` to avoid missed detection or unexpected behavior.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-359940b6f6470257ed49fa39b82361ccb50a826407072043843a36a0ed6ce4e9R425-R448'><strong>Cache inconsistency</strong></a><br>The detected currency is added to the payload but is not stored in the data cache (`get_df_payload` stores only `{"df": df, "query": self.query}`). When serving from cache, `detected_currency` is not part of cache_value and may be recomputed (causing extra queries) or differ from what the cached df represents. The detection result should be cached alongside the dataframe (or factored into the cache key) to avoid re-detection and ensure consistency between cached df and reported detected currency.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-359940b6f6470257ed49fa39b82361ccb50a826407072043843a36a0ed6ce4e9R479-R504'><strong>Performance / Duplicate Query</strong></a><br>The newly added `_detect_currency` helper likely issues an additional query to the datasource (via the shared `detect_currency` utility) even though `get_payload()` already obtains the dataframe for the same query. This can double the query cost and add latency for charts using AUTO currency. Prefer detecting currency from the already-fetched `df` when possible and only fall back to `detect_currency()` when the dataframe does not contain the configured currency column or the detection requires a separate query.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-12adcc0dab4417a345f763565aee3cd0f657eece6142615492172abe69cfd3bdR748-R852'><strong>Currency auto-detect context missing</strong></a><br>The change appears to support passing the aggregator to `format`, likely to enable row-level currency auto-detection. However, there is no row/row-data context being passed (only `agg`), so formatters that need the original row object or a currency column value may still lack the necessary context. Investigate how row-level information should be provided to the formatter (e.g., supplying raw row data or keys) so AUTO currency detection can be accurate.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-64ed1780b1eae73eaa259863ef625b5527333ea9f11a114220f3fe7f6d229f20R57-R67'><strong>Formatter argument ordering</strong></a><br>`getValueFormatter` is now called with additional args (`undefined, data, currencyCodeColumn, detectedCurrency`). Ensure this matches the expected parameter order for all call sites; a mismatch can produce incorrect formatting or silent failures.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-12adcc0dab4417a345f763565aee3cd0f657eece6142615492172abe69cfd3bdR748-R852'><strong>Possible Format Signature Mismatch</strong></a><br>The new calls pass the aggregator object as a second argument to `agg.format(aggValue, agg)`. Verify all aggregator implementations (including fallback aggregator returned by `getAggregator`) accept a second argument. If any aggregator's `format` implementation ignores extra args or expects a different shape, this may cause inconsistent formatting or unexpected behavior. Also ensure any custom aggregators written elsewhere in the codebase are updated to accept the new second argument if they rely on it.<br> - [ ] <a href='https://github.com/apache/superset/pull/36416/files#diff-f7b502ad4943c53bd516a12f9b54ecbcaf35a966833cb6fcdedde3f1cc954cf3R215-R475'><strong>Memory / duplicate growth</strong></a><br>Each aggregator now pushes seen currency codes into plain arrays (e.g. `this.currencies.push(...)`) without deduplication or bounds. For large datasets or high-cardinality result sets these arrays will grow unbounded and contain many duplicates, increasing memory usage and slowing get/dedup operations later.<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]
