-
Notifications
You must be signed in to change notification settings - Fork 1.8k
clean up duplicate information in FileOpener trait #17956
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
d649a43 to
d5a0cd9
Compare
d5a0cd9 to
3a5216a
Compare
xudong963
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.
Good cleaning, thank you.
I saw there are some API changes, so added the api-change label.
|
Do you think it's worth adding this to the upgrade guide? I think it's quite straightforward and will impact few people. So maybe not worth the noise? |
| &self, | ||
| _partition_index: usize, | ||
| file_meta: FileMeta, | ||
| file: PartitionedFile, |
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.
1 nit: we probably can name it as partition_file to be the same format as other function params
| file_meta: FileMeta, | ||
| _file: PartitionedFile, | ||
| ) -> Result<FileOpenFuture> { | ||
| fn open(&self, file: PartitionedFile) -> Result<FileOpenFuture> { |
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.
ditto
| &self, | ||
| partition_index: usize, | ||
| file_meta: FileMeta, | ||
| file: PartitionedFile, |
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.
ditto
|
@comphead I've renamed all uses to |
|
Hmm I tried to put this in the queue but it seems to have merged immediately? I think it was ready but would have liked to see CI run. Not sure if it was a glitch in the GitHub UI or what. Anwyay I'll fix any issues in |
Yeah, we are working on the merge-queue but it doesn't actually re-run CI yet -- see reason here: #6880 (comment) |
| file_meta: FileMeta, | ||
| _file: PartitionedFile, | ||
| ) -> Result<FileOpenFuture> { | ||
| fn open(&self, partitioned_file: PartitionedFile) -> Result<FileOpenFuture> { |
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.
this is a nice improvement I agree
* clean up duplicate information in FileOpener trait * remove unused struct * remove more * rename paramter * more renames * minimize diff * fix renames * fix renames * more fixes
Ever since #16014 we've been passing duplicate information in.
FileMetais created entirely fromPartitionedFile. This simplifies APIs and removes an unnecessary struct.