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

[feature] #3116: Add sorting of any values (not only u128) #3118

Merged
merged 1 commit into from Feb 10, 2023

Conversation

pesterev
Copy link
Contributor

@pesterev pesterev commented Feb 7, 2023

Signed-off-by: Vladimir Pesterev pesterev@pm.me

Description of the Change

Now we can sort by any values from metadata. Not only by u128. Also, this sorting implementation places all elements that cannot be sorted at the head of a result list.

Issue

Resolves #3116

Benefits

Better sorting feature.

Possible Drawbacks

None

Usage Examples or Tests [optional]

Take a look at tests in these changes.

Alternate Designs [optional]

Another design is to remove all elements that we can't sort.

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Feb 7, 2023
appetrosyan
appetrosyan previously approved these changes Feb 7, 2023
client/src/client.rs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Merging #3118 (9686570) into iroha2-dev (a4d5c9f) will increase coverage by 2.20%.
The diff coverage is 65.65%.

@@              Coverage Diff               @@
##           iroha2-dev    #3118      +/-   ##
==============================================
+ Coverage       62.33%   64.53%   +2.20%     
==============================================
  Files             169      172       +3     
  Lines           31218    34188    +2970     
==============================================
+ Hits            19459    22063    +2604     
- Misses          11759    12125     +366     
Impacted Files Coverage Δ
cli/src/main.rs 1.09% <0.00%> (ø)
cli/src/torii/mod.rs 27.65% <ø> (ø)
cli/src/torii/utils.rs 72.00% <0.00%> (-12.85%) ⬇️
client_cli/src/main.rs 0.24% <0.00%> (-0.01%) ⬇️
config/base/src/lib.rs 91.57% <ø> (+55.21%) ⬆️
config/src/lib.rs 33.33% <ø> (ø)
config/src/logger.rs 70.21% <ø> (ø)
config/src/path.rs 0.00% <0.00%> (ø)
core/src/lib.rs 100.00% <ø> (ø)
core/src/smartcontracts/isi/block.rs 85.71% <ø> (-3.18%) ⬇️
... and 175 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pesterev
Copy link
Contributor Author

pesterev commented Feb 7, 2023

Also, @arndey suggests adding one more query parameter to specify what the sorting should do with unsortable elements. For example,
sort_algo = "remove" - remove all unsortable elements
sort_algo = "head" - place unsortable elements at the head of a list
sort_algo = "tail" - place unsortable elements at the tail of a list

@Erigara
Copy link
Contributor

Erigara commented Feb 7, 2023

To me it's sound like unnecessary complications on the backend side.

Erigara
Erigara previously approved these changes Feb 7, 2023
@pesterev pesterev self-assigned this Feb 8, 2023
@pesterev
Copy link
Contributor Author

pesterev commented Feb 8, 2023

To me it's sound like unnecessary complications on the backend side.

@Erigara we have to choose one of the behavior algorithms anyway if we aren't going to implement @arndey suggestion. The current implementation in this PR is to place unsortable elements at the head of a result list. If it's okay, let's merge it.

@Erigara
Copy link
Contributor

Erigara commented Feb 8, 2023

place unsortable elements at the head of a result list
fine to me

appetrosyan
appetrosyan previously approved these changes Feb 8, 2023
@pesterev
Copy link
Contributor Author

pesterev commented Feb 9, 2023

Dear reviewers,

Andrey (@arndey) and I decided to change the algorithm for ordering unsortable elements, so now it places unsortable elements at the tail of a result list. And it makes sense. That implementation is more convenient for manipulating a result list.

For example,
We have a list like this:
[ A has 1 in metadata, B has 0 in metadata, C has nothing in metadata, D also has nothing in metadata, E has 2 in metadata ]

The previous implementation returned => [C, D, B, A, E]
The current implementation will return => [B, A, E, C, D]

appetrosyan
appetrosyan previously approved these changes Feb 9, 2023
Signed-off-by: Vladimir Pesterev <pesterev@pm.me>
@pesterev pesterev merged commit a4a74fb into hyperledger:iroha2-dev Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants