bito-code-review[bot] commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4192118580
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Scatter/buildQuery.ts:
##########
@@ -48,6 +48,23 @@ export interface DeckScatterFormData
max_radius?: number;
color_picker?: { r: number; g: number; b: number; a: number };
dimension?: string;
+ mcp_geographic?: boolean;
+}
+
+/**
+ * Typed MCP compatibility reads a bare numeric-string radius as a fixed size.
+ * Native charts interpret every bare string as a saved metric name.
+ */
+export function getTypedFixedRadius(
+ formData: Pick<DeckScatterFormData, 'mcp_geographic' | 'point_radius_fixed'>,
+): number | null {
+ const { mcp_geographic, point_radius_fixed } = formData;
+ return mcp_geographic &&
+ typeof point_radius_fixed === 'string' &&
+ point_radius_fixed.trim() !== '' &&
+ Number.isFinite(Number(point_radius_fixed))
+ ? Number(point_radius_fixed)
+ : null;
}
Review Comment:
<!-- Bito Reply -->
The suggestion to move the helper function is appropriate. Placing
discriminator logic like `getTypedFixedRadius` alongside its siblings in the
shared utility module improves maintainability and avoids unnecessary
dependencies between the query-builder and other modules.
##########
superset/utils/geographic.py:
##########
@@ -0,0 +1,102 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Exact-first, ambiguity-safe resolution against rendered geographic
identifiers."""
+
+import unicodedata
+from collections.abc import Iterable
+
+from superset.utils.geographic_regions import REGIONS
+
+
+def geographic_key(value: str) -> str:
+ """Fold case and diacritics, but never guess or perform fuzzy matching."""
+ return "".join(
+ c
+ for c in unicodedata.normalize("NFD", value.lower())
+ if not unicodedata.combining(c)
+ )
+
+
+def resolve_geographic_value(
+ value: object,
+ entries: Iterable[tuple[str, str]],
+ *,
+ fold_diacritics: bool = True,
+ exact: bool = False,
Review Comment:
<!-- Bito Reply -->
The decision to retain the boolean flag is appropriate given the context.
Using a boolean avoids unnecessary churn and complexity, as the renderer
already requires conditional logic based on the presence of an explicit region
format. The current implementation maintains consistent validation and error
handling across both match modes without requiring an additional enum.
##########
superset/utils/geographic.py:
##########
@@ -0,0 +1,102 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Exact-first, ambiguity-safe resolution against rendered geographic
identifiers."""
+
+import unicodedata
+from collections.abc import Iterable
+
+from superset.utils.geographic_regions import REGIONS
+
+
+def geographic_key(value: str) -> str:
+ """Fold case and diacritics, but never guess or perform fuzzy matching."""
+ return "".join(
+ c
+ for c in unicodedata.normalize("NFD", value.lower())
+ if not unicodedata.combining(c)
+ )
+
+
+def resolve_geographic_value(
+ value: object,
+ entries: Iterable[tuple[str, str]],
+ *,
+ fold_diacritics: bool = True,
+ exact: bool = False,
+) -> str:
+ """Resolve exactly first, then accept only a unique folded identifier.
+
+ With ``exact``, only identical identifiers resolve.
+ """
+ if not isinstance(value, str) or not value or len(value) > 500:
+ raise ValueError(
+ "Geographic values must be nonempty strings of at most 500
characters"
+ )
+ key = geographic_key if fold_diacritics else str.lower
+ pairs = list(entries)
+ matches = {code for alias, code in pairs if alias == value}
+ if not matches and not exact:
Review Comment:
<!-- Bito Reply -->
The suggestion to rename the parameter is appropriate. It improves code
clarity by distinguishing the boolean flag from the set of exact-match
candidates, preventing potential confusion or shadowing issues in future edits.
--
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]
