alexandrusoare opened a new pull request, #37669: URL: https://github.com/apache/superset/pull/37669
### SUMMARY When creating a deck.gl Scatterplot chart, it's possible to choose between a fixed point size, or a dynamic one (calculated with a metric). The metric popover allows you to either select an existing metric from the dataset, or create a new one via custom SQL. Charts using a custom SQL metric are showing errors. #### Root Cause This is a regression from a previous fix that addressed fixed point size not working. That fix introduced `extractMetricKey()` which converts adhoc metric objects to plain strings, losing the `expressionType` property that the backend needs to identify custom SQL metrics. **What happens:** 1. User creates a custom SQL metric like `count(*) * 1.1` 2. The UI correctly stores it as an object with `expressionType: "SQL"` and `sqlExpression` 3. `extractMetricKey()` strips it down to just the string `"count(*) * 1.1"` 4. Backend receives the string, can't identify it as an adhoc metric (missing `expressionType`), and it's not a saved metric either 5. Error: "Metric does not exist" #### Fix Modified `buildQuery.ts` to preserve the full metric object instead of extracting only the label string. Used `getMetricLabel()` only where a string representation is actually needed (for the `orderby` clause). #### Alternative Considered **Fix `extractMetricKey()` to optionally return full objects** - This would be a more "correct" fix at the utility level, but touches shared code used by other components, increasing risk of unintended side effects. The minimal fix in `buildQuery.ts` is safer and achieves the same result. #### Tests Added 5 new tests to prevent future regressions: - Adhoc SQL metrics (`count(*) * 1.1`) - Adhoc SIMPLE metrics (`AVG(column)`) - Deduplication of adhoc metrics - Fixed type with string values - Undefined metric values ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF <!--- Skip this if not applicable --> ### TESTING INSTRUCTIONS <!--- Required! What steps can be taken to manually verify the changes? --> ### ADDITIONAL INFORMATION <!--- Check any relevant boxes with "x" --> <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue --> - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
