fix: Make sort stable and handle nil comparison #1094

shahzadlone · 2023-02-14T08:00:15Z

Relevant issue(s)

Resolves #1071
Resolves #921
Blocks: #818, #1070

Description

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 Resolve panics caused by deeper nested orderby #833, which is issue interface conversion orderby panic #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, ... ]

- This does have a side effect which is if you sort in DESC the non-sortables / defaults are sorted to the beginning.
Example: LIST = 1{"x:1"}, 2{"x:2"}, *{"x:5"}, 3{"x:3"}, 2{"x:4"}
- ASC_SORTED_LIST = 1{"x:1"}, 2{"x:2"}, 2{"x:4"}, 3{"x:3"}, *{"x:5"}
- DESC_SORTED_LIST = *{"x:5"}, 3{"x:3"}, 2{"x:2"}, 2{"x:4"}, 1{"x:1"} (unsure if we want this behaviour).

More information in the issue.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

CI + make test

Specify the platform(s) on which this was tested:

WSL2 (Manjaro)

fredcarle · 2023-02-14T16:02:34Z

DESC_SORTED_LIST = *{"x:5"}, 3{"x:3"}, 2{"x:2"}, 2{"x:4"}, 1{"x:1"} (unsure if we want this behaviour).

I think that that is "normal" behaviour. Or maybe "expected" behaviour. I'm thinking about when I look things up on Kijiji or AutoTrader or other sites like that, if I sort prices from High to Low I get the highest price first. But if I sort prices from low to high I get the ones with no prices first and then the lowest prices.

BTW the I have a solution for the detect change failure. Will open the PR for it soon.

fredcarle

LGTM and the resulting behaviour is as I would expect. You can wait for other people's input if you want though.

orpheuslummis · 2023-02-14T18:39:56Z

DESC_SORTED_LIST = *{"x:5"}, 3{"x:3"}, 2{"x:2"}, 2{"x:4"}, 1{"x:1"} (unsure if we want this behaviour).

I think that that is "normal" behaviour. Or maybe "expected" behaviour. I'm thinking about when I look things up on Kijiji or AutoTrader or other sites like that, if I sort prices from High to Low I get the highest price first. But if I sort prices from low to high I get the ones with no prices first and then the lowest prices.

BTW the I have a solution for the detect change failure. Will open the PR for it soon.

either way we have the behavior, I suggest we should document clearly the behavior

shahzadlone · 2023-02-14T19:22:11Z

DESC_SORTED_LIST = *{"x:5"}, 3{"x:3"}, 2{"x:2"}, 2{"x:4"}, 1{"x:1"} (unsure if we want this behaviour).

I think that that is "normal" behaviour. Or maybe "expected" behaviour. I'm thinking about when I look things up on Kijiji or AutoTrader or other sites like that, if I sort prices from High to Low I get the highest price first. But if I sort prices from low to high I get the ones with no prices first and then the lowest prices.
BTW the I have a solution for the detect change failure. Will open the PR for it soon.

either way we have the behavior, I suggest we should document clearly the behavior

Agreed, any suggestions where you want that documentation to live?

AndrewSisley · 2023-02-14T21:55:43Z

This does have a side effect which is if you sort in DESC the non-sortables / defaults are sorted to the beginning

This seems really odd to me - it essentially means that nil/default is the highest value, not the lowest (as per Fred's comment). You can see this in the (modified) TestOneToManyToOneWithSumOfDeepOrderBySubTypeAndDeepOrderBySubtypeDescDirec

I think it is backwards to most/(all?) sorts I have come across. Nil is usually considered to be lower/less-than than any non-nil value when sorting?

With the current/proposed setup we have the below output:

a := [
    nil,
    nil,
    10,
    9,
    8,
    ...
]

AndrewSisley

I think the sort order is odd, and possibly misunderstood by Fred. Please dont merge until clarified/resolved. Code looks good

fredcarle · 2023-02-14T22:05:07Z

I think the sort order is odd, and possibly misunderstood by Fred

You're right. I thought about it wrong.

For DESC we should have

a := [
  10,
  9,
  8,
  ...
  nil,
  nil,
]

And for ASC we would have

a := [
  nil,
  nil,
  1,
  2,
  3,
  ...
]

fredcarle

Removing my approval. Sorry I had something in mind that I though was equivalent to what you had in the PR description but it was actually different. Let's revisit the order.

shahzadlone · 2023-02-15T07:03:54Z

@AndrewSisley Thanks for the confirmation I caught this last night as well, this fix has been pushed by handling nil comparisons properly, I remember this (somewhat indirectly) is why 2 of our tests also panic before.

@fredcarle Ready for re-review with the sort handling nil like we expect.

PR Description has been updated.

codecov · 2023-02-15T07:07:16Z

Codecov Report

Merging #1094 (ca33aa2) into develop (efb61cd) will decrease coverage by 0.02%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           develop    #1094      +/-   ##
===========================================
- Coverage    67.35%   67.33%   -0.02%     
===========================================
  Files          181      181              
  Lines        16730    16744      +14     
===========================================
+ Hits         11268    11275       +7     
- Misses        4513     4518       +5     
- Partials       949      951       +2

Impacted Files	Coverage Δ
db/base/compare.go	`65.21% <60.00%> (-4.28%)`	⬇️
planner/values.go	`82.19% <95.00%> (-0.42%)`	⬇️
planner/order.go	`83.15% <0.00%> (-3.16%)`	⬇️
net/server.go	`60.84% <0.00%> (+0.97%)`	⬆️

AndrewSisley

LGTM - cheers for sorting this out

AndrewSisley · 2023-02-15T15:56:00Z

tests/integration/query/one_to_many_to_one/with_order_limit_test.go

 	}

