You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "RussellSpitzer (via GitHub)" <gi...@apache.org> on 2023/02/08 21:45:24 UTC
[GitHub] [iceberg] RussellSpitzer opened a new pull request, #6777: Core: TableMetadata Always Strips Trailing Slash From Location
RussellSpitzer opened a new pull request, #6777:
URL: https://github.com/apache/iceberg/pull/6777
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on pull request #6777: Core: TableMetadata Always Strips Trailing Slash From Location
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #6777:
URL: https://github.com/apache/iceberg/pull/6777#issuecomment-1424717284
Thanks for the fix @RussellSpitzer , and thanks for the review @amogh-jahagirdar , @nastra , @yyanyy ! I will rebase #6772 after this is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #6777: Core: TableMetadata Always Strips Trailing Slash From Location
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 merged PR #6777:
URL: https://github.com/apache/iceberg/pull/6777
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #6777: Core: TableMetadata Always Strips Trailing Slash From Location
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #6777:
URL: https://github.com/apache/iceberg/pull/6777#discussion_r1100883382
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -290,7 +291,7 @@ public String toString() {
this.metadataFileLocation = metadataFileLocation;
this.formatVersion = formatVersion;
this.uuid = uuid;
- this.location = location;
+ this.location = location != null ? LocationUtil.stripTrailingSlash(location) : null;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] findepi commented on pull request #6777: Core: TableMetadata Always Strips Trailing Slash From Location
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #6777:
URL: https://github.com/apache/iceberg/pull/6777#issuecomment-1423751198
cc @ebyhr @electrum @alexjo2144
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #6777: Core: TableMetadata Always Strips Trailing Slash From Location
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #6777:
URL: https://github.com/apache/iceberg/pull/6777#issuecomment-1423281287
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #6777: Core: TableMetadata Always Strips Trailing Slash From Location
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6777:
URL: https://github.com/apache/iceberg/pull/6777#discussion_r1100891080
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -290,7 +291,7 @@ public String toString() {
this.metadataFileLocation = metadataFileLocation;
this.formatVersion = formatVersion;
this.uuid = uuid;
- this.location = location;
+ this.location = location != null ? LocationUtil.stripTrailingSlash(location) : null;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6777: Core: TableMetadata Always Strips Trailing Slash From Location
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #6777:
URL: https://github.com/apache/iceberg/pull/6777#discussion_r1101061561
##########
core/src/test/java/org/apache/iceberg/TestTableMetadata.java:
##########
@@ -1562,6 +1563,19 @@ public void testNoReservedPropertyForTableMetadataCreation() {
1));
}
+ @Test
+ public void testNoTrailingLocationSlash() {
+ String locationWithSlash = "/with_trailing_slash/";
+ String locationWithoutSlash = "/with_trailing_slash";
Review Comment:
nit: `/with_trailing_slash` -> `/without_trailing_slash`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #6777: Core: TableMetadata Always Strips Trailing Slash From Location
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #6777:
URL: https://github.com/apache/iceberg/pull/6777#discussion_r1100906013
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -290,7 +291,7 @@ public String toString() {
this.metadataFileLocation = metadataFileLocation;
this.formatVersion = formatVersion;
this.uuid = uuid;
- this.location = location;
+ this.location = location != null ? LocationUtil.stripTrailingSlash(location) : null;
Review Comment:
I see if that's the case then we should just leave as is imo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #6777: Core: TableMetadata Always Strips Trailing Slash From Location
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #6777:
URL: https://github.com/apache/iceberg/pull/6777#issuecomment-1423293372
Cleaning up tests now, we have many tests that are using trailing slashes in table location
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org