-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow selecting all variables as payload in spatial search SERVICE #1656
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1656 +/- ##
==========================================
+ Coverage 89.57% 89.59% +0.02%
==========================================
Files 381 383 +2
Lines 36792 36863 +71
Branches 4170 4178 +8
==========================================
+ Hits 32955 33029 +74
+ Misses 2525 2522 -3
Partials 1312 1312 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have thought of a way, to make the payload much simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much, only a very minor changes is missing.
@@ -95,7 +115,7 @@ SpatialJoinConfiguration SpatialQuery::toSpatialJoinConfiguration() const { | |||
"spatialSearch: { [Config Triples] { <Something> <ThatSelects> ?right " | |||
"} }."); | |||
} else if (!ignoreMissingRightChild_ && !childGraphPattern_.has_value() && | |||
!payloadVariables_.empty()) { | |||
!payloadVariables_.isAll() && !payloadVariables_.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the !isAll
call redundant, because isAll()
directly implies !empty()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. If I'm not totally mistaken, this is neither logically nor semantically redundant.
- Logically because isAll implies not-empty but not-empty does not imply isAll
- Semantically because there is a subtle edge case, where this has an actual impact: Assume the user does a max-dist-join and declares left and right outside of the service. Then they give the configuration
<payload> <all>
. This is the default behavior anyway. But without the!isAll()
here we would throw despite the user's configuration just stating the default value.
Edit: I will add a unit test for the edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try whether the undefined reference errors go away when you move the PayloadVariable
class into the parser
module and make it part of the parser
library?
We really have to cleanup our dependencies.
I have moved it. Let's wait for the checks and see what happens. The weird thing is that I cannot reproduce this problem locally... |
Conformance check passed ✅No test result changes. |
Quality Gate passedIssues Measures |
@joka921 Thanks very much. This seems to have fixed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much.
In the spatial search service (triggered via
SERVICE spatialSearch:
) all variables from the right hand side of the join (except for the join column) that shall be part of the result have to be explicitly selected via the<payload>
parameter. This PR adds a special value<all>
for this parameter, which automatically selects all the variables from the right side as payload.