Correct datatypes for string expressions #1636

DuDaAG · 2024-11-22T11:21:09Z

Ich weiß leider immer noch nicht, wie ich ich zum Testen ein Literal mit Datatype erzeugen kann.

Also in Zeile ExportExecutionTreesTest.cpp Zeile 1678 + Zeile 1686.

Gibt es hier in Github die Möglichkeit, dass ich zu einzelnen Codestellen Kommentare (Fragen an dich) einfüge, damit es übersichtlicher ist. Ich habe die Funktion nur in den einzelnen Commits gefunden, aber bis man die dann findet. Also wie mache ich es am besten?

Co-authored-by: Johannes Kalmbach <[email protected]>

Update from original Qlever

codecov · 2024-11-22T11:58:40Z

Codecov Report

Attention: Patch coverage is 78.40909% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.21%. Comparing base (6fba76f) to head (52ef1f5).
Report is 12 commits behind head on master.

Files with missing lines	Patch %	Lines
src/engine/ExportQueryExecutionTrees.cpp	81.81%	3 Missing and 7 partials ⚠️
...e/sparqlExpressions/SparqlExpressionValueGetters.h	33.33%	6 Missing ⚠️
src/engine/sparqlExpressions/StringExpressions.cpp	85.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1636      +/-   ##
==========================================
- Coverage   89.25%   89.21%   -0.05%     
==========================================
  Files         372      374       +2     
  Lines       34818    35768     +950     
  Branches     3931     4044     +113     
==========================================
+ Hits        31076    31909     +833     
- Misses       2470     2548      +78     
- Partials     1272     1311      +39

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

joka921 · 2024-11-22T19:51:17Z

Hi, You can above click at Files changed, then you get the changed files (by default from all commits, which is fine for what you want), then you can select rows and attach comments to those. Make sure to finish your(self-review) if the comments are marked as "pending", because otherwise we cannot see them.

joka921 · 2024-11-22T20:20:23Z

test/ExportQueryExecutionTreesTest.cpp

+    auto resultLiteral = ExportQueryExecutionTrees::idToLiteralOrIri(
+        qec->getIndex(), id, LocalVocab{});
+    EXPECT_EQ(resultLiteral.value().toStringRepresentation(),
+              "\"some^^<http://www.w3.org/2001/XMLSchema#string>\"");


You have a typo here, the some must be in quotes so "\"some\"^^<http...>" ("some"^^ without the surrounding technical quotes). Please don't forget to also adapt this in your input kg.

joka921 · 2024-11-22T20:21:31Z

test/ExportQueryExecutionTreesTest.cpp

+              "\"some^^<http://www.w3.org/2001/XMLSchema#string>\"");
+  }
+
+  // TODO: Case Literal With Datatype not equal String


Exactly the same thing, use "\"some\"^^<http://www.dadudeldu.com/NoSuchDatatype>", It was only a typo in the syntax of your literals with the escaping..

sparql-conformance · 2024-11-24T16:11:33Z

Conformance check passed ✅

Test Status Changes 📊

Number of Tests	Previous Status	Current Status
2	Failed	Intended

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=52ef1f59404472318302664cdde0a3b45e97d91f&prev=5f28e832043b22102ad6e6e2a90bf93ff6846169

sonarcloud · 2024-11-24T17:49:55Z

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 on everything but the tests.
Work on my comments, and contact me once you are done or are left with questions.

joka921 · 2024-11-25T10:50:53Z

src/engine/ExportQueryExecutionTrees.h

+  // Converts an Id to a LiteralOrIri based on its type and value.
+  // For VocabIndex or LocalVocabIndex: Return Literal or Iri. If
+  // `onlyReturnLiteralsWithXsdString` is true, return only literals (no IRIs)
+  // with no datatype or datatype `xsd:string`; otherwise, return any literal,
+  // but strip datatypes other than `xsd:string`. For Double, Int, Bool, Date,
+  // or GeoPoint: Return the literal without the datatype. If
+  // `onlyReturnLiteralsWithXsdString` is true return `std::nullopt`. For
+  // Undefined Id: Always return `std::nullopt`


