Initial implementation of `SpatialJoin` #1248

Jonathan24680 · 2024-01-28T17:30:04Z

Implement a an Operation SpatialJoin that takes two inputs, one variable per input, and a maximal distance d. This operation joins each row pair [left, right] where the spatial distance between the value of the two variables is less or equal than d.
It can currently be called by adding a triple using the special predicate <max-distance-in-meters:x>, where x is a positive integer that is used as the maximal distance. For example:

SELECT * WHERE {
 ?a <is-a> <tram-stop> .
 ?b <is-a> <restaurnat> .
 ?a <max-distance-in-meters:100> ?b
}

Current limitations (will be fixed in the future)

Only supports points (no linestrings, polygons, etc).
The implementation is a simple quadratic loop over the cartesian product of both inputs.
The spatial join should not be called via a special predicate, but via FILTER(geo:distance(?a, ?b) < 100).

hannahbast

Thank you for this PR. Three things I noted from looking over it quickly:

In SpatialJoin::computeResult you have two very similar code blocks for computing the Cartesian product twice (with and without condition). You probably want a common implementation for this (with a template parameter for the filtering condition).
Please use // ... for comments and not /* ... */
A newline is missing at the end of some of the new files.

P.S. No need to call the fork on your account qlever-fork, you can just call it qlever.

codecov · 2024-01-28T17:57:50Z

Codecov Report

Attention: Patch coverage is 87.90323% with 30 lines in your changes missing coverage. Please review.

Project coverage is 88.13%. Comparing base (85793e3) to head (de2c3f4).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/engine/SpatialJoin.cpp	87.43%	14 Missing and 12 partials ⚠️
src/engine/QueryPlanner.cpp	88.57%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #1248    +/-   ##
========================================
  Coverage   88.12%   88.13%            
========================================
  Files         357      359     +2     
  Lines       26764    27012   +248     
  Branches     3606     3649    +43     
========================================
+ Hits        23587    23808   +221     
- Misses       1941     1954    +13     
- Partials     1236     1250    +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joka921

I have reviewed everything but the computeResult and the VarColumnMaps.
Please also fix the build failures (mostly the warnings) and please start writing unit tests, start with your spatialJoin class (We have several examples in tests for other operations as to how you can input stuff, otherwise contact me).

src/engine/QueryPlanner.h

src/engine/QueryPlanner.cpp

src/engine/SpatialJoin.cpp

test/engine/SpatialJoinTest.cpp

joka921

Some additional requests for the missing functions.

src/engine/SpatialJoin.cpp

joka921

A round of requests on everything but SpatialJoinTest (which I will look at next).

joka921 · 2024-08-23T14:28:28Z

src/engine/SpatialJoin.h

+  std::shared_ptr<QueryExecutionTree> onlyForTestingGetLeftChild() const {
    return childLeft_;
  }

-  std::shared_ptr<QueryExecutionTree> onlyForTestingGetRightChild() {
+  std::shared_ptr<QueryExecutionTree> onlyForTestingGetRightChild() const {


If this is const then it should return shared_ptr<const QueryExecutionTree> ("shallow constness")

this doesn't work as then the call onlyForTestingGetRightChild()->getSizeEstimate() doesn't work, as this operation of the superclass isn't const.

Okay, but then you probably shouldn't mark this function const. If sonarcloud complains, that is fine in this case, as it is not smarter than the compiler.

src/engine/SpatialJoin.h

src/engine/SpatialJoin.cpp

test/QueryPlannerTest.cpp

joka921

A review of SpatialJoinTest.cpp up to line 1116

test/engine/SpatialJoinTest.cpp

joka921 · 2024-08-23T15:17:20Z

test/engine/SpatialJoinTest.cpp

+#include "engine/SpatialJoin.h"
+#include "parser/data/Variable.h"
+
+using namespace ad_utility::testing;


All the global helpers that only appear in a .cppfile should be in the anonymous namespace

namespace { // functions and variables here }

This makes them inaccessible outside this .cpp files and avoids linker problems.

i think i have done this (but named the namespace localTestHelpers). I'm not completely sure, if i have done this correctly. I have (already before this comment) created a lot of namespaces to structure the test file. The only functions which weren't contained in a namespace, where the ones in the beginning. The reason for the many namespaces is the following: if you are developing a test for a certain function, you can collapse all namespaces, which aren't required for this function, which minimizes the amount of scrolling through the source code

We just found out, that you can do both: (and should)

namespace { // avoid linking problems namespace localTestHelpers { // because the file is long // put stuff here void f() {return 42;} } } // outside of namspace, you can use localTestHel0pers:::f()

Maybe the TEST(...) macros need to be outside the anonymouse namespace to work correctly.

test/engine/SpatialJoinTest.cpp

joka921 · 2024-08-23T15:46:09Z

test/engine/SpatialJoinTest.cpp

+      expectedMaxDist10000000_rows_diff, columnNames, false);
+}
+
+}  // end of Namespace computeResultTest


I have looked at this until here.
I see you put a lot of effort into those tests which I like very much.
The last part (the manual setting up of inputs) is okay like this, because it is crucial for the correctness, so it might be a bit longer.

I will continue to look at the remainder (but you can also apply the same principles for your remaining tests).

Thanks :)
so the comment from above with the refactoring of the setting up of inputs doesn't apply anymore?

joka921

This is now much cleaner.

src/engine/SpatialJoin.cpp

src/engine/SpatialJoin.h

joka921 · 2024-09-20T16:01:06Z

test/engine/SpatialJoinTest.cpp

+#include "engine/SpatialJoin.h"
+#include "parser/data/Variable.h"
+
+using namespace ad_utility::testing;


