semantifyit / RocketRML

https://semantify.it
Creative Commons Attribution Share Alike 4.0 International
25 stars 12 forks source link

Function parameters #13

Open th1j5 opened 4 years ago

th1j5 commented 4 years ago

This is an issue to write down how this software can be improved in the future.

Now, when working with functions, the parameters are accessed like data[num] where num is the number of the occurrence in mapping.ttl. Since mapping.ttl is actually just a textual representation of triples and thus a graph, the order of these occurrences could be swapped without making a difference in any software reading this file.

It does however change the behaviour in the function (parameters are swapped).

This could probably be fixed by using named parameters, but this will require some diving in the function ontology. Further links: https://fno.io/rml/

If I could state this issue better, ask me (it's maybe not well worded). To be clear: this is future work and I personally probably won't use it after my project is done.

VladimirAlexiev commented 4 years ago

FNO also supports numbered (ordered) params: https://fno.io/spec/#fn-parameter. They are put in rdf:List to preserver the order:

ex:sumFunction a fno:Function;
    fno:expects ( ex:intParameterA ex:intParameterB ).
th1j5 commented 4 years ago

@VladimirAlexiev good to know! But since I'm not working on this anymore, I'll let this issue up to you and others (if this even needs 'handling', because -like you say - a rdf:List might fix it).

vemonet commented 3 years ago

Hi, I am facing the same problem and found it quite confusing the functions parameters cannot be accessed using the given predicate (especially that we might have some optional parameters, which can;t be handled with just the order)

I fixed it in a fork: https://github.com/vemonet/RocketRML/commit/35849bcaa8f270d168889f7c8ced48c29f37857b

It's quite a simple fix, but it changes the data parameter retrieved in the function from an array to an object {'predicate': 'paramValue'. So most existing functions will need to be rewritten to properly access their parameters using the given predicate (there are 8 tests related to functions failing)

Here is an example of retrieving the parameters when defining a RocketRML function (in index.js):

      functions : {
            'https://w3id.org/um/ids/rmlfunctions.ttl#string_split function (data) {
                const idfs = 'https://w3id.org/um/ids/rmlfunctions.ttl#'
                s = String(data[idfs + 'input'])
                split = data[idfs + 'split_on']
...

@ThibaultGerrier is there any motivation to integrate those changes to RocketRML? I could help fixing existing functions and tests

ThibaultGerrier commented 3 years ago

Hi, yes I think passing an object like that is a good and simple solution.

@vemonet could you create a PR with your changes?

vemonet commented 3 years ago

Hi @ThibaultGerrier I added support for the legacy behavior: https://github.com/vemonet/RocketRML/commit/faf23d3cfb4e8acebc92525bec8977df53c5d878

It's not an array but I am adding also entries with numbers (index) to the data object, so most previous functions are still supported

I also fixed 2 tests that were failing, but all tests now pass

I will do the PR asap

ThibaultGerrier commented 3 years ago

Unfortunately this doesn't work with array destructuring:

const obj = {
  0: 'foo',
  1: 'bar,
}
const [first, second] = obj; // throws error

which was what I was mostly doing with functions (in my mappings) :

functions : {
  'https://w3id.org/um/ids/rmlfunctions.ttl#append': function ([first, second]) {
     return first + second;
  }
}

This could be fixed by making the object iterable. something like: (works, but not sure how good this is)

result[Symbol.iterator] = function *() {
 for(let i=0;;i++) {
   if(result.hasOwnProperty(i)) {
     yield result[i];
   } else {
     break;
   }
 }
}

Another solution would be to just keep the result an array and add properties as if it were an object (which arrays are). That way stuff like iteration, .length & other array specific stuff would work out of the box.

const result = [];
parameters.forEach((p) => {
  ...
  result[p.predicate] = temp;
  result.push(temp)
});

This is probably the simplest solution that would still support legacy mappings/functions.

vemonet commented 3 years ago

const result = []; is a nice solution, but directly iterating will return each parameter twice out of the box no? You will need to filter on keys that are numbers I guess

I added it, and reverted the changes I made to the tests, all tests are now passing out of the box and you can use 'https://w3id.org/um/ids/rmlfunctions.ttl#append': function ([first, second]) {

Cf. pull request https://github.com/semantifyit/RocketRML/pull/23

ThibaultGerrier commented 3 years ago

Yes, I guess that's true, iterating with for..of would only give the numbered entries, but Object.(keys|values|entries)(arg) would return the values duplicated. Not sure if this is a big issue or how to solve it nicely.

Another possibility would be to just deprecate the old function usage, making a breaking change.

vemonet commented 3 years ago

As you want, you can keep compatibility with legacy functions in a first time, push the new use in the documentation, and remove the old function usage later (the change is quite small anyway)

ThibaultGerrier commented 3 years ago

Sounds good to me! Ill try out the changes and merge your PR :+1:

ThibaultGerrier commented 3 years ago

Alright merged and deployed with rocketrml@1.11.0 Thanks @vemonet !

vemonet commented 3 years ago

Thanks @ThibaultGerrier !

I faced another issue when using constant strings + reference variables in a function parameter, e.g. this person is: {FirstName} {LastName}

I fixed it and implemented the support for templates, cf. pull request https://github.com/semantifyit/RocketRML/pull/24