[GitHub] [iceberg] aokolnychyi commented on a change in pull request #2330: Spark: Add table location to reserved properties so that the table location shows in Spark SQL Properties.

[GitBox <gi...@apache.org>]

aokolnychyi commented on a change in pull request #2330:
URL: https://github.com/apache/iceberg/pull/2330#discussion_r600070709



##########
File path: spark3/src/test/java/org/apache/iceberg/actions/TestCreateActions.java
##########
@@ -447,6 +447,30 @@ public void testProperties() throws Exception {
     }
   }
 
+  @Test
+  public void testSparkTableReservedProperties() throws Exception {
+    String destTableName = "iceberg_reserved_properties";
+    String source = sourceName("test_reserved_properties_table");
+    String dest = destName(destTableName);
+    createSourceTable(CREATE_PARQUET, source);
+    assertMigratedFileCount(Actions.snapshot(source, dest), source, dest);
+    SparkTable table = loadTable(dest);
+
+    String[] keys = {"provider", "format", "current-snapshot-id", "location"};
+
+    for (String entry : keys) {
+      Assert.assertTrue(
+              "Created table missing reserved property " + entry, table.properties().containsKey(entry));
+    }
+
+    Assert.assertEquals("Property value is not the expected value", "iceberg", table.properties().get("provider"));
+    Assert.assertEquals("Property value is not the expected value", "iceberg/parquet",
+        table.properties().get("format"));
+    Assert.assertTrue("Current-snapshot-id doesn't exist",

Review comment:
       nit: would `assertNotEquals` be more appropriate?

##########
File path: spark3/src/main/java/org/apache/iceberg/spark/source/SparkTable.java
##########
@@ -63,7 +63,8 @@
 
   private static final Logger LOG = LoggerFactory.getLogger(SparkTable.class);
 
-  private static final Set<String> RESERVED_PROPERTIES = Sets.newHashSet("provider", "format", "current-snapshot-id");
+  private static final Set<String> RESERVED_PROPERTIES =
+      Sets.newHashSet("provider", "format", "current-snapshot-id", "location");

Review comment:
       nit: not directly related to this PR, but shall we use `ImmutableSet` for consistency?

##########
File path: spark3/src/test/java/org/apache/iceberg/actions/TestCreateActions.java
##########
@@ -447,6 +447,30 @@ public void testProperties() throws Exception {
     }
   }
 
+  @Test
+  public void testSparkTableReservedProperties() throws Exception {
+    String destTableName = "iceberg_reserved_properties";
+    String source = sourceName("test_reserved_properties_table");
+    String dest = destName(destTableName);
+    createSourceTable(CREATE_PARQUET, source);
+    assertMigratedFileCount(Actions.snapshot(source, dest), source, dest);
+    SparkTable table = loadTable(dest);
+
+    String[] keys = {"provider", "format", "current-snapshot-id", "location"};
+
+    for (String entry : keys) {
+      Assert.assertTrue(

Review comment:
       I think this would fit on one line. We have our max line length as 120.

##########
File path: spark3/src/test/java/org/apache/iceberg/actions/TestCreateActions.java
##########
@@ -447,6 +447,30 @@ public void testProperties() throws Exception {
     }
   }
 
+  @Test
+  public void testSparkTableReservedProperties() throws Exception {
+    String destTableName = "iceberg_reserved_properties";
+    String source = sourceName("test_reserved_properties_table");
+    String dest = destName(destTableName);
+    createSourceTable(CREATE_PARQUET, source);
+    assertMigratedFileCount(Actions.snapshot(source, dest), source, dest);
+    SparkTable table = loadTable(dest);
+
+    String[] keys = {"provider", "format", "current-snapshot-id", "location"};
+
+    for (String entry : keys) {
+      Assert.assertTrue(
+              "Created table missing reserved property " + entry, table.properties().containsKey(entry));
+    }
+
+    Assert.assertEquals("Property value is not the expected value", "iceberg", table.properties().get("provider"));
+    Assert.assertEquals("Property value is not the expected value", "iceberg/parquet",

Review comment:
       I think if we make the error message shorter, this would fit on one line.
   
   ```
       Assert.assertEquals("Unexpected provider", "iceberg", table.properties().get("provider"));
       Assert.assertEquals("Unexpected format", "iceberg/parquet", table.properties().get("format"));
       ...
   ```




-- 
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.

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