-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get PrefilterExpression
from SparqlExpression
#1613
Get PrefilterExpression
from SparqlExpression
#1613
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1613 +/- ##
==========================================
- Coverage 89.25% 89.25% -0.01%
==========================================
Files 372 372
Lines 34818 35089 +271
Branches 3931 3971 +40
==========================================
+ Hits 31076 31317 +241
- Misses 2470 2482 +12
- Partials 1272 1290 +18 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A thorought round of reviews again,
I'll have a look at the linking issues, the rest should be self-explanatory.
//______________________________________________________________________________ | ||
// For this test we expect that no PrefilterExpression is available. | ||
TEST(SparqlExpression, getEmptyPrefilterFromSparqlRelational) { | ||
const Variable var = Variable{"?x"}; | ||
const Iri iri = I("<Iri>"); | ||
const Literal lit = L("\"lit\""); | ||
evalAndEqualityCheck(leSprql(var, var)); | ||
evalAndEqualityCheck(neqSprql(iri, var)); | ||
evalAndEqualityCheck(eqSprql(var, iri)); | ||
evalAndEqualityCheck(neqSprql(IntId(10), DoubleId(23.3))); | ||
evalAndEqualityCheck(gtSprql(DoubleId(10), lit)); | ||
evalAndEqualityCheck(ltSprql(VocabId(10), BoolId(10))); | ||
evalAndEqualityCheck(geSprql(lit, lit)); | ||
evalAndEqualityCheck(eqSprql(iri, iri)); | ||
evalAndEqualityCheck(orSprqlExpr(eqSprql(var, var), gtSprql(var, IntId(0)))); | ||
evalAndEqualityCheck(orSprqlExpr(eqSprql(var, var), gtSprql(var, var))); | ||
evalAndEqualityCheck(andSprqlExpr(eqSprql(var, var), gtSprql(var, var))); | ||
} | ||
|
||
//______________________________________________________________________________ | ||
// For the following more complex SparqlExpression trees, we also expect an | ||
// empty PrefilterExpression vector. | ||
TEST(SparqlExpression, getEmptyPrefilterForMoreComplexSparqlExpressions) { | ||
const Variable varX = Variable{"?x"}; | ||
const Variable varY = Variable{"?y"}; | ||
const Variable varZ = Variable{"?z"}; | ||
// ?x <= 10.00 OR ?y > 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please create a separate ...Test.cpp file for the tests that test the creation of the prefilterThings from SparqlExpressions.
2. Can you give the test suite a unique name that refers to the filename? It is hard to find tests when there is 4 different files where we have TEST(SparqlExpression, ...)
in them.
So overall there's (at least) three files:
- The tests for the SparqlExpressions (already present).
- The file for the Prefiltering without the expressions (this file before the PR + the clone and format and equal tests)
- The (new) file, that tests the combination of SparqlExpressions with the prefilterExpressions (test cases already there, so this shouldn't be too much work).
Conformance check passed ✅No test result changes. |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much.
The `SparqlExpression` base class has been extended with the method `getPrefilterExpressionForMetadata`. This method constructs for suitable (logical) expressions which are used inside a `FILTER` a corresponding `PrefilterExpression` (see PR ad-freiburg#1503). These `PrefilterExpression`s can be used to prefilter the blocks of an `IndexScan` by only looking at their metadata. At the moment, the following expressions provide an overriden implementation of `getPrefilterExpressionForMetadata`: `strstarts` (preliminary), `logical-or` and `logical-and` (binary), `logical-not` (unary) and the standard `RelationalExpressions (<, ==, >, <=, >=)`.
The
SparqlExpression
base class has been extended with the methodgetPrefilterExpressionForMetadata
. This method constructs for suitable (logical) expressions which are used inside aFILTER
a correspondingPrefilterExpression
(see PR #1503). ThesePrefilterExpression
s can be used to prefilter the blocks of anIndexScan
by only looking at their metadata.At the moment, the following expressions provide an overriden implementation of
getPrefilterExpressionForMetadata
:strstarts
(preliminary),logical-or
andlogical-and
(binary),logical-not
(unary) and the standardRelationalExpressions (<, ==, >, <=, >=)
.