Fix language filter with UPDATE #2461

RobinTF · 2025-10-23T15:49:03Z

This PR extends the DeltaTriples class 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 PR clashes with some changes in #2453

Currently WIP, unit tests are missing and some code might not be ideal yet.

codecov · 2025-10-23T17:03:08Z

Codecov Report

❌ Patch coverage is 93.27731% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.04%. Comparing base (c4a8efb) to head (0d8daae).

Files with missing lines	Patch %	Lines
src/index/DeltaTriples.cpp	93.58%	1 Missing and 4 partials ⚠️
src/engine/ExportQueryExecutionTrees.cpp	71.42%	2 Missing ⚠️
src/index/Permutation.h	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2461      +/-   ##
==========================================
- Coverage   91.47%   91.04%   -0.43%     
==========================================
  Files         466      466              
  Lines       47158    39479    -7679     
  Branches     5260     5268       +8     
==========================================
- Hits        43137    35943    -7194     
+ Misses       2520     2031     -489     
- Partials     1501     1505       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sonarqubecloud · 2025-10-23T17:26:12Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

joka921

A first round of 1-1 reviews.

joka921 · 2025-10-24T13:22:55Z

src/index/IndexImpl.cpp

+    if (loadInternalPermutation) {
+      setMetadata(permutation.internalPermutation());
+    }


This can be done directly by the permutation (if the permutation has an internal permutation, then it also registers that internal perm with the delta triples.

joka921 · 2025-10-24T13:23:17Z

src/index/IndexImpl.cpp

-  // `Permutation`class, but we first have to deal with The delta triples for
-  // the additional permutations.
-  // The setting of the metadata doesn't affect the contents of the delta
-  // triples, so we don't need to call `writeToDisk`, therefore the second
-  // argument to `modify` is `false`.


part of this comment should be kept.

joka921 · 2025-10-24T13:24:09Z

src/index/DeltaTriples.h

 using LocatedTriplesPerBlockAllPermutations =
    std::array<LocatedTriplesPerBlock, Permutation::ALL.size()>;
+using LocatedTriplesPerBlockInternalPermutations =
+    std::array<LocatedTriplesPerBlock, 2>;


This magic constant can also be computed.

Permutation::INTERNAL.size()

joka921 · 2025-10-24T13:26:10Z

src/index/DeltaTriples.h

  struct LocatedTripleHandles {
    using It = LocatedTriples::iterator;
-    std::array<It, Permutation::ALL.size()> handles_;
+    std::array<It, internal ? 2 : Permutation::ALL.size()> handles_;


Also no magic constant.

joka921 · 2025-10-24T13:26:28Z

src/index/DeltaTriples.h


    LocatedTriples::iterator& forPermutation(Permutation::Enum permutation);
  };
+  template <bool internal>


really the bool, or rather the number of permutations:?

joka921 · 2025-10-24T13:42:30Z

src/index/DeltaTriples.cpp

+    Id objectId = ids.at(2);
+    auto optionalLiteralOrIri = ExportQueryExecutionTrees::idToLiteralOrIri(
+        index_, objectId, localVocab_);
+    if (optionalLiteralOrIri.has_value() &&


if (! hasValue() ) continue;.

joka921 · 2025-10-24T13:43:18Z

src/index/DeltaTriples.cpp

+      auto predicate = ExportQueryExecutionTrees::idToLiteralOrIri(
+          index_, predicateId, localVocab_);


Best do some deduplication, don't look up the same predicate n times...

joka921 · 2025-10-24T13:47:26Z

src/index/DeltaTriples.cpp

+  modifyTriplesImpl(cancellationHandle, std::move(triples), true,
                    triplesInserted_, triplesDeleted_, tracer);
+  modifyTriplesImpl(std::move(cancellationHandle), std::move(internalTriples),
+                    true, internalTriplesInserted_, internalTriplesDeleted_,
+                    tracer);


can't this become part of mdofiyTriplesImpl (which then doesn't get the triplesInserted_ but retrieves them itself.?

Or a wrapper that reduces the code duplication.

joka921 · 2025-10-24T13:53:27Z

src/index/DeltaTriples.cpp

    return;
  }
-  auto toRange = [](const TriplesToHandlesMap& map) {
+  auto toRange = [](const TriplesToHandlesMap<false>& map) {


There is a discussion (at least a TODO) whether we should store the internal triples, instead of doing the (potentially expensive) backup when restarting.

joka921 · 2025-10-24T13:54:22Z

src/engine/sparqlExpressions/SparqlExpressionValueGetters.cpp

+  return ExportQueryExecutionTrees::idToLiteral(
+      context->_qec.getIndex().getImpl(), id, context->_localVocab);


This should be an additional overload, instead of having to update all the call sites.

Qup42

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 @en@ "baz"is stored. Is this used for anything else thanFILTER(LANG(?x) = "...")`?

Qup42 · 2025-10-30T12:29:56Z

src/index/DeltaTriples.cpp

+LocatedTriples::iterator&
+DeltaTriples::LocatedTripleHandles<internal>::forPermutation(
    Permutation::Enum permutation) {
  return handles_[static_cast<size_t>(permutation)];


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 · 2025-10-30T12:36:37Z

src/index/DeltaTriples.cpp

  ql::ranges::for_each(locatedTriples(),
                       &LocatedTriplesPerBlock::updateAugmentedMetadata);
+  ql::ranges::for_each(internalLocatedTriples_,
+                       &LocatedTriplesPerBlock::updateAugmentedMetadata);


Updating the metadata currently is somewhat (but not ludicrously) expensive. We'd only want to update it for the LocatedTriples that we actually changed. So here either locatedTriples() or internalLocatedTriples_. If there are no internal triples to update not at all.

Fix language filter update

0d8daae

RobinTF requested review from Qup42 and joka921 October 23, 2025 15:49

joka921 requested changes Oct 24, 2025

View reviewed changes

Qup42 mentioned this pull request Oct 29, 2025

Faster Vocab lookup during Update #2477

Open

Qup42 requested changes Oct 30, 2025

View reviewed changes

		auto predicate = ExportQueryExecutionTrees::idToLiteralOrIri(
		index_, predicateId, localVocab_);

		return ExportQueryExecutionTrees::idToLiteral(
		context->_qec.getIndex().getImpl(), id, context->_localVocab);

Fix language filter with UPDATE #2461

Are you sure you want to change the base?

Fix language filter with UPDATE #2461

Uh oh!

Conversation

RobinTF commented Oct 23, 2025

Uh oh!

codecov bot commented Oct 23, 2025

Codecov Report

Uh oh!

sonarqubecloud bot commented Oct 23, 2025

Quality Gate passed

Uh oh!

joka921 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Qup42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants