DRILL-1065: Support for ALTER ... RESET statement

+ Support for "SET option = value" statement (assumes scope as SESSION)
+ Better error messages in SetOptionHandler
+ Changes in CompoundIdentifierConverter
  - update when rewritten operand is not deeply equal to the original operand
  - Added Override annotations
+ Default ExecutionControls option value should be at SYSTEM level

exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java

@@ -17,15 +17,13 @@
  */
 package org.apache.drill.exec.planner.sql.handlers;
 
-import java.io.IOException;
 import java.math.BigDecimal;
 
+import org.apache.calcite.sql.SqlIdentifier;
 import org.apache.calcite.sql.type.SqlTypeName;
-import org.apache.calcite.tools.RelConversionException;
 import org.apache.calcite.tools.ValidationException;
 
 import org.apache.calcite.util.NlsString;
-import org.apache.drill.common.exceptions.ExpressionParsingException;
 import org.apache.drill.common.exceptions.UserException;
 import org.apache.drill.exec.ExecConstants;
 import org.apache.drill.exec.ops.QueryContext;
@@ -49,45 +47,67 @@ public SetOptionHandler(QueryContext context) {
   }
 
   @Override
-  public PhysicalPlan getPlan(SqlNode sqlNode) throws ValidationException, RelConversionException, IOException, ForemanSetupException {
+  public PhysicalPlan getPlan(SqlNode sqlNode) throws ValidationException, ForemanSetupException {
     final SqlSetOption option = unwrap(sqlNode, SqlSetOption.class);
-    final String scope = option.getScope();
-    final String name = option.getName();
     final SqlNode value = option.getValue();
-    OptionValue.OptionType type;
-    if (value instanceof SqlLiteral) {
+    if (value != null && !(value instanceof SqlLiteral)) {
+      throw UserException.validationError()
+          .message("Drill does not support assigning non-literal values in SET statements.")
+          .build(logger);
+    }
+
+    final String scope = option.getScope();
+    final OptionValue.OptionType type;
+    if (scope == null) { // No scope mentioned assumed SESSION
+      type = OptionType.SESSION;
+    } else {
       switch (scope.toLowerCase()) {
-        case "session":
-          type = OptionValue.OptionType.SESSION;
-          break;
-        case "system":
-          type = OptionValue.OptionType.SYSTEM;
-          break;
-//        case "query":
-//          type = OptionValue.OptionType.QUERY;
-//          break;
-        default:
-          throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM.");
+      case "session":
+        type = OptionType.SESSION;
+        break;
+      case "system":
+        type = OptionType.SYSTEM;
+        break;
+      default:
+        throw UserException.validationError()
+            .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope)
+            .build(logger);
       }
+    }
 
-      if (type == OptionType.SYSTEM) {
-        // If the user authentication is enabled, make sure the user who is trying to change the system option has
-        // administrative privileges.
-        if (context.isUserAuthenticationEnabled() &&
-            !ImpersonationUtil.hasAdminPrivileges(
-                context.getQueryUserName(),
-                context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
-                context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) {
-          throw UserException.permissionError()
-              .message("Not authorized to change SYSTEM options.")
-              .build(logger);
-        }
+    if (type == OptionType.SYSTEM) {
+      // If the user authentication is enabled, make sure the user who is trying to change the system option has
+      // administrative privileges.
+      if (context.isUserAuthenticationEnabled() &&
+          !ImpersonationUtil.hasAdminPrivileges(
+            context.getQueryUserName(),
+            context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
+            context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) {
+        throw UserException.permissionError()
+            .message("Not authorized to change SYSTEM options.")
+            .build(logger);
       }
+    }
+
+    // Currently, we use one part identifiers.
+    final SqlIdentifier nameIdentifier = option.getName();
+    if (!nameIdentifier.isSimple()) {
+      throw UserException.validationError()
+        .message("Drill does not support multi-part identifier for an option name (%s).",
+          nameIdentifier.toString())
+        .build(logger);
+    }
 
+    final String name = nameIdentifier.getSimple();
+    if (value != null) { // SET option
       final OptionValue optionValue = createOptionValue(name, type, (SqlLiteral) value);
       context.getOptions().setOption(optionValue);
-    }else{
-      throw new ValidationException("Sql options can only be literals.");
+    } else { // RESET option
+      if ("ALL".equals(name)) {
+        context.getOptions().deleteAllOptions(type);
+      } else {
+        context.getOptions().deleteOption(name, type);
+      }
     }
 
     return DirectPlan.createDirectPlan(context, true, String.format("%s updated.", name));
@@ -126,8 +146,9 @@ private static OptionValue createOptionValue(final String name, final OptionValu
       return OptionValue.createBoolean(type, name, (Boolean) object);
 
     default:
-      throw new ExpressionParsingException(String.format(
-        "Drill doesn't support set option expressions with literals of type %s.", typeName));
+      throw UserException.validationError()
+        .message("Drill doesn't support assigning literals of type %s in SET statements.", typeName)
+        .build(logger);
     }
   }
 }

exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java

@@ -26,6 +26,7 @@
 import org.apache.calcite.sql.SqlNode;
 import org.apache.calcite.sql.SqlOrderBy;
 import org.apache.calcite.sql.SqlSelect;
+import org.apache.calcite.sql.SqlSetOption;
 import org.apache.calcite.sql.util.SqlShuttle;
 import org.apache.calcite.sql.util.SqlVisitor;
 
@@ -75,6 +76,7 @@ public ComplexExpressionAware(SqlCall call) {
       rewriteTypes = REWRITE_RULES.get(call.getClass());
     }
 
+    @Override
     public SqlNode result() {
       if (update) {
         return call.getOperator().createCall(
@@ -86,6 +88,7 @@ public SqlNode result() {
       }
     }
 
+    @Override
     public SqlNode visitChild(
         SqlVisitor<SqlNode> visitor,
         SqlNode expr,
@@ -107,7 +110,7 @@ public SqlNode visitChild(
       }
       SqlNode newOperand = operand.accept(CompoundIdentifierConverter.this);
       enableComplex = localEnableComplex;
-      if (newOperand != operand) {
+      if (! newOperand.equalsDeep(operand, false)) {
         update = true;
       }
       clonedOperands[i] = newOperand;
@@ -161,6 +164,7 @@ RewriteType[] should be R(D, E, D, D).
     rules.put(SqlJoin.class, R(D, D, D, D, D, E));
     rules.put(SqlOrderBy.class, R(D, E, D, D));
     rules.put(SqlDropTable.class, R(D));
+    rules.put(SqlSetOption.class, R(D, D, D));
     REWRITE_RULES = ImmutableMap.copyOf(rules);
   }
 

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

@@ -17,6 +17,8 @@
  */
 package org.apache.drill.exec.server.options;
 
+import org.apache.drill.common.exceptions.UserException;
+
 import java.util.Map;
 
 /**
@@ -25,7 +27,7 @@
  * (see {@link #options}) whereas {@link SystemOptionManager} stores options in a persistent store.
  */
 public abstract class InMemoryOptionManager extends FallbackOptionManager {
-//  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(InMemoryOptionManager.class);
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(InMemoryOptionManager.class);
 
   protected final Map<String, OptionValue> options;
 
@@ -34,6 +36,20 @@ public abstract class InMemoryOptionManager extends FallbackOptionManager {
     this.options = options;
   }
 
+  @Override
+  public void deleteOption(final String name, final OptionValue.OptionType type) {
+    throw UserException.unsupportedError()
+      .message("This manager does not support deleting an option.")
+      .build(logger);
+  }
+
+  @Override
+  public void deleteAllOptions(final OptionValue.OptionType type) {
+    throw UserException.unsupportedError()
+      .message("This manager does not support deleting options.")
+      .build(logger);
+  }
+
   @Override
   OptionValue getLocalOption(final String name) {
     return options.get(name);

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

@@ -17,6 +17,8 @@
  */
 package org.apache.drill.exec.server.options;
 
+import org.apache.drill.exec.server.options.OptionValue.OptionType;
+
 /**
  * Manager for Drill options. Implementations must be case-insensitive to the name of an option.
  */
@@ -30,9 +32,34 @@ public interface OptionManager extends Iterable<OptionValue> {
    */
   void setOption(OptionValue value);
 
+  /**
+   * Deletes the option. Unfortunately, the type is required given the fallback structure of option managers.
+   * See {@link FallbackOptionManager}.
+   *
+   * @param name option name
+   * @param type option type
+   * @throws org.apache.drill.common.exceptions.UserException message to describe error with value
+   */
+  void deleteOption(String name, OptionType type);
+
+  /**
+   * Deletes all options. Unfortunately, the type is required given the fallback structure of option managers.
+   * See {@link FallbackOptionManager}.
+   *
+   * @param type option type
+   * @throws org.apache.drill.common.exceptions.UserException message to describe error with value
+   */
+  void deleteAllOptions(OptionType type);
+
   /**
    * Gets the option value for the given option name.
    *
+   * This interface also provides convenient methods to get typed option values:
+   * {@link #getOption(TypeValidators.BooleanValidator validator)},
+   * {@link #getOption(TypeValidators.DoubleValidator validator)},
+   * {@link #getOption(TypeValidators.LongValidator validator)}, and
+   * {@link #getOption(TypeValidators.StringValidator validator)}.
+   *
    * @param name option name
    * @return the option value, null if the option does not exist
    */

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

@@ -27,11 +27,11 @@
 @JsonInclude(Include.NON_NULL)
 public class OptionValue implements Comparable<OptionValue> {
 
-  public static enum OptionType {
+  public enum OptionType {
     BOOT, SYSTEM, SESSION, QUERY
   }
 
-  public static enum Kind {
+  public enum Kind {
     BOOLEAN, LONG, STRING, DOUBLE
   }
 

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

@@ -30,6 +30,16 @@ public QueryOptionManager(OptionManager sessionOptions) {
     super(sessionOptions, CaseInsensitiveMap.<OptionValue>newHashMap());
   }
 
+  @Override
+  public void deleteOption(final String name, final OptionValue.OptionType type) {
+    fallback.deleteOption(name, type);
+  }
+
+  @Override
+  public void deleteAllOptions(final OptionValue.OptionType type) {
+    fallback.deleteAllOptions(type);
+  }
+
   @Override
   public OptionList getOptionList() {
     OptionList list = super.getOptionList();

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

@@ -20,6 +20,7 @@
 import com.google.common.base.Predicate;
 import com.google.common.collect.Collections2;
 import org.apache.commons.lang3.tuple.ImmutablePair;
+import org.apache.drill.common.exceptions.UserException;
 import org.apache.drill.common.map.CaseInsensitiveMap;
 import org.apache.drill.exec.rpc.user.UserSession;
 
@@ -30,7 +31,7 @@
  * {@link OptionManager} that holds options within {@link org.apache.drill.exec.rpc.user.UserSession} context.
  */
 public class SessionOptionManager extends InMemoryOptionManager {
-//  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SessionOptionManager.class);
+  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SessionOptionManager.class);
 
   private final UserSession session;
 
@@ -45,6 +46,33 @@ public SessionOptionManager(final OptionManager systemOptions, final UserSession
     this.session = session;
   }
 
+  @Override
+  public void deleteOption(final String name, final OptionValue.OptionType type) {
+    if (type == OptionValue.OptionType.SESSION) {
+      try { // ensure option exists
+        SystemOptionManager.getValidator(name);
+      } catch (final IllegalArgumentException e) {
+        throw UserException.validationError()
+          .message(e.getMessage())
+          .build(logger);
+      }
+      options.remove(name);
+      shortLivedOptions.remove(name);
+    } else {
+      fallback.deleteOption(name, type);
+    }
+  }
+
+  @Override
+  public void deleteAllOptions(final OptionValue.OptionType type) {
+    if (type == OptionValue.OptionType.SESSION) {
+      options.clear();
+      shortLivedOptions.clear();
+    } else {
+      fallback.deleteAllOptions(type);
+    }
+  }
+
   @Override
   boolean setLocalOption(final OptionValue value) {
     final boolean set = super.setLocalOption(value);

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

@@ -22,7 +22,9 @@
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Set;
 
+import com.google.common.collect.Sets;
 import org.apache.commons.collections.IteratorUtils;
 import org.apache.drill.common.config.DrillConfig;
 import org.apache.drill.common.map.CaseInsensitiveMap;
@@ -240,6 +242,31 @@ public void setOption(final OptionValue value) {
     options.put(name, value);
   }
 
+  @Override
+  public void deleteOption(final String name, OptionType type) {
+    assert type == OptionType.SYSTEM;
+    try { // ensure option exists
+      getValidator(name);
+    } catch (final IllegalArgumentException e) {
+      throw UserException.validationError()
+        .message(e.getMessage())
+        .build(logger);
+    }
+    options.delete(name.toLowerCase());
+  }
+
+  @Override
+  public void deleteAllOptions(OptionType type) {
+    assert type == OptionType.SYSTEM;
+    final Set<String> names = Sets.newHashSet();
+    for (final Map.Entry<String, OptionValue> entry : options) {
+      names.add(entry.getKey());
+    }
+    for (final String name : names) {
+      options.delete(name); // should be lowercase
+    }
+  }
+
   @Override
   public OptionList getOptionList() {
     return (OptionList) IteratorUtils.toList(iterator());

exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java

@@ -80,7 +80,7 @@ public static class ControlsOptionValidator extends TypeValidator {
      * @param ttl  the number of queries for which this option should be valid
      */
     public ControlsOptionValidator(final String name, final String def, final int ttl) {
-      super(name, OptionValue.Kind.STRING, OptionValue.createString(OptionType.SESSION, name, def));
+      super(name, OptionValue.Kind.STRING, OptionValue.createString(OptionType.SYSTEM, name, def));
       assert ttl > 0;
       this.ttl = ttl;
     }