Add utilities in preparation for materialized view query rewriting#2692
Add utilities in preparation for materialized view query rewriting#2692ullingerc wants to merge 18 commits intoad-freiburg:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2692 +/- ##
=======================================
Coverage 91.60% 91.61%
=======================================
Files 483 487 +4
Lines 41386 41448 +62
Branches 5496 5505 +9
=======================================
+ Hits 37912 37971 +59
+ Misses 1897 1895 -2
- Partials 1577 1582 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joka921
left a comment
joka921
left a comment
There was a problem hiding this comment.
Thank you very much and sorry for the long wait.
Please ping me for the next round, it should be faster then.
src/parser/GraphPatternOperation.h
Outdated
| // Extract all variables present in a the first `BasicGraphPattern` contained in | ||
| // a vector of `GraphPatternOperation`s. It is used for skipping some graph | ||
| // patterns in `MaterializedViewQueryAnalysis.cpp`. | ||
| // |
There was a problem hiding this comment.
The comment is a little ambigious:
- does this use the first basic pattern in the query, even if the pattern is not at the beginnning of the query (so first subquery, then BGP), or only "the first element if it is a parsed graph pattern". The comment says more the first, but as this is a function with strange uninituitive behavior, better be precise and verbose.
There was a problem hiding this comment.
It is the first BGP (which can be the first but also a later pattern). I could have
BIND(42 AS ?x)
?a geo:hasGeometry ?b .
?b geo:asWKT ?c
then I would still get the geo:hasGeometry ... as a BGP here.
There was a problem hiding this comment.
I will change the comment, but maybe this function is so specific that it should be hidden in the place where we use it. Do you agree?
| ad_utility::filterRangeOfVariantsByType<parsedQuery::BasicGraphPattern>( | ||
| graphPatterns); | ||
| if (!ql::ranges::empty(basicGraphPatterns)) { | ||
| (*basicGraphPatterns.begin()).collectAllContainedVariables(vars); |
There was a problem hiding this comment.
why not bgp.begin()-> (the start is confusing to read )
There was a problem hiding this comment.
Clang complains that it can't find the method if i use -> here. That's the reason for (*).
| // interested in the bindings for variables from `variables_` as they do not | ||
| // affect the result for these `variables_`. | ||
| // | ||
| // For example: A basic graph pattern (a list of triples) is invariant to a | ||
| // `BIND` statement whose target variable is not contained in the basic graph | ||
| // pattern, because the `BIND` only adds its own column, but neither adds nor | ||
| // deletes result rows. | ||
| // |
There was a problem hiding this comment.
Is this statement actually true:
?x <is-a> ?y .
BIND (somethingCompletelyElse as ?z) # seems invariant.
FILTER (?x > ?y) upps...
Or will you in this case consider filter the whole thing out, because you see the filter and EVERYTHING has to be invariant? in that case please comment this, that this is only true for one step.
But otherwise: The comment is now great to understand
There was a problem hiding this comment.
The filter will be considered. So everything after the graph pattern in the beginning must be invariant. We use this visitor for filtering the graph patterns and if anything except for the original basic graph pattern is left after filtering, no optimization is detected anymore.
There was a problem hiding this comment.
But for this struct all of this doesn't matter. It handles one graph pattern at a time. So it will get the Bind and say I don't care about this, but then see the Filter and say 'oh we have a filter - I always care about filters'
| ASSERT_EQ(map.size(), 2u); | ||
|
|
||
| // Lookup using `std::string_view` pairs. | ||
| auto it = map.find(StringViewPair{"hello", "world"}); |
There was a problem hiding this comment.
Am I seeing correctly as this could never allocate, as StringViewPair is not convertible to StringPair? If yes, then this could be a static assertion.
There was a problem hiding this comment.
- something i forgot in the implementation:
For the same reason (if I am right), we cannot insert via the string_view_pair (because not even explicitly they are convertible, then this should be commented as part of the interface there.
There was a problem hiding this comment.
I have added asserts. Are those the ones you mean here - or could you elaborate otherwise?
There was a problem hiding this comment.
The header is now very clear. With the static assert there is no need for runtime testing here that no allocations happened.
Overview
Conformance check passed ✅Test Status Changes 📊
|
|
joka921
left a comment
joka921
left a comment
There was a problem hiding this comment.
Thank you very much, the improvements now look clean. Write a good message, and forward this to @hannahbast
| ASSERT_EQ(map.size(), 2u); | ||
|
|
||
| // Lookup using `std::string_view` pairs. | ||
| auto it = map.find(StringViewPair{"hello", "world"}); |
There was a problem hiding this comment.
The header is now very clear. With the static assert there is no need for runtime testing here that no allocations happened.
2 participants
This change adds various utilities for the query rewriting infrastructure that will allow implicit use of materialized views.
Preparation for #2649