This comment can be shortened to be more concise and precise:

You never return the datatypes, (always strip them) unless they are xsd:string, so if the literal has any other datatype (this includes IDs that directly store their value, like Doubles), the datatypes are always empty.

If the onlyReturn... is set, then all IRIs and all literals that have a datatype that is not string (again including the encoded IDs) becomenullopt. Then also add, that these semantics are useful for the string expressions in StringExpressions.cpp`.

joka921 · 2024-11-25T10:52:15Z

src/engine/ExportQueryExecutionTrees.h

+  // thrown.
+  // If `onlyReturnLiteralsWithXsdString` is `true`, returns `std::nullopt`.
+  // If `onlyReturnLiteralsWithXsdString` is `false`, removes datatypes from
+  // literals (e.g., `42^^xsd:integer` becomes `"42"`).


Suggested change

// literals (e.g., `42^^xsd:integer` becomes `"42"`).

// literals (e.g. the integer `42` is converted to the plain literal `"42"`).

joka921 · 2024-11-25T10:53:08Z

src/engine/ExportQueryExecutionTrees.h

+  static std::optional<LiteralOrIri> idToLiteralOrIriForEncodedValue(
+      Id id, bool onlyReturnLiteralsWithXsdString = false);
+
+  // Checks and processes a LiteralOrIri based on the given flags.


Suggested change

// Checks and processes a LiteralOrIri based on the given flags.

// A helper function for the `<InsertNameHere>` function. ... remainder of comment.

joka921 · 2024-11-25T10:54:19Z

src/engine/ExportQueryExecutionTrees.cpp

+ExportQueryExecutionTrees::idToLiteralOrIriForEncodedValue(
+    Id id, bool onlyReturnLiteralsWithXsdString) {
+  auto optionalStringAndType = idToStringAndTypeForEncodedValue(id);
+  if (!optionalStringAndType || onlyReturnLiteralsWithXsdString) {


First check for onlyReturn before you call the somewhat expensive idTo... function.

joka921 · 2024-11-25T10:56:46Z

src/engine/ExportQueryExecutionTrees.cpp

+           std::string_view(
+               reinterpret_cast<const char*>(word.getDatatype().data()),
+               word.getDatatype().size()) == XSD_STRING;


Isn't this exactly the asStringViewUnsafe(word.getDatatype()) == XSD_STRING function?

joka921 · 2024-11-25T11:02:56Z

src/engine/sparqlExpressions/SparqlExpressionValueGetters.h

+// Value getter for `isBlank`.
+struct IsBlankNodeValueGetter : Mixin<IsBlankNodeValueGetter> {
+  using Mixin<IsBlankNodeValueGetter>::operator();
+  Id operator()(ValueId id, const EvaluationContext*) const {
+    return Id::makeFromBool(id.getDatatype() == Datatype::BlankNodeIndex);
+  }


Is this an artifact of you not having merge in the master branch for quite some time?

joka921 · 2024-11-25T11:06:18Z

src/engine/sparqlExpressions/SparqlExpressionValueGetters.h

+struct LiteralOrIriValueGetter : Mixin<LiteralOrIriValueGetter> {
+  using Mixin<LiteralOrIriValueGetter>::operator();
+
+  std::optional<LiteralOrIri> operator()(ValueId,
+                                         const EvaluationContext*) const;
+
+  std::optional<LiteralOrIri> operator()(const LiteralOrIri& s,
+                                         const EvaluationContext*) const {
+    return s;
+  }
+};


You also need a second valueGetter which has the semantics of the onlyReturnXsdStrings.
(so one value getter for each of your configurations of the configurations of your functions in ExportQueryExecutionTrees.cpp.

joka921 · 2024-11-25T11:07:58Z

src/engine/sparqlExpressions/SparqlExpressionValueGetters.cpp

+std::optional<LiteralOrIri> LiteralOrIriValueGetter::operator()(
+    Id id, const EvaluationContext* context) const {
+  // true means that immediately returns nullopt for everything that is not a
+  // literal


The comment is out of sync. it says true means but the only thing I see in the call is a false.

joka921 · 2024-11-25T11:11:17Z

src/engine/sparqlExpressions/StringExpressions.cpp

+    if (s->isLiteral()) {
+      if (s->hasLanguageTag()) {
+        descriptor = std::string(asStringViewUnsafe(s->getLanguageTag()));
+      } else if (s->hasDatatype()) {
+        descriptor =
+            ad_utility::triple_component::Iri::fromIrirefWithoutBrackets(
+                asStringViewUnsafe(s->getDatatype()));
+      }
+    } else {


It would be more efficient ( you get your own copy of the LiteralOrIri after all), to not create a new LiteralOrIri,
But to implement the SubStr directly inside the Literal class, s.t. you can reuse the allocation etc, this should make things somewhat faster. So a method Literal::setSubtr(...) would be the way to go.

joka921 · 2024-11-25T11:13:27Z

src/engine/sparqlExpressions/StringExpressions.cpp

+using SubstrExpression = NARY<3, FV<SubstrImpl, LiteralOrIriValueGetter,
+                                    NumericValueGetter, NumericValueGetter>>;


To make the case SUBSTR(STR(?something)) more efficient, you can implement something like the StringExpressionImpl, which does exactly the same thing, but uses your two value getters (the one is still missing), depending on whether the child is a string or not.

DuDaAG and others added 13 commits October 24, 2024 11:18

STRLEN für UTF 8 angepasst

47feb71

Test

4877e8f

test rückgäning

aefd889

find pull-request

b2cb8c6

Fix test

96b1959

Update src/engine/sparqlExpressions/StringExpressions.cpp

b7806b8

Co-authored-by: Johannes Kalmbach <[email protected]>

Update src/engine/sparqlExpressions/StringExpressions.cpp

a83d74b

Co-authored-by: Johannes Kalmbach <[email protected]>

Format

e73d1ab

new LiteralOrIriValueGetter

0ed79df

Merge branch 'my-branch' into master

2666b78

Merge pull request #3 from DuDaAG/master

27fff04

Update from original Qlever

idToLiteralAndIri with specifiactions

7078f60

some fixes

2581f4c

joka921 requested changes Nov 22, 2024

View reviewed changes

Annika Greif added 3 commits November 24, 2024 11:58

Add Test IdToLiteralOrIri and some formatting

6d7a2b2

formatting

5948dcb

Correction for sonar

52ef1f5

joka921 requested changes Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct datatypes for string expressions #1636

Correct datatypes for string expressions #1636

DuDaAG commented Nov 22, 2024

codecov bot commented Nov 22, 2024 •

edited

Loading

joka921 commented Nov 22, 2024 •

edited

Loading

joka921 Nov 22, 2024

joka921 Nov 22, 2024

sparql-conformance bot commented Nov 24, 2024

sonarcloud bot commented Nov 24, 2024

joka921 left a comment

joka921 Nov 25, 2024

joka921 Nov 25, 2024

joka921 Nov 25, 2024

joka921 Nov 25, 2024

joka921 Nov 25, 2024

joka921 Nov 25, 2024

joka921 Nov 25, 2024

joka921 Nov 25, 2024

joka921 Nov 25, 2024

joka921 Nov 25, 2024

	// literals (e.g., `42^^xsd:integer` becomes `"42"`).
	// literals (e.g. the integer `42` is converted to the plain literal `"42"`).

	// Checks and processes a LiteralOrIri based on the given flags.
	// A helper function for the `<InsertNameHere>` function. ... remainder of comment.

		using SubstrExpression = NARY<3, FV<SubstrImpl, LiteralOrIriValueGetter,
		NumericValueGetter, NumericValueGetter>>;

Correct datatypes for string expressions #1636

Are you sure you want to change the base?

Correct datatypes for string expressions #1636

Conversation

DuDaAG commented Nov 22, 2024

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

joka921 commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparql-conformance bot commented Nov 24, 2024

Conformance check passed ✅

Test Status Changes 📊

sonarcloud bot commented Nov 24, 2024

Quality Gate passed

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 22, 2024 •

edited

Loading

joka921 commented Nov 22, 2024 •

edited

Loading