-
Notifications
You must be signed in to change notification settings - Fork 287
Rest: Implement register table #1521
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
base: main
Are you sure you want to change the base?
Conversation
0bfda56
to
5c342f3
Compare
5c342f3
to
574069f
Compare
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.
Hi @gabeiglio , thanks for your contribution! I've left some suggestions
let metadata_location = response.metadata_location.as_ref().ok_or(Error::new( | ||
ErrorKind::DataInvalid, | ||
"Metadata location missing in `register_table` response!", | ||
))?; |
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.
nit: this can be slightly simplified using some syntax sugar
let Some(metadata_location) = response.metadata_location else { return Err(Error::new(...)) }
let file_io = self.load_file_io(metadata_location, None).await?;
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 think for this we would need to change the signature of load_file_io
since it is expecting a Option<&str>
. Which I think it would make sense to change since all the metadata_location
fields in the requests responses are String
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.
Thanks @gabeiglio for this pr, generally LGTM! Left some comments to fix.
574069f
to
de387fc
Compare
) -> Result<Table> { | ||
// TODO: Use overwrite in `insert_new_table` to overwrite metadata for an already existing table |
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.
Thinking this could be a 'good first issue'?
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 breaking change, and I would not do it now. We can just pass overwrite=false
to remote catalog. Currently java didn't support overwrite in api, and I don't think we need to do it now.
de387fc
to
064c927
Compare
Which issue does this PR close?
What changes are included in this PR?
Are these changes tested?
Im new to Rust so any feedback is welcomed! :)