Fix bug triggered by nested expand predicates. #3205
Conversation
Fixes #3198. Certain queries with nested expand(_all_) predicates were being rejected by the method valueVarAggregation. However, the query should have been allowed and the results after ignoring the error are what was expected. This adds an extra check to return nil from that method if the subgraph is an expand subgraph.
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @martinmr)
query/common_test.go, line 496 at r1 (raw file):
<11000> <director.film> <11003> . <11100> <node> <11100> .
Can we also use hexadecimal numbers here, or use decimal numbers in the test to make them consistent?
|
I changed the fix. There's existing logic to clear the exact type of subgraphs that is causing the issue but it's executed before the expandSubgraph is run. Moving it at the end of ProcessGraph fixed the issue. |
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @martinmr)
query/query.go, line 2235 at r2 (raw file):
} if sg.DestUIDs == nil || len(sg.DestUIDs.Uids) == 0 {
I'd look very carefully at what all edge cases can occur, because when DestUIDs is nil or empty, it has to now go through filtering code and various others. Can you think of edge cases and add more tests?
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @gitlw and @manishrjain)
query/common_test.go, line 496 at r1 (raw file):
Previously, gitlw (Lucas Wang) wrote…
Can we also use hexadecimal numbers here, or use decimal numbers in the test to make them consistent?
I don't want to use hexadecimal numbers here to make it easy to see what uids are taking for people writing new tests. And I don't think I can make dgraph spit out decimal uids.
query/query.go, line 2235 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
I'd look very carefully at what all edge cases can occur, because when DestUIDs is nil or empty, it has to now go through filtering code and various others. Can you think of edge cases and add more tests?
I think this was just an optimization to avoid going through filtering, pagination, etc if there are no results. However, it looks like expanding the graph was still needed to properly clear up internal subgraphs.
The edge cases might be the code breaking because it tried to filter on a list of no results, for example. I have added new tests to check those cases return an empty answer without catching any segmentation fault, etc.
There was a problem hiding this comment.
Reviewed 2 of 4 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gitlw and @manishrjain)
Certain queries with nested expand(_all_) predicates were being rejected by the method valueVarAggregation. However, the query should have been allowed and the results after ignoring the error are what was expected. This adds an extra check to return nil from that method if the subgraph is an expand subgraph.
Fixes #3198
Certain queries with nested
expand(_all_)predicates were being rejectedby the method valueVarAggregation. However, the query should have been
allowed and the results after ignoring the error are what was expected.
This adds an extra check to return nil from that method if the subgraph
is an expand subgraph.
This change is