aminghadersohi commented on PR #43571:
URL: https://github.com/apache/superset/pull/43571#issuecomment-5574544139
Rechecked **29f350b56f903a97c4588943b533e10608be3cb5** against the refreshed
base. The radar implementation/tests are unchanged from e168736; material
integration gaps remain:
- **Server ordering:** `map_radar_config` emits neither ordering nor
`sort_by_metric`. `_build_single_query_dict` consequently emits `row_limit=10`
without `orderby`; compile/unsaved-preview queries also get empty ordering.
This differs from Radar's frontend `buildQuery.ts` (first metric descending, or
`series_limit_metric`) and can select different polygons.
- **Native round-trip/update preservation:** Feeding the mapper's own native
form data into `GenerateChartRequest` fails validation (native SIMPLE
metrics/string groupby); saved-metric strings also fail.
`_build_update_payload` with a radar config drops existing `column_config`
bounds, `series_limit_metric`, and `shape`. Minimal MCP controls should not
silently erase existing native settings.
- **Preview fidelity:** `_generate_vega_lite_preview_from_data` produces a
bar spec for radar and encodes only the first metric, omitting the other axes.
Please provide a faithful representation or explicitly report an unsupported
preview instead.
- **Validation/coverage:** `metrics=[{"name":"model"}]` against a VARCHAR
column passes `DatasetValidator.validate_against_dataset`, then maps to
`SUM(model)`. Validate the effective aggregate or require an explicit metric
role. Explicit text SUM rejection, groupby-role rejection, and case
normalization work. Schema discovery works, but adding `_VIZ_CATEGORY["radar"]`
does not add any radar recommendation candidate.
Focused tests: **250 existing tests passed**, including all 16 radar tests,
plus **7 temporary probes** confirming the behaviors above. Please add radar
product-path regressions for query generation, native request/update
preservation, previews, and effective-aggregate validation; mapper/registry
tests alone miss these. No live-server test was possible (localhost:8088
unavailable).
--
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]
