Issue 46: Parse Construct Statement by apr94 · Pull Request #64 · google/badwolf

apr94 · 2017-03-03T01:38:16Z

Also fixed some typos that I found.

xllora · 2017-03-03T15:23:11Z

+				Elements: []Element{
+					NewTokenType(lexer.ItemLBracket),
+					NewSymbol("CLAUSES"),
+					NewTokenType(lexer.ItemRBracket),


There is part missing in this construct. It should support only 3 bindings per entry.

https://www.w3.org/TR/rdf-sparql-query/#tempatesWithBNodes

Which includes the special notation for blank nodes. The current implementation would allow an arbitrary amount that may lead to impossible to construct triples that then we would have to do extra work on the semantic to validate their number, instead of only the type.

Also we should propose a version of how are we going to deal with blank nodes. The one above is pretty simple, but does not help with the reification case. That one is usually expressed with the ; construct something along the lines

construct { ?s ?p ?o ; "some_pred" ?k . } ...

That would insert ?s ?p ?o triples, then reify it, and insert blank "some_pred" ?k triple also.

As you can see there two things going here, the first one to create blank nodes as needed, the second to deal with reification.

Yes, the ";" is something that also needs to be revamped later, together with filters in the where clauses; there reason I mention it here is because in the where clause are ways to work around the reification ; syntax, but not here, since otherwise we would have to write something along the lines of

construct { ?s ?p ?o . _:v "_subject" ?s. _:v "_predicate" ?p. _:v "_object" ?o. _:v "some_pred" ?k . } ...

Which as you can see is a bit more tedious, and prone to error if you mistype the reification predicates.

Also about the blank node syntax we can keep with _:v where v is a id of that blank node so you can introduce multiple ones, eg _:v1, _:v2 ...

apr94 · 2017-04-10T22:25:23Z

The latest commit handles triples and blank nodes. The reification ; syntax will be implemented in a different commit.

xllora

Thanks @palimarrao . I left a few comments. This looks pretty close to what we want 😄

xllora · 2017-04-11T14:32:52Z

+			},
+			{
+				Elements: []Element{
+					NewTokenType(lexer.ItemPredicateBound),


Predicate bounds can present a number of predicates. I am not sure we want this here. I think we should only support predicate and binding. For instance, if we go with

construct{
?s ?p@[,some_ts] ?o
}

Does it mean we have to filter further the binded predicates of ?p. This seems a bit redundant since it can be done by construing the where clause further or by adding a having clause. For that reason, I would suggest dropping it.

Got it. I have removed the support for binded predicates.

xllora · 2017-04-11T14:33:18Z

+			},
+			{
+				Elements: []Element{
+					NewTokenType(lexer.ItemPredicateBound),


Same comment here about the predicate bound.

xllora · 2017-04-11T14:37:41Z

 		`delete data from ?world {/room<000> "named"@[] "Hallway"^^type:text.
 		                          /room<000> "connects_to"@[] /room<001>};`,
+		// Test Construct clause.
+		`construct {?s "foo"@[,] ?o} into ?a from ?b where {?s "foo"@[,] ?o} having ?s = ?o;`,


This is exactly the point I was making above. If you want to create triples for each of the foo predicates with the right timestamp, you could write it like this instead

CONSTRUCT {?s ?p ?s} INTO ?a FROM ?b WHERE { ?s "foo"@[,] as ?p ?s } ;

Also see how you can also avoid the having by properly playing with the binding, but that is beside the point here, since we should definitely have test that cover the having clause 😄

I have updated this test case.

xllora · 2017-04-11T14:38:25Z

 }

-func TestSemanticStatementGraphClausesLenghtCorrectness(t *testing.T) {
+func TestSemanticStatementGraphClausesLengthCorrectness(t *testing.T) {


Thx for the typo fixing 😄

xllora · 2017-04-11T14:41:07Z

+		return nil
+	}
+	for {
+		if r := l.next(); !unicode.IsLetter(r) && !unicode.IsDigit(r) && r != rune('_') || r == eof {


Do we want to force the first rune to be a letter? right now this _:_ or _:1 would be valid blank node IDs?

That's a good idea. The BNode label name should begin with a letter.

xllora · 2017-04-11T14:41:27Z

 	return lexLiteral
 }

-// lexPredicate lexes a predicate of out of the input.


xllora · 2017-04-11T14:41:41Z

 }

-// lexPredicate lexes a literal of out of the input.
+// lexLiteral lexes a literal out of the input.


xllora · 2017-04-11T14:42:20Z

+				{Type: ItemBlankNode, Text: "_:v1"},
+				{Type: ItemBlankNode, Text: "_:foo_bar"},
+				{Type: ItemEOF}}},
+		{"_v1",


See my comment about if we should start with a letter always the ID of the blank node.

apr94 · 2017-04-11T19:12:44Z

Thanks for the detailed review @xllora ! I have addressed your comments.

xllora

Thanks!

xllora · 2017-04-19T03:28:10Z

 		                          /room<000> "connects_to"@[] /room<001>};`,
+		// Test Construct clause.
+		`construct {?s "foo"@[] ?o} into ?a from ?b where {?s "foo"@[,] ?o} having ?s = ?o;`,
+		`construct {?s "foo"@[] ?o} into ?a from ?b where {?s "foo"@[,] ?o};`,


Could you use a different predicate other than "foo" to avoid confusing. Above it adds a foo that is immutable as opposed to the temporal ones you are querying.

On another note we may want to provide some way to allow folks to construct temporal predicates. Immutable predicates are easy, as you shown in the above examples. But for temporal ones, we would need something along the lines of:

TEMPORAL_PREDICATE("bar"^^type:string, ?ts)
TEMPORAL_PREDICATE("bar"^^type:string, @1234)
TEMPORAL_PREDICATE("bar"^^type:string, "2017....")

What do you thing? No need to do it in this round, but maybe we should file an issue for later.

Done! For temporal predicates, can we use the same syntax as is used in the INSERT statement?
construct {?s "some_pred"@[2006-01-02T15:04:05.999999999Z07:00] ?o } into ?a from ?b where {?s "foo"@[,] ?o};
Or is the reason that we might need to construct temporal predicates from predicates returned by the WHERE clause, but with a different time anchor?

xllora · 2017-04-19T03:42:50Z

+		// Test Construct clause.
+		`construct {?s "foo"@[] ?o} into ?a from ?b where {?s "foo"@[,] ?o} having ?s = ?o;`,
+		`construct {?s "foo"@[] ?o} into ?a from ?b where {?s "foo"@[,] ?o};`,
+		`construct {?s ?p ?o} into ?a from ?b where {?s "foo"@[,] ?o} having ?s = ?o;`,


Nit pick, since it will mater later, ?p is not defined and hence this statement is not going to be semantically valid. Other statements have a similar issue :)

apr94 · 2017-04-20T05:52:15Z

Thanks for the review @xllora !

parse construct statement

435a932

xllora reviewed Mar 7, 2017

View reviewed changes

Rewrite CONSTRUCT query

728c937

apr94 force-pushed the parse_construct branch from 7c217ea to 728c937 Compare April 10, 2017 22:13

xllora reviewed Apr 11, 2017

View reviewed changes

apr94 added 2 commits April 11, 2017 11:54

Removed Predicate Bounds; Force BNode Label to start with letter

97e1eee

Fixed Lexer test that included bounded predicates

7b6a98b

apr94 closed this Apr 11, 2017

apr94 reopened this Apr 11, 2017

xllora approved these changes Apr 19, 2017

View reviewed changes

Added semantically valid test cases

3f4f00c

apr94 force-pushed the parse_construct branch from 76f9564 to 3f4f00c Compare April 20, 2017 05:22

xllora merged commit 320fbca into google:master Apr 24, 2017

Conversation

apr94 commented Mar 3, 2017

Uh oh!

xllora Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apr94 commented Apr 10, 2017

Uh oh!

xllora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apr94 Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apr94 commented Apr 11, 2017

Uh oh!

xllora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apr94 commented Apr 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xllora Mar 3, 2017 •

edited

Loading

apr94 Apr 11, 2017 •

edited

Loading