Support empty transitive path #1835
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1835 +/- ##
==========================================
+ Coverage 90.18% 90.21% +0.02%
==========================================
Files 400 400
Lines 38440 38472 +32
Branches 4306 4309 +3
==========================================
+ Hits 34666 34706 +40
+ Misses 2479 2468 -11
- Partials 1295 1298 +3 ☔ View full report in Codecov by Sentry. |
Conformance check passed ✅Test Status Changes 📊
|
|
| @@ -2535,8 +2535,12 @@ void QueryPlanner::GraphPatternPlanner::visitTransitivePath( | |||
| right.value_ = getSideValue(arg._right); | |||
| size_t min = arg._min; | |||
| size_t max = arg._max; | |||
| if (planner_.activeGraphVariable_.has_value()) { | |||
| AD_THROW("Property paths with graph variables are not supported"); | |||
There was a problem hiding this comment.
not AD_THROW but simply throw
(it is not a bug, but an unsupported feature)
and it should read Property paths inside a GRAPH clause with a graph variable are....
| if (minDist_ == 0 && !lhs_.isBoundVariable() && !rhs_.isBoundVariable() && | ||
| lhs_.isVariable() && rhs_.isVariable()) { |
There was a problem hiding this comment.
Refactor is to lhs_.isUnboundVariable for the readability.
| auto uniqueValues = ad_utility::makeExecutionTree<Distinct>( | ||
| qec, QueryExecutionTree::createSortedTree(std::move(allValues), {0}), |
There was a problem hiding this comment.
Doesn't the constructor of distinct perform the createSortedTree automatically?
There was a problem hiding this comment.
If not, it definitely should...
Fix this either here or (better) in a separate PR.
| @@ -14,10 +14,10 @@ | |||
| TransitivePathHashMap::TransitivePathHashMap( | |||
| QueryExecutionContext* qec, std::shared_ptr<QueryExecutionTree> child, | |||
| TransitivePathSide leftSide, TransitivePathSide rightSide, size_t minDist, | |||
| size_t maxDist) | |||
| size_t maxDist, Graphs activeGraphs) | |||
There was a problem hiding this comment.
If all these inheriting classes have no exclusive state, but only share the members of the base class, just with different implementations,
then you can get rid of the explicitly + redundantly reimplemented constructors and just to
using BaseClass::BaseClass to pull in the constructors of the base.
There was a problem hiding this comment.
(If true, this comment probably holds for multiple classes that start with TransitivePath...
| @@ -2981,3 +2981,53 @@ TEST(QueryPlanner, Exists) { | |||
| "GRAPH ?g { ?u ?v ?c}}}", | |||
| filter); | |||
| } | |||
|
|
|||
| // _____________________________________________________________________________ | |||
| TEST(QueryPlanner, PropertyPathWithGraph) { | |||
There was a problem hiding this comment.
Do you have a test cases for a non-empty transitive path, e.g.
SELECT * FROM <g> { ?x <blubb>+ ?y }
This case should also correctly forward the graph constraint to the index scan for <blubb>.
| @@ -2981,3 +2981,53 @@ TEST(QueryPlanner, Exists) { | |||
| "GRAPH ?g { ?u ?v ?c}}}", | |||
| filter); | |||
| } | |||
|
|
|||
| // _____________________________________________________________________________ | |||
| TEST(QueryPlanner, PropertyPathWithGraph) { | |||
There was a problem hiding this comment.
This test case mixes "property path with graph" and "empty property path": ,
at least adjust the names.
| @@ -420,6 +421,9 @@ constexpr auto OrderBy = [](const ::OrderBy::SortedVariables& sortedVariables, | |||
| // Match a `UNION` operation. | |||
| constexpr auto Union = MatchTypeAndOrderedChildren<::Union>; | |||
|
|
|||
| // Match a `DISTINCT` operation. | |||
| constexpr auto Distinct = MatchTypeAndOrderedChildren<::Distinct>; | |||
There was a problem hiding this comment.
The DISTINCT class also has the state of the selected/relevant variables.
Those should also be reflected in the matcher appropriately.
Do not merge before #1834 and #1824, otherwise this will result in bad performance and OOM crashes.
Fixes #1780