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 2021/08/26 15:06:40 UTC
[GitHub] [iceberg] bijanhoule opened a new pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
bijanhoule opened a new pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035
This PR is intended to resolve an issue with `DynamoDbCatalog.dropNamespace()` -- the dynamoDB delete fails as the conditionExpression evaluates to e.g. `attribute_exists(my_db)`, rather than `attribute_exists(namespace)`.
Example namespace record in dynamo:
```json
{
"identifier": "NAMESPACE",
"namespace": "my_db",
"updated_at": 1628190933466,
"created_at": 1628190933466,
"v": "0203ec48-317b-4197-9712-4a7c91b5c91a",
"p.owner": "bijan"
}
```
@jackye1995 - can you confirm this change makes sense / is what was intended?
@danielcweeks
--
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 #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035
--
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 #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#issuecomment-910024430
can you also add the following integration test?
```
@Test
public void testDropNamespace() {
Namespace namespace = Namespace.of(genRandomName());
catalog.createNamespace(namespace);
catalog.dropNamespace(namespace);
GetItemResponse response = dynamo.getItem(GetItemRequest.builder()
.tableName(catalogTableName)
.key(DynamoDbCatalog.namespacePrimaryKey(namespace))
.build());
Assert.assertFalse("namespace must not exist", response.hasItem());
}
```
--
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 change in pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#discussion_r699959277
##########
File path: aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java
##########
@@ -248,7 +248,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
dynamo.deleteItem(DeleteItemRequest.builder()
.tableName(awsProperties.dynamoDbTableName())
.key(namespacePrimaryKey(namespace))
- .conditionExpression("attribute_exists(" + namespace + ")")
+ .conditionExpression("attribute_exists(" + DynamoDbCatalog.COL_NAMESPACE + ")")
Review comment:
yes you are right, it was used to prevent calling the same method twice, but it likely does not matter given it's dropping the namespace. Thanks for noticing this bug, somehow I did not have a drop namesapce test. I will add it in another PR.
##########
File path: aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java
##########
@@ -248,7 +248,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
dynamo.deleteItem(DeleteItemRequest.builder()
.tableName(awsProperties.dynamoDbTableName())
.key(namespacePrimaryKey(namespace))
- .conditionExpression("attribute_exists(" + namespace + ")")
+ .conditionExpression("attribute_exists(" + DynamoDbCatalog.COL_NAMESPACE + ")")
Review comment:
nit: `COL_NAMESPACE` instead of `DynamoDbCatalog.COL_NAMESPACE`
##########
File path: aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java
##########
@@ -248,7 +248,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
dynamo.deleteItem(DeleteItemRequest.builder()
.tableName(awsProperties.dynamoDbTableName())
.key(namespacePrimaryKey(namespace))
- .conditionExpression("attribute_exists(" + namespace + ")")
+ .conditionExpression("attribute_exists(" + DynamoDbCatalog.COL_NAMESPACE + ")")
Review comment:
yes you are right, it was used to prevent calling the same method twice, but it likely does not matter given it's dropping the namespace. We can address that separately. Thanks for noticing this bug, somehow I did not have a drop namesapce 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] jackye1995 commented on a change in pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#discussion_r699959926
##########
File path: aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java
##########
@@ -248,7 +248,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
dynamo.deleteItem(DeleteItemRequest.builder()
.tableName(awsProperties.dynamoDbTableName())
.key(namespacePrimaryKey(namespace))
- .conditionExpression("attribute_exists(" + namespace + ")")
+ .conditionExpression("attribute_exists(" + DynamoDbCatalog.COL_NAMESPACE + ")")
Review comment:
nit: `COL_NAMESPACE` instead of `DynamoDbCatalog.COL_NAMESPACE`
--
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] bijanhoule commented on a change in pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
bijanhoule commented on a change in pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#discussion_r699703887
##########
File path: aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java
##########
@@ -248,7 +248,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
dynamo.deleteItem(DeleteItemRequest.builder()
.tableName(awsProperties.dynamoDbTableName())
.key(namespacePrimaryKey(namespace))
- .conditionExpression("attribute_exists(" + namespace + ")")
+ .conditionExpression("attribute_exists(" + DynamoDbCatalog.COL_NAMESPACE + ")")
Review comment:
Good question -- I'm not 100% sure, there may be edge cases that I'm not aware of.
I think that removing the `conditionExpression` might involve tweaking the exception handling [below](https://github.com/apache/iceberg/pull/3035/files#diff-76f1ec8211667ffbb03d3c237f7edff00812ccffb5b71b7d000989526e1bebd9R254) as well, I wouldn't expect the `ConditionalCheckFailedException` to be thrown any more.
--
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] bijanhoule commented on a change in pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
bijanhoule commented on a change in pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#discussion_r699703887
##########
File path: aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java
##########
@@ -248,7 +248,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
dynamo.deleteItem(DeleteItemRequest.builder()
.tableName(awsProperties.dynamoDbTableName())
.key(namespacePrimaryKey(namespace))
- .conditionExpression("attribute_exists(" + namespace + ")")
+ .conditionExpression("attribute_exists(" + DynamoDbCatalog.COL_NAMESPACE + ")")
Review comment:
Good question -- I'm not 100% sure, there may be edge cases that I'm not aware of.
I think that removing the `conditionExpression` might involve tweaking the exception handling [below](https://github.com/apache/iceberg/pull/3035/files#diff-76f1ec8211667ffbb03d3c237f7edff00812ccffb5b71b7d000989526e1bebd9R254) as well, I wouldn't expect the `ConditionalCheckFailedException` to be thrown any more.
--
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 #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#issuecomment-910024430
can you also add the following integration test?
```
@Test
public void testDropNamespace() {
Namespace namespace = Namespace.of(genRandomName());
catalog.createNamespace(namespace);
catalog.dropNamespace(namespace);
GetItemResponse response = dynamo.getItem(GetItemRequest.builder()
.tableName(catalogTableName)
.key(DynamoDbCatalog.namespacePrimaryKey(namespace))
.build());
Assert.assertFalse("namespace must not exist", response.hasItem());
}
```
--
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] bijanhoule commented on pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
bijanhoule commented on pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#issuecomment-910380944
> can you also add the following integration test?
>
> ...
Good call, thank you! Added
--
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 change in pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#discussion_r699959277
##########
File path: aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java
##########
@@ -248,7 +248,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
dynamo.deleteItem(DeleteItemRequest.builder()
.tableName(awsProperties.dynamoDbTableName())
.key(namespacePrimaryKey(namespace))
- .conditionExpression("attribute_exists(" + namespace + ")")
+ .conditionExpression("attribute_exists(" + DynamoDbCatalog.COL_NAMESPACE + ")")
Review comment:
yes you are right, it was used to prevent calling the same method twice, but it likely does not matter given it's dropping the namespace. Thanks for noticing this bug, somehow I did not have a drop namesapce test. I will add it in another PR.
--
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 change in pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#discussion_r698874451
##########
File path: aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java
##########
@@ -248,7 +248,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
dynamo.deleteItem(DeleteItemRequest.builder()
.tableName(awsProperties.dynamoDbTableName())
.key(namespacePrimaryKey(namespace))
- .conditionExpression("attribute_exists(" + namespace + ")")
+ .conditionExpression("attribute_exists(" + DynamoDbCatalog.COL_NAMESPACE + ")")
Review comment:
Is this `conditionExpression` even needed? Are there any records that don't have a key `namespace` in them?
I guess if users are adding their own metadata to records or have a projected view (GSI)?
--
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 change in pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#discussion_r698874451
##########
File path: aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java
##########
@@ -248,7 +248,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
dynamo.deleteItem(DeleteItemRequest.builder()
.tableName(awsProperties.dynamoDbTableName())
.key(namespacePrimaryKey(namespace))
- .conditionExpression("attribute_exists(" + namespace + ")")
+ .conditionExpression("attribute_exists(" + DynamoDbCatalog.COL_NAMESPACE + ")")
Review comment:
Is this `conditionExpression` even needed? Are there any records that don't have a key `namespace` in them?
I guess if users are adding their own metadata to records or have a projected view (GSI)?
--
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 change in pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#discussion_r699959277
##########
File path: aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java
##########
@@ -248,7 +248,7 @@ public boolean dropNamespace(Namespace namespace) throws NamespaceNotEmptyExcept
dynamo.deleteItem(DeleteItemRequest.builder()
.tableName(awsProperties.dynamoDbTableName())
.key(namespacePrimaryKey(namespace))
- .conditionExpression("attribute_exists(" + namespace + ")")
+ .conditionExpression("attribute_exists(" + DynamoDbCatalog.COL_NAMESPACE + ")")
Review comment:
yes you are right, it was used to prevent calling the same method twice, but it likely does not matter given it's dropping the namespace. We can address that separately. Thanks for noticing this bug, somehow I did not have a drop namesapce 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] rdblue commented on pull request #3035: AWS: update DynamoDbCatalog.dropNamespace attr check
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3035:
URL: https://github.com/apache/iceberg/pull/3035#issuecomment-918318994
Thanks for fixing this, @bijanhoule!
--
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