[GitHub] [iceberg] singhpk234 commented on a change in pull request #4248: AWS : use additionalLocations in glue table to store location info

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

singhpk234 commented on a change in pull request #4248:
URL: https://github.com/apache/iceberg/pull/4248#discussion_r817329038



##########
File path: aws/src/main/java/org/apache/iceberg/aws/glue/IcebergToGlueConverter.java
##########
@@ -175,12 +185,25 @@ static void validateTableIdentifier(TableIdentifier tableIdentifier) {
    * and should never be used by any query processing engine to infer information like schema, partition spec, etc.
    * The source of truth is stored in the actual Iceberg metadata file defined by the metadata_location table property.
    * @param tableInputBuilder Glue TableInput builder
+   * @param tableProps Table properties
    * @param metadata Iceberg table metadata
    */
-  static void setTableInputInformation(TableInput.Builder tableInputBuilder, TableMetadata metadata) {
+  static void setTableInputInformation(
+      TableInput.Builder tableInputBuilder,
+      Map<String, String> tableProps,
+      TableMetadata metadata) {
     try {
+      StorageDescriptor.Builder storageDescriptor = StorageDescriptor.builder();
+      if (!ADDITIONAL_LOCATIONS.isNoop()) {
+        ADDITIONAL_LOCATIONS.invoke(storageDescriptor.additionalLocations(),
+            ImmutableList.of(

Review comment:
       +1
   yup just checked ImmutableList.of() fails with null values... 
   - added a test case as well where in the additionalLocation the properties we are fetching values from is not present. 
   
   replaced this with location's with not null filtering, as it's not worth storing null as well.
   
   ```java
     private static final List<String> PROPERTIES_FOR_GLUE_ADDITIONAL_LOCATIONS = ImmutableList.of(
         TableProperties.WRITE_DATA_LOCATION,
         TableProperties.WRITE_METADATA_LOCATION,
         TableProperties.OBJECT_STORE_PATH,
         TableProperties.WRITE_FOLDER_STORAGE_LOCATION
     );
   
     private static List<String> getAdditionalLocations(TableMetadata tableMetadata) {
       return PROPERTIES_FOR_GLUE_ADDITIONAL_LOCATIONS.stream()
           .map(tableMetadata.properties()::get)
           .filter(Objects::nonNull)
           .collect(Collectors.toList());
     }
   ```




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