Add utilities in preparation for materialized view query rewriting#2692
Add utilities in preparation for materialized view query rewriting#2692ullingerc wants to merge 12 commits intoad-freiburg:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2692 +/- ##
=======================================
Coverage 91.60% 91.60%
=======================================
Files 483 485 +2
Lines 41360 41401 +41
Branches 5493 5501 +8
=======================================
+ Hits 37886 37925 +39
+ Misses 1897 1896 -1
- Partials 1577 1580 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overview
Conformance check passed ✅No test result changes. |
|
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.
| // Call a function for every variable contained in the triple. | ||
| void forEachVariable(auto function) const { | ||
| if (s_.isVariable()) { | ||
| function(s_.getVariable()); |
There was a problem hiding this comment.
consider using std::invoke (for the flex + member pointers of Variable:))
| } | ||
|
|
||
| // Call a function for every variable contained in the triple. | ||
| void forEachVariable(auto function) const { |
There was a problem hiding this comment.
Add a requirement (invocable with a const Variable&).
| // 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.
| 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 )
| // 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
| template <typename ValueType> | ||
| using StringPairHashMap = | ||
| ad_utility::HashMap<ad_utility::detail::StringPair, ValueType, |
There was a problem hiding this comment.
Does this support types other than StringPair and StringViewPair?
in that case you can make that clear in comments, and/or requires clauses.
| CPP_template(typename T, typename R)( | ||
| requires ql::ranges::range<R>) auto filterRangeOfVariantsByType(const R& | ||
| range) { |
There was a problem hiding this comment.
doesn't need to be constrained to const & , can be R&& , then you need a remove_cvref_t or decay for the range constraint, and (as I still haven't patched range-v3 for this) use return ad_utility::allView(AD_FWD(range)) | ....
| expectFilteredRange<V, char>(vec, {'c', 'f'}); | ||
| expectFilteredRange<V, bool>(vec, {true, false, true}); | ||
| expectFilteredRange<V, double>(vec, {}); | ||
| } |
There was a problem hiding this comment.
add tests for my suggested improvements. In particular test that auto f = filterRange<blub>(temporaryVector) doesn't dangle (which it currently does!).
There was a problem hiding this comment.
(+ checks for mutable references).
| 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.
| // The `VALUES` doesn't bind to any of the `variables_`. | ||
| ql::ranges::none_of(variables, [this](const auto& var) { | ||
| return variables_.contains(var); | ||
| }); |
There was a problem hiding this comment.
Technically this module is currently untested (at least I cannot find the tests, otherwise please point me to them). I am not sure if this is a hard problem for me, but they should be easy to vibecode etc.
2 participants
This change adds various utilities for the query rewriting infrastructure that will allow implicit use of materialized views.
Preparation for #2649