codeant-ai-for-open-source[bot] commented on code in PR #38035: URL: https://github.com/apache/superset/pull/38035#discussion_r2917720367
########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/transformProps.ts: ########## @@ -0,0 +1,241 @@ +/** + * 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. + */ +import Supercluster, { + type Options as SuperclusterOptions, +} from 'supercluster'; +import { ChartProps } from '@superset-ui/core'; +import { t } from '@apache-superset/core/translation'; +import { DEFAULT_POINT_RADIUS, DEFAULT_MAX_ZOOM } from './MapLibre'; +import roundDecimal from './utils/roundDecimal'; + +const NOOP = () => {}; + +// Geo precision to limit decimal places (matching legacy backend behavior) +const GEO_PRECISION = 10; + +interface PointProperties { + metric: number | string | null; + radius: number | string | null; +} + +interface ClusterProperties { + metric: number; + sum: number; + squaredSum: number; + min: number; + max: number; +} + +interface DataRecord { + [key: string]: string | number | null | undefined; +} + +function buildGeoJSONFromRecords( + records: DataRecord[], + lonCol: string, + latCol: string, + labelCol: string | null, + pointRadiusCol: string | null, +) { + const features: GeoJSON.Feature<GeoJSON.Point, PointProperties>[] = []; + let minLon = Infinity; + let maxLon = -Infinity; + let minLat = Infinity; + let maxLat = -Infinity; + + for (const record of records) { + const lon = Number(record[lonCol]); + const lat = Number(record[latCol]); + if (Number.isNaN(lon) || Number.isNaN(lat)) { + continue; + } + + const roundedLon = roundDecimal(lon, GEO_PRECISION); + const roundedLat = roundDecimal(lat, GEO_PRECISION); + + minLon = Math.min(minLon, roundedLon); + maxLon = Math.max(maxLon, roundedLon); + minLat = Math.min(minLat, roundedLat); + maxLat = Math.max(maxLat, roundedLat); + + const metric = labelCol != null ? (record[labelCol] ?? null) : null; + const radius = + pointRadiusCol != null ? (record[pointRadiusCol] ?? null) : null; + + features.push({ + type: 'Feature', + properties: { metric, radius }, + geometry: { + type: 'Point', + coordinates: [roundedLon, roundedLat], + }, + }); + } + + const bounds: [[number, number], [number, number]] | undefined = + features.length > 0 + ? [ + [minLon, minLat], + [maxLon, maxLat], + ] + : undefined; + + return { + geoJSON: { type: 'FeatureCollection' as const, features }, + bounds, + }; +} + +export default function transformProps(chartProps: ChartProps) { + const { + width, + height, + rawFormData: formData, + hooks, + queriesData, + } = chartProps; + const { onError = NOOP, setControlValue = NOOP } = hooks; + + const { + all_columns_x: allColumnsX, + all_columns_y: allColumnsY, + clustering_radius: clusteringRadius, + global_opacity: globalOpacity, + map_color: maplibreColor, + map_label: maplibreLabel, + map_renderer: mapProvider, + maplibre_style: maplibreStyle, + mapbox_style: mapboxStyle = '', + pandas_aggfunc: pandasAggfunc, + point_radius: pointRadius, + point_radius_unit: pointRadiusUnit, + render_while_dragging: renderWhileDragging, + } = formData; + + // Support two data formats: + // 1. Legacy/GeoJSON: queriesData[0].data is an object with { geoJSON, bounds, hasCustomMetric } + // 2. Tabular records: queriesData[0].data is an array of flat records from a SQL query + const rawData = queriesData[0]?.data; + const isLegacyFormat = rawData && !Array.isArray(rawData) && rawData.geoJSON; + + let geoJSON: { type: 'FeatureCollection'; features: any[] }; + let bounds: [[number, number], [number, number]] | undefined; + let hasCustomMetric: boolean; + + if (isLegacyFormat) { + const legacy = rawData as any; + ({ geoJSON } = legacy); + ({ bounds } = legacy); + hasCustomMetric = legacy.hasCustomMetric ?? false; + } else { + const records: DataRecord[] = (rawData as DataRecord[]) || []; + hasCustomMetric = maplibreLabel != null && maplibreLabel.length > 0; + const labelCol = hasCustomMetric ? maplibreLabel[0] : null; + const pointRadiusCol = + pointRadius && pointRadius !== 'Auto' ? pointRadius : null; Review Comment: **Suggestion:** In the tabular-data path, the `hasCustomMetric` flag is set to true whenever `map_label` is non-empty, but `buildQuery.ts` deliberately treats `"count"` as a special case and never includes a `count` column in the query; when users choose `"count"` as the label, `transformProps` will still try to aggregate a non-existent `count` field, causing Supercluster to see all point metrics as `0` and produce cluster labels of `0` instead of the actual point counts. To fix this, `transformProps` should mirror the logic in `buildQuery.ts` by treating `"count"` as non-custom (i.e., not setting `hasCustomMetric` for that value), so that the overlay falls back to using `point_count` for cluster labels as intended. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Point Cluster Map clusters show zero labels for 'count'. - ⚠️ Misleads users analyzing point densities on point_cluster_map. - ⚠️ Cluster bubble sizes under-represent true counts visually. ``` </details> ```suggestion } else { const records: DataRecord[] = (rawData as DataRecord[]) || []; hasCustomMetric = maplibreLabel != null && maplibreLabel.length > 0 && maplibreLabel[0] !== 'count'; const labelCol = hasCustomMetric ? maplibreLabel[0] : null; const pointRadiusCol = pointRadius && pointRadius !== 'Auto' ? pointRadius : null; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In Superset Explore, create a new "Point Cluster Map" chart using the plugin registered in `superset-frontend/plugins/plugin-chart-point-cluster-map/src/index.ts:29-57` (class `ScatterMapChartPlugin`). 2. In the chart controls, pick longitude/latitude columns so that `all_columns_x` and `all_columns_y` are set; in the "Metric / label" control, set `map_label` to `['count']`, and set the aggregation function (`pandas_aggfunc` in formData) to `"sum"` so it is passed through as `aggregatorName` (see `transformProps` at `transformProps.ts:115-129` and return object at `transformProps.ts:210-217`). 3. Run the query; `buildQuery` at `src/buildQuery.ts:57-62` computes `hasCustomMetric` as `map_label && map_label.length > 0 && map_label[0] !== 'count'`, so for `map_label[0] === 'count'` it does *not* push a `count` column into `columns`, meaning the tabular result rows in `queriesData[0].data` will not contain a `count` field. 4. On render, `transformProps` at `transformProps.ts:146-152` enters the tabular-data branch, sets `hasCustomMetric = maplibreLabel != null && maplibreLabel.length > 0` (which is `true` when `maplibreLabel[0] === 'count'`), and sets `labelCol = 'count'`. `buildGeoJSONFromRecords` at `transformProps.ts:49-80` reads `record['count']` for each row, but since `buildQuery` never selected such a column, `metric` is always `null`, so `opts.map` at `transformProps.ts:189-196` maps every point to `metric/sum/squaredSum/min/max = 0`, and `opts.reduce` at `transformProps.ts:197-204` aggregates those zeros. 5. The `MapLibre` component at `src/MapLibre.tsx:167-176` passes `aggregation={hasCustomMetric ? aggregatorName : undefined}` to `ScatterPlotOverlay`; with `hasCustomMetric=true` and `aggregatorName='sum'`, `ScatterPlotOverlay`'s `computeClusterLabel` at `components/ScatterPlotOverlay.tsx:60-87` uses `properties.sum` as the cluster label. Because the clusterer only computed zeros (step 4), all cluster labels and the radius scaling (`scaledRadius` at `ScatterPlotOverlay.tsx:207-210`) end up based on 0, resulting in clusters drawn with label `0` and minimal size even when the underlying `point_count` is large. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-point-cluster-map/src/transformProps.ts **Line:** 146:151 **Comment:** *Logic Error: In the tabular-data path, the `hasCustomMetric` flag is set to true whenever `map_label` is non-empty, but `buildQuery.ts` deliberately treats `"count"` as a special case and never includes a `count` column in the query; when users choose `"count"` as the label, `transformProps` will still try to aggregate a non-existent `count` field, causing Supercluster to see all point metrics as `0` and produce cluster labels of `0` instead of the actual point counts. To fix this, `transformProps` should mirror the logic in `buildQuery.ts` by treating `"count"` as non-custom (i.e., not setting `hasCustomMetric` for that value), so that the overlay falls back to using `point_count` for cluster labels as intended. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=4e9741e5c4d8ba9c3181d6f8fde3bada6cf0df361a7a34470f6b5350112904e9&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=4e9741e5c4d8ba9c3181d6f8fde3bada6cf0df361a7a34470f6b5350112904e9&reaction=dislike'>👎</a> ########## superset-frontend/plugins/preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx: ########## @@ -325,8 +325,15 @@ const CategoricalDeckGLContainer = (props: CategoricalDeckGLContainerProps) => { viewport={viewport} layers={getLayers()} setControlValue={props.setControlValue} - mapStyle={props.formData.mapbox_style} - mapboxApiAccessToken={props.mapboxApiKey} + mapStyle={ + props.formData.map_renderer === 'mapbox' + ? props.formData.mapbox_style + : props.formData.maplibre_style || props.formData.mapbox_style + } Review Comment: **Suggestion:** When the map renderer is set to MapLibre and `maplibre_style` is unset, the current fallback passes the Mapbox-specific `mapbox_style` URL through to the MapLibre map, which can result in failed tile loads or a blank map because MapLibre cannot use `mapbox://` style URLs without a Mapbox token; instead, allow the DeckGL container to fall back to its own default MapLibre-compatible style when `maplibre_style` is missing. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Categorical deck.gl basemaps can be blank under MapLibre. - ⚠️ Affects charts toggling renderer from Mapbox to MapLibre. - ⚠️ Inconsistent with point-cluster map style handling (transformProps.ts:219-222). ``` </details> ```suggestion mapStyle={ props.formData.map_renderer === 'mapbox' ? props.formData.mapbox_style : props.formData.maplibre_style } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create or open any deck.gl categorical chart that uses the categorical container (rendered via `createCategoricalDeckGLComponent` in `superset-frontend/plugins/preset-chart-deckgl/src/factory.tsx:219-257`, which mounts `CategoricalDeckGLContainer` from `superset-frontend/plugins/preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx`). 2. Ensure the chart has a Mapbox style configured (e.g. `mapbox_style = 'mapbox://styles/mapbox/streets-v12'`, consistent with Mapbox defaults defined in `DEFAULT_MAPBOX_TILES` at `superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx:67-74`), but no MapLibre style persisted in the saved form data (only `mapbox_style` is present for older charts or partially migrated configs). 3. In Explore, switch the "Map Renderer" control (configured as `map_renderer` in `Shared_DeckGL.mapProvider` at `superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx:421-438`) from `mapbox` to `maplibre` and run the query without explicitly selecting a MapLibre style, leaving `formData.maplibre_style` absent/undefined in the props passed to the React chart. 4. On render, `CategoricalDeckGLContainer` at `superset-frontend/plugins/preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx:323-335` computes `mapStyle` as `props.formData.map_renderer === 'mapbox' ? props.formData.mapbox_style : props.formData.maplibre_style || props.formData.mapbox_style`. Because `map_renderer === 'maplibre'` and `maplibre_style` is unset while `mapbox_style` contains a `mapbox://` URL, `mapStyle` is set to that Mapbox URL and `mapProvider` is set to `'maplibre'`. `DeckGLContainerStyledWrapper` wraps `DeckGLContainer` (`superset-frontend/plugins/preset-chart-deckgl/src/DeckGLContainer.tsx:189-196`), which detects `isMapbox = props.mapProvider === 'mapbox'` as `false` and renders `<MapLibreMap mapStyle={mapStyle}>` at `DeckGLContainer.tsx:172-176`. The MapLibre map then attempts to load the unsupported `mapbox://...` style, causing failed style/tile requests and resulting in a blank or broken basemap while the chart overlays still attempt to render. ``` </details> <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/CategoricalDeckGLContainer.tsx **Line:** 328:332 **Comment:** *Logic Error: When the map renderer is set to MapLibre and `maplibre_style` is unset, the current fallback passes the Mapbox-specific `mapbox_style` URL through to the MapLibre map, which can result in failed tile loads or a blank map because MapLibre cannot use `mapbox://` style URLs without a Mapbox token; instead, allow the DeckGL container to fall back to its own default MapLibre-compatible style when `maplibre_style` is missing. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=25f2f71517bf57f16b6fa542e08078d8b78c88fd06280242e2fec7c2b7a0bcb3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=25f2f71517bf57f16b6fa542e08078d8b78c88fd06280242e2fec7c2b7a0bcb3&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: [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]
