Skip to content
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

Merged
merged 16 commits into from Mar 30, 2021

Conversation

JatinDev543
Copy link
Contributor

@JatinDev543 JatinDev543 commented Mar 23, 2021

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.


This change is Reviewable

query/outputnode.go Outdated Show resolved Hide resolved
Copy link
Contributor

@NamanJain8 NamanJain8 left a 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.

query/outputnode.go Outdated Show resolved Hide resolved
query/outputnode.go Show resolved Hide resolved
query/outputnode.go Show resolved Hide resolved
@JatinDev543 JatinDev543 changed the title fix(DGRAPH): fix @normalize response with @recurse when multiple fields are selected. fix(DGRAPH): fix @normalize response when multiple fields at different levels and same alias are selected. Mar 30, 2021
@JatinDev543 JatinDev543 changed the title fix(DGRAPH): fix @normalize response when multiple fields at different levels and same alias are selected. fix(DGRAPH): fix @normalize response when multiple fields at different levels with same alias are selected. Mar 30, 2021
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: 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

Copy link
Contributor Author

@JatinDev543 JatinDev543 left a 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.

@JatinDev543 JatinDev543 merged commit 711e955 into master Mar 30, 2021
@JatinDev543 JatinDev543 deleted the jatin/DGRAPH-2937 branch March 30, 2021 16:07
JatinDev543 added a commit that referenced this pull request Apr 7, 2021
…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)
JatinDev543 added a commit that referenced this pull request Apr 7, 2021
…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)
danielmai added a commit that referenced this pull request Aug 5, 2021
…different levels with same alias are selected. (#7639) (#7691)"

This reverts commit 3ad3f61.
mangalaman93 added a commit that referenced this pull request May 31, 2023
when multiple fields at different levels with same
alias are selected, this PR flattens the response into
a list  (#7639) (#7691)"

This reverts commit 3ad3f61.
mangalaman93 added a commit that referenced this pull request May 31, 2023
when multiple fields at different levels with same
alias are selected, this PR flattens the response into
a list (#7639) (#7691)

This reverts commit 3ad3f61.
mangalaman93 added a commit that referenced this pull request May 31, 2023
when multiple fields at different levels with same
alias are selected, this PR flattens the response into
a list (#7639) (#7691)

This reverts commit 3ad3f61.
mangalaman93 added a commit that referenced this pull request Jun 9, 2023
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.
mangalaman93 added a commit that referenced this pull request Jun 10, 2023
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.
mangalaman93 added a commit that referenced this pull request Jun 10, 2023
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.
mangalaman93 added a commit that referenced this pull request Jun 12, 2023
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.
dgraph-bot pushed a commit that referenced this pull request Jul 7, 2023
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.
mangalaman93 added a commit that referenced this pull request Aug 3, 2023
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.
mangalaman93 added a commit that referenced this pull request Aug 3, 2023
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.
mangalaman93 added a commit that referenced this pull request Aug 4, 2023
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.
mangalaman93 added a commit that referenced this pull request Aug 4, 2023
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.
mangalaman93 added a commit that referenced this pull request Aug 8, 2023
…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.
jbhamra1 pushed a commit that referenced this pull request Aug 17, 2023
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants