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

Resolve panics caused by deeper nested orderby #833

Closed
shahzadlone opened this issue Sep 20, 2022 · 0 comments · Fixed by #1175
Closed

Resolve panics caused by deeper nested orderby #833

shahzadlone opened this issue Sep 20, 2022 · 0 comments · Fixed by #1175
Assignees
Labels
area/testing Related to any test or testing suite feature New feature or request
Milestone

Comments

@shahzadlone
Copy link
Member

shahzadlone commented Sep 20, 2022

This is an umbrella ticket to tackle panics happening when deeper nesting of ordering by sybtype queries are issues.

There is little chance that #826 might be related.

Here are some documented tests already in the source code (that panic):

There might be separate reasons for these panics, so make more issues upon investigation to separate them out, for example from the above tests, some might be mapper-related panic, and some related to a nil comparison.

Here is a simple example:
publishers { _sum(authors: {field: age, order: {books: rating DESC}}) { name } }

Test this with requesting children fields up till the leaf subtype node, and also test this without any requested children.

EDIT: As mentioned breaking these out into two sub-tasks (#920 and #921).

@shahzadlone shahzadlone added feature New feature or request area/testing Related to any test or testing suite labels Sep 20, 2022
@shahzadlone shahzadlone added this to the DefraDB v0.4 milestone Sep 20, 2022
@shahzadlone shahzadlone changed the title Test Deeper nested orderby on parent by subtype of subtype child Resolve panics caused by deeper nested orderby on parent by subtype Sep 23, 2022
@shahzadlone shahzadlone changed the title Resolve panics caused by deeper nested orderby on parent by subtype Resolve panics caused by deeper nested orderby Sep 23, 2022
shahzadlone added a commit that referenced this issue Sep 23, 2022
1) Resolves #588 

2) Description
- Fix: Bug that caused panic when result was ordered by the subtype / relation / join field and there were no corresponding child fields requested (from that joined type).
- Test: Added test to ensure the query now returns valid results without panicing.
- Test: Added ton more tests according to code review suggestions.

3) Limitations
- Some tests were added to document panics which will be resolved outside this PR in #833.
- Found bug #826 as part of this PR.
- Found bug #827 (fixed already).
@shahzadlone shahzadlone self-assigned this Oct 7, 2022
@shahzadlone shahzadlone modified the milestones: DefraDB v0.4, DefraDB v0.5 Dec 9, 2022
shahzadlone added a commit that referenced this issue Feb 15, 2023
- Resolves #1071

- Resolves #921

- This change uses stable sort and ensures the `Less` interface implementation only returns true if the comparison is less, otherwise returns false (this is not the full story as for when we do `DESC` the less to us then is the check of if it's greater than instead of less than). Note: Ensures the ordering of the input array is preserved, so the output is always stable.

- This change works on both GoLang v1.18.5 and v1.19.5.

- This also resolves some nil panics we were having before (one subtask of #833, which is issue #921).
	1) Fixes the test: `TestOneToManyToOneDeepOrderBySubTypeOfBothDescAndAsc`
	2) Fixes the test: `TestOneToManyToOneWithSumOfDeepOrderBySubTypeAndDeepOrderBySubtypeAscDirections`

- Ensures our sort handles `nil` sorting properly for `ASC` and `DESC`:
```
DESC = [10, 9, 8, ... nil, nil]
ASC  = [nil, nil, 1, 2, 3, ... ]
```
@islamaliev islamaliev assigned islamaliev and unassigned shahzadlone Mar 7, 2023
shahzadlone added a commit that referenced this issue Apr 13, 2023
- Resolves #1071

- Resolves #921

- This change uses stable sort and ensures the `Less` interface implementation only returns true if the comparison is less, otherwise returns false (this is not the full story as for when we do `DESC` the less to us then is the check of if it's greater than instead of less than). Note: Ensures the ordering of the input array is preserved, so the output is always stable.

- This change works on both GoLang v1.18.5 and v1.19.5.

- This also resolves some nil panics we were having before (one subtask of #833, which is issue #921).
	1) Fixes the test: `TestOneToManyToOneDeepOrderBySubTypeOfBothDescAndAsc`
	2) Fixes the test: `TestOneToManyToOneWithSumOfDeepOrderBySubTypeAndDeepOrderBySubtypeAscDirections`

- Ensures our sort handles `nil` sorting properly for `ASC` and `DESC`:
```
DESC = [10, 9, 8, ... nil, nil]
ASC  = [nil, nil, 1, 2, 3, ... ]
```
shahzadlone added a commit to shahzadlone/defradb that referenced this issue Feb 23, 2024
1) Resolves sourcenetwork#588 

2) Description
- Fix: Bug that caused panic when result was ordered by the subtype / relation / join field and there were no corresponding child fields requested (from that joined type).
- Test: Added test to ensure the query now returns valid results without panicing.
- Test: Added ton more tests according to code review suggestions.

3) Limitations
- Some tests were added to document panics which will be resolved outside this PR in sourcenetwork#833.
- Found bug sourcenetwork#826 as part of this PR.
- Found bug sourcenetwork#827 (fixed already).
shahzadlone added a commit to shahzadlone/defradb that referenced this issue Feb 23, 2024
- Resolves sourcenetwork#1071

- Resolves sourcenetwork#921

- This change uses stable sort and ensures the `Less` interface implementation only returns true if the comparison is less, otherwise returns false (this is not the full story as for when we do `DESC` the less to us then is the check of if it's greater than instead of less than). Note: Ensures the ordering of the input array is preserved, so the output is always stable.

- This change works on both GoLang v1.18.5 and v1.19.5.

- This also resolves some nil panics we were having before (one subtask of sourcenetwork#833, which is issue sourcenetwork#921).
	1) Fixes the test: `TestOneToManyToOneDeepOrderBySubTypeOfBothDescAndAsc`
	2) Fixes the test: `TestOneToManyToOneWithSumOfDeepOrderBySubTypeAndDeepOrderBySubtypeAscDirections`

- Ensures our sort handles `nil` sorting properly for `ASC` and `DESC`:
```
DESC = [10, 9, 8, ... nil, nil]
ASC  = [nil, nil, 1, 2, 3, ... ]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Related to any test or testing suite feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants