jackye1995 commented on a change in pull request #4248:
URL: https://github.com/apache/iceberg/pull/4248#discussion_r817318533
##########
File path:
aws/src/main/java/org/apache/iceberg/aws/glue/IcebergToGlueConverter.java
##########
@@ -61,6 +65,12 @@ private IcebergToGlueConverter() {
public static final String ICEBERG_FIELD_OPTIONAL = "iceberg.field.optional";
public static final String ICEBERG_FIELD_CURRENT = "iceberg.field.current";
+ // Attempt to set additionalLocations if available on the given AWS SDK
version
+ private static final DynMethods.UnboundMethod ADDITIONAL_LOCATIONS =
DynMethods.builder("additionalLocations")
Review comment:
nit: prefer verb for setter, `SET_ADDITIONAL_LOCATIONS`
##########
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,
Review comment:
nit: prefer full nouns, `properties` instead of `tableProps`
##########
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:
this should be a set, also have we tested if it's fine to have
`ImmutableList.of(...)` with null values? I think it's going to fail.
##########
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(
+ tableProps.get(TableProperties.WRITE_DATA_LOCATION),
Review comment:
this is likely wrong, the property is not a part of Glue parameters, but
`metadata.properties()`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
