Core: TableMetadata Always Strips Trailing Slash From Location#6777
Conversation
Previously we could end up in situations where we unintentionally we would create // in file paths which is an issue with non-POSIX FileIOs.
|
@jackye1995 + @rdblue + @aokolnychyi + @danielcweeks + @amogh-jahagirdar + @findepi This fixes only instances of accidental trailing slashes in the table locations. This would have a slight change in behavior for tables which are currently using S3FIleIO and have a trailing slash. new files for those tables will end up in single slash paths. Old files will be unaffected. |
|
Cleaning up tests now, we have many tests that are using trailing slashes in table location |
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
One nit, but it's non-blocking at this point, just want to get yours and others takes on it. Thanks @RussellSpitzer !
| this.formatVersion = formatVersion; | ||
| this.uuid = uuid; | ||
| this.location = location; | ||
| this.location = location != null ? LocationUtil.stripTrailingSlash(location) : null; |
There was a problem hiding this comment.
Nit: I feel like we should just change LocationUtil.stripTrailingSlash to return null if a null location is passed in instead of throwing an NPE otherwise I think we have to do this null check everywhere where we don't have a location defined in advance. But it's a public utility which may be used beyond the Iceberg library itself so I understand not wanting to change it at this point.
There was a problem hiding this comment.
I think this is the only place location could be null? In all other places location is already initialized and must be non-null.
There was a problem hiding this comment.
I see if that's the case then we should just leave as is imo
|
Thanks for the fix @RussellSpitzer , and thanks for the review @amogh-jahagirdar , @nastra , @yyanyy ! I will rebase #6772 after this is merged. |
6 participants
Previously we could end up in situations where we unintentionally we would create // in file paths which is an issue with non-POSIX FileIOs.
This is a partial fix for #6758