kbendick commented on a change in pull request #3923:
URL: https://github.com/apache/iceberg/pull/3923#discussion_r789210365
##########
File path: core/src/main/java/org/apache/iceberg/io/ResolvingFileIO.java
##########
@@ -40,17 +40,26 @@
private static final Logger LOG =
LoggerFactory.getLogger(ResolvingFileIO.class);
private static final String DEFAULT_SCHEME = "fs";
private static final String FALLBACK_IMPL =
"org.apache.iceberg.hadoop.HadoopFileIO";
+ private static final String S3_FILE_IO_IMPL =
"org.apache.iceberg.aws.s3.S3FileIO";
private static final Map<String, String> SCHEME_TO_FILE_IO = ImmutableMap.of(
DEFAULT_SCHEME, FALLBACK_IMPL,
- "s3", "org.apache.iceberg.aws.s3.S3FileIO",
- "s3a", "org.apache.iceberg.aws.s3.S3FileIO",
- "s3n", "org.apache.iceberg.aws.s3.S3FileIO"
+ "s3", S3_FILE_IO_IMPL,
+ "s3a", S3_FILE_IO_IMPL,
+ "s3n", S3_FILE_IO_IMPL
Review comment:
Nit: This change is good, but we _might_ want to put it in another PR to
make it easier for people to cherry-pick only PRs that contain bugs (in my
view, the lack of a no-arg constructor in `ResolvingFileIO` is a bug). And then
possibly update the title of this PR to make it more clear that this is fixing
a bug such as `Core: Add missing no-arg constructor in ResolvingFileIO`.
If other's feel differently, then by all means it can stay in this PR.
##########
File path: core/src/main/java/org/apache/iceberg/io/ResolvingFileIO.java
##########
@@ -40,17 +40,26 @@
private static final Logger LOG =
LoggerFactory.getLogger(ResolvingFileIO.class);
private static final String DEFAULT_SCHEME = "fs";
private static final String FALLBACK_IMPL =
"org.apache.iceberg.hadoop.HadoopFileIO";
+ private static final String S3_FILE_IO_IMPL =
"org.apache.iceberg.aws.s3.S3FileIO";
private static final Map<String, String> SCHEME_TO_FILE_IO = ImmutableMap.of(
DEFAULT_SCHEME, FALLBACK_IMPL,
- "s3", "org.apache.iceberg.aws.s3.S3FileIO",
- "s3a", "org.apache.iceberg.aws.s3.S3FileIO",
- "s3n", "org.apache.iceberg.aws.s3.S3FileIO"
+ "s3", S3_FILE_IO_IMPL,
+ "s3a", S3_FILE_IO_IMPL,
+ "s3n", S3_FILE_IO_IMPL
);
private final Map<String, FileIO> ioInstances = Maps.newHashMap();
private Map<String, String> properties;
private SerializableSupplier<Configuration> hadoopConf;
+ /**
+ * No-arg constructor to load the FileIO dynamically.
+ * <p>
+ * All fields are initialized by calling {@link
ResolvingFileIO#initialize(Map)} later.
+ */
+ public ResolvingFileIO() {
+ }
Review comment:
This is consistent with what is in other catalogs (such as `S3FileIO`).
This is fine and generally speaking required depending on where it's used. 👍
--
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]
