Sorting Tables #1471
Sorting Tables #1471
Conversation
kustosz
requested review from
iamrecursion,
radeusgd and
wdanilo
as code owners
| ## A rule used for sorting table-like structures. | ||
|
|
||
| Arguments: | ||
| - column: a value representing the underlying storage this rule is |
There was a problem hiding this comment.
"The data by which x is being sorted" or similar.
| ## A rule used for sorting table-like structures. | ||
|
|
||
| Arguments: | ||
| - column: a value representing the underlying storage this rule is |
There was a problem hiding this comment.
Please wrap directly under the bullet, not at a new level of indentation.
| - comparator: a function taking two elements of the underlying column | ||
| and returning an `Ordering`. The function may be |
There was a problem hiding this comment.
"Underlying column" -> "data being sorted by" or similar.
| - order: specifies whether the table should be sorted in an ascending | ||
| or descending order. The default value of `Nothing` delegates | ||
| the decision to the sorting function. |
There was a problem hiding this comment.
This should say something about it being the Ordering type in the standard library.
| argument is independent from `order`, i.e. missing | ||
| values will always be sorted according to this rule, | ||
| ignoring the ascending / descending setting. | ||
| type Order_Rule column comparator=Nothing order=Nothing missing_last=Nothing |
There was a problem hiding this comment.
Explain what Nothing means for missing_last.
| - order: specifies the default sort order for this operation. All the | ||
| rules specified in the `by` argument will default to this | ||
| setting, unless specified in the rule. |
There was a problem hiding this comment.
Say that this is Bases Ordering.
| - missing_last: specifies the default placement of missing values when | ||
| compared to non-missing ones. This setting may be | ||
| overriden by the particular rules of the `by` argument. |
There was a problem hiding this comment.
This should contain more details (like for Order_Rule).
| Note that certain table backends (such us database | ||
| connectors) may choose to ignore this field. |
There was a problem hiding this comment.
Maybe instead choose to ignore it should say not support this field being set to anything other than Nothing?
What I'm trying to convey is that the backend should check this field and fail with an error if it cannot support the operation, instead of silently ignoring its value (which could be super-confusing).
| - missing_last: whether the missing values should be placed at the | ||
| beginning or end of the sorted table. Note that this | ||
| argument is independent from `order`, i.e. missing | ||
| values will always be sorted according to this rule, | ||
| ignoring the ascending / descending setting. |
There was a problem hiding this comment.
I'd add here the note as above, about Nothing delegating this decision to the sorting function.
| order_bool = case order of | ||
| Sort_Order.Ascending -> True | ||
| _ -> False |
There was a problem hiding this comment.
Isn't it better to just add Sort_Order.Descending here?
If for some weird reason we get something else here, won't it be more meaningful to fail with inexhaustive pattern match saying that the argument was unexpected instead of silently falling back to some default?
| _ -> False | ||
| case rule of | ||
| Text -> | ||
| column = this.at rule |
There was a problem hiding this comment.
Since at is used here in this way, I'd suggest changing its signature to Text -> Column ! UnknownColumnError and making at throw if column is not found instead of returning Nothing. Otherwise this code may fail in a strange way.
| * Returns a new storage, ordered according to the rules specified in a mask. | ||
| * | ||
| * @param mask an order mask specifying the reordering | ||
| * @return a storage resulting from applying the reordering rules |
There was a problem hiding this comment.
It returns an Index, not a Storage.
Pull Request Description
Introduces sorting operations on tables.
Important Notes
Checklist
Please include the following checklist in your PR: