Output RDF/JS terms

[namedgraph]

@RubenVerborgh my partial attempt at #73. Is it something along the lines?

I'm not familiar with the setup, just stabbing in the dark here really. So I won't be able to fix the tests easily.

Also not sure if all of Literal grammar is covered (where createLiteral() is not called).
XSD_TRUE/XSD_FALSE constants might not be required anymore.

But otherwise I ran some queries through sparql-to-json and I'm getting what I think is expected output.

[RubenVerborgh]

@RubenVerborgh my partial attempt at #73. Is it something along the lines?

Looks good indeed!

I'm not familiar with the setup, just stabbing in the dark here really.

npm run build && npm run test

For the tests, you might want to inject a factory that gives the old output back; then you don't have to adjust any of the tests.

[namedgraph]

Updated. The approach mostly works, but there's a problem with literals, more specifically TestDataFactory.literal() (38 tests fail).

createLiteral() in JISON attempts to create a NamedNode for the datatype, but TestDataFactory.namedNode() returns string, and then when TestDataFactory.literal() is called it thinks that string is a lang tag and not a datatype.

Some hack is required there to distinguish between lang and datatype literals.

[RubenVerborgh]

then when TestDataFactory.literal() is called it thinks that string is a lang tag and not a datatype.

So can we change TestDataFactory.literal such that it recognizes things that are not lang tags?

[namedgraph]

I guess first we need to have 2 methods createdTypedLiteral()/createLangLiteral() in JISON. Because this rule

String LANGTAG  -> $1 + lowercase($2)

also needs wrapping into RDF/JS structure, correct? And current createLiteral() won't work because it only deals with typed literals.
I haven' done this yet.

[RubenVerborgh]

Can't we just delegate to Parser.factory.literal? That will do the right thing, given the right argument types. We'd only have to cheat a little bit for the tests, but that's okay.

Thanks for all the work so far!

[RubenVerborgh]

No need to create new literals every time; we can just reuse constants.

[namedgraph]

I've updated one more time, hopefully now it's easier to see the issue.

See how now there are 2 methods in JISON for literals: createLangLiteral() and createTypedLiteral().

The core issue is that both of them call Parser.factory.literal() with string as the 2nd argument:

• createLangLiteral() with a "real" string, the lang tag
• createTypedLiteral() with a string which is the result of NamedNode becoming a string in the TestDataFactory, but really means a datatype

Does that make sense? I don't see a very straightforward fix here, that can differentiate between the two cases.

[RubenVerborgh]

test/TestDataFactory.js:

function literal(value, tag) {
  if (tag)
    tag = (tag.length < 10 ? '@' : '^^') + tag;
  return '"' + value + '"' + tag;
}

[RubenVerborgh]

👍 let me know if you’re stuck, then I can take it from there

[namedgraph]

👍 let me know if you’re stuck, then I can take it from there

I'm pretty much stuck now :)

[namedgraph]

After a few extra changes all 349 tests pass for me locally. But Travis still fails for some reason.
Please take over now :)

[RubenVerborgh]

Great job, thanks. Seems like you need n3 in package.json 😉

[namedgraph]

Which version of n3?

[RubenVerborgh]

1.0 or up

[namedgraph]

@RubenVerborgh so what does this still need in order to be merged?

[namedgraph]

The TypeScript bindings probably need updating too?
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/sparqljs/index.d.ts

[RubenVerborgh]

@RubenVerborgh so what does this still need in order to be merged?

An hour or two on my side to check everything; planning that this week. Thanks for the work!

[AlexeyMz]

I think I'd have rather seen an object (possibly singleton object), but I can live with it if you can.

Ah yes, I agree with going for the object solution. Currently the code contains quite a bit of checks such as if (expr === "*" || isTerm(expr)), which could be simplified if * was a superclass of Term as was discussed above.

I would strongly suggest to reconsider this for several reasons:

1. This would needlessly break backwards compatibility which means many users will stuck on previous version.
2. Switching to RDFJS by itself breaks ability to trivially serialize query AST to JSON, but it's easy to workaround. Introducing another special term makes it even harder to keep this working.
3. Object identities (including instanceof checks) does not work across JS realms, e.g. it's really cumbersome to check if an object has specific type if you pass objects to another browser frame, send it to Worker/MessageChannel or just have multiple library versions at the same time. Basically this is the reason why it's recommended to call Array.isArray(arr) instead of arr instanceof Array.

[RubenVerborgh]

• This would needlessly break backwards compatibility which means many users will stuck on previous version.

We are breaking backward compatibility in any case; this is a semver major change.

• Switching to RDFJS by itself breaks ability to trivially serialize query AST to JSON, but it's easy to workaround. Introducing another special term makes it even harder to keep this working.

It would behave the same way as a term, so serialization to JSON would be thee same.

• Object identities (including instanceof checks) does not work across JS realms

Agreed, so there should not be a reliance on object identity; but rather on x.termType === 'x' or x.isWildcard === true or similar.

I think homogeneity (all objects, instead of some objects and some strings) weighs more strongly.

[AlexeyMz]

@rubensworks I agree, it should be OK to use a non-identity comparison.

For consistency with other nodes it may look as simple as this (in TypeScript syntax):

interface WildcardProjection {
  type: 'wildcard';
}

interface SelectQuery extends BaseQuery {
    queryType: 'SELECT';
    variables: Variable[] | WildcardProjection;
    // ...
}

[RubenVerborgh]

👍 but with termType rather than type

[AlexeyMz]

May I also suggest to add type constants to other node types as well?

Here are the current typings: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d2d3a685fe949a4e938fe2913597e49d1e703ef5/types/sparqljs/index.d.ts

It would be great to make these changes:

1. add type equal to grouping for Grouping, ordering for Ordering, variable for VariableExpression;
2. change updateType property to type in InsertDeleteOperation (currently half operations use updateType and the other one just type).

This will simplify query AST traversal/rewriting, e.g. like here: https://github.com/researchspace/researchspace/blob/master/metaphactory/web/src/main/api/sparql/QueryVisitor.ts

[RubenVerborgh]

@AlexeyMz Tracking in #88.

I now see that type was coming from sparqljs nodes, not from RDF/JS terms. Wildcard probably can use both.

[simonvbrae]

@RubenVerborgh What does this mean for the implementation of Wildcard? Currently Wildcard has termType only

[RubenVerborgh]

@simonvbrae termType is fine; rest will be followed up in #88.

[RubenVerborgh]

@simonvbrae All good from your side?

[simonvbrae]

@RubenVerborgh Wildcard support has been added, as far as I'm concerned this is ready for review and possibly to merge.

[RubenVerborgh]

Good stuff, just minor nits. You're almost off the hook, @simonvbrae 😉

Stale.

Stale.

[RubenVerborgh]

Thanks a lot! Will be merged, pending approval by @rubensworks.

[rubensworks]

Looks good to me!
Only a few minor comments.
Can be merged after those have been resolved.

[rubensworks]

Looks ready to merge! :-)

[RubenVerborgh]

Thanks all 🎉

[RubenVerborgh]

v3.0.0-beta.0 is out!