Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jonathan24680
Jonathan24680 committed Feb 7, 2025
1 parent 5f12f22 commit 978b512
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 75 deletions.
137 changes: 71 additions & 66 deletions src/engine/SpatialJoinAlgorithms.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,9 @@ std::string_view SpatialJoinAlgorithms::betweenQuotes(
}

Check warning on line 50 in src/engine/SpatialJoinAlgorithms.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/SpatialJoinAlgorithms.cpp#L49-L50 

Added lines #L49 - L50 were not covered by tests
}

int SpatialJoinAlgorithms::getAnyGeometry(
std::optional<size_t> SpatialJoinAlgorithms::getAnyGeometry(
const IdTable* idtable, size_t row, size_t col) {
auto printWarning = [this,
&spatialJoin = spatialJoin_]() mutable {
auto printWarning = [this, &spatialJoin = spatialJoin_]() mutable {
if (this->nrOfFailedParsedGeometries_ == 0) {
std::string warning =
"The input to a spatial join contained at least one element, "
Expand Down Expand Up @@ -82,40 +81,36 @@ int SpatialJoinAlgorithms::getAnyGeometry(
AnyGeometry geometry;
try {
bg::read_wkt(str, geometry);
if (geometries_.size() > 0) {
std::cerr << "size before " << geometries_.size() << std::endl;
}
geometries_.push_back(std::move(geometry));
std::cerr << str << std::endl;
auto test = &geometries_.back();
std::cerr << "ptr " << test << std::endl;
std::cerr << "size after " << geometries_.size() << std::endl;
} catch (...) {
printWarning();
return -1;
return std::nullopt;
}
return geometries_.size() - 1; // index of the last element
}

// ____________________________________________________________________________
double SpatialJoinAlgorithms::computeDist(int geometryIndex1,
int geometryIndex2) const {
return boost::apply_visitor(DistanceVisitor(), geometries_.at(geometryIndex1), geometries_.at(geometryIndex2)) * 78.630;
double SpatialJoinAlgorithms::computeDist(
const std::optional<size_t> geometryIndex1,
const std::optional<size_t> geometryIndex2) const {
return boost::apply_visitor(DistanceVisitor(),
geometries_.at(geometryIndex1.value()),
geometries_.at(geometryIndex2.value())) *
78.630;
};

// ____________________________________________________________________________
int SpatialJoinAlgorithms::convertGeoPointToPoint(GeoPoint point) {
size_t SpatialJoinAlgorithms::convertGeoPointToPoint(GeoPoint point) {
geometries_.push_back(Point(point.getLng(), point.getLat()));
return geometries_.size() - 1; // index of the last element
};

Check warning on line 106 in src/engine/SpatialJoinAlgorithms.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/SpatialJoinAlgorithms.cpp#L103-L106 

Added lines #L103 - L106 were not covered by tests

// ____________________________________________________________________________
Id SpatialJoinAlgorithms::computeDist(rtreeEntry& geo1,
rtreeEntry& geo2) {
auto convertPoint = [&](int geometryIndex,
std::optional<Box> bbox) {
Id SpatialJoinAlgorithms::computeDist(rtreeEntry& geo1, rtreeEntry& geo2) {
auto convertPoint = [&](size_t geometryIndex, std::optional<Box> bbox) {
if (!bbox.has_value()) {
bbox = boost::apply_visitor(BoundingBoxVisitor(), geometries_.at(geometryIndex));
bbox = boost::apply_visitor(BoundingBoxVisitor(),
geometries_.at(geometryIndex));
}

Check warning on line 114 in src/engine/SpatialJoinAlgorithms.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/SpatialJoinAlgorithms.cpp#L112-L114 

Added lines #L112 - L114 were not covered by tests
const auto& areaBox = bbox.value();
Point p = calculateMidpointOfBox(areaBox);
Expand All @@ -126,28 +121,32 @@ Id SpatialJoinAlgorithms::computeDist(rtreeEntry& geo1,
auto point1 = geo1.geoPoint_;
auto point2 = geo2.geoPoint_;
if (!point1) {
point1 = convertPoint(geo1.geometryIndex_, geo1.boundingBox_);
point1 =
convertPoint(geo1.geometryIndex_.value(), geo1.boundingBox_.value());
}
if (!point2) {
point2 = convertPoint(geo2.geometryIndex_, geo2.boundingBox_);
point2 =
convertPoint(geo2.geometryIndex_.value(), geo2.boundingBox_.value());
}
return Id::makeFromDouble(
ad_utility::detail::wktDistImpl(point1.value(), point2.value()));
} else {
if (geo1.geoPoint_.has_value() && geo2.geoPoint_.has_value()) {
return Id::makeFromDouble(ad_utility::detail::wktDistImpl(
geo1.geoPoint_.value(), geo2.geoPoint_.value()));

Check warning on line 136 in src/engine/SpatialJoinAlgorithms.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/SpatialJoinAlgorithms.cpp#L135-L136 

Added lines #L135 - L136 were not covered by tests
} else if (geo1.geometryIndex_ != -1 && geo2.geometryIndex_ != -1) {
} else if (geo1.geometryIndex_.has_value() &&
geo2.geometryIndex_.has_value()) {
return Id::makeFromDouble(
computeDist(geo1.geometryIndex_, geo2.geometryIndex_));
} else {
// one point and one area
if (geo1.geometryIndex_ == -1) {
if (!geo1.geometryIndex_) {
geo1.geometryIndex_ = convertGeoPointToPoint(geo1.geoPoint_.value());

Check warning on line 144 in src/engine/SpatialJoinAlgorithms.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/SpatialJoinAlgorithms.cpp#L144 

Added line #L144 was not covered by tests
} else if (geo2.geometryIndex_ == -1) {
} else if (!geo2.geometryIndex_) {
geo2.geometryIndex_ = convertGeoPointToPoint(geo2.geoPoint_.value());
}
return Id::makeFromDouble(computeDist(geo1.geometryIndex_, geo2.geometryIndex_));
return Id::makeFromDouble(
computeDist(geo1.geometryIndex_, geo2.geometryIndex_));
}

Check warning on line 150 in src/engine/SpatialJoinAlgorithms.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/SpatialJoinAlgorithms.cpp#L146-L150 

Added lines #L146 - L150 were not covered by tests
}
}
Expand All @@ -158,8 +157,9 @@ Id SpatialJoinAlgorithms::computeDist(const IdTable* idTableLeft,
size_t rowLeft, size_t rowRight,
ColumnIndex leftPointCol,
ColumnIndex rightPointCol) {
auto getAreaOrPointGeometry = [&](const IdTable* idtable, size_t row,
size_t col, std::optional<GeoPoint> point) -> int {
auto getAreaOrPointGeometry =
[&](const IdTable* idtable, size_t row, size_t col,
std::optional<GeoPoint> point) -> std::optional<size_t> {
if (!point) {
// nothing to do. When parsing a point or an area fails, a warning
// message gets printed at another place and the point/area just gets
Expand All @@ -173,21 +173,22 @@ Id SpatialJoinAlgorithms::computeDist(const IdTable* idTableLeft,
// information will be stored in an ID, just like GeoPoint
auto getAreaPoint = [&](const IdTable* idtable, size_t row,
size_t col) -> std::optional<GeoPoint> {
int geometryIndex = getAnyGeometry(idtable, row, col);
if (geometryIndex == -1) {
std::optional<size_t> geometryIndex = getAnyGeometry(idtable, row, col);
if (!geometryIndex) {
// nothing to do. When parsing a point or an area fails, a warning message
// gets printed at another place and the point/area just gets skipped
return std::nullopt;
}

Box areaBox = boost::apply_visitor(BoundingBoxVisitor(), geometries_.at(geometryIndex));
Box areaBox = boost::apply_visitor(BoundingBoxVisitor(),
geometries_.at(geometryIndex.value()));

Point p = calculateMidpointOfBox(areaBox);
return GeoPoint(p.get<1>(), p.get<0>());
};

rtreeEntry entryLeft{rowLeft, -1, std::nullopt, std::nullopt};
rtreeEntry entryRight{rowRight, -1, std::nullopt, std::nullopt};
rtreeEntry entryLeft{rowLeft, std::nullopt, std::nullopt, std::nullopt};
rtreeEntry entryRight{rowRight, std::nullopt, std::nullopt, std::nullopt};
entryLeft.geoPoint_ = getPoint(idTableLeft, rowLeft, leftPointCol);
entryRight.geoPoint_ = getPoint(idTableRight, rowRight, rightPointCol);
if (entryLeft.geoPoint_ && entryRight.geoPoint_) {
Expand All @@ -210,7 +211,8 @@ Id SpatialJoinAlgorithms::computeDist(const IdTable* idTableLeft,
idTableLeft, rowLeft, leftPointCol, entryLeft.geoPoint_);
entryRight.geometryIndex_ = getAreaOrPointGeometry(
idTableRight, rowRight, rightPointCol, entryRight.geoPoint_);
if (entryLeft.geometryIndex_ != -1 && entryRight.geometryIndex_ != -1) {
if (entryLeft.geometryIndex_.has_value() &&
entryRight.geometryIndex_.has_value()) {
return computeDist(entryLeft, entryRight);
} else {
return Id::makeUndefined();
Expand Down Expand Up @@ -631,26 +633,40 @@ double SpatialJoinAlgorithms::getMaxDistFromMidpointToAnyPointInsideTheBox(
}

// ____________________________________________________________________________
std::optional<rtreeEntry> SpatialJoinAlgorithms::getRtreeEntry(const IdTable* idTable, const size_t row,
const ColumnIndex col) {
rtreeEntry entry{row, -1, std::nullopt, std::nullopt};
entry.geoPoint_ = getPoint(idTable, row, col);

if (!entry.geoPoint_) {
entry.geometryIndex_ = getAnyGeometry(idTable, row, col);
if (entry.geometryIndex_ == -1) {
return std::nullopt;
}
entry.boundingBox_ =
boost::apply_visitor(BoundingBoxVisitor(), geometries_.at(entry.geometryIndex_));
} else {
entry.boundingBox_ =
Box(Point(entry.geoPoint_.value().getLng(),
entry.geoPoint_.value().getLat()),
Point(entry.geoPoint_.value().getLng() + 0.00000001,
entry.geoPoint_.value().getLat() + 0.00000001));
std::optional<rtreeEntry> SpatialJoinAlgorithms::getRtreeEntry(
const IdTable* idTable, const size_t row, const ColumnIndex col) {
rtreeEntry entry{row, std::nullopt, std::nullopt, std::nullopt};
entry.geoPoint_ = getPoint(idTable, row, col);

if (!entry.geoPoint_) {
entry.geometryIndex_ = getAnyGeometry(idTable, row, col);
if (!entry.geometryIndex_) {
return std::nullopt;
}
return entry;
entry.boundingBox_ = boost::apply_visitor(
BoundingBoxVisitor(), geometries_.at(entry.geometryIndex_.value()));
} else {
entry.boundingBox_ =
Box(Point(entry.geoPoint_.value().getLng(),
entry.geoPoint_.value().getLat()),
Point(entry.geoPoint_.value().getLng() + 0.00000001,
entry.geoPoint_.value().getLat() + 0.00000001));
}
return entry;
}

// ____________________________________________________________________________
std::vector<Box> SpatialJoinAlgorithms::getQueryBox(
const std::optional<rtreeEntry>& entry) const {
if (!entry.value().geoPoint_) {
auto midpoint = calculateMidpointOfBox(entry.value().boundingBox_.value());
return computeBoundingBox(
midpoint, getMaxDistFromMidpointToAnyPointInsideTheBox(
entry.value().boundingBox_.value(), midpoint));
} else {
return computeBoundingBox(Point(entry.value().geoPoint_.value().getLng(),
entry.value().geoPoint_.value().getLat()));
}
}

// ____________________________________________________________________________
Expand Down Expand Up @@ -688,7 +704,7 @@ Result SpatialJoinAlgorithms::BoundingBoxAlgorithm() {
continue;
}
rtree.insert(std::pair(entry.value().boundingBox_.value(),
std::move(entry.value())));
std::move(entry.value())));
}

// query rtree with the other child
Expand All @@ -703,18 +719,7 @@ Result SpatialJoinAlgorithms::BoundingBoxAlgorithm() {
// skipped
continue;
}
std::vector<Box> queryBox;
if (!entry.value().geoPoint_) {
auto midpoint =
calculateMidpointOfBox(entry.value().boundingBox_.value());
queryBox = computeBoundingBox(
midpoint, getMaxDistFromMidpointToAnyPointInsideTheBox(
entry.value().boundingBox_.value(), midpoint));
} else {
queryBox =
computeBoundingBox(Point(entry.value().geoPoint_.value().getLng(),
entry.value().geoPoint_.value().getLat()));
}
std::vector<Box> queryBox = getQueryBox(entry);

results.clear();

Expand Down
26 changes: 17 additions & 9 deletions src/engine/SpatialJoinAlgorithms.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ struct DistanceVisitor : public boost::static_visitor<double> {

struct rtreeEntry {
size_t row_;
int geometryIndex_ = -1; // -1 encodes not being a valid index
std::optional<size_t> geometryIndex_;
std::optional<GeoPoint> geoPoint_;
std::optional<Box> boundingBox_;
};
Expand Down Expand Up @@ -124,8 +124,8 @@ class SpatialJoinAlgorithms {
const Box& box, std::optional<Point> midpoint = std::nullopt) const;

// this function gets the string which represents the area from the idtable.
int getAnyGeometry(const IdTable* idtable, size_t row,
size_t col);
std::optional<size_t> getAnyGeometry(const IdTable* idtable, size_t row,
size_t col);

private:
// Helper function which returns a GeoPoint if the element of the given table
Expand Down Expand Up @@ -163,17 +163,25 @@ class SpatialJoinAlgorithms {
// Only for the poles, the conversion will be way to large (for the longitude
// difference). Note, that this function is expensive and should only be
// called when needed
double computeDist(const int geometryIndex1,
const int geometryIndex2) const;
double computeDist(const std::optional<size_t> geometryIndex1,
const std::optional<size_t> geometryIndex2) const;

// this helper function takes an idtable, a row and a column. It then tries
// to parse a geometry or a geoPoint of that cell in the idtable. If it
// succeeds, it returns an rtree entry of that geometry/geopoint
std::optional<rtreeEntry> getRtreeEntry(const IdTable* idTable, const size_t row,
const ColumnIndex col);

std::optional<rtreeEntry> getRtreeEntry(const IdTable* idTable,
const size_t row,
const ColumnIndex col);

// this helper function converts a GeoPoint into a boost geometry Point
int convertGeoPointToPoint(GeoPoint point);
size_t convertGeoPointToPoint(GeoPoint point);

// this helper function calculates the query box. The query box, is the box,
// which contains the area, where all possible candidates of the max distance
// query must be contained in. It returns a vector, because if the box crosses
// the poles or the -180/180 longitude line, then it is disjoint in the
// cartesian coordinates. The boxes themselves are disjoint to each other.
std::vector<Box> getQueryBox(const std::optional<rtreeEntry>& entry) const;

QueryExecutionContext* qec_;
PreparedSpatialJoinParams params_;
Expand Down

0 comments on commit 978b512

Please sign in to comment.