Improved the check that determines if the pattern trick should be used. #118
Conversation
|
This pr should also affect #104, leading to the filter being applied at the cost of using the more efficient CountAvailablePredicates operation. |
src/engine/QueryPlanner.h
Outdated
| @@ -201,4 +201,6 @@ class QueryPlanner { | |||
| SubtreePlan getTextLeafPlan(const TripleGraph::Node& node) const; | |||
|
|
|||
| SubtreePlan optionalJoin(const SubtreePlan& a, const SubtreePlan& b) const; | |||
| bool checkUsePatternTrick(ParsedQuery& pq, | |||
There was a problem hiding this comment.
Please add a method comment here. Also Google's C++ style guide, which we try to follow, forbids non-const reference parameters and they aren't really used in the rest of the code base. So these need to be pointers instead.
src/engine/QueryPlanner.cpp
Outdated
| alias._function.find("DISTINCT") == std::string::npos && | ||
| alias._function.find("distinct") == std::string::npos && | ||
| (ad_utility::startsWith(alias._function, "COUNT") || | ||
| ad_utility::startsWith(alias._function, "count"))) { |
There was a problem hiding this comment.
can we do these checks on the lowercase aliass._function so it's actually case insensitive?
src/engine/QueryPlanner.cpp
Outdated
| // Check for triples containing the has predicates triples object. | ||
| for (size_t j = 0; | ||
| j < pq._rootGraphPattern._whereClauseTriples.size() && | ||
| usePatternTrick; |
There was a problem hiding this comment.
swap the two sides of the && so one immediately sees that this only applies if usePatternTrick == true
src/engine/QueryPlanner.cpp
Outdated
| usePatternTrick = false; | ||
| } | ||
| } | ||
| // Check for filters on the has predicate triples subject or object |
There was a problem hiding this comment.
write "ql:has-predicate triple's" without the "-" the English sounds a bit weird
src/engine/QueryPlanner.cpp
Outdated
| for (const SparqlFilter& filter : pq._rootGraphPattern._filters) { | ||
| if (filter._lhs == t._o || filter._lhs == t._s || | ||
| filter._rhs == t._o || filter._rhs == t._s) { | ||
| usePatternTrick = false; |
There was a problem hiding this comment.
Ok, but in this case we still have a a ql:has-predicate in the query, am I assuming correctly that without the userPatternTrick flag bein set this will be computed in the classic SPARQL way and will still work or does the query fail?
There was a problem hiding this comment.
If the usePatternTrick flag is not set the query should still work the normal way.
src/engine/QueryPlanner.cpp
Outdated
| break; | ||
| } | ||
| } | ||
| // Check for optional parts containing the has predicate triples |
src/engine/QueryPlanner.cpp
Outdated
| pattern->_children.end()); | ||
| for (const SparqlTriple& other : pattern->_whereClauseTriples) { | ||
| if (other._s == t._o || other._p == t._o || other._o == t._o) { | ||
| usePatternTrick = false; |
src/engine/QueryPlanner.cpp
Outdated
| @@ -408,18 +408,20 @@ QueryExecutionTree QueryPlanner::createExecutionTree(ParsedQuery& pq) const { | |||
| } | |||
|
|
|||
| bool QueryPlanner::checkUsePatternTrick( | |||
| ParsedQuery& pq, SparqlTriple& patternTrickTriple) const { | |||
| ParsedQuery& pq, SparqlTriple* patternTrickTriple) const { | |||
There was a problem hiding this comment.
ParsedQuery is also passed as a non-const reference
src/engine/QueryPlanner.h
Outdated
| /** | ||
| * @brief Determines if the pattern trick (and in turn the | ||
| * CountAvailablePredicates operation) are applicable to the given | ||
| * parsed query. If a has predicate triple is found and |
src/engine/QueryPlanner.h
Outdated
| * @brief Determines if the pattern trick (and in turn the | ||
| * CountAvailablePredicates operation) are applicable to the given | ||
| * parsed query. If a has predicate triple is found and | ||
| * CountAvailblePredicates can be used for it the triple is removed from the |
There was a problem hiding this comment.
"for it, the"
Also can you add the information from your comment that if the check fails the ql:has-predicate part of the query will still be computed just without the pattern trick
There was a problem hiding this comment.
I added the comment that the query will still be computed to the function call instead, as it is not directly a part of the checkUsePatternTrick function.
|
Ok I'm running the Clueweb+ Freebase instance with this change now and it definitely fixes the original test cases in #104 and #84. So I think after addressing the current set of comments this will be ready to merge. Well done! |
floriankramer
force-pushed
the
better_pattern_trick_detection
branch
from
7411d12
to
b2c41fa
Compare
This pr removes some bugs that occurred when triples restricted the object of a ql:has-predicate, when filters filter on any part of a ql:has-predicate triple or when optional parts of a query create results for the object of ql:has-predicate.
This also fixes a bug that caused the order by operation created when constructing the CountAvailablePredicates operation being setup wrongly, leading to a segfault. This may solve #84.