-
Notifications
You must be signed in to change notification settings - Fork 378
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
[#5730] feat(client-python): Add sorts expression #5879
base: main
Are you sure you want to change the base?
Conversation
hi @SophieTech88 I help you improve this PR, Please review it, Thanks. |
|
||
@staticmethod | ||
def ascending(expression: Expression) -> SortImpl: | ||
"""Creates a sort order with ascending direction and nulls first.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Xun This document is slightly different from Java client, do we need to align these two docs ?
/**
* Create a sort order by the given expression with the ascending sort direction and nulls first
* ordering.
*
* @param expression The expression to sort by
* @return The created sort order
*/
|
||
@staticmethod | ||
def descending(expression: Expression) -> SortImpl: | ||
"""Creates a sort order with descending direction and nulls last.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
direction: SortDirection, | ||
null_ordering: NullOrdering = None, | ||
) -> SortImpl: | ||
"""Creates a sort order with the given direction and optionally specified null ordering.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
overall, LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM!
_direction: str | ||
_default_null_ordering: NullOrdering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that Python can derive member variables, but I feel like showing all member variables explicitly. It will make the code easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that Python can derive member variables, but I feel like showing all member variables explicitly. It will make the code easier to read.
_expression: Expression | ||
_direction: SortDirection | ||
_null_ordering: NullOrdering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that Python can derive member variables, but I feel like showing all member variables explicitly. It will make the code easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you define here are class properties, not instance properties.
Python is different from Java on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OH, you are right. I will fix this problem.
What changes were proposed in this pull request?
Implement sorts expression in python client, add unit test.
Why are the changes needed?
We need to support the sorts expressions in python client
Fix: #5730
Does this PR introduce any user-facing change?
No
How was this patch tested?
Need to pass all unit tests.