Implement `LiteralOrIri` #1186

greenBene · 2023-12-11T15:20:31Z

We plan for this class to be consistently used across all of QLever to represent values that cannot be folded into an ID directly (e.g. during the parsing, for local vocabs, expressions, etc.).
It is a strong type that stores the contents in a normalized way that makes it easy to work with and reason about.
This commit only introduces this type together with some unit tests, follow-up commits will roll it out across the codebase.

greenBene · 2023-12-11T15:38:29Z

Hi @joka921 ,

I finally got around to start working on the implementation of the LiteralOrIri class we spoke about last month.

This is so far just a draft and I would highly appreciate your opinion if the interface looks as you expected. The actual implementation is so far quite basic, and I wanted to introduce more logic once we are happy with the interface itself.

codecov · 2023-12-11T15:51:44Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (4ccc4d6) 87.29% compared to head (fdebba3) 87.52%.
Report is 7 commits behind head on master.

Files	Patch %	Lines
src/parser/LiteralOrIri.cpp	94.82%	2 Missing and 1 partial ⚠️
src/parser/RdfEscaping.cpp	96.77%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1186      +/-   ##
==========================================
+ Coverage   87.29%   87.52%   +0.22%     
==========================================
  Files         305      309       +4     
  Lines       27514    27911     +397     
  Branches     3104     3122      +18     
==========================================
+ Hits        24019    24429     +410     
+ Misses       2365     2347      -18     
- Partials     1130     1135       +5

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

sonarqubecloud · 2023-12-11T17:11:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
28 Code Smells

No Coverage information
0.0% Duplication

src/parser/LiteralOrIriType.h

* removing temporary main method used for testing

…ead of std::string

joka921

I have some initial comments.
Most of them are trivial cleanups which we can do ourselves.
For the deeper things (where does the parsing/normalization happen), that is something that we can discuss in person maybe next week.

src/parser/CMakeLists.txt

joka921 · 2024-01-19T14:45:35Z

src/parser/IriType.h

+  // Created a new iri object
+  explicit IriType(NormalizedString iri);
+
+  // Returns the string value of the iri object
+  [[nodiscard]] NormalizedStringView getIri() const;


The interface should probably be something like

static Iri parseFromRdf(std::string_view rdf); // takes "<someIriWithEscapingsEtc>" and calls the appropriate normalization from RdfsEsaping)" NormalizedStringView getNormalizedContent; // The normalized value without the <> std::string toRdf () // Redo the escaping and readd the <>. Typically not used so much, as we have dedicated exporting routines, but at least for debugging and printing this is useful.

just getContent() then it's the same as for the Literal class which makes the usage easier.

