AWS: Fix default warehouse path in Dynamodb catalog #7358

munendrasn · 2023-04-16T07:06:40Z

Without getting the string value of the attribute, the formed the defaultWarehousePath is not in expected format.

cc @amogh-jahagirdar @jackye1995 @yyanyy

JonasJ-ap · 2023-04-16T17:19:03Z

aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java

@@ -379,6 +379,21 @@ public void testRegisterTable() {
    Assertions.assertThat(catalog.dropNamespace(namespace)).isTrue();
  }

+  @Test
+  public void testDefaultWarehousePathWithLocation() {


Thank you for your contribution. I was wondering if it would be possible to add a unit test for this change in addition to the aws integration test. like:

iceberg/aws/src/test/java/org/apache/iceberg/aws/glue/TestGlueCatalog.java

Lines 143 to 153 in b78d336

@Test

public void testDefaultWarehouseLocationDbUri() {

Mockito.doReturn(

GetDatabaseResponse.builder()

.database(Database.builder().name("db").locationUri("s3://bucket2/db").build())

.build())

.when(glue)

.getDatabase(Mockito.any(GetDatabaseRequest.class));

String location = glueCatalog.defaultWarehouseLocation(TableIdentifier.of("db", "table"));

Assert.assertEquals("s3://bucket2/db/table", location);

}

Since the AWS integration test is not currently run with CI, having a unit test could help catch errors more quickly and ensure that future development reflects this change accurately. What do you think?

+1 for adding a unit test, @munendrasn let me know if you could add that as suggested, other things looks good to me.

@JonasJ-ap @jackye1995 Thanks for the review. I will include the unit test as suggested

I have included the unit tests, but for now, included tests only for defaultwarehouse location path in unit tests

jackye1995

looks good to me, pending addition of unit test

JonasJ-ap

Thank you for adding the unit tests. Just some tiny comments remaining.

I think the fix for Spark-CI is in master branch now. You can rebase and CI shall pass.

JonasJ-ap · 2023-04-23T16:35:46Z

aws/src/test/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java

+        .when(dynamo)
+        .getItem(any(GetItemRequest.class));
+    String location = catalogWithSlash.defaultWarehouseLocation(TableIdentifier.of("db", "table"));
+    Assert.assertEquals(WAREHOUSE_PATH + "/db.db/table", location);


How about using Assertj here too?

Suggested change

Assert.assertEquals(WAREHOUSE_PATH + "/db.db/table", location);

Assertions.assertThat(location).isEqualTo(WAREHOUSE_PATH + "/db.db/table");

JonasJ-ap · 2023-04-23T16:38:20Z

aws/src/test/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java

+import software.amazon.awssdk.services.dynamodb.model.AttributeValue;
+import software.amazon.awssdk.services.dynamodb.model.GetItemRequest;
+import software.amazon.awssdk.services.dynamodb.model.GetItemResponse;
+import software.amazon.awssdk.services.glue.model.*;


Seems this is not used in any test. Could you please delete this import? I think running spotlessApply can automatically delete this import and fix some other checkstyle issue if exists

Spotless doesn't seems to be delete this unused import, maybe because of *. I have manually deleted it

amogh-jahagirdar

Non-blocking nit in the test, everything else looks good to me, thanks @munendrasn !

amogh-jahagirdar · 2023-04-24T15:15:43Z

aws/src/test/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java

+    Mockito.doReturn(GetItemResponse.builder().item(Maps.newHashMap()).build())
+        .when(dynamo)
+        .getItem(any(GetItemRequest.class));
+    String location = catalogWithSlash.defaultWarehouseLocation(TableIdentifier.of("db", "table"));


Nit: can we move TableIdentifier.of("db", "table") to a constant? It's used multiple times

Thanks for the review, moved it to constant

JonasJ-ap

LGTM! Thanks for the contribution.

jackye1995 · 2023-04-24T18:25:02Z

Thanks, merging

github-actions bot added the AWS label Apr 16, 2023

JonasJ-ap reviewed Apr 16, 2023

View reviewed changes

jackye1995 approved these changes Apr 19, 2023

View reviewed changes

JonasJ-ap reviewed Apr 23, 2023

View reviewed changes

munendrasn added 3 commits April 24, 2023 18:10

fix default warehousepath in dynamodb catalog

7509852

add unit test for dynamodb catalog

7ebe2de

incorporate review suggestions

9a20214

munendrasn force-pushed the default-warehouse branch from f367a21 to 9a20214 Compare April 24, 2023 12:43

amogh-jahagirdar approved these changes Apr 24, 2023

View reviewed changes

address review suggestion

e3e74f5

JonasJ-ap approved these changes Apr 24, 2023

View reviewed changes

jackye1995 merged commit 69c3249 into apache:master Apr 24, 2023
33 checks passed

coufon pushed a commit to coufon/iceberg that referenced this pull request Apr 25, 2023

AWS: Fix default warehouse path in Dynamodb catalog (apache#7358)

a84e3ed

manisin pushed a commit to Snowflake-Labs/iceberg that referenced this pull request May 9, 2023

AWS: Fix default warehouse path in Dynamodb catalog (apache#7358)

11db204

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS: Fix default warehouse path in Dynamodb catalog #7358

AWS: Fix default warehouse path in Dynamodb catalog #7358

munendrasn commented Apr 16, 2023

JonasJ-ap Apr 16, 2023

jackye1995 Apr 19, 2023

munendrasn Apr 20, 2023

munendrasn Apr 23, 2023

jackye1995 left a comment

JonasJ-ap left a comment •

edited

JonasJ-ap Apr 23, 2023

munendrasn Apr 24, 2023

JonasJ-ap Apr 23, 2023

munendrasn Apr 24, 2023

amogh-jahagirdar left a comment

amogh-jahagirdar Apr 24, 2023

munendrasn Apr 24, 2023

JonasJ-ap left a comment

jackye1995 commented Apr 24, 2023

	@Test
	public void testDefaultWarehouseLocationDbUri() {
	Mockito.doReturn(
	GetDatabaseResponse.builder()
	.database(Database.builder().name("db").locationUri("s3://bucket2/db").build())
	.build())
	.when(glue)
	.getDatabase(Mockito.any(GetDatabaseRequest.class));
	String location = glueCatalog.defaultWarehouseLocation(TableIdentifier.of("db", "table"));
	Assert.assertEquals("s3://bucket2/db/table", location);
	}

	Assert.assertEquals(WAREHOUSE_PATH + "/db.db/table", location);
	Assertions.assertThat(location).isEqualTo(WAREHOUSE_PATH + "/db.db/table");

AWS: Fix default warehouse path in Dynamodb catalog #7358

AWS: Fix default warehouse path in Dynamodb catalog #7358

Conversation

munendrasn commented Apr 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackye1995 left a comment

Choose a reason for hiding this comment

JonasJ-ap left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasJ-ap left a comment

Choose a reason for hiding this comment

jackye1995 commented Apr 24, 2023

JonasJ-ap left a comment •

edited