Reworked RDFEntity interface and added SemanticEntity #37

niniemann · 2018-07-17T16:13:53Z

This PR implements two major changes which build upon each other:

The RDFEntity has essentially been renamed to RDFVector. The RDFEntity-Class still exists, but is only used as an interface to containers of rdf triples. Every RDFEntity must be iterable, but can provide its own implementation of the iterator. Using this I also simplified the RDFPropertyMap, which had a really messed up structure as it had to store everything twice -- once in a map, once in the RDFEntitys triple-vector. Now the RDFPropertyMap only stores a map and constructs the triples on demand.
I implemented the SemanticEntity-class, which follows a completely different approach to provide RDF triples to the data: You can register member variables to make them directly visible, which allows you to use them as usual (type safety etc), without any casting involved. See the updated README for more information.

Of course there are still some limitations. One of them is the fact that you need to derive from SemanticEntity to make use of its features... This may be improved in the future.

Oh, and I'm also not quite satisfied with the RDFEntity-iterator-stuff. Both the RDFPropertyMap and the SemanticEntity do not store any actual Triples , but the iterator constructs them on demand, without storing them. I use a static variable in the iterators dereference-operator for that, but that means that the returned const Triple& is not really const! As soon as the iterator is incremented and dereferenced again, the static local variable changes its value... And when the iterator goes out of scope? 🙁

So, be careful with that, I haven't really figured out how to do this properly. Any ideas?
(Take a look at this part of the code to see what I mean).

niniemann · 2018-07-18T07:07:46Z

By the way, although this implementation differs from what I proposed previously, it fixes #31. The overhaul of RDFPropertyMap has removed the need for the workaround when deleting entries, and the SemanticEntity is the static alternative.

ctieben

First review comments.

ctieben · 2018-08-06T07:25:22Z

README.md

+
+Internally, for every registered property an instance of `RegisteredPropertyBase` is stored in a vector of the SemanticEntity. Those objects simply provide an interface to `virtual std::string object() const = 0;` which is provided by the (templated) derived classes `RegisteredProperty`. The `RegisteredProperty`s in turn have to major modifications:
+
+1. They also provide a `bool isValid() const;` which is implemented differently for `std::shared_ptr`, in which case they return if the property/variable/member is not null. In case of non-shared-ptrs it always returns true. This method is used to check if a registered property should be skipped on iteration, as a nullptr is (IMHO) best represented by the absence of a triple.


I would expect such a details description as code documentation not as general in the readme.

The code is well-documented, of course :D. Everything you read here is described in code-comments, too -- this is just a more structured explanation of how stuff works internally that can be read in one go.

Also, it is written beneath the header "Implementation details", so IMHO this is totally fine. 😉

ctieben · 2018-08-06T07:47:09Z

include/sempr/entity/RDFEntity.hpp

