DRILL-3497: Throw UserException#validationError for errors when modifying options

exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java

@@ -25,7 +25,7 @@
 import java.util.concurrent.atomic.AtomicLong;
 
 import org.apache.drill.common.config.DrillConfig;
-import org.apache.drill.common.exceptions.ExpressionParsingException;
+import org.apache.drill.common.exceptions.UserException;
 import org.apache.drill.exec.compile.ClassTransformer.ClassNames;
 import org.apache.drill.exec.exception.ClassTransformationException;
 import org.apache.drill.exec.server.options.OptionManager;
@@ -39,18 +39,20 @@
 import com.google.common.collect.MapMaker;
 
 public class QueryClassLoader extends URLClassLoader {
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(QueryClassLoader.class);
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(QueryClassLoader.class);
 
   public static final String JAVA_COMPILER_OPTION = "exec.java_compiler";
   public static final StringValidator JAVA_COMPILER_VALIDATOR = new StringValidator(JAVA_COMPILER_OPTION, CompilerPolicy.DEFAULT.toString()) {
     @Override
-    public void validate(OptionValue v) throws ExpressionParsingException {
+    public void validate(OptionValue v) {
       super.validate(v);
       try {
         CompilerPolicy.valueOf(v.string_val.toUpperCase());
       } catch (IllegalArgumentException e) {
-        throw new ExpressionParsingException(String.format("Invalid value '%s' specified for option '%s'. Valid values are %s.",
-            v.string_val, getOptionName(), Arrays.toString(CompilerPolicy.values())));
+        throw UserException.validationError()
+            .message("Invalid value '%s' specified for option '%s'. Valid values are %s.",
+              v.string_val, getOptionName(), Arrays.toString(CompilerPolicy.values()))
+            .build(logger);
       }
     }
   };

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/BaseOptionManager.java

@@ -23,13 +23,12 @@
 import org.apache.drill.exec.server.options.TypeValidators.StringValidator;
 
 abstract class BaseOptionManager implements OptionManager {
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BaseOptionManager.class);
-
+//  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BaseOptionManager.class);
 
   private OptionValue getOptionSafe(OptionValidator validator){
     OptionValue value = getOption(validator.getOptionName());
     if(value == null){
-      throw new IllegalArgumentException(String.format("Unknown value for boolean option `%s`.", validator.getOptionName()));
+      throw new IllegalArgumentException(String.format("Unknown value for option `%s`.", validator.getOptionName()));
     }
     return value;
   }

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java

@@ -48,13 +48,32 @@ public OptionValue getOption(String name) {
     }
   }
 
+  /**
+   * Gets the option values managed by this manager as an iterable.
+   *
+   * @return iterable of option values
+   */
   abstract Iterable<OptionValue> optionIterable();
+
+  /**
+   * Gets the option value from this manager without falling back.
+   *
+   * @param name the option name
+   * @return the option value, or null if the option does not exist locally
+   */
   abstract OptionValue getLocalOption(String name);
+
+  /**
+   * Sets the option value for this manager without falling back.
+   *
+   * @param value the option value
+   * @return true iff the value was successfully set
+   */
   abstract boolean setLocalOption(OptionValue value);
 
   @Override
   public void setOption(OptionValue value) {
-    fallback.getAdmin().validate(value);
+    getAdmin().validate(value);
     setValidatedOption(value);
   }
 

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FragmentOptionManager.java

@@ -23,7 +23,7 @@
 import com.google.common.collect.Maps;
 
 public class FragmentOptionManager extends InMemoryOptionManager {
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FragmentOptionManager.class);
+//  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FragmentOptionManager.class);
 
   public FragmentOptionManager(OptionManager systemOptions, OptionList options) {
     super(systemOptions, getMapFromOptionList(options));
@@ -32,7 +32,7 @@ public FragmentOptionManager(OptionManager systemOptions, OptionList options) {
   private static Map<String, OptionValue> getMapFromOptionList(OptionList options){
     Map<String, OptionValue> tmp = Maps.newHashMap();
     for(OptionValue v : options){
-      tmp.put(v.name, v);
+      tmp.put(v.name.toLowerCase(), v);
     }
     return ImmutableMap.copyOf(tmp);
   }

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/InMemoryOptionManager.java

@@ -22,7 +22,8 @@
 public abstract class InMemoryOptionManager extends FallbackOptionManager {
   static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(InMemoryOptionManager.class);
 
-  final Map<String, OptionValue> options;
+  // NOTE: CRUD operations must use lowercase keys
+  protected final Map<String, OptionValue> options;
 
   InMemoryOptionManager(OptionManager fallback, Map<String, OptionValue> options) {
     super(fallback);
@@ -31,13 +32,13 @@ public abstract class InMemoryOptionManager extends FallbackOptionManager {
 
   @Override
   OptionValue getLocalOption(String name) {
-    return options.get(name);
+    return options.get(name.toLowerCase());
   }
 
   @Override
   boolean setLocalOption(OptionValue value) {
     if(supportsOption(value)){
-      options.put(value.name, value);
+      options.put(value.name.toLowerCase(), value);
       return true;
     }else{
       return false;
@@ -50,6 +51,12 @@ Iterable<OptionValue> optionIterable() {
     return options.values();
   }
 
+  /**
+   * Check (e.g. option type) to see if implementations of this manager support this option.
+   *
+   * @param value the option value
+   * @return true iff the option value is supported
+   */
   abstract boolean supportsOption(OptionValue value);
 
 }

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionManager.java

@@ -17,26 +17,137 @@
  */
 package org.apache.drill.exec.server.options;
 
+import org.apache.drill.common.exceptions.UserException;
 import org.apache.calcite.sql.SqlLiteral;
 
+/**
+ * Manager for Drill options. Implementations must be case-insensitive to the name of the option, specifically the
+ * {@link #setOption}, {@link #getOption} and {@link #getDefault} methods must ignore the case of the option name.
+ */
 public interface OptionManager extends Iterable<OptionValue> {
-  public OptionValue getOption(String name);
-  public void setOption(OptionValue value) throws SetOptionException;
-  public void setOption(String name, SqlLiteral literal, OptionValue.OptionType type) throws SetOptionException;
-  public OptionAdmin getAdmin();
-  public OptionManager getSystemManager();
-  public OptionList getOptionList();
-  public OptionValue getDefault(final String name);
-
-  public boolean getOption(TypeValidators.BooleanValidator validator);
-  public double getOption(TypeValidators.DoubleValidator validator);
-  public long getOption(TypeValidators.LongValidator validator);
-  public String getOption(TypeValidators.StringValidator validator);
 
+  /**
+   * Gets the option value for the given option name.
+   *
+   * @param name option name
+   * @return the option value, null if the option does not exist
+   */
+  OptionValue getOption(String name);
+
+  /**
+   * Sets an option value.
+   *
+   * @param value option value
+   * @throws UserException message to describe error with value
+   */
+  void setOption(OptionValue value);
+
+  /**
+   * Sets the option value built from the given parameters.
+   *
+   * @param name    option name
+   * @param literal sql literal
+   * @param type    option type
+   * @throws UserException message to describe error with value
+   */
+  void setOption(String name, SqlLiteral literal, OptionValue.OptionType type);
+
+  /**
+   * Gets the option admin for this manager.
+   *
+   * @return the option admin
+   */
+  OptionAdmin getAdmin();
+
+  /**
+   * Gets the {@link SystemOptionManager} for this manager.
+   *
+   * @return the system option manager
+   */
+  OptionManager getSystemManager();
+
+  /**
+   * Gets the list of options managed this manager.
+   *
+   * @return the list of options
+   */
+  OptionList getOptionList();
+
+  /**
+   * Gets the default option value for the given option name.
+   *
+   * @param name option name
+   * @return the default option value, or null if the option does not exist
+   */
+  OptionValue getDefault(String name);
+
+  /**
+   * Gets the boolean value (from the option value) for the given boolean validator.
+   *
+   * @param validator the boolean validator
+   * @return the boolean value
+   */
+  boolean getOption(TypeValidators.BooleanValidator validator);
+
+  /**
+   * Gets the double value (from the option value) for the given double validator.
+   *
+   * @param validator the double validator
+   * @return the double value
+   */
+  double getOption(TypeValidators.DoubleValidator validator);
+
+  /**
+   * Gets the long value (from the option value) for the given long validator.
+   *
+   * @param validator the long validator
+   * @return the long value
+   */
+  long getOption(TypeValidators.LongValidator validator);
+
+  /**
+   * Gets the string value (from the option value) for the given string validator.
+   *
+   * @param validator the string validator
+   * @return the string value
+   */
+  String getOption(TypeValidators.StringValidator validator);
+
+  /**
+   * Administrator that is used to validate the options.
+   */
   public interface OptionAdmin {
-    public void registerOptionType(OptionValidator validator);
-    public OptionValidator getValidator(String name);
-    public void validate(OptionValue v) throws SetOptionException;
-    public OptionValue validate(String name, SqlLiteral value, OptionValue.OptionType optionType) throws SetOptionException;
+
+    /**
+     * Gets the validator for the given option name. Implementations must be case insensitive to the name.
+     *
+     * @param name option name
+     * @return the option validator, or null if the validator does not exist
+     * @throws UserException message to describe error with value
+     */
+    OptionValidator getValidator(String name);
+
+    /**
+     * Validates the option value.
+     *
+     * @param value option value
+     * @throws UserException message to describe error with value
+     */
+    void validate(OptionValue value);
+
+    /**
+     * Validate the option value built from the parameters. For options that support some ambiguity in their settings,
+     * such as case-insensitivity for string options, this method returns a modified version of the passed value that
+     * is considered the standard format of the option that should be used for system-internal representation.
+     *
+     * @param name       option name
+     * @param value      sql literal
+     * @param optionType option type
+     * @return the value requested, in its standard format to be used for representing the value within Drill
+     * Example: all lower case values for strings, to avoid ambiguities in how values are stored
+     * while allowing some flexibility for users
+     * @throws UserException message to describe error with value
+     */
+    OptionValue validate(String name, SqlLiteral value, OptionValue.OptionType optionType);
   }
 }

exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java

@@ -17,13 +17,11 @@
  ******************************************************************************/
 package org.apache.drill.exec.server.options;
 
-import org.apache.drill.common.exceptions.ExpressionParsingException;
+import org.apache.drill.common.exceptions.UserException;
 import org.apache.calcite.sql.SqlLiteral;
 
 /**
  * Validates the values provided to Drill options.
- *
- * @param <E>
  */
 public abstract class OptionValidator {
   // Stored here as well as in the option static class to allow insertion of option optionName into
@@ -35,19 +33,10 @@ public OptionValidator(String optionName) {
   }
 
   /**
-   * This method determines if a given value is a valid setting for an option. For options that support some
-   * ambiguity in their settings, such as case-insensitivity for string options, this method returns a modified
-   * version of the passed value that is considered the standard format of the option that should be used for
-   * system-internal representation.
+   * Gets the name of the option for this validator.
    *
-   * @param value - the value to validate
-   * @return - the value requested, in its standard format to be used for representing the value within Drill
-   *            Example: all lower case values for strings, to avoid ambiguities in how values are stored
-   *            while allowing some flexibility for users
-   * @throws ExpressionParsingException - message to describe error with value, including range or list of expected values
+   * @return the option name
    */
-  public abstract OptionValue validate(SqlLiteral value, OptionValue.OptionType optionType) throws ExpressionParsingException;
-
   public String getOptionName() {
     return optionName;
   }
@@ -60,6 +49,7 @@ public String getOptionName() {
    * (1) override this method to return true, and
    * (2) return the number of queries for which the option is valid through {@link #getTtl}.
    * E.g. {@link org.apache.drill.exec.testing.ExecutionControls.ControlsOptionValidator}
+   *
    * @return if this validator is for a short-lived option
    */
   public boolean isShortLived() {
@@ -69,6 +59,7 @@ public boolean isShortLived() {
   /**
    * If an option is short-lived, this returns the number of queries for which the option is valid.
    * Please read the note at {@link #isShortLived}
+   *
    * @return number of queries for which the option should be valid
    */
   public int getTtl() {
@@ -78,11 +69,36 @@ public int getTtl() {
     return 0;
   }
 
-  public String getDefaultString() {
-    return null;
-  }
-
+  /**
+   * Gets the default option value for this validator.
+   *
+   * @return default option value
+   */
   public abstract OptionValue getDefault();
 
-  public abstract void validate(OptionValue v) throws ExpressionParsingException;
+  /**
+   * This method determines if a given value is a valid setting for an option. For options that support some
+   * ambiguity in their settings, such as case-insensitivity for string options, this method returns a modified
+   * version of the passed value that is considered the standard format of the option that should be used for
+   * system-internal representation.
+   *
+   * @param value the value to validate
+   * @throws UserException message to describe error with value, including range or list of expected values
+   */
+  public abstract void validate(OptionValue value);
+
+  /**
+   * This method determines if a given value is a valid setting for an option. For options that support some
+   * ambiguity in their settings, such as case-insensitivity for string options, this method returns a modified
+   * version of the passed value that is considered the standard format of the option that should be used for
+   * system-internal representation.
+   *
+   * @param value      the sql literal to validate
+   * @param optionType the option type
+   * @return the value requested, in its standard format to be used for representing the value within Drill
+   *         Example: all lower case values for strings, to avoid ambiguities in how values are stored
+   *         while allowing some flexibility for users
+   * @throws UserException message to describe error with value, including range or list of expected values
+   */
+  public abstract OptionValue validate(SqlLiteral value, OptionValue.OptionType optionType);
 }