Implement construct PrefilterExpression from SparqlExpression.
#1573
Conversation
Co-authored-by: Johannes Kalmbach <[email protected]>
Co-authored-by: Johannes Kalmbach <[email protected]>
Co-authored-by: Johannes Kalmbach <[email protected]>
Co-authored-by: Johannes Kalmbach <[email protected]>
…neral implementation
Co-authored-by: Johannes Kalmbach <[email protected]>
Co-authored-by: Johannes Kalmbach <[email protected]>
…ng correct prefix in Constants.h
realHannes
changed the title
PrefilterExpression from SparqlExpression.
realHannes
changed the title
PrefilterExpression from SparqlExpression.PrefilterExpression from SparqlExpression.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1573 +/- ##
==========================================
- Coverage 89.21% 89.20% -0.01%
==========================================
Files 372 372
Lines 34723 35049 +326
Branches 3915 3961 +46
==========================================
+ Hits 30979 31267 +288
- Misses 2471 2490 +19
- Partials 1273 1292 +19 ☔ View full report in Codecov by Sentry. |
| [[maybe_unused]] bool isNegated) const { | ||
| return std::nullopt; | ||
| }; | ||
|
|
There was a problem hiding this comment.
You have forgotten one case where you can apply a filter
(I also didn't tell you this).
In the RegexExpression there is the special case of a PrefixRegex which is applied using binary search and can basically be something like ` ?x begins with "hannes" -> ?x >= hannes && ?x " (but of course the logic is a little more involved. You can have a look at it, but maybe this can also be a separate PR as I am currently also refactoring the RegexExpression to not get conflicts.
There was a problem hiding this comment.
And while i am at it : The startswith can probably also be implemented in that way.
Conformance check passed ✅No test result changes. |
|
| if (ptr) { | ||
| ptr->setPrefilterExpression(prefilterVec); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Some detailed comments here:
-
You may only pass on PrefilterExpressions to a child execution tree, if the variable of the expression is visible in the variable to column map. Otherwise you have the case that subqueries filter out too much.
-
It is fishy (and leads to unexpected behavior) if you just change the
Operationwithout informing theshared_ptr<ExecutionTree>that owns it.
So what you need, is aforThisExecutionTreeAndAllDescendents(shared_ptr) that completely replaces the shared_ptr with a completely new execution tree (viamake_sharedor respectivelyad_utility::createExecutionTree) that stores the IndexScan. Then you best have a function in the IndexScanClass that can do something likeshared_ptr makeCopyWithAddedPrefilters(Prefilters) const(we have something similar in theTransitivePathclass calledbindLeftOrRightSide`. -
You should make two PRs out of this, one for the extraction of the prefilters out of the expression (the first part of this PR), and then this second one here which is based on that one. Because then we can merge the first part earlier (which is further in the review process), and it gets much easier to review.
|
This has been merged already in several other smaller PRs. |
The
SparqlExpressionbase class has been extended with the methodgetPrefilterExpressionForMetadata. This method constructs for suitable (logical) expressions a correspondingPrefilterExpression(see PR #1503). ThosePrefilterExpression(s)preselect the relevant data blocks with the availableCompressedBlockMetadataover theIndexScanprocedure. This saves a lot of compute in the actual expression evaluation later on.At the moment, the following expressions provide an overriden implementation of
getPrefilterExpressionForMetadata:logical-orandlogical-and(binary),logical-not(unary) and the standardRelationalExpressions.