Add additional documentation and builder APIs to SortOptions#6441
Add additional documentation and builder APIs to SortOptions#6441alamb merged 5 commits intoapache:masterfrom
SortOptions#6441Conversation
| descending: true, | ||
| nulls_first: false, | ||
| }, | ||
| SortOptions::default().desc().with_nulls_first(false), |
There was a problem hiding this comment.
Here is an example of using the new API
Not only is this more concise, I believe the intent is easier to reason about now (asc/desc)
| /// ``` | ||
| /// | ||
| /// # Example operations | ||
| /// It is also possible to negate the sort options using the `!` operator. |
There was a problem hiding this comment.
I found the use of the ! in DataFusion a bit surprising at first
alamb
changed the title
SortOptionsSortOptions
arrow-schema/src/lib.rs
Outdated
| if self.nulls_first { | ||
| // purposely don't display default NULLs value |
There was a problem hiding this comment.
The default for nulls_first doesn't seem to be documented, so wouldn't it be clearer to print the value here?
There was a problem hiding this comment.
Yes, I think you are right -- it is more consistent even if more verbose
| /// let options = SortOptions::default() | ||
| /// .desc() | ||
| /// .with_nulls_first(true); | ||
| /// assert_eq!(options.to_string(), "DESC NULLS FIRST"); |
There was a problem hiding this comment.
| /// assert_eq!(options.to_string(), "DESC NULLS FIRST"); | |
| /// assert_eq!(options.to_string(), "DESC"); |
If fmt continues to print nothing for the default.
| } | ||
|
|
||
| /// Set this sort options to sort nulls first if argument is true | ||
| pub fn with_nulls_first(mut self, nulls_first: bool) -> Self { |
There was a problem hiding this comment.
We can keep this API for sure, but to be consistent with what you propose at apache/datafusion#12595 (nulls_first() / nulls_last() API for PhysicalSortExpr), should we add nulls_first() and nulls_last() here, too?
|
Close/reopen to trigger CI |
|
integration CI is failing due to #6448 |
|
Thanks again @etseidl and @berkaysynnada for the review |
3 participants
Which issue does this PR close?
Related to apache/datafusion#12446
Rationale for this change
While working on apache/datafusion#12562 I found I kept having to do mental gymnastics to remember the default value of
SortOptionsI also found creating them was a bit confuisng as if I want to make
ASCthe default I have to dodescending: falseand the double negative was making my head hurt.What changes are included in this PR?
ASCsort withoptions.asc()rather thandescending:falseAre there any user-facing changes?
New API and docs, no changes to existing APIs