We just found out, that you can do both: (and should)

namespace { // avoid linking problems namespace localTestHelpers { // because the file is long // put stuff here void f() {return 42;} } } // outside of namspace, you can use localTestHel0pers:::f()

joka921 · 2024-09-20T16:01:38Z

test/engine/SpatialJoinTest.cpp

+#include "engine/SpatialJoin.h"
+#include "parser/data/Variable.h"
+
+using namespace ad_utility::testing;


Maybe the TEST(...) macros need to be outside the anonymouse namespace to work correctly.

joka921

Thank you very much for this initial infrastructure, I am looking forward to the upcoming more efficient implementations.

joka921

Thank you very much for this initial infrastructure, I am looking forward to the more efficient implementations.

Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

sonarcloud · 2024-09-27T14:54:22Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Jonathan24680 and others added 6 commits January 25, 2024 01:45

merge changes with new qlever version

0fe8bd2

merge adaptions

4497ae5

maxDistance can be given as parameter

1b49817

cleaning up

16381e7

Merge branch 'ad-freiburg:master' into SpatialJoinClass

6084f6d

cleaning up

522cef2

hannahbast requested changes Jan 28, 2024

View reviewed changes

joka921 requested changes Jan 29, 2024

View reviewed changes

Jonathan24680 added 20 commits February 4, 2024 21:55

Merge branch 'master' into SpatialJoinClass

95e39ec

Merge branch 'master' into SpatialJoinClass

22e3276

move the spatialJoin constant to the constant file and cleaning up

16e47a6

backup commit - work in progress

6783ddb

add creation of test dataset

2f1146f

backup commit

d40b5c1

backup commit

f49918f

backup commit

0ca9bbe

backup commit

04af121

test of computeResult in the SpatialJoin

4a79fc3

forgot to save some edits

d202972

maxDistance Parsing tests

0f55a78

backup

fd0dae9

probably overflow distance mistake somewhere

a76bbb1

added many tests

df7084b

added knownEmptyResultTest

b6add0d

SpatialJoin getDescriptor and getCacheKeyImpl and tests

12435ce

backup commit with testing the multiplicity of the indexscan

87c42a8

multiplicities and sizeEstimate implementation and test

d5913fe

improved multiplicity and size estimate tests added sortedOn test

cbce5bf

Jonathan24680 added 5 commits August 20, 2024 18:07

first QueryPlannerTest

5cc5fba

solve merge conflict

9e083a1

add additional QueryPlannerTests

7b964f7

Sonarqube issues

d90ea82

change array to vector because of pipeline mistake

707cc32

joka921 requested changes Aug 23, 2024

View reviewed changes

Jonathan24680 added 7 commits August 25, 2024 13:10

sonarqube and merge request changes

53dfcf5

merge request comments on mostly SpatialJoin cpp

d33ac37

clang-format

b64e4a2

Sonarqube

a91456f

merge request comments

35240bd

improving tests

884ceb5

format check und typo

81c09a2

joka921 requested changes Sep 20, 2024

View reviewed changes

Jonathan24680 added 5 commits September 20, 2024 18:10

remove unneeded code

a1e9dd1

Merge branch 'master' into SpatialJoinClass

ece9b0b

compile error changes

dca72ac

format and namespace

ef95bb3

remove unused function

40ffa04

joka921 approved these changes Sep 27, 2024

View reviewed changes

Merge branch 'master' into SpatialJoinClass

84ae818

joka921 approved these changes Sep 27, 2024

View reviewed changes

joka921 and others added 3 commits September 27, 2024 11:31

Merge branch 'master' into SpatialJoinClass

63fadec

Fix the merge with the old bla.

2f263f0

Signed-off-by: Johannes Kalmbach <johannes.kalmbach@gmail.com>

Update SpatialJoin.cpp

de2c3f4

joka921 changed the title ~~infrastructure for the SpatialJoin class~~ First implementation of SpatialJoin Sep 28, 2024

joka921 changed the title ~~First implementation of SpatialJoin~~ Initial implementation of SpatialJoin Sep 28, 2024

joka921 merged commit 2e0ac35 into ad-freiburg:master Sep 28, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of `SpatialJoin` #1248

Initial implementation of `SpatialJoin` #1248

Jonathan24680 commented Jan 28, 2024 •

edited by joka921

Loading

hannahbast left a comment •

edited

Loading

codecov bot commented Jan 28, 2024 •

edited

Loading

joka921 left a comment

joka921 left a comment

joka921 left a comment

joka921 Aug 23, 2024

Jonathan24680 Aug 25, 2024

joka921 Aug 25, 2024

joka921 left a comment

joka921 Aug 23, 2024

Jonathan24680 Aug 27, 2024

joka921 Sep 20, 2024

joka921 Sep 20, 2024

joka921 Aug 23, 2024

Jonathan24680 Aug 27, 2024

joka921 left a comment

joka921 Sep 20, 2024

joka921 Sep 20, 2024

joka921 left a comment

joka921 left a comment

sonarcloud bot commented Sep 27, 2024

Initial implementation of SpatialJoin #1248

Initial implementation of SpatialJoin #1248

Conversation

Jonathan24680 commented Jan 28, 2024 • edited by joka921 Loading

hannahbast left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 28, 2024 • edited Loading

Codecov Report

joka921 left a comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 27, 2024

Quality Gate passed

Initial implementation of `SpatialJoin` #1248

Initial implementation of `SpatialJoin` #1248

Jonathan24680 commented Jan 28, 2024 •

edited by joka921

Loading

hannahbast left a comment •

edited

Loading

codecov bot commented Jan 28, 2024 •

edited

Loading