Copilot commented on code in PR #38028:
URL: https://github.com/apache/superset/pull/38028#discussion_r2874011717
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/package.json:
##########
@@ -52,7 +52,8 @@
"prop-types": "^15.8.1",
"underscore": "^1.13.7",
"urijs": "^1.19.11",
- "xss": "^1.0.15"
+ "xss": "^1.0.15",
+ "h3-js": "^4.4.0"
Review Comment:
The PR description claims "Zero new dependencies", but this change adds a
new direct dependency on `h3-js`. Either update the PR description to reflect
the added dependency, or refactor to avoid introducing it (e.g., reuse an
existing dependency/version already present in the workspace).
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/H3Hexagon/buildQuery.ts:
##########
@@ -0,0 +1,88 @@
+/**
+ * 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 {
+ buildQueryContext,
+ ensureIsArray,
+ normalizeOrderBy,
+ QueryFormColumn,
+ QueryFormData,
+} from '@superset-ui/core';
+import { addTooltipColumnsToQuery } from '../buildQueryUtils';
+
+export interface H3FormData extends QueryFormData {
+ h3_index: string | string[];
+ metric?: string;
+ js_columns?: string[];
+ tooltip_contents?: unknown[];
+}
+
+export default function buildQuery(formData: H3FormData) {
+ let { h3_index: h3Index } = formData;
+ const {
+ metric,
+ js_columns: jsColumns,
+ tooltip_contents: tooltipContents,
+ } = formData;
+
+ if (Array.isArray(h3Index)) {
+ h3Index = h3Index[0];
+ }
+
+ if (!h3Index) {
+ throw new Error('H3 index is required');
+ }
+
+ return buildQueryContext(formData, {
+ buildQuery: baseQueryObject => {
+ let columns: QueryFormColumn[] = [h3Index];
+ const metrics = metric ? [metric] : [];
+
+ if (jsColumns?.length) {
+ jsColumns.forEach((col: string) => {
+ if (!columns.includes(col)) {
+ columns.push(col);
+ }
+ });
+ }
+
+ columns = addTooltipColumnsToQuery(columns, tooltipContents);
+
+ const filters = ensureIsArray(baseQueryObject.filters || []);
+ filters.push({
+ col: h3Index,
+ op: 'IS NOT NULL',
+ });
Review Comment:
This filter clause uses `col: h3Index`, but `h3Index` may be a column object
(from the DnD column control). Filter clauses expect a string column name, so
this can produce an invalid query payload. Use `getColumnLabel(h3Index)` (or
equivalent) when constructing `filters`.
##########
superset-frontend/package-lock.json:
##########
@@ -53769,6 +53804,17 @@
"internmap": "^1.0.0"
}
},
+ "plugins/legacy-preset-chart-deckgl/node_modules/h3-js": {
+ "version": "4.4.0",
+ "resolved": "https://registry.npmjs.org/h3-js/-/h3-js-4.4.0.tgz";,
+ "integrity":
"sha512-DvJh07MhGgY2KcC4OeZc8SSyA+ZXpdvoh6uCzGpoKvWtZxJB+g6VXXC1+eWYkaMIsLz7J/ErhOalHCpcs1KYog==",
+ "license": "Apache-2.0",
Review Comment:
`package-lock.json` now installs `[email protected]` under
`plugins/legacy-preset-chart-deckgl` while another `h3-js` version is already
present at the workspace root. Multiple versions increase bundle size and can
cause subtle mismatches. Consider aligning to a single version so this
dependency can be deduped.
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/H3Hexagon/buildQuery.ts:
##########
@@ -0,0 +1,88 @@
+/**
+ * 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 {
+ buildQueryContext,
+ ensureIsArray,
+ normalizeOrderBy,
+ QueryFormColumn,
+ QueryFormData,
+} from '@superset-ui/core';
+import { addTooltipColumnsToQuery } from '../buildQueryUtils';
+
+export interface H3FormData extends QueryFormData {
+ h3_index: string | string[];
+ metric?: string;
+ js_columns?: string[];
+ tooltip_contents?: unknown[];
+}
+
+export default function buildQuery(formData: H3FormData) {
+ let { h3_index: h3Index } = formData;
+ const {
+ metric,
+ js_columns: jsColumns,
Review Comment:
This new layer adds custom query-building/transform logic but does not add
unit tests. Other DeckGL layers in this preset include `buildQuery.test.ts`
coverage; adding similar tests for `buildQuery`/`transformProps` (H3 column
selection, metric handling, invalid H3 values) would help prevent regressions.
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/H3Hexagon/transformProps.ts:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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 { ChartProps, getMetricLabel } from '@superset-ui/core';
+import {
+ createBaseTransformResult,
+ getRecordsFromQuery,
+ parseMetricValue,
+} from '../transformUtils';
+
+interface H3Feature {
+ hexagon: string;
+ fillColor?: number[];
+ elevation?: number;
+ [key: string]: unknown;
+}
+
+export default function transformProps(chartProps: ChartProps) {
+ const { formData } = chartProps;
+ const { h3_index: h3IndexRaw, metric, js_columns: jsColumns } = formData;
+
+ const records = getRecordsFromQuery(chartProps.queriesData);
+
+ // Resolve the H3 column name
+ const h3Index = (() => {
+ const columnName = Array.isArray(h3IndexRaw) ? h3IndexRaw[0] : h3IndexRaw;
+ if (columnName) {
+ return columnName;
+ }
Review Comment:
`h3_index` is configured via `sharedControls.groupby`, which commonly yields
a column object (or array of column objects), not a plain string. Returning
that directly from `h3Index` means `record[h3Index]` will coerce the key to
"[object Object]" and produce empty/invalid hexagon values. Convert the
selected column to a string label (e.g., `getColumnLabel`) before using it to
index query records.
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/H3Hexagon/H3Hexagon.tsx:
##########
@@ -0,0 +1,109 @@
+/**
+ * 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 { H3HexagonLayer } from '@deck.gl/geo-layers';
+import { cellToBoundary } from 'h3-js';
+import { JsonObject, QueryFormData, t } from '@superset-ui/core';
+import { commonLayerProps } from '../common';
+import { createDeckGLComponent, GetLayerType } from '../../factory';
+import TooltipRow from '../../TooltipRow';
+import { createTooltipContent } from '../../utilities/tooltipUtils';
+import sandboxedEval from '../../utils/sandbox';
+
+function defaultTooltipContent(_formData: QueryFormData) {
+ const TooltipContent = (o: JsonObject) => {
+ const obj = o.object as any;
+ return (
+ <div className="deckgl-tooltip">
+ <TooltipRow label={`${t('H3 Index')}: `} value={obj?.hexagon} />
+ {obj?.elevation !== undefined && (
+ <TooltipRow label={`${t('Value')}: `} value={`${obj.elevation}`} />
+ )}
+ </div>
+ );
+ };
+ TooltipContent.displayName = 'H3HexagonTooltipContent';
+ return TooltipContent;
+}
+
+export function getPoints(data: JsonObject[]) {
+ const points: [number, number][] = [];
+
+ data.forEach((d: JsonObject) => {
+ const boundary = cellToBoundary(d.hexagon);
+ if (boundary && boundary.length > 0) {
+ const point: [number, number] = [boundary[0][1], boundary[0][0]];
+ points.push(point);
+ }
+ });
+
+ return points;
+}
+
+export const getLayer: GetLayerType<H3HexagonLayer> = function ({
+ formData,
+ payload,
+ setTooltip,
+ setDataMask,
+ filterState,
+ onContextMenu,
+ emitCrossFilters,
+}) {
+ const fd = formData;
+ const {
+ extruded = true,
+ coverage = 1,
+ elevation_scale: elevationScale = 1,
+ } = fd;
+
+ let data = [...payload.data.features];
+
+ if (fd.js_data_mutator) {
+ const jsFnMutator = sandboxedEval(fd.js_data_mutator);
+ data = jsFnMutator(data);
+ }
+
+ const tooltipContentGenerator = createTooltipContent(
+ fd,
+ defaultTooltipContent(fd),
+ );
+
+ return new H3HexagonLayer({
+ id: `h3-hexagon-layer-${fd.slice_id}`,
+ data,
+ extruded: Boolean(extruded),
+ coverage,
+ elevationScale: elevationScale,
+
+ getHexagon: (d: JsonObject) => d.hexagon,
+ getFillColor: (d: JsonObject) => [255, (1 - d.elevation / 500) * 255, 0],
+ getElevation: (d: JsonObject) => d.elevation || 0,
Review Comment:
`getFillColor` uses `d.elevation` without a default, so when no metric is
selected (or a record has null), `(1 - d.elevation / 500)` becomes `NaN` and
produces an invalid color array. It also ignores the `fill_color_picker`
control and hard-codes the `500` scaling. Default elevation to 0 (or compute a
domain from data) and incorporate the configured color control(s) so the UI
settings affect rendering.
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/H3Hexagon/H3Hexagon.tsx:
##########
@@ -0,0 +1,109 @@
+/**
+ * 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 { H3HexagonLayer } from '@deck.gl/geo-layers';
+import { cellToBoundary } from 'h3-js';
+import { JsonObject, QueryFormData, t } from '@superset-ui/core';
+import { commonLayerProps } from '../common';
+import { createDeckGLComponent, GetLayerType } from '../../factory';
+import TooltipRow from '../../TooltipRow';
+import { createTooltipContent } from '../../utilities/tooltipUtils';
+import sandboxedEval from '../../utils/sandbox';
+
+function defaultTooltipContent(_formData: QueryFormData) {
+ const TooltipContent = (o: JsonObject) => {
+ const obj = o.object as any;
+ return (
+ <div className="deckgl-tooltip">
+ <TooltipRow label={`${t('H3 Index')}: `} value={obj?.hexagon} />
+ {obj?.elevation !== undefined && (
+ <TooltipRow label={`${t('Value')}: `} value={`${obj.elevation}`} />
+ )}
+ </div>
+ );
+ };
+ TooltipContent.displayName = 'H3HexagonTooltipContent';
+ return TooltipContent;
+}
+
+export function getPoints(data: JsonObject[]) {
+ const points: [number, number][] = [];
+
+ data.forEach((d: JsonObject) => {
+ const boundary = cellToBoundary(d.hexagon);
+ if (boundary && boundary.length > 0) {
+ const point: [number, number] = [boundary[0][1], boundary[0][0]];
+ points.push(point);
+ }
Review Comment:
`cellToBoundary(d.hexagon)` will throw for invalid/empty H3 indices. Since
data can contain bad values (and transformProps can emit an empty string), this
can crash autozoom/rendering for the entire chart. Guard with
validation/try-catch and skip invalid cells when building points.
```suggestion
const hexagon = d.hexagon;
if (!hexagon) {
// Skip entries without a valid H3 index
return;
}
try {
const boundary = cellToBoundary(hexagon);
if (boundary && boundary.length > 0) {
const point: [number, number] = [boundary[0][1], boundary[0][0]];
points.push(point);
}
} catch {
// Skip entries with invalid H3 indices that cause cellToBoundary to
throw
}
```
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/H3Hexagon/H3Hexagon.tsx:
##########
@@ -0,0 +1,109 @@
+/**
+ * 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 { H3HexagonLayer } from '@deck.gl/geo-layers';
+import { cellToBoundary } from 'h3-js';
+import { JsonObject, QueryFormData, t } from '@superset-ui/core';
+import { commonLayerProps } from '../common';
+import { createDeckGLComponent, GetLayerType } from '../../factory';
+import TooltipRow from '../../TooltipRow';
+import { createTooltipContent } from '../../utilities/tooltipUtils';
+import sandboxedEval from '../../utils/sandbox';
+
+function defaultTooltipContent(_formData: QueryFormData) {
+ const TooltipContent = (o: JsonObject) => {
+ const obj = o.object as any;
+ return (
Review Comment:
Avoid using `any` here (the repo explicitly disallows `any` in new TS).
`o.object` can be typed based on the layer datum shape (e.g., `{ hexagon:
string; elevation?: number; ... }`) and then safely accessed without dropping
type safety.
--
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]
