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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

JENA-632: Generate JSON from SPARQL directly #114

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@kinow
Member

kinow commented Dec 27, 2015

This pull request contains code for JENA-632. The original work is still in a branch in my fork of Jena. It has been updated after the work on Jena 3 (mainly package renaming). And the web layer has been implemented in fuseki 2, but not backported to fuseki 1.

Besides reviewing the code, the follow steps can be used to quickly test the code.

  • Start Fuseki (debug in Eclipse after checking out this branch, for example)
  • Load the books.ttl from fuseki1/Data directory
  • Query with something as
PREFIX purl: <http://purl.org/dc/elements/1.1/>
PREFIX w3: <http://www.w3.org/2001/vcard-rdf/3.0#> 
PREFIX : <http://example.org/book/> 

JSON {
"author": ?author, 
"title": ?title 
}
WHERE 
{
?book purl:creator ?author .
?book purl:title ?title . 
FILTER (?author = 'J.K. Rowling')
}

Which follows the syntax proposed in the issue in JIRA.

I am still reviewing the code after porting to the new code base, but an extra pair of eyes reviewing it is always welcome! 馃榿

ps: the SPARQL editor may need some tweaking to support the new syntax

ps2: tried to change the key name in the JSON query but it didn't work. Will try to update the PR if that's really a bug in the code in the next days

