drill-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From adene...@apache.org
Subject drill git commit: DRILL-1065: Support for ALTER ... RESET statement
Date Fri, 02 Oct 2015 03:35:44 GMT
Repository: drill
Updated Branches:
  refs/heads/master 9c74c7f78 -> 401119f3b


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

+ Support for "SET option = value" statement (assumes scope as SESSION)
+ Bump Calcite version to include CALCITE-823 (Parser support for "ALTER
  ... RESET" statement). This commit includes a breaking change:
  SqlSetOption#getName now returns a SqlIdentifier rather than a String
  => option names are multi-part identifiers, and do not
  require escaping

+ Add rule in CompoundIdentifierConverter (+ Override annotations)
+ Improve error messages in SetOptionHandler
+ Add documentation (CompoundIdentifierConverter, OptionValue,
  SessionOptionManager, SystemOptionManager)
- Does not include support for deleting short lived options

+ Default ExecutionControls option value should be at SYSTEM level
+ Change asserts to preconditions in SystemOptionManager
+ Add a precondition to TypeValidator's ctor to ensure default value are
  set at SYSTEM level

this closes #159


Project: http://git-wip-us.apache.org/repos/asf/drill/repo
Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/401119f3
Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/401119f3
Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/401119f3

Branch: refs/heads/master
Commit: 401119f3bf55c6c67deec1d64f94d84d1d6d1180
Parents: 9c74c7f
Author: Sudheesh Katkam <skatkam@maprtech.com>
Authored: Tue Sep 15 16:13:23 2015 -0700
Committer: adeneche <adeneche@gmail.com>
Committed: Thu Oct 1 18:36:45 2015 -0700

----------------------------------------------------------------------
 .../planner/sql/handlers/SetOptionHandler.java  |  92 +++++---
 .../sql/parser/CompoundIdentifierConverter.java |  13 +-
 .../server/options/FallbackOptionManager.java   |  55 ++++-
 .../server/options/FragmentOptionManager.java   |   3 +-
 .../server/options/InMemoryOptionManager.java   |  32 ++-
 .../exec/server/options/OptionManager.java      |  34 ++-
 .../drill/exec/server/options/OptionValue.java  |  10 +-
 .../exec/server/options/QueryOptionManager.java |   4 +-
 .../server/options/SessionOptionManager.java    |  15 +-
 .../server/options/SystemOptionManager.java     |  45 +++-
 .../exec/server/options/TypeValidators.java     |   3 +
 .../drill/exec/testing/ExecutionControls.java   |   2 +-
 .../apache/drill/exec/server/TestOptions.java   | 234 ++++++++++++++++++-
 pom.xml                                         |   2 +-
 14 files changed, 465 insertions(+), 79 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java
index 85ab528..f278989 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java
@@ -17,20 +17,18 @@
  */
 package org.apache.drill.exec.planner.sql.handlers;
 
-import java.io.IOException;
 import java.math.BigDecimal;
 
 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;
 import org.apache.drill.exec.physical.PhysicalPlan;
 import org.apache.drill.exec.planner.sql.DirectPlan;
+import org.apache.drill.exec.server.options.OptionManager;
 import org.apache.drill.exec.server.options.OptionValue;
 import org.apache.drill.exec.server.options.OptionValue.OptionType;
 import org.apache.drill.exec.util.ImpersonationUtil;
@@ -39,6 +37,12 @@ import org.apache.calcite.sql.SqlLiteral;
 import org.apache.calcite.sql.SqlNode;
 import org.apache.calcite.sql.SqlSetOption;
 
+/**
+ * Converts a {@link SqlNode} representing "ALTER .. SET option = value" and "ALTER ... RESET
..." statements to a
+ * {@link PhysicalPlan}. See {@link SqlSetOption}. These statements have side effects i.e.
the options within the
+ * system context or the session context are modified. The resulting {@link DirectPlan} returns
to the client a string
+ * that is the name of the option that was updated.
+ */
 public class SetOptionHandler extends AbstractSqlHandler {
   private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SetOptionHandler.class);
 
@@ -49,45 +53,60 @@ public class SetOptionHandler extends AbstractSqlHandler {
   }
 
   @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);
-        }
+    final OptionManager options = context.getOptions();
+    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(),
+            options.getOption(ExecConstants.ADMIN_USERS_VALIDATOR),
+            options.getOption(ExecConstants.ADMIN_USER_GROUPS_VALIDATOR))) {
+        throw UserException.permissionError()
+            .message("Not authorized to change SYSTEM options.")
+            .build(logger);
       }
