rdblue commented on a change in pull request #1531:
URL: https://github.com/apache/iceberg/pull/1531#discussion_r499920637
##########
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:
Since you're catching the exception, it probably makes more sense to use
`buildChecked` so the exception class isn't generic. That exception also has
good information about why all of the implementations failed, so I would
recommend adding that exception as a cause to this one that is thrown. I like
throwing this one for context, though!
----------------------------------------------------------------
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]
