Fix language filter with UPDATE #2461
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2461 +/- ##
==========================================
- Coverage 91.20% 91.20% -0.01%
==========================================
Files 473 473
Lines 40233 40312 +79
Branches 5378 5387 +9
==========================================
+ Hits 36695 36765 +70
- Misses 2006 2007 +1
- Partials 1532 1540 +8 ☔ 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.
A first round of 1-1 reviews.
src/index/DeltaTriples.cpp
Outdated
| auto predicate = ExportQueryExecutionTrees::idToLiteralOrIri( | ||
| index_, predicateId, localVocab_); |
There was a problem hiding this comment.
Best do some deduplication, don't look up the same predicate n times...
There was a problem hiding this comment.
A question and a change from me for the review.
My understanding is that for triples where the object has a language tag the internal permutations stores the that triple with the predicate also having the language tag. So simplified for <foo> <bar> "baz"@en" something like <foo> <bar>@en@ "baz" is stored. Is this used for anything else than FILTER(LANG(?x) = "...")?
| LocatedTriples::iterator& | ||
| DeltaTriples::LocatedTripleHandles<internal>::forPermutation( | ||
| Permutation::Enum permutation) { | ||
| return handles_[static_cast<size_t>(permutation)]; |
There was a problem hiding this comment.
Does this assumes that the internal permutations PSO and POS are first and second in the enum for the index into the array to work out?
Qup42
left a comment
Qup42
left a comment
There was a problem hiding this comment.
- Once the timing statements are added we can investigate the performance impact of the changes.
| ql::ranges::for_each(internalLocatedTriples_, | ||
| &LocatedTriplesPerBlock::updateAugmentedMetadata); |
There was a problem hiding this comment.
Probably more a note to myself. To reduce the performance impact it would be nice to have a way to only update the metadata of the internal permutation if they have actually been changed. How important this is also depends on how large the internal permutations are compared to the external permutations.
| auto optionalLiteralOrIri = ExportQueryExecutionTrees::idToLiteralOrIri( | ||
| index_, objectId, localVocab_); |
There was a problem hiding this comment.
(Some thinking aloud) We start with TripleComponents from the parser, lookup their IDs and now resolve the IDs back to the literal Values. That is a bit strange. But I also don't see an obvious place to do this where everything is present in TripleComponents. Variables are only bound to the value from the result once everything has been translated to IDs.
| AD_CORRECTNESS_CHECK(predicate.has_value() && predicate.value().isIri()); | ||
| auto specialPredicate = ad_utility::convertToLanguageTaggedPredicate( | ||
| predicate.value().getIri(), | ||
| std::string{ |
There was a problem hiding this comment.
Is it possible to make this parameter of ad_utility::convertToLanguageTaggedPredicate a std::string_view?
| void DeltaTriples::insertTriples(CancellationHandle cancellationHandle, | ||
| Triples triples, | ||
| ad_utility::timer::TimeTracer& tracer) { | ||
| auto internalTriples = makeInternalTriples(triples); |
There was a problem hiding this comment.
| auto internalTriples = makeInternalTriples(triples); | |
| tracer.beginTrace("makeInternalTriples"); | |
| auto internalTriples = makeInternalTriples(triples); | |
| tracer.endTrace("makeInternalTriples"); |
| modifyTriplesImpl(cancellationHandle, std::move(triples), true, | ||
| triplesInserted_, triplesDeleted_, tracer); | ||
| modifyTriplesImpl(std::move(cancellationHandle), std::move(internalTriples), | ||
| true, internalTriplesInserted_, internalTriplesDeleted_, | ||
| tracer); |
There was a problem hiding this comment.
| modifyTriplesImpl(cancellationHandle, std::move(triples), true, | |
| triplesInserted_, triplesDeleted_, tracer); | |
| modifyTriplesImpl(std::move(cancellationHandle), std::move(internalTriples), | |
| true, internalTriplesInserted_, internalTriplesDeleted_, | |
| tracer); | |
| tracer.beginTrace("externalPermutation"); | |
| modifyTriplesImpl(cancellationHandle, std::move(triples), true, | |
| triplesInserted_, triplesDeleted_, tracer); | |
| tracer.endTrace("externalPermutation"); | |
| tracer.beginTrace("internalPermutation"); | |
| modifyTriplesImpl(std::move(cancellationHandle), std::move(internalTriples), | |
| true, internalTriplesInserted_, internalTriplesDeleted_, | |
| tracer); | |
| tracer.endTrace("internalPermutation"); |
The same for deleteTriples. Some deduplication of these functions would be nice at this point.
Overview
Conformance check passed ✅No test result changes. |
|
3 participants
This PR extends the
DeltaTriplesclass to also store changes for internal triples. It also implements a hook that adds internal triples for every object literal with language literals so that these internal triples are filled when required. This does not include theql:langtagtriples, these need a little bit more consideration.Currently WIP, unit tests are missing and some code might not be ideal yet.