sadpandajoe commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4152507961
##########
superset-frontend/plugins/plugin-chart-world-map/src/countries.ts:
##########
@@ -2010,21 +2010,44 @@ export const countries: CountryInfo[] = [
},
];
-const lookups: Record<CountryFieldType, Map<string, CountryInfo>> = {
- name: new Map(),
+/** Match country names without guessing or folding short country codes. */
+function foldName(value: string): string {
+ return value
+ .toLowerCase()
+ .normalize('NFD')
+ .replace(/[\u0300-\u036F]/g, '');
+}
+
+const exactNames = new Map(countries.map(country => [country.name, country]));
+const foldedNames = new Map<string, CountryInfo[]>();
+// Names resolve through exactNames/foldedNames; codes match
case-insensitively.
+type CountryCodeField = Exclude<CountryFieldType, 'name'>;
+const lookups: Record<CountryCodeField, Map<string, CountryInfo>> = {
cca2: new Map(),
cca3: new Map(),
cioc: new Map(),
};
-(Object.keys(lookups) as CountryFieldType[]).forEach(field => {
+(Object.keys(lookups) as CountryCodeField[]).forEach(field => {
countries.forEach(country => {
lookups[field].set(country[field].toLowerCase(), country);
});
});
+countries.forEach(country => {
+ const key = foldName(country.name);
+ const matches = foldedNames.get(key) ?? [];
+ matches.push(country);
+ foldedNames.set(key, matches);
+});
export function getCountry(
field: string,
symbol: string,
): CountryInfo | undefined {
- return lookups[field as CountryFieldType]?.get(symbol.toLowerCase());
+ if (field === 'name') {
+ const exact = exactNames.get(symbol);
+ if (exact) return exact;
+ const matches = foldedNames.get(foldName(symbol));
Review Comment:
A source value such as `Curaçao` now resolves and renders, but
`transformData` replaces it with the bundled name `Curacao`, which `WorldMap`
then emits for cross-filter and drill actions; an exact-value filter will match
no source rows. Could the original country value be retained for interactions
while the normalized value is used only for rendering?
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -2630,3 +2645,97 @@ def preserve_previous_adhoc_filters(
merged_filters.append(generated_filter)
new_form_data["adhoc_filters"] = merged_filters
+
+
+def merge_geographic_update_form_data( # noqa: C901
+ existing: dict[str, Any],
+ mapped: dict[str, Any],
+ config: ChartConfig,
+ *,
+ dataset_rebind: bool,
+) -> dict[str, Any]:
+ """Preserve omitted native controls; clear explicit nullable roles and
filters.
+
+ Rebinding drops old dataset references and Jinja/query state. All optional
+ roles are remapped from the complete target-dataset config.
+ """
+ if dataset_rebind:
+ existing = {
+ key: value
+ for key, value in existing.items()
+ if key
+ in {
+ "linear_color_scheme",
+ "number_format",
+ "max_bubble_size",
+ "point_unit",
+ "color_by",
+ "color_picker",
+ "color_scheme",
+ "y_axis_format",
+ "map_renderer",
+ "maplibre_style",
+ "mapbox_style",
+ "min_radius",
+ "max_radius",
+ "multiplier",
+ }
+ }
+ field_map = {
+ "country": "select_country",
+ "region_format": "region_format",
+ "country_format": "country_fieldtype",
+ "entity": "entity",
+ "metric": "metric",
+ "secondary_metric": "secondary_metric",
+ "dimension": "dimension",
+ "show_bubbles": "show_bubbles",
+ "max_bubble_size": "max_bubble_size",
+ "sort_by_metric": "sort_by_metric",
+ "linear_color_scheme": "linear_color_scheme",
+ "number_format": "number_format",
+ "row_limit": "row_limit",
+ "time_range": "time_range",
+ "point_unit": "point_unit",
+ }
+ fields = config.model_fields_set
+ patch = dict(mapped)
+ # Native UI-only presentation controls are not part of this public config.
+ for key in (
+ "color_by",
+ "color_picker",
+ "color_scheme",
+ "y_axis_format",
+ "map_renderer",
+ "maplibre_style",
+ "mapbox_style",
+ "viewport",
+ "autozoom",
+ "min_radius",
+ "max_radius",
+ "multiplier",
+ ):
+ if key in existing:
+ patch.pop(key, None)
+ for source, target in field_map.items():
+ if source not in fields and target in existing:
Review Comment:
Updating a native point map with saved `row_limit: 50000` while omitting
that field retains 50000 and adds `mcp_geographic: true`, although compilation
validates only the first 10000 rows. Invalid coordinates beyond that sample can
therefore pass the update and reach a native query outside the advertised
bound; could inherited limits be validated or capped to match the geographic
compile limit?
##########
superset-frontend/plugins/plugin-chart-world-map/src/transformData.ts:
##########
@@ -66,6 +74,28 @@ export default function transformData(
typeof row.country === 'string' && fieldtype
? getCountry(fieldtype, row.country)
: undefined;
Review Comment:
With `countryFieldtype: 'cioc'`, an empty source value resolves through the
lookup's empty-string entry to New Caledonia, so this strict check accepts it
and shades that country after a data refresh even though backend validation
rejects empty identifiers. Should empty country values be rejected before
lookup so the renderer follows the same contract?
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -2630,3 +2645,97 @@ def preserve_previous_adhoc_filters(
merged_filters.append(generated_filter)
new_form_data["adhoc_filters"] = merged_filters
+
+
+def merge_geographic_update_form_data( # noqa: C901
+ existing: dict[str, Any],
+ mapped: dict[str, Any],
+ config: ChartConfig,
+ *,
+ dataset_rebind: bool,
+) -> dict[str, Any]:
+ """Preserve omitted native controls; clear explicit nullable roles and
filters.
+
+ Rebinding drops old dataset references and Jinja/query state. All optional
+ roles are remapped from the complete target-dataset config.
+ """
+ if dataset_rebind:
+ existing = {
+ key: value
+ for key, value in existing.items()
+ if key
+ in {
+ "linear_color_scheme",
+ "number_format",
+ "max_bubble_size",
+ "point_unit",
+ "color_by",
+ "color_picker",
+ "color_scheme",
+ "y_axis_format",
+ "map_renderer",
+ "maplibre_style",
+ "mapbox_style",
+ "min_radius",
+ "max_radius",
+ "multiplier",
+ }
+ }
+ field_map = {
+ "country": "select_country",
+ "region_format": "region_format",
+ "country_format": "country_fieldtype",
+ "entity": "entity",
+ "metric": "metric",
+ "secondary_metric": "secondary_metric",
+ "dimension": "dimension",
+ "show_bubbles": "show_bubbles",
+ "max_bubble_size": "max_bubble_size",
+ "sort_by_metric": "sort_by_metric",
+ "linear_color_scheme": "linear_color_scheme",
+ "number_format": "number_format",
+ "row_limit": "row_limit",
+ "time_range": "time_range",
+ "point_unit": "point_unit",
+ }
+ fields = config.model_fields_set
+ patch = dict(mapped)
+ # Native UI-only presentation controls are not part of this public config.
+ for key in (
+ "color_by",
+ "color_picker",
+ "color_scheme",
+ "y_axis_format",
+ "map_renderer",
+ "maplibre_style",
+ "mapbox_style",
+ "viewport",
+ "autozoom",
+ "min_radius",
+ "max_radius",
+ "multiplier",
+ ):
+ if key in existing:
+ patch.pop(key, None)
+ for source, target in field_map.items():
+ if source not in fields and target in existing:
+ patch.pop(target, None)
+ if not {"radius", "radius_metric"} & fields and "point_radius_fixed" in
existing:
+ patch.pop("point_radius_fixed", None)
+ if "filters" not in fields:
+ source_form = dict(existing)
+ if "temporal_column" in fields:
+ source_form["adhoc_filters"] =
_without_generated_dashboard_time_filter(
+ existing
+ )
+ preserve_previous_adhoc_filters(patch, source_form)
+ merged = {**existing, **patch}
+ if "temporal_column" in fields and config.temporal_column is None:
Review Comment:
With `temporal_column: null` on a dataset with `main_dttm_col`, the mapper
adds a new default temporal predicate; this merge removes the old predicate and
then its marker, but keeps the newly mapped predicate. Could explicit temporal
clearing remove the generated predicate from the patch too, so dashboard time
filters do not remain bound after the caller clears them?
##########
superset-frontend/plugins/plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -178,6 +182,11 @@ function CountryMap(element: HTMLElement, props:
CountryMapProps) {
// Track mouse position to distinguish clicks from drags
let mousedownPos: { x: number; y: number } | null = null;
+ const sourceValue = (code: string) =>
+ sourceValues && Object.prototype.hasOwnProperty.call(sourceValues, code)
+ ? sourceValues[code]
Review Comment:
For an abbreviation-format map whose result is limited to `CA`, Texas is
still clickable but is absent from `sourceValues`, so this fallback sends
`US-TX` to cross-filter and drill queries instead of the source identifier
`TX`. Could unmapped boundaries avoid emitting a guessed ISO value when an
explicit region format is active?
##########
superset/mcp_service/chart/plugins/__init__.py:
##########
@@ -82,3 +90,7 @@
"WaterfallChartPlugin",
"XYChartPlugin",
]
+
+register(CountryMapChartPlugin())
Review Comment:
These registrations also make disabled geographic types available through
`update_chart_preview`: it looks up and maps the config with
`include_disabled=True` even when no `form_data_key` is supplied, so a fresh
preview bypasses the operator setting that blocks `generate_chart`. Could fresh
previews and type conversions enforce availability, reserving disabled-plugin
access for an existing preview of that same type?
--
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]
