From f785c3ba4d50bfe5a7ff77a8d37b3589bbe1c1cf Mon Sep 17 00:00:00 2001 From: Sudheesh Katkam Date: Tue, 15 Sep 2015 16:13:23 -0700 Subject: [PATCH] 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 --- .../sql/handlers/SetOptionHandler.java | 91 ++++++++------ .../parser/CompoundIdentifierConverter.java | 6 +- .../server/options/InMemoryOptionManager.java | 18 ++- .../exec/server/options/OptionManager.java | 27 +++++ .../exec/server/options/OptionValue.java | 4 +- .../server/options/QueryOptionManager.java | 10 ++ .../server/options/SessionOptionManager.java | 30 ++++- .../server/options/SystemOptionManager.java | 27 +++++ .../drill/exec/testing/ExecutionControls.java | 2 +- .../apache/drill/exec/server/TestOptions.java | 113 +++++++++++++++++- pom.xml | 2 +- 11 files changed, 285 insertions(+), 45 deletions(-) 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 85ab5288039..66c69350bac 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,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); } } } 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 ebe6d396910..8aeab271026 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,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 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); } 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 dbff3e26332..41722606a42 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.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 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); 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 8ff0f9444f2..4b6d09f4709 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,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 { */ 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 */ 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 b73b669e9b5..3d392cbc816 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 @@ -27,11 +27,11 @@ @JsonInclude(Include.NON_NULL) public class OptionValue implements Comparable { - public static enum OptionType { + public enum OptionType { BOOT, SYSTEM, SESSION, QUERY } - public static enum Kind { + public enum Kind { BOOLEAN, LONG, STRING, DOUBLE } 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 26cf6883371..06fac73513b 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 @@ -30,6 +30,16 @@ public QueryOptionManager(OptionManager sessionOptions) { super(sessionOptions, CaseInsensitiveMap.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(); 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 eb0da0382d8..640511f1811 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,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); 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 118f7ad7549..d0e427da7ee 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.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 names = Sets.newHashSet(); + for (final Map.Entry 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()); 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 8f9589dec08..9673394bd55 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 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; } 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 f20fd257091..fe437d6f98c 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.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,124 @@ public void testOptions() throws Exception{ @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, + test(String.format("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 unsupportedMultipartIdentifierValidation() throws Exception { + thrownException.expect(new UserExceptionMatcher(VALIDATION, + "Drill does not support multi-part identifier for an option name")); + test("ALTER session SET `drill`.`baby`.`drill` = 'yes';"); + } + + @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); + } } diff --git a/pom.xml b/pom.xml index 93f423a65a8..b2d4cc42bd4 100644 --- a/pom.xml +++ b/pom.xml @@ -1227,7 +1227,7 @@ org.apache.calcite calcite-core - 1.4.0-drill-r2 + 1.4.0-drill-r3 org.jgrapht