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
Added support for HAVING. Fixes #104. #130
Added support for HAVING. Fixes #104. #130
Conversation
Also added utiliztion of having for CountAvailablePredicates by automatically replacing filters on a ql:has-predicate triple with HAVING statements if the pattern trick is used.
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.
LGTM except for adding a few more e2e examples. While this actually adds a feature the PR overall looks like a simplification so that's great!
?x ql:has-predicate ?predicate . | ||
} | ||
GROUP BY ?predicate | ||
HAVING (?predicate < <Z) (?predicate = <Religion>) |
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.
So these are multiple HAVING
clauses which are treated as AND
right? I think we should document that somewhere besides the SPARQL standard. Also I think we should have a few more examples in the e2e Tests, especially also the very basic usage. I like using them as normal examples and to look up how some syntax works and thus having a few different cases is both a useful test and an example.
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 added another e2e test and expanded the GROUP BY paragraph in the readme.
@@ -1510,6 +1486,61 @@ void QueryPlanner::applyFiltersIfPossible( | |||
} | |||
} | |||
|
|||
// _____________________________________________________________________________ | |||
std::shared_ptr<Operation> QueryPlanner::createFilterOperation( |
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.
this is much nicer as its own method, thanks!
} | ||
} | ||
|
||
// _____________________________________________________________________________ | ||
void SparqlParser::addFilter(const string& str, | ||
ParsedQuery::GraphPattern* pattern) { | ||
vector<SparqlFilter>* _filters) { |
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 think this is actually quite a bit clearer than the old version
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.
LGTM
Also added utiliztion of having for CountAvailablePredicates by
automatically replacing filters on a ql:has-predicate triple with
HAVING statements if the pattern trick is used.