Feat/permsort #435

2021-11-14T20:19:09+03:00

alexismanin commented

2021-11-14 20:19:09 +03:00

(Migrated from github.com)

Hello,

This PR adds a prototype for permutation sorting. Related issue: #431
This only adds two public extensions:

Buffer<V: Comparable>.permSort()
ColumnarData<Buffer<V : Comparable>>.permSort()

Notes:

As I do not know well the project, I am not sure the PR follows the right coding style.
Not sure either to have put code at the right place
I've tried to add my thoughts about a common API for data access by key/index.

I wait for any guidance and feedback to improve the PR.

Thank you,

Hello, This PR adds a prototype for permutation sorting. Related issue: #431 This only adds two public extensions: * `Buffer<V: Comparable>.permSort()` * `ColumnarData<Buffer<V : Comparable>>.permSort()` Notes: * As I do not know well the project, I am not sure the PR follows the right coding style. * Not sure either to have put code at the right place * I've tried to add my thoughts about a common API for data access by key/index. I wait for any guidance and feedback to improve the PR. Thank you,

🚀 1

alexismanin commented

2021-11-15 16:06:27 +03:00

(Migrated from github.com)

Ok, so I'll try to fix the branch (I admit I have only launched jvmTest task while working, my mistake).

Before that, do you want me to rebase it over a specific branch (dev ?) ?

Ok, so I'll try to fix the branch (I admit I have only launched *jvmTest* task while working, my mistake). Before that, do you want me to rebase it over a specific branch (dev ?) ?

altavir commented

2021-11-15 22:13:25 +03:00

(Migrated from github.com)

Yes, all contributions are made to dev.

Yes, all contributions are made to `dev`.

altavir (Migrated from github.com) requested changes 2021-11-17 12:24:36 +03:00

altavir (Migrated from github.com) left a comment

Using a generic ordered index structure seems to be an overengineering. We do not have the aim to cover all data structures like database entities. On the other hand, using custom sequences for indexing significantly impacts performance, which could not be mitigated.

I suggest sticking with Buffers. We can add new functionality by wrapping everything we need in Buffer. The Buffer interface covers any random-access structure.

Using a generic ordered index structure seems to be an overengineering. We do not have the aim to cover all data structures like database entities. On the other hand, using custom sequences for indexing significantly impacts performance, which could not be mitigated. I suggest sticking with Buffers. We can add new functionality by wrapping everything we need in Buffer. The Buffer interface covers any random-access structure.

alexismanin (Migrated from github.com) reviewed 2021-11-17 14:43:26 +03:00

alexismanin commented

2021-11-17 14:53:53 +03:00

(Migrated from github.com)

Other generic question : As I'll rework my PR to provide a single Buffer<V : Comparable>.permSort() function, I've got code style/management questions :

Do you want me to move the code in another file/package of the project, or does is look fine as is ?
Should I leave descending configuration as a boolean argument, or should I create two methods permSort and permSortDescending ?
I think I should mark it with @PerformancePitfall for now, because standard utilities does not allow to sort indices in-place, so the sorting causes memory-management overhead. Do you think it is appropriate ?

Other generic question : As I'll rework my PR to provide a single `Buffer<V : Comparable>.permSort()` function, I've got code style/management questions : * Do you want me to move the code in another file/package of the project, or does is look fine as is ? * Should I leave descending configuration as a boolean argument, or should I create two methods `permSort` and `permSortDescending` ? * I think I should mark it with `@PerformancePitfall` for now, because standard utilities does not allow to sort indices in-place, so the sorting causes memory-management overhead. Do you think it is appropriate ?

altavir commented

2021-11-17 16:01:05 +03:00

(Migrated from github.com)

Do you want me to move the code in another file/package of the project, or does is look fine as is ?

I think it is OK

Should I leave descending configuration as a boolean argument, or should I create two methods permSort and permSortDescending ?

Good question. I am not sure yet. It seems like it is a good idea to pass comparator to the method for non-naturally ordered entities. For naturally ordered it seems to simple enough to reverse the resulting list.

I think I should mark it with @PerformancePitfall for now, because standard utilities does not allow to sort indices in-place, so the sorting causes memory-management overhead. Do you think it is appropriate ?

Indeed. We can always remove it later.

> * Do you want me to move the code in another file/package of the project, or does is look fine as is ? I think it is OK > * Should I leave descending configuration as a boolean argument, or should I create two methods `permSort` and `permSortDescending` ? Good question. I am not sure yet. It seems like it is a good idea to pass comparator to the method for non-naturally ordered entities. For naturally ordered it seems to simple enough to reverse the resulting list. > * I think I should mark it with `@PerformancePitfall` for now, because standard utilities does not allow to sort indices in-place, so the sorting causes memory-management overhead. Do you think it is appropriate ? Indeed. We can always remove it later.

alexismanin commented

2021-11-18 12:25:42 +03:00

(Migrated from github.com)

It seems like it is a good idea to pass comparator to the method for non-naturally ordered entities.

You're right. I'll add fun Buffer<V>.permSortWith(Comparator<V>) and fun <V, C : Comparable> Buffer<V>permSortBy((V) -> C) alternatives.

For descending, it sounds like a good idea to manage it directly when sorting. If we manage it directly, users don't have to add a manual inversion themselves (which would require an additional O(n/2) cost). Kotlin stdlib rather use separate functions for ascending/descending sorting (surely to make the inversion more explicit). Maybe I should use the same strategy.

> It seems like it is a good idea to pass comparator to the method for non-naturally ordered entities. You're right. I'll add ` fun Buffer<V>.permSortWith(Comparator<V>)` and `fun <V, C : Comparable> Buffer<V>permSortBy((V) -> C)` alternatives. For descending, it sounds like a good idea to manage it directly when sorting. If we manage it directly, users don't have to add a manual inversion themselves (which would require an additional O(n/2) cost). Kotlin stdlib rather use separate functions for ascending/descending sorting (surely to make the inversion more explicit). Maybe I should use the same strategy.

altavir (Migrated from github.com) approved these changes 2021-11-21 20:21:04 +03:00

Sign in to join this conversation.

No reviewers

altavir