GroupBy uid shouldn't panic, and other logic fixes. #2952
Conversation
…DummyValues a bit.
…ue-2936_query_groupby_uid_causes_panic
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @srfrog)
query/groupby.go, line 222 at r1 (raw file):
srcUid := child.SrcUIDs.Uids[i] // Ignore uids which are not part of srcUid. if algo.IndexOf(ul, srcUid) == -1 {
Am I missing something or is this change is a no-op? algo.IndexOf() returns an index 0..n if found, -1 otherwise. If not found, both -1 < 0 and -1 == -1 evaluate to TRUE. If found at position 0, both 0 < 0 and 0 == -1 evaluate to FALSE.
In that case, I think the original test is better since it would continue working if IndexOf() were to return other error codes in the future.
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @codexnull)
query/groupby.go, line 222 at r1 (raw file):
Previously, codexnull (Javier Alvarado) wrote…
Am I missing something or is this change is a no-op? algo.IndexOf() returns an index 0..n if found, -1 otherwise. If not found, both
-1 < 0and-1 == -1evaluate to TRUE. If found at position 0, both0 < 0and0 == -1evaluate to FALSE.In that case, I think the original test is better since it would continue working if IndexOf() were to return other error codes in the future.
Maybe, but the issue is that 0 is a valid index value and the test ignores it. If we actually return -2, for example, then we would have to consider all test cases individually.
There was a problem hiding this comment.
Ok.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @codexnull)
query/groupby.go, line 222 at r1 (raw file):
Previously, srfrog (Gus) wrote…
Maybe, but the issue is that
0is a valid index value and the test ignores it. If we actually return -2, for example, then we would have to consider all test cases individually.
Agree with @codexnull . Looks like a no-op. Here and elsewhere. All negative values should be treated as index not found.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @codexnull)
query/groupby.go, line 222 at r1 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Agree with @codexnull . Looks like a no-op. Here and elsewhere. All negative values should be treated as index not found.
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, 2 of 3 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
* added check for nil destuid in child sg. also fixed a couple of logic bugs. * fixed another logic bug related to algo.IndexOf * fixed logic bug, made algo.ApplyFilter clearer, and simplified appendDummyValues a bit. * added tests * change algo.IndexOf to less explicit form
This PR fixes an issue with queries using
@groupby(uid)directive that would cause Alpha to panic.@groupbypanic was due to the fact that we don't create tasks for children subgraphs usinguidas attribute. Therefore, we don't generateDestUIDslist for them. When the groupby is processed the field is nil and we panic when it tries to access the uids from that list.DestUIDsisn't nil. This will move the result list to useSrcUIDsinstead which is expected.Closes #2936
This change is