korbit-ai[bot] commented on code in PR #35919:
URL: https://github.com/apache/superset/pull/35919#discussion_r2488121649
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js:
##########
@@ -115,8 +120,17 @@ function WorldMap(element, props) {
const getCrossFilterDataMask = source => {
const selected = Object.values(filterState.selectedValues || {});
const key = source.id || source.country;
- const country =
- countryFieldtype === 'name' ? mapData[key]?.name : mapData[key]?.country;
+
+ let country;
+ if (countryFieldtype === 'name') {
+ country = mapData[key]?.name;
+ } else if (countryFieldtype === 'cca2') {
+ country = mapData[key]?.codes?.cca2;
+ } else if (countryFieldtype === 'cioc') {
+ country = mapData[key]?.codes?.cioc;
+ } else {
+ country = mapData[key]?.country;
+ }
Review Comment:
### Duplicated Country Resolution Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The country field resolution logic is duplicated in both
getCrossFilterDataMask and handleContextMenu functions, violating the DRY
principle.
###### Why this matters
Code duplication increases maintenance burden and risk of inconsistencies
when changes are needed. If the country resolution logic needs to change, it
must be updated in multiple places.
###### Suggested change ∙ *Feature Preview*
Extract the country resolution logic into a separate function:
```javascript
const resolveCountryField = (key, countryFieldtype) => {
if (countryFieldtype === 'name') return mapData[key]?.name;
if (countryFieldtype === 'cca2') return mapData[key]?.codes?.cca2;
if (countryFieldtype === 'cioc') return mapData[key]?.codes?.cioc;
return mapData[key]?.country;
};
```
Then use this function in both places.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67d5da65-a5a7-448d-b5de-63389a7f29d5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67d5da65-a5a7-448d-b5de-63389a7f29d5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67d5da65-a5a7-448d-b5de-63389a7f29d5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67d5da65-a5a7-448d-b5de-63389a7f29d5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67d5da65-a5a7-448d-b5de-63389a7f29d5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:aa355af8-86d3-4c6d-9426-6c0339fa43d8 -->
[](aa355af8-86d3-4c6d-9426-6c0339fa43d8)
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js:
##########
@@ -115,8 +120,17 @@ function WorldMap(element, props) {
const getCrossFilterDataMask = source => {
const selected = Object.values(filterState.selectedValues || {});
const key = source.id || source.country;
- const country =
- countryFieldtype === 'name' ? mapData[key]?.name : mapData[key]?.country;
+
+ let country;
+ if (countryFieldtype === 'name') {
+ country = mapData[key]?.name;
+ } else if (countryFieldtype === 'cca2') {
+ country = mapData[key]?.codes?.cca2;
+ } else if (countryFieldtype === 'cioc') {
+ country = mapData[key]?.codes?.cioc;
+ } else {
+ country = mapData[key]?.country;
+ }
Review Comment:
### Duplicated country field resolution logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Duplicated country field type resolution logic exists in both
getCrossFilterDataMask and handleContextMenu functions.
###### Why this matters
Code duplication leads to maintenance overhead, increased bundle size, and
potential inconsistencies if one instance is updated but not the other.
###### Suggested change ∙ *Feature Preview*
Extract the country field resolution logic into a reusable helper function:
```javascript
const getCountryValue = (key, countryFieldtype, mapData) => {
if (countryFieldtype === 'name') {
return mapData[key]?.name;
} else if (countryFieldtype === 'cca2') {
return mapData[key]?.codes?.cca2;
} else if (countryFieldtype === 'cioc') {
return mapData[key]?.codes?.cioc;
} else {
return mapData[key]?.country;
}
};
```
Then replace both duplicated blocks with calls to this function.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fdd191f0-31d0-49da-a8da-e44da7ce7aab/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fdd191f0-31d0-49da-a8da-e44da7ce7aab?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fdd191f0-31d0-49da-a8da-e44da7ce7aab?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fdd191f0-31d0-49da-a8da-e44da7ce7aab?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fdd191f0-31d0-49da-a8da-e44da7ce7aab)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:562e1eaf-23a7-4ccc-9139-bee18be6e4db -->
[](562e1eaf-23a7-4ccc-9139-bee18be6e4db)
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js:
##########
@@ -115,8 +120,17 @@ function WorldMap(element, props) {
const getCrossFilterDataMask = source => {
const selected = Object.values(filterState.selectedValues || {});
const key = source.id || source.country;
- const country =
- countryFieldtype === 'name' ? mapData[key]?.name : mapData[key]?.country;
+
+ let country;
+ if (countryFieldtype === 'name') {
+ country = mapData[key]?.name;
+ } else if (countryFieldtype === 'cca2') {
+ country = mapData[key]?.codes?.cca2;
+ } else if (countryFieldtype === 'cioc') {
+ country = mapData[key]?.codes?.cioc;
+ } else {
+ country = mapData[key]?.country;
+ }
Review Comment:
### Incorrect mapData key lookup for new country field types <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes mapData is keyed by country codes, but when
countryFieldtype is 'cca2' or 'cioc', it should be keyed by those respective
codes instead of the default country field.
###### Why this matters
This will cause lookups to fail when using cca2 or cioc field types because
the mapData object is always keyed by d.country, but the source.id or
source.country may contain cca2/cioc values that don't match the keys in
mapData.
###### Suggested change ∙ *Feature Preview*
The mapData object should be keyed appropriately based on the
countryFieldtype. Modify the mapData creation logic:
```javascript
const mapData = {};
processedData.forEach(d => {
let key;
if (countryFieldtype === 'name') {
key = d.name;
} else if (countryFieldtype === 'cca2') {
key = d.codes?.cca2;
} else if (countryFieldtype === 'cioc') {
key = d.codes?.cioc;
} else {
key = d.country;
}
if (key) {
mapData[key] = d;
}
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58793161-eea8-4919-942d-02d6d710117c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58793161-eea8-4919-942d-02d6d710117c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58793161-eea8-4919-942d-02d6d710117c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58793161-eea8-4919-942d-02d6d710117c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58793161-eea8-4919-942d-02d6d710117c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d60628f9-3465-4398-9563-1e3dd2837340 -->
[](d60628f9-3465-4398-9563-1e3dd2837340)
##########
superset/viz.py:
##########
@@ -1313,6 +1313,11 @@ def get_data(self, df: pd.DataFrame) -> VizData:
self.form_data["country_fieldtype"], row["country"]
)
if country:
+ row["codes"] = {
+ "cca2": country["cca2"],
+ "cca3": country["cca3"],
+ "cioc": country["cioc"],
+ }
Review Comment:
### Inefficient dictionary creation in loop <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Dictionary creation inside a loop creates unnecessary object allocation
overhead for each row iteration.
###### Why this matters
Creating a new dictionary for each row in the loop results in repeated
memory allocations and garbage collection pressure, especially when processing
large datasets with many countries.
###### Suggested change ∙ *Feature Preview*
Pre-create a template dictionary outside the loop and use dictionary
comprehension or copy operations to reduce allocation overhead:
```python
codes_template = {"cca2": None, "cca3": None, "cioc": None}
for row in data:
# ... existing code ...
if country:
row["codes"] = {
key: country[key] for key in codes_template
}
```
Alternatively, consider if all three codes are always needed or if the codes
dictionary could be flattened into separate row fields to avoid nested
dictionary creation entirely.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0ee27ab-63d6-4b91-8f90-a4f06e0cf39f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0ee27ab-63d6-4b91-8f90-a4f06e0cf39f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0ee27ab-63d6-4b91-8f90-a4f06e0cf39f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0ee27ab-63d6-4b91-8f90-a4f06e0cf39f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0ee27ab-63d6-4b91-8f90-a4f06e0cf39f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2f265e20-1e22-40e4-a404-61ff5e75994c -->
[](2f265e20-1e22-40e4-a404-61ff5e75994c)
##########
superset/viz.py:
##########
@@ -1313,6 +1313,11 @@ def get_data(self, df: pd.DataFrame) -> VizData:
self.form_data["country_fieldtype"], row["country"]
)
if country:
+ row["codes"] = {
+ "cca2": country["cca2"],
+ "cca3": country["cca3"],
+ "cioc": country["cioc"],
+ }
Review Comment:
### Missing key validation for country codes <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes that all country dictionary objects contain the keys
'cca2', 'cca3', and 'cioc', but does not handle cases where these keys might be
missing or None.
###### Why this matters
This could cause KeyError exceptions at runtime if the country data
structure is incomplete or inconsistent, leading to application crashes when
processing world map visualizations.
###### Suggested change ∙ *Feature Preview*
Add validation to check if the required keys exist in the country dictionary
before accessing them:
```python
row["codes"] = {
"cca2": country.get("cca2"),
"cca3": country.get("cca3"),
"cioc": country.get("cioc"),
}
```
Or add a conditional check:
```python
if all(key in country for key in ["cca2", "cca3", "cioc"]):
row["codes"] = {
"cca2": country["cca2"],
"cca3": country["cca3"],
"cioc": country["cioc"],
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/749237f9-b46b-4e59-bb2e-8c8d6e01f9ce/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/749237f9-b46b-4e59-bb2e-8c8d6e01f9ce?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/749237f9-b46b-4e59-bb2e-8c8d6e01f9ce?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/749237f9-b46b-4e59-bb2e-8c8d6e01f9ce?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/749237f9-b46b-4e59-bb2e-8c8d6e01f9ce)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:bd953737-82a7-48e9-9bdc-c10b004f1947 -->
[](bd953737-82a7-48e9-9bdc-c10b004f1947)
--
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]