+class TripleIterator_impl {
+    friend class TripleIterator;
+protected:
+    virtual ~TripleIterator_impl();


Strange naming with *_impl and it implements nothing its just an interface.
Also the child's are named in this way :-(

The problem is that I want to return an iterator by value, and not a pointer to an iterator that points to some triples. The latter would force the user to write stuff like
(**it).subject; or *(*it)->subject and to delete the iterator in the end. If I'd just return a FooIterator, but the method signature in the RDFEntity class specifies a TripleIterator, the result would be a TripleIterator created from a FooIterator... not what I wanted. So my solution is to give the TripleIterator class a pointer to the actual implementation which has to be provided by the entity types that are RDF-iterable. The TripleIterator_impl is the interface for this implementation, which we need for the TripleIterator to work correctly (virtual methods...).

I agree that the naming of the actual implementations [...]_impl is a bit weird, but it simply follows the pattern. Do you have any suggestion how to improve this?

In this case I think the TripleIterator shall be renamed because its an abstract wrapper for the real iterators. It could be renamed to something like AbstractTripleIterator or TripleIteratorWrapper after these step the *_impl could be removed.

Agreed. I'm on it.

ctieben · 2018-08-06T07:51:58Z

include/sempr/entity/SemanticEntity.hpp

+namespace sempr {
+
+namespace rdf {
+namespace traits {


Is there no better place to define the rdf traits?

Maybe. Any suggestions?

Mhhh it could be moved to a Utils or Traits Header in a (not existing) rdf subdir.

In general all the RDF specific entities could be moved in a entity/rdf subdir. What do you think?

In principle: Yes. But this leads to changes in everything that uses sempr, again... Maybe postpone it until we do a complete overhaul of the structure? I'm not really happy with the overall setup of directories and namespaces...

For now I'm fine with those traits being defined in the SemanticEntity.hpp, since only the SemanticEntity uses them.

Ok for now its fine.

ctieben · 2018-08-06T10:55:19Z

src/entity/RDFPropertyMap.cpp

+
+const Triple& RDFPropertyMapIterator_impl::operator*() const
+{
+    static Triple t;


Why static?
t is changed by every call. It also could be non static and non reference to get a copy of the created object.

Because then the operator ->() const wouldn't work anymore.

ctieben · 2018-08-06T10:58:53Z

src/entity/RDFPropertyMap.cpp

+
+const Triple* RDFPropertyMapIterator_impl::operator->() const
+{
+    return &(**this);


And now it gets dirty. You will always return the address to the static local variable with this.

It works. And it's a static variable. So what? ;D
I agree that this is a bit ugly. Maybe I was way too focused on creating an iterator that I favored the signatures over sensible implementation.

niniemann · 2018-08-07T09:27:57Z

I've updated the TripleIterator to return triples by value instead of reference. I'm still open for naming-suggestions, but wouldn't mind to leave it as it is. 😉

ctieben

I have only found some out commeted stuff in a second review.

ctieben · 2018-08-07T11:33:09Z

src/entity/RDFVector.cpp

+{
+}
+
+// std::vector<Triple>::const_iterator RDFVector::begin() const


Could this begin() and end() be removed? (also in the header file)

ctieben · 2018-08-07T11:35:47Z

include/sempr/entity/example/PropertyTestEntity.hpp

+    // RDFProperty<std::string> stringValue_;
+    // RDFProperty<Person::Ptr> entityValue_;
+
+    // TODO test composite value


Do you still like to test it? :-)

Composite values are... not so straight forward. I'll leave this for a future extension, just like vectors. So the answer is: No. ^^

Will you create a issue for this topic?

ctieben

I found a small thing in the 'TripleIteratorWrapper'.

ctieben · 2018-08-07T13:17:18Z

src/entity/RDFEntity.cpp

 {
-    return triples_.begin();
+    if (!impl_)
+        throw std::exception();


Didn't you trust yourself?
The impl_ point could be stored as const in the wrapper and the c'tor can check the null expr.
These functions could also be declared as inline (in a future release).

Well... the TripleIteratorWrapper relies on the user providing him with a valid implementation. I don't know, it just felt right to check if I actually set it. Though this is probably a bit exaggerated... What do you want me to change? Just get rid of it? Or leave it as it is? Or do the check in the c'tor to fail more early?

My idea is to const the pointer and to have only one check in the c'tor. All other methods can than expect that the pointer is valid.

Like this? 4a6d8e4

niniemann added 8 commits July 11, 2018 10:56

reduced RDFEntity to be merely an interface for iterable RDF containers

ab32f8b

RDFVector replaces the old RDFEntity

d7bc716

simplified implementation of RDFPropertyMap

c8c264f

moved is_shared_ptr traits to Utility.hpp

3b69b78

added SemanticEntity

751b1df

added test for SemanticEntity (WIP)

b7cd6ae

minor simplification of PropertyTestEntity

3950dc8

SemanticEntity description in README

faf62fe

niniemann added enhancement feature labels Jul 17, 2018

niniemann mentioned this pull request Jul 18, 2018

added RDFValue::operator == (const T&) #38

Merged

Merge branch 'master' into rdfentity-interface

276ee9a

ctieben self-requested a review August 6, 2018 07:07

fixed include path in PropertyTestEntity.hpp

b75f0a2

ctieben reviewed Aug 6, 2018

View reviewed changes

TripleIterator returns 'const Triple' by value

372a14c

ctieben reviewed Aug 7, 2018

View reviewed changes

niniemann added 2 commits August 7, 2018 14:35

renamed TripleIterator_impl to TripleIterator

f6aedda

minor cleanups

97cfeb2

ctieben reviewed Aug 7, 2018

View reviewed changes

made iterator impl* const

4a6d8e4

ctieben approved these changes Aug 8, 2018

View reviewed changes

niniemann merged commit 958d77d into master Aug 8, 2018

niniemann deleted the rdfentity-interface branch October 1, 2018 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworked RDFEntity interface and added SemanticEntity #37

Reworked RDFEntity interface and added SemanticEntity #37

niniemann commented Jul 17, 2018

niniemann commented Jul 18, 2018

ctieben left a comment

ctieben Aug 6, 2018

niniemann Aug 6, 2018

ctieben Aug 7, 2018

ctieben Aug 6, 2018

niniemann Aug 6, 2018

ctieben Aug 7, 2018

niniemann Aug 7, 2018

ctieben Aug 6, 2018

niniemann Aug 6, 2018

ctieben Aug 7, 2018

niniemann Aug 7, 2018

ctieben Aug 7, 2018

ctieben Aug 6, 2018

niniemann Aug 6, 2018

ctieben Aug 6, 2018

niniemann Aug 6, 2018

niniemann commented Aug 7, 2018

ctieben left a comment

ctieben Aug 7, 2018

ctieben Aug 7, 2018

niniemann Aug 7, 2018

ctieben Aug 7, 2018

ctieben left a comment

ctieben Aug 7, 2018

niniemann Aug 8, 2018

ctieben Aug 8, 2018

niniemann Aug 8, 2018

ctieben Aug 8, 2018


		Internally, for every registered property an instance of `RegisteredPropertyBase` is stored in a vector of the SemanticEntity. Those objects simply provide an interface to `virtual std::string object() const = 0;` which is provided by the (templated) derived classes `RegisteredProperty`. The `RegisteredProperty`s in turn have to major modifications:

		1. They also provide a `bool isValid() const;` which is implemented differently for `std::shared_ptr`, in which case they return if the property/variable/member is not null. In case of non-shared-ptrs it always returns true. This method is used to check if a registered property should be skipped on iteration, as a nullptr is (IMHO) best represented by the absence of a triple.

Reworked RDFEntity interface and added SemanticEntity #37

Reworked RDFEntity interface and added SemanticEntity #37

Conversation

niniemann commented Jul 17, 2018

niniemann commented Jul 18, 2018

ctieben 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

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

niniemann commented Aug 7, 2018

ctieben 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

ctieben 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