+    }
 
+    // Currently, we convert multi-part identifier to a string.
+    final String name = option.getName().toString();
+    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.");
+      options.setOption(optionValue);
+    } else { // RESET option
+      if ("ALL".equalsIgnoreCase(name)) {
+        options.deleteAllOptions(type);
+      } else {
+        options.deleteOption(name, type);
+      }
     }
 
     return DirectPlan.createDirectPlan(context, true, String.format("%s updated.", name));
@@ -126,8 +145,9 @@ public class SetOptionHandler extends AbstractSqlHandler {
       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);
     }
   }
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java
index 3e4c59c..61a4c9f 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java
@@ -26,14 +26,22 @@ import org.apache.calcite.sql.SqlJoin;
 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;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 
+/**
+ * Implementation of {@link SqlVisitor} that converts bracketed compound {@link SqlIdentifier}
to bracket-less compound
+ * {@link SqlIdentifier} (also known as {@link DrillCompoundIdentifier}) to provide ease
of use while querying complex
+ * types.
+ * <p/>
+ * For example, this visitor converts {@code a['b'][4]['c']} to {@code a.b[4].c}
+ */
 public class CompoundIdentifierConverter extends SqlShuttle {
-  static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(CompoundIdentifierConverter.class);
+//  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(CompoundIdentifierConverter.class);
 
   private boolean enableComplex = true;
 
@@ -75,6 +83,7 @@ public class CompoundIdentifierConverter extends SqlShuttle {
       rewriteTypes = REWRITE_RULES.get(call.getClass());
     }
 
+    @Override
     public SqlNode result() {
       if (update) {
         return call.getOperator().createCall(
@@ -86,6 +95,7 @@ public class CompoundIdentifierConverter extends SqlShuttle {
       }
     }
 
+    @Override
     public SqlNode visitChild(
         SqlVisitor<SqlNode> visitor,
         SqlNode expr,
@@ -162,6 +172,7 @@ public class CompoundIdentifierConverter extends SqlShuttle {
     rules.put(SqlOrderBy.class, R(D, E, D, D));
     rules.put(SqlDropTable.class, R(D));
     rules.put(SqlRefreshMetadata.class, R(D));
+    rules.put(SqlSetOption.class, R(D, D, D));
     REWRITE_RULES = ImmutableMap.copyOf(rules);
   }
 

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java
index 7c864b2..25ba0ad 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java
@@ -20,7 +20,7 @@ package org.apache.drill.exec.server.options;
 import java.util.Iterator;
 
 import com.google.common.collect.Iterables;
-import org.apache.drill.common.exceptions.UserException;
+import org.apache.drill.exec.server.options.OptionValue.OptionType;
 
 /**
  * An {@link OptionManager} which allows for falling back onto another {@link OptionManager}.
This way method calls can
@@ -32,7 +32,7 @@ import org.apache.drill.common.exceptions.UserException;
  * manager. {@link QueryOptionManager} uses {@link SessionOptionManager} as the fall back
manager.
  */
 public abstract class FallbackOptionManager extends BaseOptionManager {
-  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FallbackOptionManager.class);
+//  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FallbackOptionManager.class);
 
   protected final OptionManager fallback;
 
@@ -83,16 +83,33 @@ public abstract class FallbackOptionManager extends BaseOptionManager
{
    */
   abstract boolean setLocalOption(OptionValue value);
 
+  /**
+   * Deletes all options for this manager without falling back.
+   *
+   * If no options are set, calling this method should be no-op. See {@link OptionManager#deleteAllOptions}.
+   *
+   * @param type option type
+   * @return true iff the option type is supported
+   */
+  abstract boolean deleteAllLocalOptions(OptionType type);
+
+  /**
+   * Deletes the option with given name for this manager without falling back.
+   *
+   * This method will be called with an option name that is guaranteed to have an option
validator. Also, if option
+   * with {@param name} does not exist within the manager, calling this method should be
a no-op. See
+   * {@link OptionManager#deleteOption}.
+   *
+   * @param name option name
+   * @param type option type
+   * @return true iff the option type is supported
+   */
+  abstract boolean deleteLocalOption(String name, OptionType type);
+
   @Override
   public void setOption(OptionValue value) {
-    final OptionValidator validator;
-    try {
-      validator = SystemOptionManager.getValidator(value.name);
-    } catch (final IllegalArgumentException e) {
-      throw UserException.validationError()
-        .message(e.getMessage())
-        .build(logger);
-    }
+    final OptionValidator validator = SystemOptionManager.getValidator(value.name);
+
     validator.validate(value); // validate the option
 
     // fallback if unable to set locally
@@ -102,6 +119,24 @@ public abstract class FallbackOptionManager extends BaseOptionManager
{
   }
 
   @Override
+  public void deleteOption(final String name, final OptionType type) {
+    SystemOptionManager.getValidator(name); // ensure the option exists
+
+    // fallback if unable to delete locally
+    if (!deleteLocalOption(name, type)) {
+      fallback.deleteOption(name, type);
+    }
+  }
+
+  @Override
+  public void deleteAllOptions(final OptionType type) {
+    // fallback if unable to delete locally
+    if (!deleteAllLocalOptions(type)) {
+      fallback.deleteAllOptions(type);
+    }
+  }
+
+  @Override
   public OptionList getOptionList() {
     final OptionList list = new OptionList();
     for (final OptionValue value : getLocalOptions()) {

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FragmentOptionManager.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FragmentOptionManager.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FragmentOptionManager.java
index 46e534a..39f86d1 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FragmentOptionManager.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FragmentOptionManager.java
@@ -19,6 +19,7 @@ package org.apache.drill.exec.server.options;
 
 import com.google.common.collect.Maps;
 import org.apache.drill.common.map.CaseInsensitiveMap;
+import org.apache.drill.exec.server.options.OptionValue.OptionType;
 
 import java.util.Map;
 
@@ -41,7 +42,7 @@ public class FragmentOptionManager extends InMemoryOptionManager {
   }
 
   @Override
-  boolean supportsOption(OptionValue value) {
+  boolean supportsOptionType(OptionType type) {
     throw new UnsupportedOperationException("FragmentOptionManager does not support the given
option value.");
   }
 

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/InMemoryOptionManager.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/InMemoryOptionManager.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/InMemoryOptionManager.java
index dbff3e2..7fc837e 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/InMemoryOptionManager.java
+++ b/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.exec.server.options.OptionValue.OptionType;
+
 import java.util.Map;
 
 /**
@@ -41,7 +43,7 @@ public abstract class InMemoryOptionManager extends FallbackOptionManager
{
 
   @Override
   boolean setLocalOption(final OptionValue value) {
-    if (supportsOption(value)) {
+    if (supportsOptionType(value.type)) {
       options.put(value.name, value);
       return true;
     } else {
@@ -54,12 +56,32 @@ public abstract class InMemoryOptionManager extends FallbackOptionManager
{
     return options.values();
   }
 
+  @Override
+  boolean deleteAllLocalOptions(final OptionType type) {
+    if (supportsOptionType(type)) {
+      options.clear();
+      return true;
+    } else {
+      return false;
+    }
+  }
+
+  @Override
+  boolean deleteLocalOption(final String name, final OptionType type) {
+    if (supportsOptionType(type)) {
+      options.remove(name);
+      return true;
+    } else {
+      return false;
+    }
+  }
+
   /**
-   * Check to see if implementations of this manager support the given option value (e.g.
check for option type).
+   * Check to see if implementations of this manager support the given option type.
    *
-   * @param value the option value
-   * @return true iff the option value is supported
+   * @param type option type
+   * @return true iff the type is supported
    */
-  abstract boolean supportsOption(OptionValue value);
+  abstract boolean supportsOptionType(OptionType type);
 
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionManager.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionManager.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionManager.java
index 8ff0f94..dc9d9cf 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionManager.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionManager.java
@@ -17,8 +17,10 @@
  */
 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.
+ * Manager for Drill {@link OptionValue options}. Implementations must be case-insensitive
to the name of an option.
  */
 public interface OptionManager extends Iterable<OptionValue> {
 
@@ -31,8 +33,38 @@ 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}.
+   *
+   * If the option name is valid (exists in {@link SystemOptionManager#VALIDATORS}),
+   * but the option was not set within this manager, calling this method should be a no-op.
+   *
+   * @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}.
+   *
+   * If no options are set, calling this method should be no-op.
+   *
+   * @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
    */

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java
index b73b669..a2b2e93 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java
@@ -24,14 +24,20 @@ import com.fasterxml.jackson.annotation.JsonInclude.Include;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.base.Preconditions;
 
+/**
+ * An {@link OptionValue option value} is used by an {@link OptionManager} to store a run-time
setting. This setting,
+ * for example, could affect a query in execution stage. Instances of this class are JSON
serializable and can be stored
+ * in a {@link org.apache.drill.exec.store.sys.PStore persistent store} (see {@link SystemOptionManager#options}),
or
+ * in memory (see {@link InMemoryOptionManager#options}).
+ */
 @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
   }
 

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/QueryOptionManager.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/QueryOptionManager.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/QueryOptionManager.java
index 26cf688..77ca3d0 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/QueryOptionManager.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/QueryOptionManager.java
@@ -38,7 +38,7 @@ public class QueryOptionManager extends InMemoryOptionManager {
   }
 
   @Override
-  boolean supportsOption(OptionValue value) {
-    return value.type == OptionType.QUERY;
+  boolean supportsOptionType(OptionType type) {
+    return type == OptionType.QUERY;
   }
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java
index eb0da03..38f8556 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java
@@ -20,14 +20,23 @@ package org.apache.drill.exec.server.options;
 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;
+import org.apache.drill.exec.server.options.OptionValue.OptionType;
 
 import java.util.Collection;
 import java.util.Map;
 
 /**
- * {@link OptionManager} that holds options within {@link org.apache.drill.exec.rpc.user.UserSession}
context.
+ * {@link OptionManager} that holds options within {@link org.apache.drill.exec.rpc.user.UserSession}
context. Options
+ * set at the session level only apply to queries that you run during the current Drill connection.
Session level
+ * settings override system level settings.
+ *
+ * NOTE that currently, the effects of deleting a short lived option (see {@link OptionValidator#isShortLived})
are
+ * undefined. For example, we inject an exception (passed through an option), then try to
delete the option, depending
+ * on where the exception was injected, the reset query could either succeed or the exception
could actually be thrown
+ * in the reset query itself.
  */
 public class SessionOptionManager extends InMemoryOptionManager {
 //  private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SessionOptionManager.class);
@@ -108,7 +117,7 @@ public class SessionOptionManager extends InMemoryOptionManager {
   }
 
   @Override
-  boolean supportsOption(OptionValue value) {
-    return value.type == OptionValue.OptionType.SESSION;
+  boolean supportsOptionType(OptionType type) {
+    return type == OptionType.SESSION;
   }
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
index c58bc08..4cd61c2 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
@@ -22,7 +22,9 @@ import java.util.HashMap;
 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;
@@ -37,9 +39,12 @@ import org.apache.drill.exec.store.sys.PStoreConfig;
 import org.apache.drill.exec.store.sys.PStoreProvider;
 import org.apache.drill.exec.util.AssertionUtil;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 /**
  * {@link OptionManager} that holds options within {@link org.apache.drill.exec.server.DrillbitContext}.
- * Only one instance of this class exists per drillbit.
+ * Only one instance of this class exists per drillbit. Options set at the system level affect
the entire system and
+ * persist between restarts.
  */
 public class SystemOptionManager extends BaseOptionManager {
   private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SystemOptionManager.class);
@@ -186,12 +191,14 @@ public class SystemOptionManager extends BaseOptionManager {
    *
    * @param name name of the option
    * @return the associated validator
-   * @throws IllegalArgumentException - if the validator is not found
+   * @throws UserException - if the validator is not found
    */
   public static OptionValidator getValidator(final String name) {
     final OptionValidator validator = VALIDATORS.get(name);
     if (validator == null) {
-      throw new IllegalArgumentException("Unknown option: " + name);
+      throw UserException.validationError()
+          .message(String.format("The option '%s' does not exist.", name))
+          .build(logger);
     }
     return validator;
   }
@@ -225,16 +232,10 @@ public class SystemOptionManager extends BaseOptionManager {
 
   @Override
   public void setOption(final OptionValue value) {
-    assert value.type == OptionType.SYSTEM;
+    checkArgument(value.type == OptionType.SYSTEM, "OptionType must be SYSTEM.");
     final String name = value.name.toLowerCase();
-    final OptionValidator validator;
-    try {
-      validator = getValidator(name);
-    } catch (final IllegalArgumentException e) {
-      throw UserException.validationError()
-        .message(e.getMessage())
-        .build(logger);
-    }
+    final OptionValidator validator = getValidator(name);
+
     validator.validate(value); // validate the option
 
     if (options.get(name) == null && value.equals(validator.getDefault())) {
@@ -244,6 +245,26 @@ public class SystemOptionManager extends BaseOptionManager {
   }
 
   @Override
+  public void deleteOption(final String name, OptionType type) {
+    checkArgument(type == OptionType.SYSTEM, "OptionType must be SYSTEM.");
+
+    getValidator(name); // ensure option exists
+    options.delete(name.toLowerCase());
+  }
+
+  @Override
+  public void deleteAllOptions(OptionType type) {
+    checkArgument(type == OptionType.SYSTEM, "OptionType must be 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());
   }

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java
index 53cd4f3..ced448c 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java
@@ -24,6 +24,8 @@ import org.apache.drill.common.exceptions.UserException;
 import org.apache.drill.exec.server.options.OptionValue.Kind;
 import org.apache.drill.exec.server.options.OptionValue.OptionType;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 public class TypeValidators {
   private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TypeValidators.class);
 
@@ -179,6 +181,7 @@ public class TypeValidators {
 
     public TypeValidator(final String name, final Kind kind, final OptionValue defValue)
{
       super(name);
+      checkArgument(defValue.type == OptionType.SYSTEM, "Default value must be SYSTEM type.");
       this.kind = kind;
       this.defaultValue = defValue;
     }

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
b/exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
index 8f9589d..9673394 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java
@@ -80,7 +80,7 @@ public final class ExecutionControls {
      * @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;
     }

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java
----------------------------------------------------------------------
diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java b/exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java
index f20fd25..2761faa 100644
--- a/exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java
+++ b/exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java
@@ -22,6 +22,8 @@ import org.apache.drill.exec.ExecConstants;
 import org.apache.drill.test.UserExceptionMatcher;
 import org.junit.Test;
 
+import static org.apache.drill.exec.ExecConstants.ENABLE_VERBOSE_ERRORS_KEY;
+import static org.apache.drill.exec.ExecConstants.SLICE_TARGET;
 import static org.apache.drill.exec.proto.UserBitShared.DrillPBError.ErrorType.VALIDATION;
 
 public class TestOptions extends BaseTestQuery{
@@ -46,19 +48,243 @@ public class TestOptions extends BaseTestQuery{
   @Test
   public void checkValidationException() throws Exception {
     thrownException.expect(new UserExceptionMatcher(VALIDATION));
-    test(String.format("ALTER session SET `%s` = '%s';", ExecConstants.SLICE_TARGET, "fail"));
+    test("ALTER session SET %s = '%s';", SLICE_TARGET, "fail");
   }
 
   @Test // DRILL-3122
   public void checkChangedColumn() throws Exception {
-    test(String.format("ALTER session SET `%s` = %d;", ExecConstants.SLICE_TARGET,
-      ExecConstants.SLICE_TARGET_DEFAULT));
+    test("ALTER session SET `%s` = %d;", SLICE_TARGET,
+      ExecConstants.SLICE_TARGET_DEFAULT);
     testBuilder()
-        .sqlQuery(String.format("SELECT status FROM sys.options WHERE name = '%s' AND type
= 'SESSION'", ExecConstants.SLICE_TARGET))
+        .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'",
SLICE_TARGET)
         .unOrdered()
         .baselineColumns("status")
         .baselineValues("DEFAULT")
         .build()
         .run();
   }
+
+  @Test
+  public void setAndResetSessionOption() throws Exception {
+    // check unchanged
+    testBuilder()
+      .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'",
SLICE_TARGET)
+      .unOrdered()
+      .expectsEmptyResultSet()
+      .build()
+      .run();
+
+    // change option
+    test("SET `%s` = %d;", SLICE_TARGET, 10);
+    // check changed
+    test("SELECT status, type, name FROM sys.options WHERE type = 'SESSION';");
+    testBuilder()
+      .sqlQuery("SELECT num_val FROM sys.options WHERE name = '%s' AND type = 'SESSION'",
SLICE_TARGET)
+      .unOrdered()
+      .baselineColumns("num_val")
+      .baselineValues((long) 10)
+      .build()
+      .run();
+
+    // reset option
+    test("RESET `%s`;", SLICE_TARGET);
+    // check reverted
+    testBuilder()
+      .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'",
SLICE_TARGET)
+      .unOrdered()
+      .expectsEmptyResultSet()
+      .build()
+      .run();
+  }
+
+  @Test
+  public void setAndResetSystemOption() throws Exception {
+    // check unchanged
+    testBuilder()
+      .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("status")
+      .baselineValues("DEFAULT")
+      .build()
+      .run();
+
+    // change option
+    test("ALTER system SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+    // check changed
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+
+    // reset option
+    test("ALTER system RESET `%s`;", ENABLE_VERBOSE_ERRORS_KEY);
+    // check reverted
+    testBuilder()
+      .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("status")
+      .baselineValues("DEFAULT")
+      .build()
+      .run();
+  }
+
+  @Test
+  public void resetAllSessionOptions() throws Exception {
+    // change options
+    test("SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+    // check changed
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE type = 'SESSION' AND name = '%s'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+
+    // reset all options
+    test("RESET ALL;");
+    // check no session options changed
+    testBuilder()
+      .sqlQuery("SELECT status FROM sys.options WHERE status <> 'DEFAULT' AND type
= 'SESSION'")
+      .unOrdered()
+      .expectsEmptyResultSet()
+      .build()
+      .run();
+  }
+
+  @Test
+  public void changeSessionAndSystemButRevertSession() throws Exception {
+    // change options
+    test("ALTER SESSION SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+    test("ALTER SYSTEM SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+    // check changed
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE type = 'SESSION' AND name = '%s'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+    // check changed
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE type = 'SYSTEM' AND name = '%s'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+
+    // reset session option
+    test("RESET `%s`;", ENABLE_VERBOSE_ERRORS_KEY);
+    // check reverted
+    testBuilder()
+      .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .expectsEmptyResultSet()
+      .build()
+      .run();
+    // check unchanged
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE type = 'SYSTEM' AND name = '%s'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+    // reset system option
+    test("ALTER SYSTEM RESET `%s`;", ENABLE_VERBOSE_ERRORS_KEY);
+  }
+
+  @Test
+  public void changeSessionAndNotSystem() throws Exception {
+    // change options
+    test("ALTER SESSION SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+    test("ALTER SYSTEM SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+    // check changed
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE type = 'SESSION' AND name = '%s'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+    // check changed
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE type = 'SYSTEM' AND name = '%s'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+
+    // reset all session options
+    test("ALTER SESSION RESET ALL;");
+    // check no session options changed
+    testBuilder()
+      .sqlQuery("SELECT status FROM sys.options WHERE status <> 'DEFAULT' AND type
= 'SESSION'")
+      .unOrdered()
+      .expectsEmptyResultSet()
+      .build()
+      .run();
+    // check changed
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE type = 'SYSTEM' AND name = '%s'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+  }
+
+  @Test
+  public void changeSystemAndNotSession() throws Exception {
+    // change options
+    test("ALTER SESSION SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+    test("ALTER SYSTEM SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+    // check changed
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE type = 'SESSION' AND name = '%s'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+    // check changed
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE type = 'SYSTEM' AND name = '%s'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+
+    // reset option
+    test("ALTER system RESET `%s`;", ENABLE_VERBOSE_ERRORS_KEY);
+    // check reverted
+    testBuilder()
+      .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("status")
+      .baselineValues("DEFAULT")
+      .build()
+      .run();
+    // check changed
+    testBuilder()
+      .sqlQuery("SELECT bool_val FROM sys.options WHERE type = 'SESSION' AND name = '%s'",
ENABLE_VERBOSE_ERRORS_KEY)
+      .unOrdered()
+      .baselineColumns("bool_val")
+      .baselineValues(true)
+      .build()
+      .run();
+  }
+
+  @Test
+  public void unsupportedLiteralValidation() throws Exception {
+    thrownException.expect(new UserExceptionMatcher(VALIDATION,
+      "Drill doesn't support assigning literals of type"));
+    test("ALTER session SET `%s` = DATE '1995-01-01';", ENABLE_VERBOSE_ERRORS_KEY);
+  }
 }

http://git-wip-us.apache.org/repos/asf/drill/blob/401119f3/pom.xml
----------------------------------------------------------------------
diff --git a/pom.xml b/pom.xml
index 162efb1..9d59e2e 100644
--- a/pom.xml
+++ b/pom.xml
@@ -1228,7 +1228,7 @@
           <dependency>
             <groupId>org.apache.calcite</groupId>
             <artifactId>calcite-core</artifactId>
-            <version>1.4.0-drill-r5</version>
+            <version>1.4.0-drill-r6</version>
             <exclusions>
               <exclusion>
                 <groupId>org.jgrapht</groupId>


Mime
View raw message