korbit-ai[bot] commented on code in PR #33789:
URL: https://github.com/apache/superset/pull/33789#discussion_r2157334699
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Geojson/index.ts:
##########
@@ -33,6 +33,7 @@ const metadata = new ChartMetadata({
thumbnail,
useLegacyApi: true,
tags: [t('deckGL'), t('2D')],
+ behaviors: [Behavior.InteractiveChart],
Review Comment:
### Missing interactive behavior explanation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The newly added behavior property lacks documentation explaining why this
chart is considered interactive.
###### Why this matters
Future maintainers won't understand what interactive features this chart
provides and when to update this behavior flag.
###### Suggested change ∙ *Feature Preview*
Update the description in metadata to mention: "The GeoJsonLayer takes in
GeoJSON formatted data and renders it as interactive polygons, lines and points
(circles, icons and/or texts). Users can interact with the map through pan,
zoom, and hover interactions with the rendered GeoJSON elements."
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/081ff572-ac6c-4b0a-94bc-ed4353043539/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/081ff572-ac6c-4b0a-94bc-ed4353043539?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/081ff572-ac6c-4b0a-94bc-ed4353043539?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/081ff572-ac6c-4b0a-94bc-ed4353043539?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/081ff572-ac6c-4b0a-94bc-ed4353043539)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:579939c0-f98b-46f9-b132-ac8e4102a1e2 -->
[](579939c0-f98b-46f9-b132-ac8e4102a1e2)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils/crossFiltersDataMask.ts:
##########
@@ -0,0 +1,441 @@
+/**
+ * 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 { PickingInfo, Viewport } from '@deck.gl/core';
+import {
+ ContextMenuFilters,
+ FilterState,
+ QueryObjectFilterClause,
+ SqlaFormData,
+} from '@superset-ui/core';
+import ngeohash from 'ngeohash';
+
+const GEOHASH_PRECISION = 12;
+const VIEWPORT_BUFFER_FACTOR = 0.01;
+const ZOOM_DIVISOR = 2;
+
+export const spatialTypes = {
+ latlong: 'latlong',
+ delimited: 'delimited',
+ geohash: 'geohash',
+} as const;
+
+type SpatialType = (typeof spatialTypes)[keyof typeof spatialTypes];
+
+export type SpatialData = {
+ latCol?: string;
+ lonCol?: string;
+ lonlatCol?: string;
+ reverseCheckbox?: boolean;
+ delimiter?: string;
+ type: SpatialType;
+ geohashCol?: string;
+ line_column?: string;
+};
+
+export interface LayerFormData extends SqlaFormData {
+ start_spatial?: SpatialData;
+ end_spatial?: SpatialData;
+ spatial?: SpatialData;
+ line_column?: string;
+ geojson?: string;
+}
+
+export interface FilterResult {
+ filters: QueryObjectFilterClause[];
+ values: FilterState;
+}
+
+export interface PositionBounds {
+ from: [number, number];
+ to: [number, number];
+}
+
+export interface ValidatedPickingData {
+ position?: [number, number];
+ positionBounds?: PositionBounds;
+ sourcePosition?: [number, number];
+ targetPosition?: [number, number];
+ path?: string;
+ geometry?: any;
+}
+
+const getFiltersBySpatialType = ({
+ position,
+ positionBounds,
+ spatialData,
+ customColumnLabel,
+}: {
+ position: [number, number];
+ spatialData: SpatialData;
+ positionBounds?: PositionBounds;
+ customColumnLabel?: string;
+}) => {
+ const {
+ lonCol,
+ latCol,
+ lonlatCol,
+ geohashCol,
+ reverseCheckbox,
+ type,
+ delimiter,
+ } = spatialData;
+ let values: any[] = [];
+ let filters: QueryObjectFilterClause[] = [];
+
+ if (!position && !positionBounds)
+ throw new Error('Position of picked data is required');
+
+ switch (type) {
+ case spatialTypes.latlong: {
+ if (lonCol != null && latCol != null) {
+ const cols = [lonCol, latCol];
+
+ if (position) {
+ values = position;
+
+ filters = [
+ ...cols.map(
+ (col, index) =>
+ ({
+ col: {
+ expressionType: 'SQL',
+ sqlExpression: `"${col}"`,
+ label: customColumnLabel ?? `${lonCol}, ${latCol}`,
+ },
+ op: '==',
+ val: position[index],
+ }) as QueryObjectFilterClause,
+ ),
+ ];
+ } else if (positionBounds) {
+ values = [positionBounds.from, positionBounds.to];
+
+ const crossFilterColumnLabel =
+ customColumnLabel ??
+ `From ${lonCol}, ${latCol} to ${lonCol}, ${latCol}`;
+
+ filters = [
+ ...cols.map(
+ (col, index) =>
+ ({
+ col: {
+ expressionType: 'SQL',
+ sqlExpression: `"${col}"`,
+ label: crossFilterColumnLabel,
+ },
+ op: '>=',
+ val: positionBounds.from[index],
+ }) as QueryObjectFilterClause,
+ ),
+ ...cols.map(
+ (col, index) =>
+ ({
+ col: {
+ expressionType: 'SQL',
+ sqlExpression: `"${col}"`,
+ label: crossFilterColumnLabel,
+ },
+ op: '<=',
+ val: positionBounds.to[index],
+ }) as QueryObjectFilterClause,
+ ),
+ ];
+ }
+ }
+
+ break;
+ }
+ case spatialTypes.delimited: {
+ const col = lonlatCol ?? geohashCol;
+
+ if (!col) throw new Error('Column is required');
+
+ const val = (reverseCheckbox ? position.reverse() : position).join(
+ delimiter,
+ );
+
+ values = [val];
+
+ filters = [
+ {
+ col: {
+ expressionType: 'SQL',
+ sqlExpression: `"${col}"`,
+ label: customColumnLabel ?? col,
+ },
+ op: '==',
+ val,
+ },
+ ];
+
+ break;
+ }
+ case spatialTypes.geohash: {
+ const col = lonlatCol ?? geohashCol;
+
+ if (!col) throw new Error('Column is required');
+
+ const [lon, lat] = position;
+ const val = ngeohash.encode(lat, lon, GEOHASH_PRECISION);
+
+ values = [val];
+
+ filters = [
+ {
+ col: {
+ expressionType: 'SQL',
+ sqlExpression: `"${col}"`,
+ label: customColumnLabel ?? col,
+ },
+ op: '==',
+ val,
+ },
+ ];
+
+ break;
+ }
+ default: {
+ values = [];
+ }
+ }
+
+ return {
+ filters,
+ values,
+ };
+};
+
+const calculatePickedPositionBounds = ({
+ pickedCoordinates,
+ viewport,
+}: {
+ pickedCoordinates: number[];
+ viewport: Viewport;
+}): PositionBounds => {
+ const buffer =
+ VIEWPORT_BUFFER_FACTOR / Math.pow(2, viewport.zoom / ZOOM_DIVISOR);
Review Comment:
### Unclear Mathematical Formula <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Mathematical formula with magic numbers lacks clarity about its purpose.
###### Why this matters
The calculation's intent is not immediately clear. Breaking it down with
intermediate variables would help explain the logic.
###### Suggested change ∙ *Feature Preview*
```typescript
const zoomFactor = viewport.zoom / ZOOM_DIVISOR;
const scalingFactor = Math.pow(2, zoomFactor);
const buffer = VIEWPORT_BUFFER_FACTOR / scalingFactor;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4dbfb718-45f3-41fb-8975-560b9894819f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4dbfb718-45f3-41fb-8975-560b9894819f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4dbfb718-45f3-41fb-8975-560b9894819f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4dbfb718-45f3-41fb-8975-560b9894819f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4dbfb718-45f3-41fb-8975-560b9894819f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:92504c35-3a66-4d61-82c3-9a3b907ab6ff -->
[](92504c35-3a66-4d61-82c3-9a3b907ab6ff)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/utils/crossFiltersDataMask.ts:
##########
@@ -0,0 +1,441 @@
+/**
+ * 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 { PickingInfo, Viewport } from '@deck.gl/core';
+import {
+ ContextMenuFilters,
+ FilterState,
+ QueryObjectFilterClause,
+ SqlaFormData,
+} from '@superset-ui/core';
+import ngeohash from 'ngeohash';
+
+const GEOHASH_PRECISION = 12;
+const VIEWPORT_BUFFER_FACTOR = 0.01;
+const ZOOM_DIVISOR = 2;
+
+export const spatialTypes = {
+ latlong: 'latlong',
+ delimited: 'delimited',
+ geohash: 'geohash',
+} as const;
+
+type SpatialType = (typeof spatialTypes)[keyof typeof spatialTypes];
+
+export type SpatialData = {
+ latCol?: string;
+ lonCol?: string;
+ lonlatCol?: string;
+ reverseCheckbox?: boolean;
+ delimiter?: string;
+ type: SpatialType;
+ geohashCol?: string;
+ line_column?: string;
+};
+
+export interface LayerFormData extends SqlaFormData {
+ start_spatial?: SpatialData;
+ end_spatial?: SpatialData;
+ spatial?: SpatialData;
+ line_column?: string;
+ geojson?: string;
+}
+
+export interface FilterResult {
+ filters: QueryObjectFilterClause[];
+ values: FilterState;
+}
+
+export interface PositionBounds {
+ from: [number, number];
+ to: [number, number];
+}
+
+export interface ValidatedPickingData {
+ position?: [number, number];
+ positionBounds?: PositionBounds;
+ sourcePosition?: [number, number];
+ targetPosition?: [number, number];
+ path?: string;
+ geometry?: any;
+}
+
+const getFiltersBySpatialType = ({
+ position,
+ positionBounds,
+ spatialData,
+ customColumnLabel,
+}: {
+ position: [number, number];
+ spatialData: SpatialData;
+ positionBounds?: PositionBounds;
+ customColumnLabel?: string;
+}) => {
+ const {
+ lonCol,
+ latCol,
+ lonlatCol,
+ geohashCol,
+ reverseCheckbox,
+ type,
+ delimiter,
+ } = spatialData;
+ let values: any[] = [];
+ let filters: QueryObjectFilterClause[] = [];
+
+ if (!position && !positionBounds)
+ throw new Error('Position of picked data is required');
+
+ switch (type) {
+ case spatialTypes.latlong: {
+ if (lonCol != null && latCol != null) {
+ const cols = [lonCol, latCol];
+
+ if (position) {
+ values = position;
+
+ filters = [
+ ...cols.map(
+ (col, index) =>
+ ({
+ col: {
+ expressionType: 'SQL',
+ sqlExpression: `"${col}"`,
+ label: customColumnLabel ?? `${lonCol}, ${latCol}`,
+ },
+ op: '==',
+ val: position[index],
+ }) as QueryObjectFilterClause,
+ ),
+ ];
+ } else if (positionBounds) {
+ values = [positionBounds.from, positionBounds.to];
+
+ const crossFilterColumnLabel =
+ customColumnLabel ??
+ `From ${lonCol}, ${latCol} to ${lonCol}, ${latCol}`;
+
+ filters = [
+ ...cols.map(
+ (col, index) =>
+ ({
+ col: {
+ expressionType: 'SQL',
+ sqlExpression: `"${col}"`,
+ label: crossFilterColumnLabel,
+ },
+ op: '>=',
+ val: positionBounds.from[index],
+ }) as QueryObjectFilterClause,
+ ),
+ ...cols.map(
+ (col, index) =>
+ ({
+ col: {
+ expressionType: 'SQL',
+ sqlExpression: `"${col}"`,
+ label: crossFilterColumnLabel,
+ },
+ op: '<=',
+ val: positionBounds.to[index],
+ }) as QueryObjectFilterClause,
+ ),
+ ];
+ }
+ }
+
+ break;
+ }
+ case spatialTypes.delimited: {
+ const col = lonlatCol ?? geohashCol;
+
+ if (!col) throw new Error('Column is required');
+
+ const val = (reverseCheckbox ? position.reverse() : position).join(
+ delimiter,
+ );
+
+ values = [val];
+
+ filters = [
+ {
+ col: {
+ expressionType: 'SQL',
+ sqlExpression: `"${col}"`,
+ label: customColumnLabel ?? col,
+ },
+ op: '==',
+ val,
+ },
+ ];
+
+ break;
+ }
+ case spatialTypes.geohash: {
+ const col = lonlatCol ?? geohashCol;
+
+ if (!col) throw new Error('Column is required');
+
+ const [lon, lat] = position;
+ const val = ngeohash.encode(lat, lon, GEOHASH_PRECISION);
+
+ values = [val];
+
+ filters = [
+ {
+ col: {
+ expressionType: 'SQL',
+ sqlExpression: `"${col}"`,
+ label: customColumnLabel ?? col,
+ },
+ op: '==',
+ val,
+ },
+ ];
+
+ break;
+ }
+ default: {
+ values = [];
+ }
+ }
+
+ return {
+ filters,
+ values,
+ };
+};
+
+const calculatePickedPositionBounds = ({
+ pickedCoordinates,
+ viewport,
+}: {
+ pickedCoordinates: number[];
+ viewport: Viewport;
+}): PositionBounds => {
+ const buffer =
+ VIEWPORT_BUFFER_FACTOR / Math.pow(2, viewport.zoom / ZOOM_DIVISOR);
+
+ return {
+ from: [pickedCoordinates[0] - buffer, pickedCoordinates[1] - buffer],
+ to: [pickedCoordinates[0] + buffer, pickedCoordinates[1] + buffer],
+ };
+};
+
+const getSpatialColumnLabel = ({
+ latCol,
+ lonCol,
+ geohashCol,
+ line_column,
+}: {
+ latCol?: string;
+ lonCol?: string;
+ geohashCol?: string;
+ line_column?: string;
+}) => {
+ if (latCol && lonCol) {
+ return `${latCol}, ${lonCol}`;
+ }
+ if (geohashCol) {
+ return geohashCol;
+ }
+ if (line_column) {
+ return line_column;
+ }
+ return '';
+};
+
+const getStartEndSpatialFilters = ({
+ formData,
+ data,
+}: {
+ formData: LayerFormData;
+ data: PickingInfo;
+}): FilterResult => {
+ const sourcePosition: [number, number] = data.object?.sourcePosition;
+ const targetPosition: [number, number] = data.object?.targetPosition;
+
+ if (!sourcePosition || !targetPosition)
+ throw new Error('Position of picked data is required');
+
+ if (!formData.start_spatial || !formData.end_spatial)
+ throw new Error('Spatial data is required');
+
+ const customColumnLabel = `Start
${getSpatialColumnLabel(formData.start_spatial)} end
${getSpatialColumnLabel(formData.end_spatial)}`;
+
+ const startSpatialFilters = getFiltersBySpatialType({
+ position: sourcePosition,
+ spatialData: formData.start_spatial,
+ customColumnLabel,
+ });
+
+ const endSpatialFilters = getFiltersBySpatialType({
+ position: targetPosition,
+ spatialData: formData.end_spatial,
+ customColumnLabel,
+ });
+
+ if (!startSpatialFilters || !endSpatialFilters) throw new Error('');
Review Comment:
### Empty Error Message <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Empty error message in getStartEndSpatialFilters function when
startSpatialFilters or endSpatialFilters is falsy
###### Why this matters
Empty error messages make debugging difficult and provide no context about
what went wrong to users or developers
###### Suggested change ∙ *Feature Preview*
Add a meaningful error message:
```typescript
throw new Error('Failed to generate spatial filters for start or end
positions');
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cfcab9df-5bba-4284-8b8c-3e65c43b7978/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cfcab9df-5bba-4284-8b8c-3e65c43b7978?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cfcab9df-5bba-4284-8b8c-3e65c43b7978?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cfcab9df-5bba-4284-8b8c-3e65c43b7978?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cfcab9df-5bba-4284-8b8c-3e65c43b7978)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:63920ad8-f9f4-4ab0-a89b-84e097757d75 -->
[](63920ad8-f9f4-4ab0-a89b-84e097757d75)
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/Polygon.tsx:
##########
@@ -153,9 +164,18 @@ export function getLayer(
getElevation: (d: any) => getElevation(d, colorScaler),
elevationScale: fd.multiplier,
fp64: true,
Review Comment:
### Unnecessary High Precision <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using 64-bit floating point precision (fp64) by default is unnecessary for
most geospatial visualizations and impacts performance.
###### Why this matters
64-bit precision significantly increases GPU computation overhead while
providing minimal visual benefit for typical map visualizations.
###### Suggested change ∙ *Feature Preview*
Remove fp64 or set it to false by default unless specifically needed:
```typescript
fp64: false, // or remove the line entirely
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/26b592ef-563a-4569-bb07-1a0338056470/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/26b592ef-563a-4569-bb07-1a0338056470?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/26b592ef-563a-4569-bb07-1a0338056470?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/26b592ef-563a-4569-bb07-1a0338056470?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/26b592ef-563a-4569-bb07-1a0338056470)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:69d33f4f-fe88-4400-82c0-51bd2149e62c -->
[](69d33f4f-fe88-4400-82c0-51bd2149e62c)
--
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]
