bito-code-review[bot] commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4192080371
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Misplaced shared helper</b></div>
<div id="fix">
`getTypedFixedRadius` is a fixed-vs-metric discriminator like
`isMetricValue`, `isFixedValue`, and `getFixedValue`, which all live in
`layers/utils/metricUtils.ts`; placing it in `buildQuery.ts` forces
`transformProps.ts` to import a helper from the query-builder module. Moving it
beside its siblings keeps discriminator logic in one shared module with no
behavior change.
</div>
</div>
<small><i>Code Review Run #5bf46d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Boolean flag over explicit mode enum</b></div>
<div id="fix">
Adding a keyword-only boolean `exact` to both `resolve_geographic_value` and
`resolve_region` collapses two match modes into a flag that callers
re-discriminate (`exact=not region_format` in
`EChartsChartPlugin.row_identifier`). Per BITO.md adaptive rule 12784, prefer
an explicit match-mode enum so intent and error handling stay with the
operation. Needs a small design decision, so no patch attached.
</div>
</div>
<small><i>Code Review Run #5bf46d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Parameter shadows deleted set name</b></div>
<div id="fix">
The new `exact` parameter shadows the exact-match set that the deleted line
named `exact` (`exact = {code for alias, code in pairs if alias == value}`). In
the current loop-free body this is harmless, but any later edit below line 52
that references the set silently gets the flag instead, and readers must
mentally rebind the name. Rename the parameter (e.g. `exact_only`) or the set
(e.g. `exact_matches`).
</div>
</div>
<small><i>Code Review Run #5bf46d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
