Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Base IRI to objects with relative IRIs #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Base IRI to objects with relative IRIs #45

wants to merge 1 commit into from

Conversation

marioscrock
Copy link

Hi everyone,
the R2RML specification says the base IRI is added to a term if the term map's term type is rr:IRI and the IRI is relative. As far as I understand, in the current version, this behavior is implemented only with respect to subjects so I modified the Executor to behave similarly also for named objects. Please let me know if the behavior described is correct and, eventually if the proposed changes can be the right way to introduce the modification. I tested it and it seems to behave as expected.

@andimou
Copy link

andimou commented Jan 7, 2021

@marioscrock thank you for your remark. I will let one of the other guys look into the implementation but I will just add a clarification following your comment:

There are actually multiple ways of generating an IRI in RML and we need to make sure that the base IRI is properly handled in all cases. I think the table in this chapter: http://ebooks.iospress.nl/volumearticle/56006 has a very detailed table with all possible cases.

@DylanVanAssche
Copy link
Contributor

Hi @marioscrock

Apologies for the long response time!
I agree with your approach but we need to extend some parts of it to fully comply with the [R2]RML spec.

When generating an absolute IRI from a relative one, a string concat is used, but this may not always be correct as
the R2RML spec says that we can only concatenate the base IRI with the relative one if the following conditions are met:

image

As @andimou mentioned, we need to take into account all possible cases for this:

image

Your patch only does this for objects from Object Maps, while the vanilla RMLMapper does it only for subjects of Subject Maps.
We may need to extend this so it happens for any R2RML Term Map as mentioned in the R2RML spec:

image

In short:

  • We should check these conditions first before doing that, if there's one that fails, we can print a warning.
  • We need to handle all possible cases for IRI generation.
  • We need to create a set of tests to verify this behavior.

Please let us know what you think :)

@DylanVanAssche DylanVanAssche added the bug Something isn't working label Oct 7, 2021
@marioscrock
Copy link
Author

marioscrock commented Nov 8, 2021

Thank you @DylanVanAssche and @andimou for your reply and very detailed explanation. I think this pull request is associated with a quite old version of the source code, so if you prefer we can close it and maybe add an issue on this topic.
Some comments on the proposed patch:

  • I assumed isValidIRI performed the checks mentioned
  • To avoid the problem of dealing with different Term Maps and term types, I added the patch before writing the triples in the output store and checking only for NamedNodes. However, I agree that a set of tests should be defined to check that all the possible cases are handled correctly (relevant also for https://rml.io/test-cases/?)

@DylanVanAssche
Copy link
Contributor

DylanVanAssche commented Nov 16, 2021

@marioscrock
Yes, the validation of IRIs is verified using isValidIRI but does not enforce the R2RML conditions mentioned in the spec to easily concat them.

Indeed, we need a set of test cases first, we can discuss that and add them at https://github.com/kg-construct/rml-test-cases-support :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants