Implemented planner for the CONSTRUCT statement.#76
Conversation
xllora
left a comment
xllora
left a comment
There was a problem hiding this comment.
Some comments. Also thanks for keep fixing all those typos I left sprinkled everywhere :/
| var ts []*triple.Triple | ||
| for _, cc := range p.stm.ConstructClauses() { | ||
| for _, r := range tbl.Rows() { | ||
| sbj, prd, obj := cc.S, cc.P, cc.O |
There was a problem hiding this comment.
This is a very long function. Can you break the logic inside the look into smaller, easier to follow functions?
There was a problem hiding this comment.
Done. I can refactor this further when I refactor the construct and reification clauses into a single struct in a follow-up PR.
| trace(p.tracer, func() []string { | ||
| return []string{"Inserting triples to graph \"" + g.ID(ctx) + "\""} | ||
| }) | ||
| return g.AddTriples(ctx, d) |
There was a problem hiding this comment.
This can run on large collections of data. I would suggest that instead of pilling them up on a large slice, you pass them in a channel where you have a goroutine consuming it. See how buffered channels are used elsewhere. This would also help batch the inserts into the the gorouting doing a few calls to AddTriples instead of a big one. You can make the case that the table can also be big, yes, that is something I have on the todo to revisit :/
There was a problem hiding this comment.
Done. Let me know if it looks fine.
| }) | ||
| } | ||
|
|
||
| func (p *constructPlan) String() string { |
| trps: sts + dts, | ||
| }, | ||
| { | ||
| s: `construct {?s "met"@[] ?o; "location"@[] /city<New York>} into ?dest from ?src where {?s "met"@[] ?o};`, |
There was a problem hiding this comment.
Can you please break the construct queries into multiple lines?
| } | ||
| } | ||
|
|
||
| func TestPlannerConstruct(t *testing.T) { |
There was a problem hiding this comment.
Could you add one more test to check that the triples returned are the ones expected, not only the right number?
There was a problem hiding this comment.
Done. Since we do not know what the blank node id is going to be, I had to add a lot of logic here to match place holder blank nodes to blank nodes generated by triples.go. Let me know if looks fine.
There was a problem hiding this comment.
Also I ran this test with -count=100 to ensure that there was no flakiness.
… new construct test case.
|
Thanks for the review. PTAL, I have addressed your comments! |
xllora
left a comment
xllora
left a comment
There was a problem hiding this comment.
Couple of suggestions and minor nits.
| done := make(chan bool) | ||
|
|
||
| go func() { | ||
| ts := []*triple.Triple{} |
| } | ||
| for elem := range tripChan { | ||
| ts = append(ts, elem) | ||
| if len(ts) >= 10 { |
There was a problem hiding this comment.
Shouldn't we use bulk triple op instead of hardcoding 10?
There was a problem hiding this comment.
Done. Have plumbed BulkTripleOpSize through to here and ensured that all tests pass and bw compiles and runs.
|
|
||
| for _, cc := range p.stm.ConstructClauses() { | ||
| for _, r := range tbl.Rows() { | ||
| sbj, prd, obj := cc.S, cc.P, cc.O |
There was a problem hiding this comment.
Can you move the code in the loop into a separate function?
| } | ||
| if prd == nil { | ||
| if tbl.HasBinding(cc.PBinding) { | ||
|
|
| } | ||
| if obj == nil { | ||
| if tbl.HasBinding(cc.OBinding) { | ||
|
|
| } | ||
| prd = v.P | ||
| } else if cc.PTemporal && cc.PAnchorBinding != "" { | ||
|
|
| } | ||
| obj = co | ||
| } else if cc.OTemporal && cc.OAnchorBinding != "" { | ||
|
|
| return nil, err | ||
| } | ||
| if len(cc.ReificationClauses()) > 0 { | ||
|
|
| bql := `construct {?s "met"@[?t] ?o; "location"@[] /city<New York>; | ||
| "outcome"@[] "good"^^type:text. | ||
| ?s "connected_to"@[] ?o } | ||
| into ?dest |
There was a problem hiding this comment.
Oops..gonna blame Vim for this one :)
| fmt.Sprintf("%s\t%s\t%s", `/_<b2>`, `"outcome"@[]`, `"good"^^type:text`): false, | ||
| } | ||
|
|
||
| // First, we map each blank node generated to a potential blank node placeholder (such as b1 or b2.) |
There was a problem hiding this comment.
All this complex testing because the blank nodes are unique and randomly generated right?
There was a problem hiding this comment.
Yep. It could be avoided if we can mock uuid.NewRandom() to produce deterministic output, though currently this setup is pretty verbose and informative when the set of stored triples are not as expected. Do you think it worth exploring the mocking route?
|
PTAL, I have addressed the review comments! |
2 participants
The planner for the construct statement + tests. Also the triples loading for tests has been refactored so that all test cases can use a single method.