Open vpetzel opened 1 month ago
Hi @vpetzel , if you want to, please add a PR with a flag that allows disabling the check. I’m not sure why anyone would want to add 10.000+ merges into a spreadsheet, but if we can step out of the way, for this rare case we can allow the user to disable the check.
Using
mergeCells
can be very slow when doing many merges. This is caused by a check for overlapping merges. This is done by extracting all affected fields from all previous merges and by checking if any of these happen to fall in the fields affected by the previous merge. All of these things are done in a very expensive manner, as merges are stored as xml strings, and thus require expensive parsing back to integers. The check for intersection is done in a very expensive way too: openxlsx calculates a vector of all positions in the rectangles and subsequently compares these members for intersection.But even then it is easy to see that this method will have a complexity of Θ(n) for adding a new merge to n existing merges (assuming limited size of the merges), and thus a complexity of Θ(n²) for adding n merges.
This code showcases this issue:
Here we see the current behaviour, one that avoids all of the expensive string conversion overhead (formatting would only need to happen when writing the xlsx file) and instead stores merges in a data frame of corner coordinates. Intersection are determined using corner points only. The third version does not check for intersections.
We see that the current implementation is really expensive and gets worse as the area of merges increases, while the alternative structure is still quadratic, but much faster and does not depend on area. But not doing the check at all is of course very fast and nice (and could be even faster if done vectorized). Extending the behaviour we see that even with an area of 2 with 10000 merges it should take about 13 minutes, for 50000 6 hours (for an area of 1000 fields this would be over 5 days), while the alternative would take 2 Minutes, and the one without checks 40 seconds (meanwhile doing it vectorized would take 0.3 seconds).
So I do suggest to maybe include a flag
check_overlap = TRUE
or whatever to allow skipping that check. If set toFALSE
would be the responsibilty of the user to make sure that the inputs are sane with a large gain of performance.In the long term switching from the current flow to one that does not require as many string transformations would also very significantly speed up the regular use.