You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/05/30 18:30:25 UTC
[GitHub] [iceberg] bryanck opened a new pull request, #4906: Add additional options for catalog test implementations
bryanck opened a new pull request, #4906:
URL: https://github.com/apache/iceberg/pull/4906
This PR adds a couple of options to the catalog tests so it is more flexible for different catalog implementations. The first option is to support testing against catalogs that manage the table location. The second option is to support testing against catalogs that do not support slashes in the namespace or table name.
--
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] rdblue merged pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4906:
URL: https://github.com/apache/iceberg/pull/4906
--
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] kbendick commented on a diff in pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4906:
URL: https://github.com/apache/iceberg/pull/4906#discussion_r885039255
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -155,6 +155,14 @@ protected boolean supportsServerSideRetry() {
return false;
}
+ protected boolean supportsServerManagedLocation() {
+ return false;
+ }
+
+ protected boolean supportsNamesWithSlashes() {
Review Comment:
Is that for escaped slashes in the typical way or for the REST catalogs escaped slashes that use `%00` (Null byte)?
I’m good with not requiring that server implementations require this (I’m almost certain it’s already not required). But would want to ensure the spec is up to date on that re: having to support it or not. But if the null-byte can open up security problems (which in general it can) then I can understand people deciding tables / namespaces really don’t benefit from having slashes in them.
--
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] rdblue commented on a diff in pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4906:
URL: https://github.com/apache/iceberg/pull/4906#discussion_r885053026
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -155,6 +155,14 @@ protected boolean supportsServerSideRetry() {
return false;
}
+ protected boolean supportsServerManagedLocation() {
+ return false;
+ }
+
+ protected boolean supportsNamesWithSlashes() {
Review Comment:
Sounds reasonable to me.
--
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] rdblue commented on pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4906:
URL: https://github.com/apache/iceberg/pull/4906#issuecomment-1141504515
Thanks, @bryanck!
--
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] bryanck commented on a diff in pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #4906:
URL: https://github.com/apache/iceberg/pull/4906#discussion_r885039999
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -155,6 +155,14 @@ protected boolean supportsServerSideRetry() {
return false;
}
+ protected boolean supportsServerManagedLocation() {
+ return false;
+ }
+
+ protected boolean supportsNamesWithSlashes() {
Review Comment:
The null byte is used as a delimiter AFAIK, the slash is just URL encoded, i.e. `%2F`
--
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] rdblue commented on a diff in pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4906:
URL: https://github.com/apache/iceberg/pull/4906#discussion_r885031491
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -155,6 +155,14 @@ protected boolean supportsServerSideRetry() {
return false;
}
+ protected boolean supportsServerManagedLocation() {
+ return false;
+ }
+
+ protected boolean supportsNamesWithSlashes() {
Review Comment:
Why is this not supported? Slashes should be properly escaped in paths, which is why we have this test.
--
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] bryanck commented on a diff in pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #4906:
URL: https://github.com/apache/iceberg/pull/4906#discussion_r885032943
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -1129,7 +1141,9 @@ public void testCompleteCreateTransaction() {
Assert.assertEquals("Table properties should be a superset of the requested properties",
properties.entrySet(),
Sets.intersection(properties.entrySet(), table.properties().entrySet()));
- Assert.assertEquals("Table location should match requested", "file:/tmp/ns/table", table.location());
+ if (!supportsServerManagedLocation()) {
Review Comment:
I like `overridesRequestedLocation()`, that seems more clear. I'll make that change.
--
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] bryanck commented on a diff in pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #4906:
URL: https://github.com/apache/iceberg/pull/4906#discussion_r885034181
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -1129,7 +1141,9 @@ public void testCompleteCreateTransaction() {
Assert.assertEquals("Table properties should be a superset of the requested properties",
properties.entrySet(),
Sets.intersection(properties.entrySet(), table.properties().entrySet()));
- Assert.assertEquals("Table location should match requested", "file:/tmp/ns/table", table.location());
+ if (!supportsServerManagedLocation()) {
Review Comment:
This is done
--
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] kbendick commented on a diff in pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4906:
URL: https://github.com/apache/iceberg/pull/4906#discussion_r885039255
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -155,6 +155,14 @@ protected boolean supportsServerSideRetry() {
return false;
}
+ protected boolean supportsServerManagedLocation() {
+ return false;
+ }
+
+ protected boolean supportsNamesWithSlashes() {
Review Comment:
Is that for escaped slashes in the typical way or for the REST catalogs escaped slashes that use `%00` (Null byte)?
I’m good with not requiring that server implementations require this (I’m almost certain it’s already not required) but would want to ensure the spec is up to date. But if the null-byte can open up security problems then I can understand people deciding tables / namespaces really don’t benefit from slashes in them.
--
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] rdblue commented on a diff in pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #4906:
URL: https://github.com/apache/iceberg/pull/4906#discussion_r885031999
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -1129,7 +1141,9 @@ public void testCompleteCreateTransaction() {
Assert.assertEquals("Table properties should be a superset of the requested properties",
properties.entrySet(),
Sets.intersection(properties.entrySet(), table.properties().entrySet()));
- Assert.assertEquals("Table location should match requested", "file:/tmp/ns/table", table.location());
+ if (!supportsServerManagedLocation()) {
Review Comment:
Should this be `supportsClientLocations`? I think it's reasonable to say that the server doesn't support client requested locations, rather than "supports" managing locations (which is to say it overrides them).
Or may be this should be `overridesRequestedLocation()`?
--
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] bryanck commented on a diff in pull request #4906: Add additional options for catalog test implementations
Posted by GitBox <gi...@apache.org>.
bryanck commented on code in PR #4906:
URL: https://github.com/apache/iceberg/pull/4906#discussion_r885032465
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -155,6 +155,14 @@ protected boolean supportsServerSideRetry() {
return false;
}
+ protected boolean supportsServerManagedLocation() {
+ return false;
+ }
+
+ protected boolean supportsNamesWithSlashes() {
Review Comment:
For REST based catalogs, some servers don't allow escaped slashes in the path by default, like Tomcat. There are workarounds but it can open up potential security issues, so some may choose not to support this.
--
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