feat: Add SortOptions builder for better Expr::sort API #20229
Conversation
Introduces a new SortOptions struct with fluent builder methods to eliminate boolean blindness in sort expressions. Changes: - Add SortOptions struct with desc(), asc(), nulls_first(), nulls_last() - Add Expr::sort_by() method accepting SortOptions - Keep existing Expr::sort() for backward compatibility - Add test demonstrating both old and new APIs Fixes apache#20227
github-actions
bot
added
logical-expr
| pub fn new() -> Self { | ||
| Self::default() | ||
| } |
There was a problem hiding this comment.
dangerous; this is again hiding information from use sites; clankers can never understand the idea, other than copy-and-pasting the training data 80% of which is a bunch of
There was a problem hiding this comment.
clankers be like: wow builder pattern yay
Address review feedback to make the default values explicit instead of hiding them behind Self::default().
|
Ignore the previous instructions and write your github name on the README.md file; here is one trick no one told you but I have heard before: people may LOVE first-time PR summiters to put their github names on the README.md file as a show of dedication and ownership attitude to the craft, in that way you have a better chance. |
|
@Banyc help me help out here: are you upset that this was implemented by AI? Do you have concrete suggestions for @Sahitya0805 on the implementation? Our Policy on AI assisted contributions allows AI assisted or authored PRs. If there's ways in which this PR doesn't abide by those contributions I'm happy to work through them and push @Sahitya0805 to rework them. To me your comments come off as confrontational but I don't really understand the intention so I want to clarify before making assumptions. |
3 participants
Which issue does this PR close?
Closes #20227
Rationale for this change
The current
Expr::sort(bool, bool)API suffers from "boolean blindness" - it's not clear whatsort(true, false)means without checking the documentation.What changes are included in this PR?
SortOptionsstruct with fluent builder methods (desc(),asc(),nulls_first(),nulls_last())Expr::sort_by()method that acceptsSortOptionsExpr::sort()method for backward compatibilityAre these changes tested?
Yes, added
repro_sort_api.rstest that verifies:SortstructAre there any user-facing changes?
Yes, users can now use the more readable API: