Copilot commented on code in PR #41262:
URL: https://github.com/apache/superset/pull/41262#discussion_r3445847271
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts:
##########
@@ -83,98 +83,92 @@ 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: {},
+ };
Review Comment:
`addPropertiesToFeature` can copy a record field named
`extraProps`/`metrics` onto the feature and overwrite the internal objects you
rely on (e.g. JS columns stored in `extraProps`). This can also lead to runtime
errors later when the code assumes these fields are objects. Consider excluding
these reserved keys from `addPropertiesToFeature` the same way other DeckGL
layers exclude internal fields.
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts:
##########
@@ -83,98 +83,92 @@ 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: {},
+ };
- 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;
- }
- 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;
- }
- }
+ feature = addJsColumnsToExtraProps(feature, record, js_columns);
+ feature = addPropertiesToFeature(feature, record, excludeKeys);
- if (reverse_long_lat && polygonCoords.length > 0) {
- polygonCoords = polygonCoords.map(coord => [coord[1], coord[0]]);
- }
-
- feature.polygon = polygonCoords;
+ const rawPolygonData = record[line_column];
+ if (!rawPolygonData) {
+ return [];
+ }
- if (fixedElevationValue !== undefined) {
- feature.elevation = fixedElevationValue;
- } else if (elevationLabel && record[elevationLabel] != null) {
- const elevationValue = parseMetricValue(record[elevationLabel]);
- if (elevationValue !== undefined) {
- feature.elevation = elevationValue;
+ try {
+ // One ring per polygon; a MultiPolygon yields several rings, all other
+ // shapes yield exactly one.
+ let rings: number[][][];
+
+ switch (line_type) {
+ case 'json': {
+ const parsed =
+ typeof rawPolygonData === 'string'
+ ? JSON.parse(rawPolygonData)
+ : rawPolygonData;
+ // Unwrap GeoJSON Feature ({ geometry: { type, coordinates } })
+ const geom = parsed.geometry ?? parsed;
+
+ if (geom.type === 'MultiPolygon') {
+ // Only the outer ring of each polygon is used; inner rings
(holes) are
+ // intentionally ignored because the downstream layer does not
support them.
+ rings = geom.coordinates.map((poly: number[][][]) => poly[0]);
+ } else if (geom.coordinates) {
+ // coordinates[0] is the outer ring for Polygon; holes are not
rendered.
+ rings = [geom.coordinates[0] || geom.coordinates];
+ } else if (Array.isArray(geom)) {
+ rings = [geom];
+ } else {
+ return [];
}
+ break;
}
-
- if (metricLabel && record[metricLabel] != null) {
- const metricValue = record[metricLabel];
- if (
- typeof metricValue === 'string' ||
- typeof metricValue === 'number'
- ) {
- feature.metrics![metricLabel] = metricValue;
+ case 'geohash': {
+ const decoded = decode_bbox(String(rawPolygonData));
+ if (!decoded) {
+ return [];
}
+ rings = [
+ [
+ [decoded[1], decoded[0]], // SW
+ [decoded[1], decoded[2]], // NW
+ [decoded[3], decoded[2]], // NE
+ [decoded[3], decoded[0]], // SE
+ [decoded[1], decoded[0]], // close
+ ],
+ ];
+ break;
}
- } catch {
- return null;
+ case 'zipcode':
+ default:
+ rings = [Array.isArray(rawPolygonData) ? rawPolygonData : []];
+ }
+
+ let elevation: number | undefined;
+ if (fixedElevationValue !== undefined) {
+ elevation = fixedElevationValue;
+ } else if (elevationLabel && record[elevationLabel] != null) {
+ elevation = parseMetricValue(record[elevationLabel]);
}
- return feature;
- })
- .filter((feature): feature is PolygonFeature => feature !== null);
+ const metrics = { ...feature.metrics };
+ const metricValue = metricLabel ? record[metricLabel] : undefined;
+ if (typeof metricValue === 'string' || typeof metricValue === 'number') {
+ metrics[metricLabel!] = metricValue;
+ }
Review Comment:
`const metrics = { ...feature.metrics };` will throw if `feature.metrics` is
ever `null`/`undefined` (e.g. if a dataset has a `metrics` column that
overwrote the internal field). Since this spread is unconditional, it can crash
even when no metric is configured. Use a null-safe default and avoid the
non-null assertion on `metricLabel`.
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts:
##########
@@ -83,98 +83,92 @@ 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: {},
+ };
- 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;
- }
- 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;
- }
- }
+ feature = addJsColumnsToExtraProps(feature, record, js_columns);
+ feature = addPropertiesToFeature(feature, record, excludeKeys);
- if (reverse_long_lat && polygonCoords.length > 0) {
- polygonCoords = polygonCoords.map(coord => [coord[1], coord[0]]);
- }
-
- feature.polygon = polygonCoords;
+ const rawPolygonData = record[line_column];
+ if (!rawPolygonData) {
+ return [];
+ }
- if (fixedElevationValue !== undefined) {
- feature.elevation = fixedElevationValue;
- } else if (elevationLabel && record[elevationLabel] != null) {
- const elevationValue = parseMetricValue(record[elevationLabel]);
- if (elevationValue !== undefined) {
- feature.elevation = elevationValue;
+ try {
+ // One ring per polygon; a MultiPolygon yields several rings, all other
+ // shapes yield exactly one.
+ let rings: number[][][];
+
+ switch (line_type) {
+ case 'json': {
+ const parsed =
+ typeof rawPolygonData === 'string'
+ ? JSON.parse(rawPolygonData)
+ : rawPolygonData;
+ // Unwrap GeoJSON Feature ({ geometry: { type, coordinates } })
+ const geom = parsed.geometry ?? parsed;
+
+ if (geom.type === 'MultiPolygon') {
+ // Only the outer ring of each polygon is used; inner rings
(holes) are
+ // intentionally ignored because the downstream layer does not
support them.
+ rings = geom.coordinates.map((poly: number[][][]) => poly[0]);
+ } else if (geom.coordinates) {
Review Comment:
In the `MultiPolygon` branch, `poly[0]` can be `undefined` for
malformed/empty polygons. That would cause `ring.map(...)` to throw and the
whole record to be dropped by the catch, even if other polygons in the
multipolygon are valid. Filtering out missing rings avoids losing all polygons
for a partially-invalid geometry.
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.test.ts:
##########
@@ -409,6 +409,90 @@ describe('Polygon transformProps', () => {
]);
});
+ test('should explode a MultiPolygon into one feature per polygon', () => {
+ const multiPolygonProps = {
+ ...mockChartProps,
+ queriesData: [
+ {
+ data: [
+ {
+ geom: JSON.stringify({
+ type: 'MultiPolygon',
+ coordinates: [
+ [
+ [
+ [-122.4, 37.8],
+ [-122.3, 37.8],
+ [-122.3, 37.9],
+ [-122.4, 37.8],
+ ],
+ ],
+ [
+ [
+ [-121.4, 36.8],
+ [-121.3, 36.8],
+ [-121.3, 36.9],
+ [-121.4, 36.8],
+ ],
+ ],
+ ],
+ }),
+ population: 50000,
+ },
+ ],
+ },
+ ],
+ };
+
+ const result = transformProps(multiPolygonProps as ChartProps);
+
+ const features = result.payload.data.features as PolygonFeature[];
+ expect(features).toHaveLength(2);
+ expect(features[0]?.polygon).toEqual([
+ [-122.4, 37.8],
+ [-122.3, 37.8],
+ [-122.3, 37.9],
+ [-122.4, 37.8],
+ ]);
+ expect(features[1]?.polygon).toEqual([
+ [-121.4, 36.8],
+ [-121.3, 36.8],
+ [-121.3, 36.9],
+ [-121.4, 36.8],
+ ]);
+ });
+
+ test('should explode a MultiPolygon wrapped in a GeoJSON Feature', () => {
+ const featureProps = {
+ ...mockChartProps,
+ queriesData: [
+ {
+ data: [
+ {
+ geom: JSON.stringify({
+ type: 'Feature',
+ geometry: {
+ type: 'MultiPolygon',
+ coordinates: [
+ [[[0, 0], [1, 0], [1, 1], [0, 0]]],
+ [[[2, 2], [3, 2], [3, 3], [2, 2]]],
+ ],
+ },
+ }),
+ },
+ ],
+ },
+ ],
+ };
+
+ const result = transformProps(featureProps as ChartProps);
+
+ const features = result.payload.data.features as PolygonFeature[];
+ expect(features).toHaveLength(2);
+ expect(features[0]?.polygon).toEqual([[0, 0], [1, 0], [1, 1], [0, 0]]);
+ expect(features[1]?.polygon).toEqual([[2, 2], [3, 2], [3, 3], [2, 2]]);
+ });
+
test('should handle geohash decoding successfully', () => {
Review Comment:
The new behavior to *skip* records whose geohash fails to decode isn't
covered by tests. Adding a case with a mix of valid + invalid geohashes would
guard against regressions (and ensure invalid rows don't break rendering for
valid ones).
--
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]