-	testUtils.AssertPanicAndSkipChangeDetection(t, func() { executeTestCase(t, test) })


sweet :) Nice fix

AndrewSisley · 2023-02-15T15:59:07Z

tests/integration/query/one_to_many_to_one/with_order_test.go

-					"yearOpened": uint64(1999),
-				},
-			},
-			{


praise: Cheers for sorting this out - is horrible that we let this test into develop though, should have been flagged in review

fredcarle

LGTM. Thanks for changing the behaviour and sorry again for my confusion.

…SubTypeOfBothDescAndAsc`

…epOrderBySubTypeAndDeepOrderBySubtypeAscDirections`

- 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, ... ] ```

- 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, ... ] ```

shahzadlone added bug Something isn't working area/testing Related to any test or testing suite area/planner Related to the planner system labels Feb 14, 2023

shahzadlone added this to the DefraDB v0.5 milestone Feb 14, 2023

shahzadlone requested a review from a team February 14, 2023 08:00

shahzadlone self-assigned this Feb 14, 2023

shahzadlone added the action/no-benchmark Skips the action that runs the benchmark. label Feb 14, 2023

sourcenetwork deleted a comment from source-devs Feb 14, 2023

fredcarle approved these changes Feb 14, 2023

View reviewed changes

AndrewSisley requested changes Feb 14, 2023

View reviewed changes

fredcarle requested changes Feb 14, 2023

View reviewed changes

shahzadlone mentioned this pull request Feb 15, 2023

chore: Bump to GoLang v1.19 #818

Merged

shahzadlone force-pushed the lone/fix/unstable-sort-ordering-for-cids branch 2 times, most recently from 52dd021 to 1a59d71 Compare February 15, 2023 07:00

shahzadlone changed the title ~~fix: Make sort stable and ensure less is valid~~ fix: Make sort stable and handle nil comparison Feb 15, 2023

shahzadlone requested review from fredcarle and AndrewSisley February 15, 2023 07:04

shahzadlone force-pushed the lone/fix/unstable-sort-ordering-for-cids branch from 1a59d71 to fd35ebc Compare February 15, 2023 07:19

AndrewSisley approved these changes Feb 15, 2023

View reviewed changes

fredcarle approved these changes Feb 15, 2023

View reviewed changes

shahzadlone added 3 commits February 15, 2023 14:41

PR: Fix unstable sort, nil sorting, & less func.

bc7cfd7

PR: Fix test that used to panic called `TestOneToManyToOneDeepOrderBy…

27a6aa5

…SubTypeOfBothDescAndAsc`

PR: Fix test that used to panic called `TestOneToManyToOneWithSumOfDe…

15361a8

…epOrderBySubTypeAndDeepOrderBySubtypeAscDirections`

shahzadlone added 3 commits February 15, 2023 14:41

PR: Document the sub-task todo issue number for remaining panics.

69a53fa

PR: Fix the commit test cids to stable sort's

1bff975

PR: Fix the 1-M-1 case with proper nil sort.

ca33aa2

shahzadlone force-pushed the lone/fix/unstable-sort-ordering-for-cids branch from fd35ebc to ca33aa2 Compare February 15, 2023 19:41

shahzadlone merged commit 9a0fe20 into develop Feb 15, 2023

shahzadlone deleted the lone/fix/unstable-sort-ordering-for-cids branch February 15, 2023 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Make sort stable and handle nil comparison #1094

fix: Make sort stable and handle nil comparison #1094

shahzadlone commented Feb 14, 2023 •

edited

Loading

fredcarle commented Feb 14, 2023

fredcarle left a comment

orpheuslummis commented Feb 14, 2023

shahzadlone commented Feb 14, 2023

AndrewSisley commented Feb 14, 2023 •

edited

Loading

AndrewSisley left a comment

fredcarle commented Feb 14, 2023

fredcarle left a comment

shahzadlone commented Feb 15, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Feb 15, 2023

AndrewSisley Feb 15, 2023

fredcarle left a comment

		}

		testUtils.AssertPanicAndSkipChangeDetection(t, func() { executeTestCase(t, test) })

fix: Make sort stable and handle nil comparison #1094

fix: Make sort stable and handle nil comparison #1094

Conversation

shahzadlone commented Feb 14, 2023 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

fredcarle commented Feb 14, 2023

fredcarle left a comment

Choose a reason for hiding this comment

orpheuslummis commented Feb 14, 2023

shahzadlone commented Feb 14, 2023

AndrewSisley commented Feb 14, 2023 • edited Loading

AndrewSisley left a comment

Choose a reason for hiding this comment

fredcarle commented Feb 14, 2023

fredcarle left a comment

Choose a reason for hiding this comment

shahzadlone commented Feb 15, 2023 • edited Loading

codecov bot commented Feb 15, 2023 • edited Loading

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley Feb 15, 2023

Choose a reason for hiding this comment

AndrewSisley Feb 15, 2023

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

shahzadlone commented Feb 14, 2023 •

edited

Loading

AndrewSisley commented Feb 14, 2023 •

edited

Loading

shahzadlone commented Feb 15, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading