-
Notifications
You must be signed in to change notification settings - Fork 66
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FILTER isImmutable #153
FILTER isImmutable #153
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some tests in memory_test.go as well?
09c1036
to
2712e2f
Compare
In the second to last commit I have already added a total of 22 new tests in |
2712e2f
to
7fbc5a3
Compare
id: "FILTER isImmutable predicate", | ||
lo: &storage.LookupOptions{FilterOptions: &filter.StorageOptions{Operation: filter.IsImmutable, Field: filter.PredicateField}}, | ||
s: testutil.MustBuildNodeFromStrings(t, "/u", "john"), | ||
p: testutil.MustBuildPredicate(t, `"parent_of"@[]`), | ||
want: map[string]int{"/u<paul>": 1}, | ||
}, | ||
{ | ||
id: "FILTER isImmutable object", | ||
lo: &storage.LookupOptions{FilterOptions: &filter.StorageOptions{Operation: filter.IsImmutable, Field: filter.ObjectField}}, | ||
s: testutil.MustBuildNodeFromStrings(t, "/_", "bn"), | ||
p: testutil.MustBuildPredicate(t, `"_predicate"@[]`), | ||
want: map[string]int{`"height_cm"@[]`: 1}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change the name of the test struct a bit?
You could explain what is the want, e.g.,
want -> ExpectedCountByObjectName.
s -> query_subject
p-> query_predicate
Also the logic in the test body seems a bit too complex for tests.
Instead of decrementing the map counter and deleting if the count gets to 0,
WDYT of building the "got" map and calling equal on both?
instead of:
for s := range ss {
sStr := s.String()
if _, ok := entry.want[sStr]; !ok {
t.Fatalf("g.Subjects(%s, %s, %s) retrieved unexpected %s", p, o, entry.lo, sStr)
}
entry.want[sStr] = entry.want[sStr] - 1
if entry.want[sStr] == 0 {
delete(entry.want, sStr)
}
}
something like:
got := map[string]int{}
for o := range os {
oStr := s.String()
got[oStr] += 1
}
if !cmp.Equals(entry.want, got){
t.Fatalf("g.Subjects(%s, %s, %s) retrieved unexpected %s", p, o, entry.lo, sStr)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we commented through chat, I can be doing this in a separate PR after this PR and the PR 154 are merged. Also, I will be letting this task as P2 for now to focus on current tasks, as we commented through chat as well.
This PR comes to add support for the
isImmutable
FILTER function in BadWolf, as detailed in #129. It also comes to help resolving what was requested by #115.To illustrate its implementation on the driver side, this PR also adds support for this FILTER in the volatile OSS driver (in
memory.go
).Now, the user can write queries like:
That would return only rows for which the predicate
?p
is not temporal. For other examples of queries have a look at the newly added tests inplanner_test.go
.This FILTER function also works for object bindings in the clause, along with predicate aliases and object aliases too.