-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(DGRAPH): fix @normalize response when multiple fields at different levels with same alias are selected. #7639
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.
Looks good. 🎉
Some minor nit-picks.
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.
Got a few comments. Thanks @NamanJain8 for the review.
Reviewed 1 of 2 files at r2, 1 of 1 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jatindevdg, @NamanJain8, and @pawanrawal)
query/outputnode.go, line 156 at r4 (raw file):
func (enc *encoder) MergeSort(headRef *fastJsonNode) { head := *headRef var a fastJsonNode
move this closer to usage.
query/outputnode.go, line 158 at r4 (raw file):
var a fastJsonNode var b fastJsonNode
Remove unnecessary vertical spaces.
query/outputnode.go, line 171 at r4 (raw file):
} func (enc *encoder) sortedMerge(a fastJsonNode, b fastJsonNode) fastJsonNode {
Rename to mergeSortedLists
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.
Reviewable status: 1 of 2 files reviewed, 7 unresolved discussions (waiting on @manishrjain, @NamanJain8, and @pawanrawal)
query/outputnode.go, line 155 at r3 (raw file):
Previously, JatinDevDG (Jatin Dev) wrote…
done
done.
query/outputnode.go, line 194 at r3 (raw file):
Previously, NamanJain8 (Naman Jain) wrote…
return strings.Compare(enc.attrForID(attri), enc.attrForID(attrj)) <= 0
done.
query/outputnode.go, line 958 at r3 (raw file):
Previously, JatinDevDG (Jatin Dev) wrote…
done.
done.
query/outputnode.go, line 156 at r4 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
move this closer to usage.
moved.
query/outputnode.go, line 158 at r4 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Remove unnecessary vertical spaces.
removed.
query/outputnode.go, line 171 at r4 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Rename to mergeSortedLists
renamed.
…t levels with same alias are selected. (#7639) We are generating incorrect order of the fields in a result of the @normalize directive when there are multiple fields with same Alias at nested levels. As a result of that in resulting JSON, the nodes with the same Alias were not converted to list. For example, if we have below data in dgraph . { "_id": "Process", "Name": "Process", "Extends": { "_id": "Core/Namespace", "Name": "Namespace" } } And if we do below query with @recurse and @normalize directive { item as var(func:eq(_id,"Process")) b2(func:uid(item))@normalize@recurse { Name:Name id:_id Extends } } We will get this response from Dgraph where fields with same alias are not together and while converting response to JSON , we are not able to combine fields with same Alias in a list. "b2": [ { "Name": "Process", "id": "Process", "Name":"Namespace", "id": "Core/Namespace" } ] In this PR we fix this in normalize, by putting all fields with the same Alias together. So now response of above query will be "b2": [ { "Name": [ "Process", "Namespace" ], "id": [ "Process", "Core/Namespace" ] } ] This behaviour is broken by PR #6362. Prior to this , we used sorting on fields which guarantee that fields with same name or Alias comes together. (cherry picked from commit 711e955)
…t levels with same alias are selected. (#7639) (#7691) We are generating incorrect order of the fields in a result of the @normalize directive when there are multiple fields with same Alias at nested levels. As a result of that in resulting JSON, the nodes with the same Alias were not converted to list. For example, if we have below data in dgraph . { "_id": "Process", "Name": "Process", "Extends": { "_id": "Core/Namespace", "Name": "Namespace" } } And if we do below query with @recurse and @normalize directive { item as var(func:eq(_id,"Process")) b2(func:uid(item))@normalize@recurse { Name:Name id:_id Extends } } We will get this response from Dgraph where fields with same alias are not together and while converting response to JSON , we are not able to combine fields with same Alias in a list. "b2": [ { "Name": "Process", "id": "Process", "Name":"Namespace", "id": "Core/Namespace" } ] In this PR we fix this in normalize, by putting all fields with the same Alias together. So now response of above query will be "b2": [ { "Name": [ "Process", "Namespace" ], "id": [ "Process", "Core/Namespace" ] } ] This behaviour is broken by PR #6362. Prior to this , we used sorting on fields which guarantee that fields with same name or Alias comes together. (cherry picked from commit 711e955)
This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
…8929) This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
…8929) This feature flag allows configuring @normalize in the query language such that if there are multiple fields with same alias, they could be returned as a list or non-list. There was a breaking change at some point with the PR #7639. Some customers do not prefer a list response in case of @normalize. We would like to not make any breaking changes going forward, and hence, introducing this feature flag enables users to make that choice.
We are generating incorrect order of the fields in a result of the @normalize directive when there are multiple fields with same Alias at nested levels. As a result of that in resulting JSON, the nodes with the same Alias were not converted to list.
For example, if we have below data in dgraph .
And if we do below query with @recurse and @normalize directive
We will get this response from Dgraph where fields with same alias are not together and while converting response to JSON , we are not able to combine fields with same Alias in a list.
In this PR we fix this in normalize, by putting all fields with the same Alias together. So now response of above query will be
This behaviour is broken by PR #6362. Prior to this , we used sorting on fields which guarantee that fields with same name or Alias comes together.
This change is