mickjermsurawong-stripe commented on a change in pull request #1531:
URL: https://github.com/apache/iceberg/pull/1531#discussion_r499887225
##########
File path: core/src/main/java/org/apache/iceberg/LocationProviders.java
##########
@@ -36,7 +37,29 @@ private LocationProviders() {
}
public static LocationProvider locationsFor(String location, Map<String,
String> properties) {
- if (PropertyUtil.propertyAsBoolean(properties,
+ if (properties.containsKey(TableProperties.WRITE_LOCATION_PROVIDER_IMPL)) {
+ String impl =
properties.get(TableProperties.WRITE_LOCATION_PROVIDER_IMPL);
+ DynConstructors.Ctor<LocationProvider> ctor;
+ try {
+ ctor = DynConstructors.builder(LocationProvider.class)
+ .impl(impl, String.class, Map.class)
+ .impl(impl).build(); // fall back to no-arg constructor
+ } catch (RuntimeException e) {
+ throw new IllegalArgumentException(String.format(
+ "Unable to find a constructor for implementation %s of %s. " +
+ "Make sure the implementation is in classpath, and that it
either " +
+ "has a public no-arg constructor or a two-arg constructor " +
+ "taking in the string base table location and its property
string map.",
+ impl, LocationProvider.class));
+ }
Review comment:
If provided class has the specified constructor signature, but not
implementing `LocationProvider`, it can still be loaded into the
dynConstructor. The following block differentiates this missing interface more
explicitly.
Might be a bit more code churn, but it does give more signal to users.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
