Issue 46: Parse Construct Statement #64

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

Also fixed some typos that I found.

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

bql/grammar/grammar.go

+				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

bql/grammar/grammar.go

+			},
+			{
+				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

bql/grammar/grammar.go

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


Same comment here about the predicate bound.

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

bql/grammar/grammar_test.go

@@ -113,6 +113,16 @@ func TestAcceptByParse(t *testing.T) {
 		                          /room<000> "connects_to"@[] /room<001>};`,
 		`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

bql/grammar/grammar_test.go

@@ -344,7 +365,7 @@ func TestRejectByParseAndSemantic(t *testing.T) {
 	}
 }

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


Thx for the typo fixing 😄

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

bql/lexer/lexer.go

+		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

bql/lexer/lexer.go

@@ -618,7 +647,7 @@ func lexPredicateOrLiteral(l *lexer) stateFn {
 	return lexLiteral
 }

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


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

bql/lexer/lexer.go

@@ -669,7 +698,7 @@ func lexPredicate(l *lexer) stateFn {
 	return lexSpace
 }

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


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

bql/lexer/lexer_test.go

+				{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

bql/grammar/grammar_test.go

@@ -113,6 +113,16 @@ func TestAcceptByParse(t *testing.T) {
 		                          /room<000> "connects_to"@[] /room<001>};`,
 		`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;`,
+		`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

bql/grammar/grammar_test.go

+		// 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 46: Parse Construct Statement #64

Issue 46: Parse Construct Statement #64

apr94 commented Mar 3, 2017

xllora Mar 3, 2017 •

edited

Loading

apr94 commented Apr 10, 2017

xllora left a comment

xllora Apr 11, 2017

apr94 Apr 11, 2017

xllora Apr 11, 2017

apr94 Apr 11, 2017

xllora Apr 11, 2017

apr94 Apr 11, 2017

xllora Apr 11, 2017

xllora Apr 11, 2017

apr94 Apr 11, 2017 •

edited

Loading

xllora Apr 11, 2017

xllora Apr 11, 2017

xllora Apr 11, 2017

apr94 commented Apr 11, 2017

xllora left a comment

xllora Apr 19, 2017

apr94 Apr 20, 2017

xllora Apr 19, 2017

apr94 Apr 20, 2017

apr94 commented Apr 20, 2017

Issue 46: Parse Construct Statement #64

Issue 46: Parse Construct Statement #64

Conversation

apr94 commented Mar 3, 2017

xllora Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

apr94 commented Apr 10, 2017

xllora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apr94 Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apr94 commented Apr 11, 2017

xllora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apr94 commented Apr 20, 2017

xllora Mar 3, 2017 •

edited

Loading

apr94 Apr 11, 2017 •

edited

Loading