-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Provide a builder for ListingOptions #4180
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
Conversation
alamb
left a comment
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.
Thank you @mvanschellebeeck -- I think it looks great!
I'll leave this PR open for a day or two so others can comment if they want
cc @tustvold
| .with_table_partition_cols(vec![]) | ||
| .with_collect_stat(true) | ||
| .with_target_partitions(1); |
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.
We could further simplify this code and avoid explicitly specifying the default values (last three lines)
But that can also be done as a follow on PR
| self | ||
| } | ||
|
|
||
| /// Set number of target partitions on [`ListingOptions`] and returns self. |
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.
❤️
andygrove
left a comment
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.
LGTM. Thanks @mvanschellebeeck and @alamb
|
Thanks again @mvanschellebeeck -- I took the liberty of merging up from master to resolve the conflicts (that I caused with d2814c9 / #4170) |
|
I couldn't push directly to this PR But I put my proposed changes in |
|
Thanks again @mvanschellebeeck -- this is so much nicer |
Which issue does this PR close?
Closes #4178
What changes are included in this PR?
Improves the ergonomics of Listing Options by providing a builder.