<JSON> { getQuery().setQueryJsonType() ; }
<LBRACE>
s = String() < PNAME_NS >
(

This comment has been minimized.

@afs

afs Dec 29, 2015

Member

Shouldn't PNAME_NS be COLON? PNAME_NS matches foo: so "s = String() < PNAME_NS >" matches "abcd" xyz:

It may be hard to parse JSON using the SPARQL tokens,in which case using some javacc ability to switch details may be needed.

This comment has been minimized.

@kinow

kinow Jan 1, 2016

Member

I will need some time to check in Eclipse why I used PNAME_NS. If I remember correctly, using COLON, JavaCC would find it ambiguous with some other character or rule?

This comment has been minimized.

@afs

afs Jan 6, 2016

Member

Yes - javacc will tokenize to PNAME_NS. These is no COLON and if there were, theer would be other problems.

I have a devious idea!

Use the PNAME_NS and follow with a java fragment that does additional checking:

   t = <PNAME_NS>
   { 
       if ( t.image is not exactly a ":" )
          throwParseException("message",  t.beginLine, t.beginColumn)
   }

This then restricts the legal token and means you won't to have to play complicated games with javacc to switch to a different token set (if that is even possible due to lookahead of characters).

This comment has been minimized.

@kinow

kinow Mar 31, 2017

Member

Replying to my previous comment. The reason why we are not using COLON, is that we get this error:

[2017-04-01 00:44:24] Fuseki     INFO  [1] Query = JSON { "name" : ?name } WHERE { ?name ?a ?b } LIMIT 2
[2017-04-01 00:44:24] Fuseki     INFO  [1] 400 Parse error: 
JSON { "name" : ?name } WHERE { ?name ?a ?b } LIMIT 2

Encountered " <PNAME_NS> ": "" at line 1, column 15.
Was expecting:
    ":" ...
     (27 ms)

The PNAME_NS (which is <PNAME_NS: (<PN_PREFIX>)? ":" >) matches the colon, probably for the default namespace declaration. And probably because PNAME_NS is declared before COLON.

Maybe moving COLON before PNAME_NS, and using COLON in the PNAME_NS rule declaration? Not sure if that could introduce other unwanted bugs.

For the time being going with Andy's suggestion to indicate when we were expecting a COLON, and actually have something matching PNAME_NS.

@@ -2166,6 +2227,17 @@ String String() : { Token t ; String lex ; }
}
}
Number Number() : { Token t ; Number number ; }
{

This comment has been minimized.

@afs

afs Dec 29, 2015

Member

Number -> JSONNumber

But will rule NumericLiteral work here? Then parse the lexical form to a number.

However, generally, working in nodes is probably going to be easier because bindings map variables to Nodes.

@@ -100,6 +100,38 @@ import org.apache.jena.sparql.core.Quad ;
public class CLASS extends PARSERBASE

This comment has been minimized.

@afs

afs Dec 29, 2015

Member

Please update tokens.txt as well - it is used to generate HTML.

This comment has been minimized.

@kinow

kinow Jan 1, 2016

Member

Looking at the Notes file, there is a comment to run "jj2tokens sparql_11.jj > tokens.txt" to create an initial tokens.txt file, and then manually tidy it up.

Should I re-run jj2tokens, or just manually add the missing entries?

Not sure if that hasn't been used in a while and could generate strange behaviour in other grammars later.

This comment has been minimized.

@afs

afs Jan 6, 2016

Member

tokens.txt is used to produce HTML - hence the additional syntax to indicate inline vs a token rule for each definition.

It is safer to hand edit for minor changes because you risk loosing the additional edits alreayd made.

(If running jj2tokens, send to a temporary file and pick out the new bits and edit in manually)

This comment has been minimized.

@kinow

kinow Mar 31, 2017

Member

Done. First executed jj2tokens, compared the output (digressing: IRIRef seems to have been updated I guess) and added the missing JSON token.

@@ -431,6 +433,7 @@
"\"select\"",
"\"distinct\"",
"\"reduced\"",
"\"json\"",

This comment has been minimized.

@afs

afs Dec 29, 2015

Member

This the new JSON construct sneaking into pure SPARQL 1.1

This comment has been minimized.

@kinow

kinow Jan 1, 2016

Member

Oh, when updating the branch (it was written when Jena was 2.x) I simply applied the same changes to master.jj and sparql_11.jj, and looked at other classes that I had patched, and looked for similar classes that looked like had to be updated as well.

But maybe I added Should I revert this one line?

This comment has been minimized.

@afs

afs Jan 6, 2016

Member

This is a generated file - Put inside a #ifdef ARQ ... #endif in master.jj.

@@ -100,6 +100,38 @@ import org.apache.jena.sparql.core.Quad ;
public class CLASS extends PARSERBASE
{
boolean allowAggregatesInExpressions = false ;
public static void main(String args[]) {
while (true) {

This comment has been minimized.

@afs

afs Dec 29, 2015

Member

This looks like debug code - could it go elsewhere?

This comment has been minimized.

@kinow

kinow Jan 1, 2016

Member

Not sure where else it could go. I copied (shamelessly) from an example I found online while re-reading about JavaCC. With this main method, you can run the grammar in Eclipse, and in the Eclipse Console it will be waiting for a String+LFLF (two break lines IIRC).

Then it will use the grammar to parse the string and will output the QueryUnit. I found it useful for reviewing the changes without running some extra class with a main method, or Fuseki.

What do you think? I'm OK with removing it, or moving it somewhere else. Just don't know where else it could go :-)

This comment has been minimized.

@afs

afs Jan 6, 2016

Member

New CLASS don't appear very often so this could be in a java source file for each language

In fact, it only needs to work for language ARQ which is a superset of SPARQL 1.1

Also - have you seen the command arq.qparse? It reads in a query and prints it out (and performs internal checks on .equals, .hashcode, output sameas input and the algebra generated.

@afs

This comment has been minimized.

Member

afs commented Dec 29, 2015

Main comment : the grammar changes haven't been put inside #ifdef ARQ so they happen in strict SPARQL 1.1 as well as the extended ARQ language.

Keeping the SPARQL 1.1 parser exactly as the SPARQL 1.1 spec is important. There should just be some noise changes and no more. (see for example the construct-quad changes).

@ajs6f

This comment has been minimized.

Member

ajs6f commented Feb 9, 2017

@kinow is this still live?

@kinow

This comment has been minimized.

Member

kinow commented Mar 20, 2017

@ajs6f sorry for the delay. Checked out the branch, just need to refresh my memory. Will need a few more days before pinging you guys to check out this PR again. Next month finally will have some time to play with Jena again, then plan to have this PR completed, and check if there's any Fuseki or easy tasks in JIRA :-)

Cheers
Bruno

@ajs6f

This comment has been minimized.

Member

ajs6f commented Mar 20, 2017

No prob-- good to see you back, @kinow !

@kinow

Updated master.jj incorporating Andy's suggestions. Updated tokens.txt. Merged with master. All tests passing locally. Let me know if I should rebase & squash. I think this pull request should be ready for review again now 馃帀

@tommals

This comment has been minimized.

tommals commented May 13, 2017

Relevant to this thread Return nested JSON results

@ajs6f

This comment has been minimized.

Member

ajs6f commented Mar 6, 2018

Hey, @kinow , is this ready for rebase and review? I can try rebasing it into a separate PR, or you can if you have time... then (hopefully) we can get it in!

@kinow

This comment has been minimized.

Member

kinow commented Mar 8, 2018

Hey @ajs6f , I think so. My weak memory seems to tell me that the last time I did that... shortly after, there was some work on JSON (JSON-LD? Or maybe pure JSON) somewhere else in Jena; and when I saw that happening, I wondered if that would perhaps be a replacement for the functionality in this ticket.

I wonder if you or @afs would be able to confirm that there isn't already another way to get JSON responses in Jena, and that this feature is still a nice to have feature, please? :)

@afs

This comment has been minimized.

Member

afs commented Mar 8, 2018

The JSON ecosystem moves fast! GraphQL is now significant and how that might be used is also very interesting. There are different ways to use GraphQL and maybe it is not the solution to all needs anyway - this ticket is more about result sets in easy-to-consume JSON than other work in the GraphQL-RDF area and more about JSON services.

To move this ticket forward, how about putting the feature in with a big label saying "Experimental - subject to change" and really mean it? By getting it out, it generates feedback, requirements and insight.

You can ask for result sets in JSON but the JSON (http://www.w3.org/TR/sparql11-results-json/) or JSON-LD graphs but that isn't what general data consumers would consider to be "easy-to-consume".

Is the RDF literal to JSON aligned with:
RDF-JS mapping? (which only came along recently).

@kinow

This comment has been minimized.

Member

kinow commented Mar 9, 2018

Rebased using GitHub's interface 馃帀 (never used it before, hope didn't make any silly mistakes :-) ).

Good points @afs ! I noticed the existing SPARQL to JSON result writers, so adjusted a bit the code to use that.

Now when I try to rebase against the master branch I get a bunch of conflicts, but GitHub web interface seems to indicate it's all good. I was trying to find a simple way to rebase it, but if I can't fix the conflicts and compile & test it, I'll just revert to what was before and then just manually rebase :)

@kinow

This comment has been minimized.

Member

kinow commented Mar 10, 2018

Rebasing again, now manually, testing in Eclipse. Previous branch before the GitHub rebase archived at https://github.com/kinow/jena/tree/JENA-632-3

@kinow

This comment has been minimized.

Member

kinow commented Mar 10, 2018

Rebased!

Tested the ARQParser main method (which is added here, but I find very handy for quickly testing it, but happy to remove if others prefer). And got the following:

Enter input: JSON { "name": ?name } WHERE { ?name ?a ?b } LIMIT 3


log4j:WARN No appenders could be found for logger (Jena).
log4j:WARN Please initialize the log4j system properly.
log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info.
Parsed query successfully!
---

Enter input: 

Then did a mvn install -DskipTests -Pdev, and copied apache-jena-fuseki-3.7.0-SNAPSHOT.tar.gz elsewhere. Unpacked it, and started fuseki-server.

Used an in-memory dataset in Fuseki2, loading the books.ttl example shipped with Jena code.

Finally, used the query found in the JIRA ticket to retrieve the JSON result:

PREFIX purl: <http://purl.org/dc/elements/1.1/>
PREFIX w3: <http://www.w3.org/2001/vcard-rdf/3.0#> 
PREFIX : <http://example.org/book/> 

JSON {
"author": ?author, 
"title": ?title 
}
WHERE 
{
?book purl:creator ?author .
?book purl:title ?title . 
FILTER (?author = 'J.K. Rowling')
}

And here's the output:

[ { 
  "author" : "J.K. Rowling" ,
  "title" : "Harry Potter and the Deathly Hallows"
}
{ 
  "author" : "J.K. Rowling" ,
  "title" : "Harry Potter and the Philosopher's Stone"
}
{ 
  "author" : "J.K. Rowling" ,
  "title" : "Harry Potter and the Order of the Phoenix"
}
{ 
  "author" : "J.K. Rowling" ,
  "title" : "Harry Potter and the Half-Blood Prince"
}
 ]

Going to push-force onto this branch now, so the pull request should be rebased onto the latest changes in a couple minutes.

@kinow

This comment has been minimized.

Member

kinow commented Mar 10, 2018

Done, and ready for review again.

@afs

This comment has been minimized.

Member

afs commented Mar 12, 2018

I applied 114.patch locally and it integrated just fine.

I tried JSON { "s": ?s , "p": ?p , "o" : ?o } WHERE { ?s ?p ?o } on data

:s :p 123 .
:s :p "abc" .
:s :p "def"@en .

and got:

[ { "o" : "123^^http://www.w3.org/2001/XMLSchema#integer" } ,
  { "o" : "abc" } ,
  { "o" : "def@en" }
]

I think we should use a more aggressive version of the conversion from RDF term to JS used in the upcoming JS custom Functions, code https://github.com/apache/jena/blob/master/jena-arq/src/main/java/org/apache/jena/sparql/function/js/NV.java. That works on NodeValues to make functions more natural to write and is two-way.

For JSON-SELECT we only need a one way conversion and getting info out even if it is some string is better IMO.

RDF Javascript
String String
XSD numeric JSON number
XSD boolean boolean
String@lang String, no lang
URI String
"lex"^^SomeDatatype String, no datatype
Unbound JSON null

If the app wants RDF term details, it can use application/result-set+json. This feature provides provides a simpler, more JSONy view of the data for consuming RDF-unaware applications.

@afs

This comment has been minimized.

Member

afs commented Mar 12, 2018

Tests?

@kinow

This comment has been minimized.

Member

kinow commented Mar 12, 2018

Going to check how to get the correct values with JSON. Great catch, as I was using just Strings. But shouldn't be too hard.

Will also include the initial tests with the next changes, covering both previous features, and the conversion from the table you provided :-)

Thanks!

@afs

This comment has been minimized.

Member

afs commented Mar 13, 2018

Conversion code (draft): 2dd4418 (on a branch in my GH at the moment).

The code is in RDFTerm2Json.fromNode.

@kinow

This comment has been minimized.

Member

kinow commented Mar 14, 2018

+1 for the current draft

I would use it here and here.

This way instead of looking only at the literals, we'd have the output from the table above.

So far I have just two unit tests in my working copy

    @Test public void testExecJson() {
        // JENA-632
        Query query = QueryFactory.create("JSON { \"s\": ?s , \"p\": ?p , \"o\" : ?o } "
                + "WHERE { ?s ?p ?o }", Syntax.syntaxARQ);
        
        try ( QueryExecution qexec = QueryExecutionFactory.create(query, m) ) {
            JsonArray jsonArray = qexec.execJson();
            assertNotNull( jsonArray );
            assertEquals(3, jsonArray.size());
        }
    }

    @Test public void testExecJsonItems() {
        // JENA-632
        Model model = ModelFactory.createDefaultModel();
        {
            Resource r = model.createResource(AnonId.create("s"));
            Property p = model.getProperty("");
            RDFNode node = ResourceFactory.createTypedLiteral("123", XSDDatatype.XSDdecimal);
            model.add(r, p, node);
            r = model.createResource(AnonId.create("s"));
            p = model.getProperty("");
            node = ResourceFactory.createTypedLiteral("abc", XSDDatatype.XSDstring);
            model.add(r, p, node);
            r = model.createResource(AnonId.create("s"));
            p = model.getProperty("");
            node = ResourceFactory.createLangLiteral("def", "en");
            model.add(r, p, node);
        }
        Query query = QueryFactory.create("JSON { \"s\": ?s , \"p\": ?p , \"o\" : ?o } "
                + "WHERE { ?s ?p ?o }", Syntax.syntaxARQ);
        
        try ( QueryExecution qexec = QueryExecutionFactory.create(query, model) ) {
            Iterator<JsonObject> execJsonItems = qexec.execJsonItems();
            while(execJsonItems.hasNext()) {
                JsonObject next = execJsonItems.next();
                System.out.println(next);
            }
        }

Where the second test is doing - more or less - the same example you provided in your previous comment. Once we have the conversion in place, I will update it to confirm I get what's expected.

Then after that I was thinking about quickly running with jacoco to see how much of this PR is being tested.

@kinow

This comment has been minimized.

Member

kinow commented Mar 14, 2018

FWIW, the output of the second test right now is

{ 
  "s" : "s" ,
  "p" : "" ,
  "o" : "def@en"
}
{ 
  "s" : "s" ,
  "p" : "" ,
  "o" : "abc"
}
{ 
  "s" : "s" ,
  "p" : "" ,
  "o" : "123^^http://www.w3.org/2001/XMLSchema#decimal"
}

@afs are you planning to send another PR when the draft is ready (it looks good to me as-is now :-) ) or would you prefer to include in this PR and Jena-632? It does seem to be related with/necessary for JENA-632. Happy to get updates to this branch :-)

@afs

This comment has been minimized.

Member

afs commented Mar 14, 2018

PR #383 - we can still change it based on experience.

BTW The grammar accepts the JSON field names in single quotes to make embedding in strings easier. It's a STRING1, either one " or one ' delimiting (not mixed!).

I doubt it will need many tests.

@kinow

This comment has been minimized.

Member

kinow commented Mar 15, 2018

Most of the tests are done, just need to add at least basic coverage for the SPARQL changes. Then will spend some minutes playing with the queries in Fuseki, before updating the PR. Aiming to push the changes Friday night NZ time, or this weekend.

@afs

This comment has been minimized.

Member

afs commented Mar 15, 2018

Great timing! but as ever, it it makes it, it makes it, and if it doesn't, it doesn't.

(I will not be able to build 3.7.0 until next week anyway.)

PR #383 merged.

@@ -707,6 +719,8 @@ public void visit(QueryVisitor visitor)
visitor.visitDescribeResultForm(this) ;
if ( this.isAskType() )
visitor.visitAskResultForm(this) ;
if ( this.isJsonType() )

This comment has been minimized.

@kinow

kinow Mar 16, 2018

Member

@afs added this one in order to be able to use the Query object in some tests...

for (String resultVar : resultVars)
{
Node n = binding.get(Var.alloc(resultVar)) ;
JsonValue value = RDFTerm2Json.fromNode(n) ;

This comment has been minimized.

@kinow

kinow Mar 16, 2018

Member

@afs updated the code where I mentioned before...

JsonObject jsonObject = new JsonObject() ;
for (String resultVar : resultVars) {
Node n = binding.get(Var.alloc(resultVar)) ;
JsonValue value = RDFTerm2Json.fromNode(n) ;

This comment has been minimized.

@kinow

kinow Mar 16, 2018

Member

@afs and here

@Override
public void visitJsonResultForm(Query query) {
out.print("JSON {");

This comment has been minimized.

@kinow

kinow Mar 16, 2018

Member

This is used only when we create the Query manually. Only developers/advanced users might be interested in using it. The results via HTTP are not serialized this way.

This comment has been minimized.

@afs

afs Mar 20, 2018

Member

The qparse tool uses serialization (and it parses the result back again).

This comment has been minimized.

@afs

afs Mar 20, 2018

Member

More test suggestions: testParse("JSON { "s": "FOO" , "p" : ?p , "o" : ?o } WHERE { }") to parse to Q, serialize, reparse to Q2 and check Q.equals(Q2).

e.g.a new org.apache.jena.sparql.syntax.TestJSONParsing

JSON { "s": "FOO" } becomes JSON { "s": FOO }

This comment has been minimized.

@kinow

kinow Apr 18, 2018

Member

JSON { "s": "FOO" } becomes JSON { "s": FOO }

Shouldn't it be

JSON { "s": "FOO" } becomes JSON { "s": "FOO" } // last FOO still with double quotes / a string?
{
JsonValue value = entry.getValue() ;
String val = "";
if (value.isString()) {

This comment has been minimized.

@kinow

kinow Mar 16, 2018

Member

@afs but here I didn't have a Node, only the JsonValue, and had to convert from JsonValue to String

This comment has been minimized.

@afs

afs Mar 19, 2018

Member

"outputAsJSON" is a bit confusing because the other functions of the same name produce SPARQL results in JSON. How about "output"?

In the code, numbers still come out as strings.

JSWriter seems to be only good for its current usage. The code below seems to work: it manages the outer array itself and uses JsonValue.output to write each item.

 public static void outputAsJSON(OutputStream outStream, Iterator<JsonObject> jsonItems)
    {
        IndentedWriter out = new IndentedWriter(outStream);
        out.println("[");
        out.incIndent();

        while (jsonItems.hasNext()) {
            JsonObject jsonItem = jsonItems.next() ;
            jsonItem.output(out);
            if ( jsonItems.hasNext() )
                out.println(" ,");
            else
                out.println();
        }
        out.decIndent();
        out.println("]");
        out.flush();
    }

This comment has been minimized.

@kinow

kinow Mar 20, 2018

Member

"outputAsJSON" is a bit confusing because the other functions of the same name produce SPARQL results in JSON. How about "output"?

Sounds good to me. Done!

In the code, numbers still come out as strings.

Oh, d'oh me! You raised the issue, I updated the code but not the part that writes it back to the user. And even added a test that was testing it was getting "123" and "true" in the output. Fixed the test, and thanks heaps for the code snippet! Otherwise I'd be scratching my head and thinking how to use JSWriter.

Updated, and uploaded the data below

@prefix :          <http://example.org/> .

:s :p 123 .
:s :p "abc" .
:s :p "def"@en .

Then queried JSON { "s": ?s , "p": ?p , "o" : ?o } WHERE { ?s ?p ?o }. Screen shot below with the 123 value - hopefully - correctly displayed! 馃暫 馃帀

screenshot_2018-03-20_21-02-30

@kinow

This comment has been minimized.

Member

kinow commented Mar 16, 2018

screenshot_2018-03-17_00-18-10
screenshot_2018-03-17_00-17-44

Screenshots of Fuseki running some JSON queries. Everything seems to be working fine 馃帀 ready for review, take 2 !

@kinow

This comment has been minimized.

Member

kinow commented Mar 20, 2018

Replied @afs `s last comment, and also rebased the code. Even though it's rebased, there's quite a few commits... let me know if I should squash the commits or if it's preferable to leave the commit history :-) ready to review, take 3?

@kinow

This comment has been minimized.

Member

kinow commented Mar 20, 2018

And only started working again on this because you bumped this PR @ajs6f :-) learning a lot working on this ticket, and with JENA-1488, hope to start contributing back to Jena more now.

@kinow

This comment has been minimized.

Member

kinow commented Mar 20, 2018

@afs had a quick look at your gist and it looks good! I won't be able to have a proper look at it and to commit for the next ~8 hours. But feel free to commit to this branch and update the pull request too if you prefer :-)

@afs

This comment has been minimized.

Member

afs commented Mar 20, 2018

Eval tests: https://gist.github.com/afs/9e57b705a0d53889606346f6723238ce

Framework : needs more tests!

@afs

This comment has been minimized.

Member

afs commented Mar 20, 2018

If it does not make the release then - whatever! - there'll be another along soon!

I may be able to do some work on it after the release. I think I now understand it better and it makes a useful contribution to developing applications that want JSON simply.

Have fun in Japan!

@kinow

This comment has been minimized.

Member

kinow commented Apr 18, 2018

Rebase'd against master. Added TestJsonEval.java, with a couple more tests. Then understood why the projected variables were not working.

Updated the grammar to call the method that populates the projectedVars collection. Now queries like JSON { 'F' : 'string' } WHERE { } resolve to [ { 'F' : 'string' } ].

Still need to work around the other comment about Doubles.

@kinow

This comment has been minimized.

Member

kinow commented Apr 25, 2018

@afs also removed the Number() clause as that was not being used any more. Ready for review again 馃憤

And big thanks for all the feedback (and patience!) so far.

@@ -515,6 +519,18 @@ public void addHavingCondition(Expr expr)
havingExprs.add(expr) ;
}
// SELECT JSON
private Map<String, Object> jsonMapping = new LinkedHashMap<>();

This comment has been minimized.

@afs

afs Apr 26, 2018

Member

This can be types as <String, Node> (variables are Nodes)

private Map<String, Node> jsonMapping = new LinkedHashMap<>();

public void addJsonMapping(String key, Node value) {
    jsonMapping.put(key, value);
}

public Map<String, Node> getJsonMapping() {
    return Collections.unmodifiableMap(jsonMapping);
}

This comment has been minimized.

@kinow

kinow Apr 26, 2018

Member

Done!

public void visitJsonResultForm(Query query) {
out.print("JSON {");
List<String> terms = new ArrayList<>();
for (Map.Entry<String, Object> entry : query.getJsonMapping().entrySet()) {

This comment has been minimized.

@afs

afs Apr 26, 2018

Member

Some suggestions for formatting output:

  • Use format utils to get pretty forms
  • Print JSON vertically so that long templates go down not off the right-hand side.

I had to catch the prologue for formatting as it is formatted. Adding a "JSON formatter" seems like a very heavy way to do it.

Patch from your JENA-632-2 branch:

jena-632-2-afs.txt

This comment has been minimized.

@kinow

kinow Apr 26, 2018

Member

Patch applied! Thanks Andy! TIL learned about that Prologue object.

Regarding the indentation, in QuerySerializer.java, there are other 2 occurrences of incIndent/decIndent, but instead of using 4, they use the class constant

static final int BLOCK_INDENT = 2 ;

Should we use that for JSON as well?

Thanks!!!

This comment has been minimized.

@afs

afs Apr 27, 2018

Member

Sorry about the patch - I couldn't figure how to clone your repo because GH did not like the fact I already had a repo called "jena". Yes, a constant is better. I thought that 4 looked nicer than 2 and it does not get indented further. Just use incIndent twice!

This comment has been minimized.

@afs

afs Apr 27, 2018

Member

Is it ready to merge? Any area to look at specially? I've used qparse on JSON queries and run a few test queries with all the cases I can think of.

This comment has been minimized.

@kinow

kinow Apr 27, 2018

Member

Just use incIndent twice!

Haha, I like it. Done!

Is it ready to merge? Any area to look at specially? I've used qparse on JSON queries and run a few test queries with all the cases I can think of.

I think so ! Only minor thing on my mind now are some expressions. We have not implemented support for queries with expressions like

JSON { 'F' : sum(1+2)} WHERE { }

Or other functions. Perhaps that could - if necessary - be implemented later?

Oh, and let me know if there are too many commits. We can squash all commits into a single commit; though I would first create a branch like JENA-632-before-merge in my personal fork, just in case we need to revisit it in the near future.

This comment has been minimized.

@afs

afs Apr 27, 2018

Member

Expressions can be done with BIND at the end of the WHERE pattern or a nested SELECT

JSON {
    "subject" :    ?s ,
    "predicate" :  ?p ,
    "count" :      ?c }
WHERE
  { SELECT  ?s ?p (COUNT(?o) AS ?c)
      { ?s  ?p  ?o }
    GROUP BY ?s ?p
  }

This comment has been minimized.

@kinow

kinow Apr 27, 2018

Member

Oh, got it! Nice! So no more comments from me 馃槂

@afs

afs approved these changes Apr 27, 2018

@kinow

This comment has been minimized.

Member

kinow commented Apr 27, 2018

馃帀 should I proceed with the merging? If so, I will squash the 18 commits into a single one before, as said before, but with a backup branch just-in-case. And then start working on the documentation in order to update the ticket in JIRA.

@afs

This comment has been minimized.

Member

afs commented Apr 27, 2018

That works for me.

@kinow

This comment has been minimized.

Member

kinow commented Apr 27, 2018

Branch created: JENA-632-before-merge. Won't delete it this year, in case we need to revisit.

Followed that by:

$ git rebase -i HEAD~18
$   # fixed-up all but the first commit
$ git diff JENA-632-before-merge 
$   # empty response, code is identical even though different commit tree
$ git checkout master
$ git merge JENA-632-2
$ git commit --amend
$   # added This closes #114 message to close this pull request
$ mvn clean test install -Pdev
$ git push git-at-apache master

@asfgit asfgit closed this in 4bf5e3c Apr 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment