Fix PointLayer polygon filtering for GeoJSON column mode#3301
Fix PointLayer polygon filtering for GeoJSON column mode#3301TanishqChavan10 wants to merge 1 commit intokeplergl:masterfrom
Conversation
|
Update: Fixed this by making PointLayer.getPositionAccessor return parsed numeric coords (not raw WKT/GeoJSON), added caching, and updated polygon filtering to support MultiPoint. Tests included. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in PointLayer's polygon/rectangle filtering when using GeoJSON column mode. Previously, the position accessor returned unparsed WKT/GeoJSON strings instead of numeric coordinates, causing polygon filtering to fail completely.
Changes:
- Added
getGeojsonPointPositionFromRawutility to parse WKT/GeoJSON strings into numeric coordinates with support for Point, MultiPoint, and GeometryCollection - Updated
getPositionAccessorin PointLayer to return numeric coordinates with caching to avoid re-parsing - Enhanced polygon filter logic to handle MultiPoint geometries using "any point inside" semantics
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/layers/src/geojson-layer/geojson-position-utils.ts | New utility file that parses raw geo fields into Point/MultiPoint coordinates |
| src/layers/src/point-layer/point-layer.ts | Updated getPositionAccessor to return numeric coordinates instead of raw WKT/GeoJSON, added caching logic |
| src/utils/src/filter-utils.ts | Enhanced polygon filter to handle MultiPoint geometries with "any point inside" logic |
| src/utils/src/filter-utils.spec.ts | Added tests for WKT POINT parsing and MultiPoint filtering behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| accu.push(...g.coordinates); | ||
| } | ||
| return accu; | ||
| }, []); |
There was a problem hiding this comment.
The reduce function is missing an explicit type annotation for the initial accumulator value. The empty array literal on line 32 needs to be typed as number[][] to ensure type safety. Without this, TypeScript cannot properly infer the accumulator type, which could lead to type errors or runtime issues if the code is modified in the future.
| }, []); | |
| }, [] as number[][]); |
| // Fallback: sometimes utilities pass the whole row as an array. | ||
| if (Array.isArray(d)) { | ||
| const {geojson} = this.config.columns; | ||
| return getGeojsonPointPositionFromRaw(d[geojson.fieldIdx]); |
There was a problem hiding this comment.
On line 278, accessing geojson.fieldIdx without checking if geojson exists or if fieldIdx is valid could lead to undefined behavior. Unlike line 264 which has the proper check geojson?.fieldIdx > -1, this fallback path is missing the same validation. Consider adding the same validation here or returning null if geojson is not properly configured.
| return getGeojsonPointPositionFromRaw(d[geojson.fieldIdx]); | |
| if (geojson?.fieldIdx > -1) { | |
| return getGeojsonPointPositionFromRaw(d[geojson.fieldIdx]); | |
| } | |
| return null; |
| describe('filterUtils - polygon filter', () => { | ||
| const squarePolygon = { | ||
| type: 'Feature', | ||
| properties: {}, | ||
| geometry: { | ||
| type: 'Polygon', | ||
| coordinates: [ | ||
| [ | ||
| [-1, -1], | ||
| [1, -1], | ||
| [1, 1], | ||
| [-1, 1], | ||
| [-1, -1] | ||
| ] | ||
| ] | ||
| } | ||
| }; | ||
|
|
||
| test('WKT POINT string can be used for polygon filter', () => { | ||
| const layer = { | ||
| type: 'point', | ||
| getPositionAccessor: () => (d: {raw: unknown}) => getGeojsonPointPositionFromRaw(d.raw) | ||
| }; | ||
|
|
||
| const filter = {value: squarePolygon}; | ||
| const fn = getPolygonFilterFunctor(layer, filter, null); | ||
|
|
||
| expect(getGeojsonPointPositionFromRaw('POINT (0.5 0.5)')).toEqual([0.5, 0.5]); | ||
|
|
||
| expect(fn({raw: 'POINT (0.5 0.5)'})).toBe(true); | ||
| expect(fn({raw: 'POINT (10 10)'})).toBe(false); | ||
| }); | ||
|
|
||
| test('MultiPoint keeps row if any point is inside polygon', () => { | ||
| const layer = { | ||
| type: 'point', | ||
| getPositionAccessor: () => (d: {pos: any}) => d.pos | ||
| }; | ||
|
|
||
| const filter = {value: squarePolygon}; | ||
| const fn = getPolygonFilterFunctor(layer, filter, null); | ||
|
|
||
| expect( | ||
| fn({ | ||
| pos: [ | ||
| [10, 10], | ||
| [0.2, 0.2] | ||
| ] | ||
| }) | ||
| ).toBe(true); | ||
|
|
||
| expect( | ||
| fn({ | ||
| pos: [ | ||
| [10, 10], | ||
| [20, 20] | ||
| ] | ||
| }) | ||
| ).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test coverage could be improved by adding test cases for additional scenarios: 1) GeometryCollection containing Points and MultiPoints, 2) handling of null or invalid input to getGeojsonPointPositionFromRaw, 3) handling of empty GeometryCollections, and 4) regular Point geometries (not just MultiPoint). These additional tests would help ensure the robustness of the polygon filtering logic across all supported geometry types.
Problem: Polygon/rectangle filtering failed because getPositionAccessor returned raw WKT/GeoJSON values instead of numeric coordinates.
Fix: Accessor now returns parsed coordinates (or null), caches parsed results, and polygon filtering supports MultiPoint.