Optimize cartodiagram WKB/WKT polygon feature handling#41
Optimize cartodiagram WKB/WKT polygon feature handling#41pierrejego wants to merge 2 commits intoterrestris:5.0.0-customfrom
Conversation
- Reuse parsed OpenLayers features to avoid repeated geometry parsing - Fit map from existing features instead of re-parsing records - Simplify feature construction in dataRecordsToOlFeatures
|
Thanks for the improvement; it meets our needs and will help load larger amounts of data. |
|
Thanks @pierrejego. I will try to take a look at it this week. |
jansule
left a comment
jansule
left a comment
There was a problem hiding this comment.
Nice work @pierrejego ! This looks mainly good. Just a few remarks that should be easy to solve.
Please also run the linter as there are some parts that do not follow the current linting rules.
| dataFeatures, | ||
| getMapExtentPadding(mapExtentPadding), | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
Will the else branch ever be reached, or is this obsolete?
| return data; | ||
| }, [data, geomColumn, geomFormat]); | ||
|
|
||
| const dataFeatures = useMemo(() => { |
There was a problem hiding this comment.
Please add a comment on where/when to use this instead of another data related variable, e.g. processedData, so future devs can more easily decide which to use when.
| getMapExtentPadding(mapExtentPadding), | ||
| ); | ||
| } else { | ||
| fitMapToDataRecords( |
| source?.addFeatures(features); | ||
| }); | ||
| }, [currentDataLayers, filteredData, geomColumn, geomFormat]); | ||
| }, [currentDataLayers, filteredData, filteredFeatures, geomColumn, geomFormat]); |
There was a problem hiding this comment.
Looks like geomColumn can be removed from the list of dependencies
| */ | ||
| import { DataRecord } from '@superset-ui/core'; | ||
| import { useEffect, useMemo, useState } from 'react'; | ||
| import { useEffect, useMemo, useRef, useState } from 'react'; |
|
Also, do you have an indicator/metric on how much the performance was improved due to your changes? |
clarify dataFeatures usage and clean up OlChartMap
During development, I added some temporary indicators to get a rough measure of the impact. In addition, for large and complex geometries, using ST_SimplifyPreserveTopology(geom, 50) AS geom in the query improves performance even further. In that case, rendering drops to under 1 second on my dataset, and the map appears before the tables. |
3 participants
fix(cartodiagram): avoid repeated geometry parsing
SUMMARY
Reuse parsed OpenLayers features for WKB/WKT geometries to avoid re-parsing on render and fit operations. This reduces repeated geometry parsing when polygon layers are large.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (performance-only change)
Before
After
TESTING INSTRUCTIONS
Using
ADDITIONAL INFORMATION
Funding: This work was funded by the Région Hauts-de-France (France).
link to apache#37843