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 <skat...@maprtech.com> Authored: Tue Sep 15 16:13:23 2015 -0700 Committer: adeneche <adene...@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>
