You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "richardfogaca (via GitHub)" <gi...@apache.org> on 2026/05/14 00:30:18 UTC
[PR] fix(deckgl): render all MultiPolygon parts in Polygon chart [superset]
richardfogaca opened a new pull request, #40100:
URL: https://github.com/apache/superset/pull/40100
### SUMMARY
Fixes #39572.
DeckGL Polygon previously treated every GeoJSON payload as if it were a single Polygon and selected coordinates[0]. That is the outer ring for a Polygon, but for a MultiPolygon it is only the first polygon part, so valid remaining parts were dropped before the layer rendered.
This change keeps the fix at the existing Polygon transformProps conversion boundary and expands GeoJSON MultiPolygon geometry into one DeckGL Polygon feature per polygon part. Each emitted part keeps the source row metric/elevation/extra properties, so one MultiPolygon row renders completely without requiring duplicated dataset rows.
Reviewer focus:
- `superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts` for the JSON geometry normalization and per-part feature expansion.
- `superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.test.ts` for the MultiPolygon regression coverage and adjacent conversion guarantees.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screenshots not attached to this description; reproducible from the steps below.
Live validation:
- Runtime / environment: local Superset development stack from this repo's `docker-compose.yml`, with the standard `examples` data initialized.
- Entry point / scenario: a single-row dataset whose boundary column is a GeoJSON `MultiPolygon` with two separated square parts (`[[[-10,0],[-5,0],[-5,5],[-10,5],[-10,0]]]` and `[[[5,0],[10,0],[10,5],[5,5],[5,0]]]`), surfaced through a DeckGL Polygon Explore chart with `line_type=json` against that boundary column. Configure `Metric` so the legend value is non-empty (the row metric in the probe was `100`).
- Observed signal (before): the rendered map showed only the western polygon part; the eastern polygon part was missing despite being present in the same MultiPolygon row. Pixel-level component analysis on the chart canvas counted exactly 1 large filled polygon component.
- Observed signal (after): the same chart on the same row rendered both separated polygon parts at their expected map positions, and the metric legend still showed the source row value (`100`) for both parts. Pixel-level component analysis counted 2 large filled polygon components of equal area, matching the two equal-sized squares in the fixture.
- Pass / regression condition: the DeckGL Polygon canvas must render one filled polygon per polygon part of a GeoJSON `MultiPolygon` boundary value, and each rendered part must carry the source row's metric value. Regression would be either fewer rendered parts than the input MultiPolygon contains, or per-part metrics differing from the source row.
### TESTING INSTRUCTIONS
- [x] `cd superset-frontend && npm run test -- plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.test.ts` - verifies MultiPolygon expansion plus adjacent Polygon, nested geometry, invalid JSON, geohash/default, metric, and elevation behavior at the transform boundary.
- [x] `cd superset-frontend && npx oxlint --config oxlint.json --quiet plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.test.ts` - verifies lint coverage for the changed frontend files.
- [x] `cd superset-frontend && npm run type` - verifies the TypeScript project typecheck.
- [x] `pre-commit run --files superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.test.ts` - verifies repository pre-commit hooks for the changed files.
- [x] Manual browser check in a local Superset development runtime: open a DeckGL Polygon chart configured with one GeoJSON MultiPolygon row containing two separated polygon parts; confirm both parts render and the metric legend still reflects the source row value.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: Fixes #39572
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): render all MultiPolygon parts in Polygon chart [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40100:
URL: https://github.com/apache/superset/pull/40100#issuecomment-4446292703
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | f87ba31b6c6754602b38335b348781eb69be8449 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a05179b0c99190008ce08d6 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40100--superset-docs-preview.netlify.app](https://deploy-preview-40100--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AstMyK_SNLU3NjXRTUpOz03N0C_JzKtPz83RzS3NKMqEcoIaU1IKc_ErPFKAWs0QDU0NzyySDZEtLQ0sDAwOL5FQDixQzpVoA) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40100--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): render all MultiPolygon parts in Polygon chart [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40100:
URL: https://github.com/apache/superset/pull/40100#issuecomment-4456220242
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): render all MultiPolygon parts in Polygon chart [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca commented on PR #40100:
URL: https://github.com/apache/superset/pull/40100#issuecomment-4477650215
@aminghadersohi I pushed d131f0d1f90c56c3f404a19ec68ec3b07d001002 with a small cleanup for your notes and the automated review suggestions:
- Replaced direct dynamic record reads with an own-property helper, without restricting valid Superset column names.
- Narrowed the polygon parsing catch to only skip expected parse/shape errors and rethrow unexpected failures.
- Left interior-ring/hole handling unchanged since that is pre-existing behavior and better suited for follow-up scope.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): render all MultiPolygon parts in Polygon chart [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40100:
URL: https://github.com/apache/superset/pull/40100#issuecomment-4488262818
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): render all MultiPolygon parts in Polygon chart [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40100:
URL: https://github.com/apache/superset/pull/40100#discussion_r3245399804
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts:
##########
@@ -83,98 +126,102 @@ function processPolygonData(
const excludeKeys = new Set([line_column, ...(js_columns || [])]);
- return records
- .map(record => {
- let feature: PolygonFeature = {
- extraProps: {},
- metrics: {},
- };
-
- feature = addJsColumnsToExtraProps(feature, record, js_columns);
- const updatedFeature = addPropertiesToFeature(
- feature as unknown as Record<string, unknown>,
- record,
- excludeKeys,
- );
- feature = updatedFeature as unknown as PolygonFeature;
-
- const rawPolygonData = record[line_column];
- if (!rawPolygonData) {
- return null;
- }
+ return records.flatMap(record => {
+ let feature: PolygonFeature = {
+ extraProps: {},
+ metrics: {},
+ };
+
+ feature = addJsColumnsToExtraProps(feature, record, js_columns);
+ const updatedFeature = addPropertiesToFeature(
+ feature as unknown as Record<string, unknown>,
+ record,
+ excludeKeys,
+ );
+ feature = updatedFeature as unknown as PolygonFeature;
+
+ const rawPolygonData = record[line_column];
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-1321: Object injection via dynamic property access</b></div>
<div id="fix">
Dynamic property access `record[line_column]` on line 143 is flagged as a potential object injection vulnerability ([CWE-1321](https://cwe.mitre.org/data/definitions/1321.html)). While `line_column` is a form data parameter, validate or sanitize it to prevent prototype pollution attacks. Similar issues exist on lines 198, 199, 205, 206, and 211.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- const rawPolygonData = record[line_column];
+ // Validate line_column is a safe property key
+ if (typeof line_column !== 'string' || !line_column.match(/^[a-zA-Z0-9_]+$/)) {
+ return [];
+ }
+ const rawPolygonData = record[line_column];
if (!rawPolygonData) {
return [];
}
@@ -198,6 +211,10 @@
} else if (elevationLabel && record[elevationLabel] != null) {
+ // Validate elevationLabel is a safe property key
+ if (typeof elevationLabel !== 'string' || !elevationLabel.match(/^[a-zA-Z0-9_]+$/)) {
+ return [];
+ }
const elevationValue = parseMetricValue(record[elevationLabel]);
@@ -205,6 +218,10 @@
if (metricLabel && record[metricLabel] != null) {
+ // Validate metricLabel is a safe property key
+ if (typeof metricLabel !== 'string' || !metricLabel.match(/^[a-zA-Z0-9_]+$/)) {
+ return [];
+ }
const metricValue = record[metricLabel];
```
</div>
</details>
</div>
<small><i>Code Review Run #343f21</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-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts:
##########
@@ -83,98 +126,102 @@ function processPolygonData(
const excludeKeys = new Set([line_column, ...(js_columns || [])]);
- return records
- .map(record => {
- let feature: PolygonFeature = {
- extraProps: {},
- metrics: {},
- };
-
- feature = addJsColumnsToExtraProps(feature, record, js_columns);
- const updatedFeature = addPropertiesToFeature(
- feature as unknown as Record<string, unknown>,
- record,
- excludeKeys,
- );
- feature = updatedFeature as unknown as PolygonFeature;
-
- const rawPolygonData = record[line_column];
- if (!rawPolygonData) {
- return null;
- }
+ return records.flatMap(record => {
+ let feature: PolygonFeature = {
+ extraProps: {},
+ metrics: {},
+ };
+
+ feature = addJsColumnsToExtraProps(feature, record, js_columns);
+ const updatedFeature = addPropertiesToFeature(
+ feature as unknown as Record<string, unknown>,
+ record,
+ excludeKeys,
+ );
+ feature = updatedFeature as unknown as PolygonFeature;
+
+ const rawPolygonData = record[line_column];
+ if (!rawPolygonData) {
+ return [];
+ }
- try {
- let polygonCoords: number[][];
-
- switch (line_type) {
- case 'json': {
- const parsed =
- typeof rawPolygonData === 'string'
- ? JSON.parse(rawPolygonData)
- : rawPolygonData;
-
- if (parsed.coordinates) {
- polygonCoords = parsed.coordinates[0] || parsed.coordinates;
- } else if (parsed.geometry?.coordinates) {
- // Non-standard format with nested geometry
- polygonCoords =
- parsed.geometry.coordinates[0] || parsed.geometry.coordinates;
- } else if (Array.isArray(parsed)) {
- polygonCoords = parsed;
- } else {
- return null;
- }
- break;
+ try {
+ let polygonCoordParts: PolygonCoordinates[];
+
+ switch (line_type) {
+ case 'json': {
+ const parsed =
+ typeof rawPolygonData === 'string'
+ ? JSON.parse(rawPolygonData)
+ : rawPolygonData;
+ const parsedPolygonCoordParts = getPolygonCoordinateParts(parsed);
+
+ if (!parsedPolygonCoordParts) {
+ return [];
}
- case 'geohash':
- polygonCoords = [];
- const decoded = decode_bbox(String(rawPolygonData));
- if (decoded) {
- polygonCoords.push([decoded[1], decoded[0]]); // SW (minLon, minLat)
- polygonCoords.push([decoded[1], decoded[2]]); // NW (minLon, maxLat)
- polygonCoords.push([decoded[3], decoded[2]]); // NE (maxLon, maxLat)
- polygonCoords.push([decoded[3], decoded[0]]); // SE (maxLon, minLat)
- polygonCoords.push([decoded[1], decoded[0]]); // SW (close polygon)
- }
- break;
- case 'zipcode':
- default: {
- polygonCoords = Array.isArray(rawPolygonData) ? rawPolygonData : [];
- break;
+
+ polygonCoordParts = parsedPolygonCoordParts;
+ break;
+ }
+ case 'geohash': {
+ const polygonCoords: PolygonCoordinates = [];
+ const decoded = decode_bbox(String(rawPolygonData));
+ if (decoded) {
+ polygonCoords.push([decoded[1], decoded[0]]); // SW (minLon, minLat)
+ polygonCoords.push([decoded[1], decoded[2]]); // NW (minLon, maxLat)
+ polygonCoords.push([decoded[3], decoded[2]]); // NE (maxLon, maxLat)
+ polygonCoords.push([decoded[3], decoded[0]]); // SE (maxLon, minLat)
+ polygonCoords.push([decoded[1], decoded[0]]); // SW (close polygon)
}
+ polygonCoordParts = [polygonCoords];
+ break;
}
-
- if (reverse_long_lat && polygonCoords.length > 0) {
- polygonCoords = polygonCoords.map(coord => [coord[1], coord[0]]);
+ case 'zipcode':
+ default: {
+ polygonCoordParts = [
+ Array.isArray(rawPolygonData)
+ ? (rawPolygonData as PolygonCoordinates)
+ : [],
+ ];
+ break;
}
+ }
- feature.polygon = polygonCoords;
+ if (reverse_long_lat) {
+ polygonCoordParts = polygonCoordParts.map(polygonCoords =>
+ polygonCoords.map(coord => [coord[1], coord[0]]),
+ );
+ }
- if (fixedElevationValue !== undefined) {
- feature.elevation = fixedElevationValue;
- } else if (elevationLabel && record[elevationLabel] != null) {
- const elevationValue = parseMetricValue(record[elevationLabel]);
- if (elevationValue !== undefined) {
- feature.elevation = elevationValue;
- }
+ if (fixedElevationValue !== undefined) {
+ feature.elevation = fixedElevationValue;
+ } else if (elevationLabel && record[elevationLabel] != null) {
+ const elevationValue = parseMetricValue(record[elevationLabel]);
+ if (elevationValue !== undefined) {
+ feature.elevation = elevationValue;
}
+ }
- if (metricLabel && record[metricLabel] != null) {
- const metricValue = record[metricLabel];
- if (
- typeof metricValue === 'string' ||
- typeof metricValue === 'number'
- ) {
- feature.metrics![metricLabel] = metricValue;
- }
+ if (metricLabel && record[metricLabel] != null) {
+ const metricValue = record[metricLabel];
+ if (
+ typeof metricValue === 'string' ||
+ typeof metricValue === 'number'
+ ) {
+ feature.metrics![metricLabel] = metricValue;
}
- } catch {
- return null;
}
- return feature;
- })
- .filter((feature): feature is PolygonFeature => feature !== null);
+ return polygonCoordParts.map(polygonCoords => ({
+ ...feature,
+ extraProps: { ...feature.extraProps },
+ metrics: { ...feature.metrics },
+ polygon: polygonCoords,
+ }));
+ } catch {
+ return [];
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broad Exception Handling</b></div>
<div id="fix">
The catch block uses a broad exception handler that catches all exceptions, potentially masking unexpected errors that indicate bugs. Per BITO.md adaptive rule [10033], replace with specific exception types (e.g., SyntaxError for JSON.parse, TypeError for array operations) and rethrow others to allow proper debugging.
</div>
</div>
<small><i>Code Review Run #343f21</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): render all MultiPolygon parts in Polygon chart [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca merged PR #40100:
URL: https://github.com/apache/superset/pull/40100
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): render all MultiPolygon parts in Polygon chart [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40100:
URL: https://github.com/apache/superset/pull/40100#issuecomment-4446281864
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): render all MultiPolygon parts in Polygon chart [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40100:
URL: https://github.com/apache/superset/pull/40100#discussion_r3258842667
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts:
##########
@@ -41,6 +41,60 @@ interface PolygonFeature {
metrics?: Record<string, number | string>;
}
+type PolygonCoordinates = number[][];
+
+function isPlainRecord(value: unknown): value is Record<string, unknown> {
+ return typeof value === 'object' && value !== null && !Array.isArray(value);
+}
+
+function getOwnRecordValue(
+ record: DataRecord,
+ key: string | undefined,
+): string | number | null | undefined {
+ if (!key || !Object.prototype.hasOwnProperty.call(record, key)) {
+ return undefined;
+ }
+
+ return record[key];
+}
+
+function getGeoJsonGeometry(value: unknown): Record<string, unknown> | null {
+ if (!isPlainRecord(value)) {
+ return null;
+ }
+
+ if (value.coordinates) {
+ return value;
+ }
+
+ return isPlainRecord(value.geometry) ? value.geometry : null;
+}
+
+function getPolygonCoordinateParts(
+ value: unknown,
+): PolygonCoordinates[] | null {
+ if (Array.isArray(value)) {
+ return [value as PolygonCoordinates];
+ }
+
+ const geometry = getGeoJsonGeometry(value);
+ if (!geometry?.coordinates || !Array.isArray(geometry.coordinates)) {
+ return null;
+ }
+
+ if (geometry.type === 'MultiPolygon') {
+ return geometry.coordinates.flatMap(polygon =>
+ Array.isArray(polygon)
+ ? [(polygon[0] || polygon) as PolygonCoordinates]
+ : [],
+ );
Review Comment:
**Suggestion:** The MultiPolygon conversion drops interior rings by selecting only `polygon[0]`, so polygons with holes are rendered as fully filled shapes. Keep the full ring structure for each polygon part instead of truncating to the outer ring, otherwise valid GeoJSON topology is lost during transformation. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Polygon chart misrenders GeoJSON polygons containing interior holes.
- ⚠️ Spatial analyses may ignore excluded internal regions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a deck.gl Polygon chart using `PolygonChartPlugin` defined in
`superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/index.ts:43-51`, with
`rawFormData.line_type = 'json'` and `rawFormData.line_column = 'geom'` as in the test
fixture at `transformProps.test.ts:37-41`.
2. Ensure the query returns a row where the `geom` column is a JSON string representing a
GeoJSON `MultiPolygon` whose first polygon contains an outer ring and at least one inner
ring (hole), e.g.
`{"type":"MultiPolygon","coordinates":[[[[x1,y1],...outer...],[...[hole]...]]]}`.
3. When the chart runs, `PolygonChartPlugin` calls `transformProps` from
`./transformProps` (import at `index.ts:25`), which invokes `processPolygonData` in
`transformProps.ts:98-101`; for `line_type: 'json'` this calls
`getPolygonCoordinateParts(parsed)` at `transformProps.ts:162-168`.
4. Inside `getPolygonCoordinateParts` (`transformProps.ts:73-95`), the `MultiPolygon`
branch at lines `85-90` executes `geometry.coordinates.flatMap(polygon =>
Array.isArray(polygon) ? [(polygon[0] || polygon) as PolygonCoordinates] : [])`, so for
each polygon only `polygon[0]` (the outer ring) is returned and all subsequent rings
(holes) are dropped, resulting in `polygonCoordParts` that no longer contain inner rings,
and the resulting features rendered by `Polygon.tsx` (PolygonLayer `getPolygon` accessor
at `Polygon.tsx:8-19` display filled polygons instead of polygons with holes.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fplugins%2Fpreset-chart-deckgl%2Fsrc%2Flayers%2FPolygon%2FtransformProps.ts%0A%2A%2ALine%3A%2A%2A%2086%3A90%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20The%20MultiPolygon%20conversion%20drops%20interior%20rings%20by%20selecting%20only%20%60polygon%5B0%5D%60%2C%20so%20polygons%20with%20holes%20are%20rendered%20as%20fully%20filled%20shapes.%20Keep%20the%20full%20ring%20structure%20for%20each%20polygon%20part%20instead%20of%20truncating%20to%20the%20outer%20ring%2C%20otherwise%20valid%20GeoJSON%20topology%20is%20lost%20during%20transformation.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%2
0check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fplugins%2Fpreset-chart-deckgl%2Fsrc%2Flayers%2FPolygon%2FtransformProps.ts%0A%2A%2ALine%3A%2A%2A%2086%3A90%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20The%20MultiPolygon%20conversion%20drops%20interior%20rings%20by%20selecting%20only%20%60polygon%5B0%5D%60%2C%20so%20polygons%20with%20holes%20are%20rendered%20as%20fully%20filled%20shapes.%20Keep%20the%20full%20ring%20structure%20for%20each%20polygon%20part%20instead%20of%20truncating%20to%20the%20outer%20ring%2C%20otherwise%20valid%20GeoJSO
N%20topology%20is%20lost%20during%20transformation.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts
**Line:** 86:90
**Comment:**
*Incomplete Implementation: The MultiPolygon conversion drops interior rings by selecting only `polygon[0]`, so polygons with holes are rendered as fully filled shapes. Keep the full ring structure for each polygon part instead of truncating to the outer ring, otherwise valid GeoJSON topology is lost during transformation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40100&comment_hash=8c958ab776a7f4e84b5370b7be367fa86675b76895f09da93f1cb7c81d6ad064&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40100&comment_hash=8c958ab776a7f4e84b5370b7be367fa86675b76895f09da93f1cb7c81d6ad064&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org