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

JENA-2065: RDF-star #951

Merged
merged 4 commits into from
Mar 10, 2021
Merged

JENA-2065: RDF-star #951

merged 4 commits into from
Mar 10, 2021

Conversation

afs
Copy link
Member

@afs afs commented Mar 9, 2021

This update RDF-star handling to the current state of the RDF-star group work.

@afs afs marked this pull request as ready for review March 9, 2021 12:07
@afs
Copy link
Member Author

afs commented Mar 9, 2021

Key points:

  1. If the query does not have any RDF-star related syntax, processing is the normal path. Each of the three RDF-star point (in-memory, TDB1, TDB2) starts with an "if" that sees whether RDF-star processing is needed.
  2. The test suite does run for general, TIM, TDB1, and TDB2 datasets if the redirection to RDF-star processing is off.
  3. Some processing interfaces have changed: additional to ExprVisitor, removals from algebra visit and transform (due to taking out the old RDF* mode behaviour). This partially accounts for the large number of files.

@kinow
Copy link
Member

kinow commented Mar 9, 2021

A good chance to try to read the latest w3c draft, go through some points, and see if I can get my Jena project to build and run from main locally. No worry if it's merged before 👍

@afs
Copy link
Member Author

afs commented Mar 9, 2021

The spec (WIP - live editors draft) is https://w3c.github.io/rdf-star/cg-spec/editors_draft.html

I'm sure detail will change.
Hopefully not too much because there is a reasonable sized and growing test suite.

@kinow
Copy link
Member

kinow commented Mar 9, 2021

The spec (WIP - live editors draft) is https://w3c.github.io/rdf-star/cg-spec/editors_draft.html

I'm sure detail will change.
Hopefully not too much because there is a reasonable sized and growing test suite.

I'm watching the w3c repository and trying to at least read each PR/issue while lurking. But it's moving quite fast, have some issue/PR to read pretty much every day in my notifications.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments, but no blockers. I will try to start Fuseki now, and see if I manage to load some data and try a few queries, but I might have to stop and do something else, and then would have time again to play with new code only in 2 days. So no problem if it's merged right now 👍

Thanks @afs!

case PNAME_NS:
case PNAME_LN:{
iri = iri();
o = createNode(iri, token.beginLine, token.beginColumn) ;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this auto-generated? If not maybe we can fix the indentation here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember we had a javacc grammar file somewhere I think, used to produce these files, but I didn't see the grammar file when I was sifting through the changes (I skip a few files that are not *.java, or that are tests, so I may have missed it).

jj_consume_token(-1);
throw new ParseException();
}
{if ("" != null) return o;}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

}
// Normally true.
public // For integration testing only
static final boolean DATAPATH = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 or should we make it into a Javadoc code so users don't use it? I remember someone asking about some code recently-ish in the mailing list that changed… I think the code was not supposed to be used externally, or maybe there was an alternative but it wasn't clear to the user.

return true;

// Deep substitute. This happens anyway as we walk structures.
// nPattern = Substitute.substitute(nPattern, input);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space here, but not important.

import org.apache.jena.sparql.util.Context;

/**
* For reference only: this code uses the triple index for matchign and it is PG mode..
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/matchign/matching (also extra period at the end of sentence?)

import org.apache.jena.sparql.graph.NodeTransform ;

/**
* RDF-start triple term in an expression.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/RDF-start/star (worth looking for RDF-start in the IDE if using one; that's one error that spell/grammar checkers would miss I guess)

dispatch.put(Tags.tagAdd, buildPlus);
dispatch.put(Tags.symMinus, buildMinus);
dispatch.put(Tags.tagSubtract, buildMinus);
dispatch.put(Tags.tagMinus, buildMinus); // Not to be confused with Op for SPARQL MINUS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked up at the Tags class to see if there was an Tags.tagPlus… just because I thought that as symMinus is for symPlus, and tagSubtract is for tagAdd, then tagMinus would be for tagPlus (there's even an unary plus) 😆

*/

public class FmtUtils
{
// OLD CODE - being replaced by riot.NodeFmtLib
// See alsoriot.NodeFmtLib
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space? Not really important

return execute(ntt, graph.getGraphName(), pattern, input, filter, execCxt);
}

/** Non-reordering execution of a quad pattern, given a iterator of bindings as input.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a/a iterator/an iterator

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another global search done ...

default:
throw new TDBException("Index is not recognized: "+idx);
throw new InternalErrorException("Tuple of unknow length");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/unknow/unknown

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's "uck" that there is code copy on but unifying TDB1 into DBOE/TDB2 framework risks destablizing the code.

Copying the code, so it is just import changes, is slightly nudging them together. But in nearby classes the code is appreciably different.

"Sometime".

@afs
Copy link
Member Author

afs commented Mar 10, 2021

Thank for the comments!

@afs afs merged commit 686a200 into apache:main Mar 10, 2021
@afs afs deleted the rdfx branch March 10, 2021 10:21
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

Successfully merging this pull request may close these issues.

None yet

2 participants