Skip to content

Implementation of the DECONSTRUCT statement.#79

Merged
xllora merged 6 commits into
google:masterfrom
apr94:deconstruct_impl
Jul 25, 2017
Merged

Implementation of the DECONSTRUCT statement.#79
xllora merged 6 commits into
google:masterfrom
apr94:deconstruct_impl

Conversation

@apr94
Copy link
Copy Markdown
Contributor

@apr94 apr94 commented Jul 20, 2017

No description provided.

Comment thread bql/grammar/grammar.go Outdated
Elements: []Element{
NewTokenType(lexer.ItemDeconstruct),
NewSymbol("DECONSTRUCT_FACTS"),
NewTokenType(lexer.ItemAt),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

into instead of at?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed to 'in' instead of 'at'.

Comment thread bql/grammar/grammar_test.go Outdated
@@ -319,14 +354,30 @@ func TestAcceptGraphOpsByParseAndSemantic(t *testing.T) {
?s "old_predicate_2"@[,] ?o2.
?s "old_predicate_3"@[,] ?o3};`, empty, []string{"?b"}, []string{"?a"}, 0},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here and below, can you separate this into multiple lines?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. The current indentation is after gofmt. Wonder if it should be indented more to the left same col as the statement param?

Comment thread bql/grammar/grammar_test.go Outdated
`deconstruct {?s "new_predicate"@[] ?o} in ?a from ?b where {?s "old_predicate"@[,] ?o} having ?s = ?o;`,
`deconstruct {?s "new_predicate"@[] ?o} in ?a from ?b where {?s "old_predicate"@[,] ?o};`,
`deconstruct {?s ?p ?o.
_:v "_subject"@[] ?s.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mmh, I recall we discussed to start with the triple syntax with not reification. I thought that would include not supporting blank node syntax for deconstruct right now. Also not sure what the value for those would be since those are uniquely generated, likely it would not deconstruct anything.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, you are right, DECONSTRUCT does not support reification. However, the reason I added support for blank node is that it is possible in the CONSTRUCT statement to explicitly have blank nodes as the subject for a triple. So while we cannot DECONSTRUCT reified blank nodes with this syntax (without knowing the random uuid), we can DECONSTRUCT explicit blank nodes with known uuids. However, I am fine removing this functionality since it could be confusing, but I think it has a valid use case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I found this a bit misleading. The main reason in my head is the following. When you query in the where clause, you query blank nodes if you need to. This means, that after all, you get them like any other node/regular binding. Hence, for the deconstruct part you would actually use the regular binding you have obtained from the where clause query.

The blank node notation, needs to be consistent. If you have a blank node placeholder it must generate a new unique blank node. That would mostly mean, generating non preexisting nodes by definition, hence, never deleting anything.

For the reasons above, I would suggest remove the _:v notation from the syntax of destruct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I have removed blank node support.

@xllora
Copy link
Copy Markdown
Member

xllora commented Jul 25, 2017

Thanks for the change @palimarrao

@xllora xllora merged commit 3f5c2e3 into google:master Jul 25, 2017
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.

2 participants