Okay after further thinking:
Maybe we mostly use the parser for the parsing and taking the NormalizedString directly is fine, but then you really really have to document whether this should be with or without the <>. And maybe warn if it starts or ends with those (theoretically you can escape them, but what are the odds?

So bottomline: It is important to maake it very clear when you get <> and when not.

src/parser/IriType.h

src/parser/LiteralType.h

joka921 · 2024-01-19T14:56:20Z

src/parser/LiteralType.h

+  // Returns true if the literal has an assigned datatype
+  bool hasDatatype() const;
+
+  // Returns the value of the literal, without any datatype or language tag


Also make it always clear whether you get the quotation marks.
I am still not sure the more I am thinking about this, whether we shouldn't doe the parsing directly inside these classes, then we can enforce more invariants about the " @ and <> characters.

Or at least make the constructor private and have only certain authorized parsers create such objects.
But we can discuss this at a later point.

joka921 · 2024-01-19T15:13:23Z

src/parser/LiteralOrIriType.cpp

+  if (!isIri()) {
+    AD_THROW(
+        "LiteralOrIriType object does not contain an IriType object and thus "
+        "cannot return it");
+  }


Also use `AD_CONTRACT_CHECK as soon as I've merged it with the additional message.

src/parser/LiteralOrIriType.cpp

joka921 · 2024-01-19T15:14:13Z

src/parser/LiteralOrIriType.cpp

+  if (!isLiteral()) {
+    AD_THROW(
+        "LiteralOrIriType object does not contain an LiteralType object and "
+        "thus cannot return it");
+  }


same comment about AD_CONTRACT_CHECK

src/parser/LiteralOrIriType.cpp

…uding tests

greenBene · 2024-01-19T15:48:44Z

@joka921 Thank you for all the comments! I'll add the suggested changes and missing documentation on Monday ! :)

I created a parser LiteralOrIriType LiteralOrIriType::fromStringToLiteralOrIri(std::stringview) and think that this should be the main way to create LiteralOrIriType from string. We can change the visibility of the constructors accordingly.

If you are available, I´d be happy for a call to discuss some open points :) I'll write you a mail for scheduling.

* Added documentation to Iri interface

* Made code more c++-esque * minor bug fixes

* removed unnecessary todo

greenBene · 2024-01-29T08:52:09Z

@joka921 Hi :) I implemented the changes as discussed on Monday. Does it now looks as expected?

The CI step measure-code-coverage / build fails and I don't quite understand why. If you have time the coming days could you maybe have look?

joka921

I am sorry, I did not send the review last week...
However this already looks very nice.
I will get back to you via Email.

src/parser/Literal.h

src/parser/LiteralOrIri.h

src/parser/Iri.h

src/parser/NormalizedString.h

src/parser/NormalizedString.cpp

test/parser/LiteralOrIriTypeTest.cpp

src/parser/NormalizedString.cpp

src/parser/RdfEscaping.cpp

joka921

This should be almost the last round,
we are converging.

joka921 · 2024-01-30T09:35:50Z

src/parser/Iri.cpp

+// __________________________________________
+Iri Iri::prefixed(const Iri& prefix, const std::string& suffix) {
+  return Iri{prefix, RdfEscaping::normalizeIriWithoutBrackets(suffix)};
+}


I think technically this might be subtely wrong, but that is due to preexisting bugs in corner cases of the escaping module, which are unrelated to this PR.
Most notably there actually is not much to "escape" for an iriref, but that requires some other changes by me.

In which way might this be subtly wrong?

src/parser/Literal.h

src/parser/LiteralOrIri.h

test/parser/CMakeLists.txt

test/parser/LiteralOrIriTest.cpp

…reated using the LiteralOrIri::literalWith[out]Quotes

greenBene · 2024-02-01T12:59:12Z

Hi @joka921

Thank you for fixing the subtle bug and for the review!

I implemented all the suggested changes, thank you especially for the suggestion how to handle the Literal creation functions! :)

Is there anything missing in this PR?

joka921

We are almost there, it is only very minor stuff:)

src/parser/Literal.h

src/parser/Literal.cpp

src/parser/LiteralOrIri.h

test/parser/LiteralOrIriTest.cpp

greenBene · 2024-02-02T09:06:40Z

Thank you! :) Changes are implemented

…ation' into literal-or-iri-type-implementation # Conflicts: # src/parser/Literal.cpp # src/parser/Literal.h # src/parser/LiteralOrIri.h # test/parser/LiteralOrIriTest.cpp

sonarqubecloud · 2024-02-02T16:30:27Z

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

So far, literals and IRIs were stored as a `std::string` (in a normalized way, that is, without escaping) and the distinction between literals and IRIs was made via the first character (`"` or `<`). The code to deal with datatypes (the stuff after `^^`) and language tags (e.g., `@en`) was also ad-hoc using low-level string operations. Now there are proper classes `Literal` and `Iri`, which internally still store their data as normalized strings starting with `"` or `<` (just like before), but they are now two different types and each with a proper interface. This continues work started with #1186. The new classes are used in the `TripleComponent` class, which is used by the Turtle and SPARQL parsers. As a consequence, a lot of code is affected by this change. The new classes are not yet used by `LocalVocab` and `ExportQueryExecutionTrees`.

Continue work started with #1186 (which introduced the class `LiteralOrIri`) and #1318 (which used `LiteralOrIri` instead of `std::string` in all SPARQL expressions). This change now uses `LiteralOrIri` instead of `std::string` whenever dealing with words from a `LocalVocab`.

greenBene marked this pull request as draft December 11, 2023 15:21

greenBene commented Dec 15, 2023

View reviewed changes

src/parser/LiteralOrIriType.h Outdated Show resolved Hide resolved

greenBene commented Dec 15, 2023

View reviewed changes

src/parser/LiteralOrIriType.h Outdated Show resolved Hide resolved

greenBene added 6 commits January 18, 2024 15:38

* Rebase

16c6904

* Rebase

66fd51c

* Formatting

bac405d

* Added test cases for draft implementation

d12a556

* Formatting

959636f

* Formatting of header

3524d3a

greenBene force-pushed the literal-or-iri-type-implementation branch from 6ee3e90 to 3524d3a Compare January 18, 2024 14:45

greenBene added 5 commits January 19, 2024 10:28

* basic implementation of NormalizedString

a8cebea

* fixed formatting

eca253d

* added warning

f326fae

* removing temporary main method used for testing

* Updated Literal, Iri, and LiteralOrIri to use NormalizedString inst…

8e52000

…ead of std::string

* tidying up and adding comments to interface

3c6153d

joka921 reviewed Jan 19, 2024

View reviewed changes

* created function to parse arbitrary string to LiteralOrIriType incl…

49d267d

…uding tests

greenBene added 9 commits January 22, 2024 07:43

* Removed unnecessary filenames from parser/CMakeLists

bd32608

* Removed "Type" suffix from new classes

e27f682

* Removed "Type" suffix from new classes

3b614fe

* Updated Iri interface to be more similar to Literal interface

9507c9d

* Added documentation to Iri interface

* Improved documentation

6ff0ecf

* Made code more c++-esque * minor bug fixes

* Removed unused code

e0372aa

* Fixed code smells reported by sonarcloud

bb8b089

* Adapted code formatting

f3ba1db

* Improved comments and variable naming

bd9b819

* formatting

d3e4e97

* removed unnecessary todo

joka921 requested changes Jan 29, 2024

View reviewed changes

greenBene and others added 3 commits January 29, 2024 17:52

* Implemented Review Feedback

e072cf4

Move the Literal type into a namespace.

f74084b

Move everything into a namespace.

2ca364a

joka921 requested changes Jan 30, 2024

View reviewed changes

greenBene added 4 commits February 1, 2024 11:09

* Unified Literal constructing functions

4aef562

* added new line in test/parser/CMakeLists.txt

0fb6684

* Added test to ensure the given literal is encoded if a literal is c…

72a9673

…reated using the LiteralOrIri::literalWith[out]Quotes

* Removed code duplication

74797f7

greenBene requested a review from joka921 February 1, 2024 12:51

joka921 requested changes Feb 1, 2024

View reviewed changes

greenBene and others added 5 commits February 1, 2024 18:02

* Used std::visit to avoid runtime AD_THROW

a5d0649

* Adapted comments

cb5155e

* switched from raw literals to standard literals

d91739e

* adapted documentation to avoid duplication

f91df95

Some small improvements for the interface.

7d1c9b8

joka921 added 2 commits February 2, 2024 10:08

Merge remote-tracking branch 'greenBene/literal-or-iri-type-implement…

fe42703

…ation' into literal-or-iri-type-implementation # Conflicts: # src/parser/Literal.cpp # src/parser/Literal.h # src/parser/LiteralOrIri.h # test/parser/LiteralOrIriTest.cpp

A tiny change.

fdebba3

joka921 changed the title ~~LiteralOrIriType Implementation~~ Implement LiteralOrIri Feb 2, 2024

joka921 approved these changes Feb 2, 2024

View reviewed changes

joka921 merged commit 9813a28 into ad-freiburg:master Feb 2, 2024
17 of 18 checks passed

hannahbast mentioned this pull request Mar 21, 2024

Own classes for both Literal and Iri #1301

Merged

hannahbast mentioned this pull request May 3, 2024

Use LiteralOrIri instead of std::string in LocalVocab #1333

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `LiteralOrIri` #1186

Implement `LiteralOrIri` #1186

greenBene commented Dec 11, 2023 •

edited by joka921

Loading

greenBene commented Dec 11, 2023

codecov bot commented Dec 11, 2023 •

edited

Loading

sonarqubecloud bot commented Dec 11, 2023

joka921 left a comment

joka921 Jan 19, 2024

joka921 Jan 19, 2024

joka921 Jan 19, 2024

joka921 Jan 19, 2024

joka921 Jan 19, 2024

joka921 Jan 19, 2024

joka921 Jan 19, 2024

joka921 Jan 19, 2024

greenBene commented Jan 19, 2024

greenBene commented Jan 29, 2024

joka921 left a comment

joka921 left a comment

joka921 Jan 30, 2024

greenBene Feb 1, 2024

greenBene commented Feb 1, 2024

joka921 left a comment

greenBene commented Feb 2, 2024

sonarqubecloud bot commented Feb 2, 2024

Implement LiteralOrIri #1186

Implement LiteralOrIri #1186

Conversation

greenBene commented Dec 11, 2023 • edited by joka921 Loading

greenBene commented Dec 11, 2023

codecov bot commented Dec 11, 2023 • edited Loading

Codecov Report

sonarqubecloud bot commented Dec 11, 2023

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

greenBene commented Jan 19, 2024

greenBene commented Jan 29, 2024

joka921 left a comment

Choose a reason for hiding this comment

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

greenBene commented Feb 1, 2024

joka921 left a comment

Choose a reason for hiding this comment

greenBene commented Feb 2, 2024

sonarqubecloud bot commented Feb 2, 2024

Quality Gate passed

Implement `LiteralOrIri` #1186

Implement `LiteralOrIri` #1186

greenBene commented Dec 11, 2023 •

edited by joka921

Loading

codecov bot commented Dec 11, 2023 •

edited

Loading