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

Function parameters #13

Open
th1j5 opened this issue Apr 28, 2020 · 12 comments
Open

Function parameters #13

th1j5 opened this issue Apr 28, 2020 · 12 comments

Comments

@th1j5
Copy link
Contributor

th1j5 commented Apr 28, 2020

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
Copy link

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
Copy link
Contributor Author

th1j5 commented Nov 13, 2020

@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
Copy link
Contributor

vemonet commented Aug 11, 2021

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: vemonet@35849bc

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
Copy link
Collaborator

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
Copy link
Contributor

vemonet commented Aug 11, 2021

Hi @ThibaultGerrier I added support for the legacy behavior: vemonet@faf23d3

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
Copy link
Collaborator

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
Copy link
Contributor

vemonet commented Aug 11, 2021

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 #23

@ThibaultGerrier
Copy link
Collaborator

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
Copy link
Contributor

vemonet commented Aug 11, 2021

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
Copy link
Collaborator

Sounds good to me! Ill try out the changes and merge your PR 👍

@ThibaultGerrier
Copy link
Collaborator

Alright merged and deployed with [email protected]
Thanks @vemonet !

@vemonet
Copy link
Contributor

vemonet commented Aug 12, 2021

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 #24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants