Spatial join area #1713
Spatial join area #1713
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1713 +/- ##
==========================================
+ Coverage 89.86% 89.95% +0.08%
==========================================
Files 389 393 +4
Lines 37308 37765 +457
Branches 4204 4240 +36
==========================================
+ Hits 33527 33970 +443
- Misses 2485 2491 +6
- Partials 1296 1304 +8 ☔ View full report in Codecov by Sentry. |
Conformance check passed ✅No test result changes. |
|
| using AnyGeometry = boost::variant<Point, Linestring, Polygon, MultiPoint, | ||
| MultiLinestring, MultiPolygon>; |
There was a problem hiding this comment.
What is the difference between boos::variant and std::variant, and do we need the non-std one here?
| Id OnlyForTestingWrapperComputeDist(const IdTable* idTableLeft, | ||
| const IdTable* idTableRight, | ||
| size_t rowLeft, size_t rowRight, | ||
| ColumnIndex leftPointCol, | ||
| ColumnIndex rightPointCol) const { | ||
| return computeDist(idTableLeft, idTableRight, rowLeft, rowRight, | ||
| leftPointCol, rightPointCol); | ||
| } |
There was a problem hiding this comment.
Okay, now those are really too many OnlyForTestingWrappers for const functions.
With const there is no harm in making the functions public (you cannot break the class), so just make them public, and get rid of the clutter.
| // This is different to the query box, which is a box, which contains the area | ||
| // where all results are contained in |
There was a problem hiding this comment.
I don't understand this sentence.
| // This is different to the query box, which is a box, which contains the area | ||
| // where all results are contained in | ||
| BoostGeometryNamespace::Box calculateBoundingBoxOfArea( | ||
| const std::string& wktString) const; |
There was a problem hiding this comment.
Why is this a string? shouldn't you first parse the geometry, and then in a second step calculate the box etc?
| // calculates the midpoint of the given Box | ||
| BoostGeometryNamespace::Point calculateMidpointOfBox( | ||
| const BoostGeometryNamespace::Box& box) const; |
There was a problem hiding this comment.
I think you can shorten (and thereby make more readable) this class by adding
using Point = BlaNamespace::Poin insde the SpatialJoinAlgoirthms class.
then you can here just write Point calculateMidpoint(const Bos& box) const
In the .cpp file you can write
auto SpatialJoinAlgoirithms::calculateMidpoint(const Box& box) const -> Point
(with the trailing return types you are inide the class namespace.
There was a problem hiding this comment.
Do this for every function in this class, it will make the code much more readable.
| // ____________________________________________________________________________ | ||
| Id SpatialJoinAlgorithms::computeDist(const IdTable* idTableLeft, | ||
| const IdTable* idTableRight, | ||
| size_t rowLeft, size_t rowRight, | ||
| ColumnIndex leftPointCol, | ||
| ColumnIndex rightPointCol) const { | ||
| auto getAreaString = [&](const IdTable* idtable, size_t row, size_t col) { |
There was a problem hiding this comment.
I think your algorithm needs the parsing at multiple points
(to compute the box, and to compute the actual distance), so you should have functions that only do the parsing (and return your variant type), and then functions to compute the distance/the box/the whatnotelse from the given parsed type (the variant)
| } catch (...) { | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
does boost geometry have a mode or overload of readWkt that doesn't use exceptions? then use that.
| return Id::makeUndefined(); | ||
| } | ||
| // convert to m and return. Note that the 78630 is an approximation, which | ||
| // does not provide accurate results for the poles. |
There was a problem hiding this comment.
What exactly is 78630?
Can you link to some doku/reference/the name of this thing?
| try { | ||
| std::string areastring = betweenQuotes( | ||
| ExportQueryExecutionTrees::idToStringAndType( | ||
| qec_->getIndex(), smallerResult->at(i, smallerResJoinCol), {}) | ||
| .value() | ||
| .first); | ||
| bbox = calculateBoundingBoxOfArea(areastring); | ||
| } catch (...) { | ||
| printWarning(); | ||
| continue; | ||
| } | ||
| } else { | ||
| // create a box with a side length of at most 1mm to approximate the point |
There was a problem hiding this comment.
As stated, separate the parsing from the remaining functionality.
| if (!geopoint) { | ||
| try { | ||
| std::string areastring = betweenQuotes( | ||
| ExportQueryExecutionTrees::idToStringAndType( | ||
| qec_->getIndex(), otherResult->at(i, otherResJoinCol), {}) | ||
| .value() | ||
| .first); | ||
| auto areaBox = calculateBoundingBoxOfArea(areastring); | ||
| auto midpoint = calculateMidpointOfBox(areaBox); | ||
| bbox = computeBoundingBox( | ||
| midpoint, | ||
| getMaxDistFromMidpointToAnyPointInsideTheBox(areaBox, midpoint)); | ||
| } catch (...) { | ||
| printWarning(); | ||
| continue; | ||
| } | ||
| } else { | ||
| bbox = computeBoundingBox( | ||
| Point(geopoint.value().getLng(), geopoint.value().getLat())); |
There was a problem hiding this comment.
That is an exact code dupliacation that can be extracted.
This merge request adds the possibility to use arbitrary WKT Geometries (i.e. areas) in SpatialJoin queries. Right now, areas can get approximated as the center point or the true distance between them is estimated. In addition to that a future PR will add pre computed bounding boxes to the index generation to enable even faster query times (like it already exists for geopoints)