diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java index d49b5006c28..c4eaae88ad6 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/compile/QueryClassLoader.java @@ -25,7 +25,7 @@ import java.util.concurrent.atomic.AtomicLong; import org.apache.drill.common.config.DrillConfig; -import org.apache.drill.common.exceptions.ExpressionParsingException; +import org.apache.drill.common.exceptions.UserException; import org.apache.drill.exec.compile.ClassTransformer.ClassNames; import org.apache.drill.exec.exception.ClassTransformationException; import org.apache.drill.exec.server.options.OptionManager; @@ -39,18 +39,20 @@ import com.google.common.collect.MapMaker; public class QueryClassLoader extends URLClassLoader { - static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(QueryClassLoader.class); + private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(QueryClassLoader.class); public static final String JAVA_COMPILER_OPTION = "exec.java_compiler"; public static final StringValidator JAVA_COMPILER_VALIDATOR = new StringValidator(JAVA_COMPILER_OPTION, CompilerPolicy.DEFAULT.toString()) { @Override - public void validate(OptionValue v) throws ExpressionParsingException { + public void validate(OptionValue v) { super.validate(v); try { CompilerPolicy.valueOf(v.string_val.toUpperCase()); } catch (IllegalArgumentException e) { - throw new ExpressionParsingException(String.format("Invalid value '%s' specified for option '%s'. Valid values are %s.", - v.string_val, getOptionName(), Arrays.toString(CompilerPolicy.values()))); + throw UserException.validationError() + .message("Invalid value '%s' specified for option '%s'. Valid values are %s.", + v.string_val, getOptionName(), Arrays.toString(CompilerPolicy.values())) + .build(logger); } } }; diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/BaseOptionManager.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/BaseOptionManager.java index e15eddf1442..2e0b7973943 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/BaseOptionManager.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/BaseOptionManager.java @@ -23,13 +23,12 @@ import org.apache.drill.exec.server.options.TypeValidators.StringValidator; abstract class BaseOptionManager implements OptionManager { - static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BaseOptionManager.class); - +// private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(BaseOptionManager.class); private OptionValue getOptionSafe(OptionValidator validator){ OptionValue value = getOption(validator.getOptionName()); if(value == null){ - throw new IllegalArgumentException(String.format("Unknown value for boolean option `%s`.", validator.getOptionName())); + throw new IllegalArgumentException(String.format("Unknown value for option `%s`.", validator.getOptionName())); } return value; } 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 142fa0ebf7a..18ebe53ecbf 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 @@ -48,13 +48,32 @@ public OptionValue getOption(String name) { } } + /** + * Gets the option values managed by this manager as an iterable. + * + * @return iterable of option values + */ abstract Iterable optionIterable(); + + /** + * Gets the option value from this manager without falling back. + * + * @param name the option name + * @return the option value, or null if the option does not exist locally + */ abstract OptionValue getLocalOption(String name); + + /** + * Sets the option value for this manager without falling back. + * + * @param value the option value + * @return true iff the value was successfully set + */ abstract boolean setLocalOption(OptionValue value); @Override public void setOption(OptionValue value) { - fallback.getAdmin().validate(value); + getAdmin().validate(value); setValidatedOption(value); } 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 e4dbbf83240..ca85da1146b 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 @@ -23,7 +23,7 @@ import com.google.common.collect.Maps; public class FragmentOptionManager extends InMemoryOptionManager { - static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FragmentOptionManager.class); +// private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(FragmentOptionManager.class); public FragmentOptionManager(OptionManager systemOptions, OptionList options) { super(systemOptions, getMapFromOptionList(options)); @@ -32,7 +32,7 @@ public FragmentOptionManager(OptionManager systemOptions, OptionList options) { private static Map getMapFromOptionList(OptionList options){ Map tmp = Maps.newHashMap(); for(OptionValue v : options){ - tmp.put(v.name, v); + tmp.put(v.name.toLowerCase(), v); } return ImmutableMap.copyOf(tmp); } 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 b033a034ce1..f35f9dbe4a3 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 @@ -22,7 +22,8 @@ public abstract class InMemoryOptionManager extends FallbackOptionManager { static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(InMemoryOptionManager.class); - final Map options; + // NOTE: CRUD operations must use lowercase keys + protected final Map options; InMemoryOptionManager(OptionManager fallback, Map options) { super(fallback); @@ -31,13 +32,13 @@ public abstract class InMemoryOptionManager extends FallbackOptionManager { @Override OptionValue getLocalOption(String name) { - return options.get(name); + return options.get(name.toLowerCase()); } @Override boolean setLocalOption(OptionValue value) { if(supportsOption(value)){ - options.put(value.name, value); + options.put(value.name.toLowerCase(), value); return true; }else{ return false; @@ -50,6 +51,12 @@ Iterable optionIterable() { return options.values(); } + /** + * Check (e.g. option type) to see if implementations of this manager support this option. + * + * @param value the option value + * @return true iff the option value is supported + */ abstract boolean supportsOption(OptionValue value); } 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 414713a96ce..b1896f362ac 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,26 +17,137 @@ */ package org.apache.drill.exec.server.options; +import org.apache.drill.common.exceptions.UserException; import org.apache.calcite.sql.SqlLiteral; +/** + * Manager for Drill options. Implementations must be case-insensitive to the name of the option, specifically the + * {@link #setOption}, {@link #getOption} and {@link #getDefault} methods must ignore the case of the option name. + */ public interface OptionManager extends Iterable { - public OptionValue getOption(String name); - public void setOption(OptionValue value) throws SetOptionException; - public void setOption(String name, SqlLiteral literal, OptionValue.OptionType type) throws SetOptionException; - public OptionAdmin getAdmin(); - public OptionManager getSystemManager(); - public OptionList getOptionList(); - public OptionValue getDefault(final String name); - - public boolean getOption(TypeValidators.BooleanValidator validator); - public double getOption(TypeValidators.DoubleValidator validator); - public long getOption(TypeValidators.LongValidator validator); - public String getOption(TypeValidators.StringValidator validator); + /** + * Gets the option value for the given option name. + * + * @param name option name + * @return the option value, null if the option does not exist + */ + OptionValue getOption(String name); + + /** + * Sets an option value. + * + * @param value option value + * @throws UserException message to describe error with value + */ + void setOption(OptionValue value); + + /** + * Sets the option value built from the given parameters. + * + * @param name option name + * @param literal sql literal + * @param type option type + * @throws UserException message to describe error with value + */ + void setOption(String name, SqlLiteral literal, OptionValue.OptionType type); + + /** + * Gets the option admin for this manager. + * + * @return the option admin + */ + OptionAdmin getAdmin(); + + /** + * Gets the {@link SystemOptionManager} for this manager. + * + * @return the system option manager + */ + OptionManager getSystemManager(); + + /** + * Gets the list of options managed this manager. + * + * @return the list of options + */ + OptionList getOptionList(); + + /** + * Gets the default option value for the given option name. + * + * @param name option name + * @return the default option value, or null if the option does not exist + */ + OptionValue getDefault(String name); + + /** + * Gets the boolean value (from the option value) for the given boolean validator. + * + * @param validator the boolean validator + * @return the boolean value + */ + boolean getOption(TypeValidators.BooleanValidator validator); + + /** + * Gets the double value (from the option value) for the given double validator. + * + * @param validator the double validator + * @return the double value + */ + double getOption(TypeValidators.DoubleValidator validator); + + /** + * Gets the long value (from the option value) for the given long validator. + * + * @param validator the long validator + * @return the long value + */ + long getOption(TypeValidators.LongValidator validator); + + /** + * Gets the string value (from the option value) for the given string validator. + * + * @param validator the string validator + * @return the string value + */ + String getOption(TypeValidators.StringValidator validator); + + /** + * Administrator that is used to validate the options. + */ public interface OptionAdmin { - public void registerOptionType(OptionValidator validator); - public OptionValidator getValidator(String name); - public void validate(OptionValue v) throws SetOptionException; - public OptionValue validate(String name, SqlLiteral value, OptionValue.OptionType optionType) throws SetOptionException; + + /** + * Gets the validator for the given option name. Implementations must be case insensitive to the name. + * + * @param name option name + * @return the option validator, or null if the validator does not exist + * @throws UserException message to describe error with value + */ + OptionValidator getValidator(String name); + + /** + * Validates the option value. + * + * @param value option value + * @throws UserException message to describe error with value + */ + void validate(OptionValue value); + + /** + * Validate the option value built from the parameters. For options that support some ambiguity in their settings, + * such as case-insensitivity for string options, this method returns a modified version of the passed value that + * is considered the standard format of the option that should be used for system-internal representation. + * + * @param name option name + * @param value sql literal + * @param optionType option type + * @return the value requested, in its standard format to be used for representing the value within Drill + * Example: all lower case values for strings, to avoid ambiguities in how values are stored + * while allowing some flexibility for users + * @throws UserException message to describe error with value + */ + OptionValue validate(String name, SqlLiteral value, OptionValue.OptionType optionType); } } diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java index 31a25fc5e4e..2beaa1de72f 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValidator.java @@ -17,13 +17,11 @@ ******************************************************************************/ package org.apache.drill.exec.server.options; -import org.apache.drill.common.exceptions.ExpressionParsingException; +import org.apache.drill.common.exceptions.UserException; import org.apache.calcite.sql.SqlLiteral; /** * Validates the values provided to Drill options. - * - * @param */ public abstract class OptionValidator { // Stored here as well as in the option static class to allow insertion of option optionName into @@ -35,19 +33,10 @@ public OptionValidator(String optionName) { } /** - * This method determines if a given value is a valid setting for an option. For options that support some - * ambiguity in their settings, such as case-insensitivity for string options, this method returns a modified - * version of the passed value that is considered the standard format of the option that should be used for - * system-internal representation. + * Gets the name of the option for this validator. * - * @param value - the value to validate - * @return - the value requested, in its standard format to be used for representing the value within Drill - * Example: all lower case values for strings, to avoid ambiguities in how values are stored - * while allowing some flexibility for users - * @throws ExpressionParsingException - message to describe error with value, including range or list of expected values + * @return the option name */ - public abstract OptionValue validate(SqlLiteral value, OptionValue.OptionType optionType) throws ExpressionParsingException; - public String getOptionName() { return optionName; } @@ -60,6 +49,7 @@ public String getOptionName() { * (1) override this method to return true, and * (2) return the number of queries for which the option is valid through {@link #getTtl}. * E.g. {@link org.apache.drill.exec.testing.ExecutionControls.ControlsOptionValidator} + * * @return if this validator is for a short-lived option */ public boolean isShortLived() { @@ -69,6 +59,7 @@ public boolean isShortLived() { /** * If an option is short-lived, this returns the number of queries for which the option is valid. * Please read the note at {@link #isShortLived} + * * @return number of queries for which the option should be valid */ public int getTtl() { @@ -78,11 +69,36 @@ public int getTtl() { return 0; } - public String getDefaultString() { - return null; - } - + /** + * Gets the default option value for this validator. + * + * @return default option value + */ public abstract OptionValue getDefault(); - public abstract void validate(OptionValue v) throws ExpressionParsingException; + /** + * This method determines if a given value is a valid setting for an option. For options that support some + * ambiguity in their settings, such as case-insensitivity for string options, this method returns a modified + * version of the passed value that is considered the standard format of the option that should be used for + * system-internal representation. + * + * @param value the value to validate + * @throws UserException message to describe error with value, including range or list of expected values + */ + public abstract void validate(OptionValue value); + + /** + * This method determines if a given value is a valid setting for an option. For options that support some + * ambiguity in their settings, such as case-insensitivity for string options, this method returns a modified + * version of the passed value that is considered the standard format of the option that should be used for + * system-internal representation. + * + * @param value the sql literal to validate + * @param optionType the option type + * @return the value requested, in its standard format to be used for representing the value within Drill + * Example: all lower case values for strings, to avoid ambiguities in how values are stored + * while allowing some flexibility for users + * @throws UserException message to describe error with value, including range or list of expected values + */ + public abstract OptionValue validate(SqlLiteral value, OptionValue.OptionType optionType); } 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 fa9223d730c..316362bbec8 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 @@ -22,13 +22,12 @@ import org.apache.drill.exec.server.options.OptionValue.OptionType; public class QueryOptionManager extends InMemoryOptionManager { - 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); public QueryOptionManager(OptionManager sessionOptions) { super(sessionOptions, new HashMap()); } - @Override public OptionList getOptionList() { OptionList list = super.getOptionList(); @@ -36,11 +35,9 @@ public OptionList getOptionList() { return list; } - @Override boolean supportsOption(OptionValue value) { return value.type == OptionType.QUERY; } - } 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 340358fd62d..4f91a53a5ee 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 @@ -32,6 +32,7 @@ public class SessionOptionManager extends InMemoryOptionManager { /** * Map of short lived options. Key: option name, Value: [ start, end ) + * NOTE: CRUD operations must use lowercase keys */ private final ConcurrentHashMap> shortLivedOptions = new ConcurrentHashMap<>(); @@ -43,30 +44,34 @@ public SessionOptionManager(final OptionManager systemOptions, final UserSession @Override boolean setLocalOption(final OptionValue value) { final boolean set = super.setLocalOption(value); - final String name = value.name; - final OptionValidator validator = fallback.getAdmin().getValidator(name); + if (!set) { + return false; + } + final String name = value.name.toLowerCase(); + final OptionValidator validator = getAdmin().getValidator(name); final boolean shortLived = validator.isShortLived(); - if (set && shortLived) { + if (shortLived) { final int start = session.getQueryCount() + 1; // start from the next query final int ttl = validator.getTtl(); final int end = start + ttl; shortLivedOptions.put(name, new ImmutablePair<>(start, end)); } - return set; + return true; } @Override - OptionValue getLocalOption(final String name) { - final OptionValue value = options.get(name); + OptionValue getLocalOption(String name) { + name = name.toLowerCase(); + final OptionValue value = super.getLocalOption(name); if (shortLivedOptions.containsKey(name)) { - if (withinRange(value)) { + if (withinRange(name)) { return value; } final int queryNumber = session.getQueryCount(); final int start = shortLivedOptions.get(name).getLeft(); // option is not in effect if queryNumber < start if (queryNumber < start) { - return fallback.getAdmin().getValidator(name).getDefault(); + return getAdmin().getValidator(name).getDefault(); // reset if queryNumber <= end } else { options.remove(name); @@ -77,9 +82,9 @@ OptionValue getLocalOption(final String name) { return value; } - private boolean withinRange(final OptionValue value) { + private boolean withinRange(final String name) { final int queryNumber = session.getQueryCount(); - final ImmutablePair pair = shortLivedOptions.get(value.name); + final ImmutablePair pair = shortLivedOptions.get(name); final int start = pair.getLeft(); final int end = pair.getRight(); return start <= queryNumber && queryNumber < end; @@ -88,8 +93,8 @@ private boolean withinRange(final OptionValue value) { private final Predicate isLive = new Predicate() { @Override public boolean apply(final OptionValue value) { - final String name = value.name; - return !shortLivedOptions.containsKey(name) || withinRange(value); + final String name = value.name.toLowerCase(); + return !shortLivedOptions.containsKey(name) || withinRange(name); } }; diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SetOptionException.java b/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SetOptionException.java deleted file mode 100644 index dd698c382ab..00000000000 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SetOptionException.java +++ /dev/null @@ -1,62 +0,0 @@ -/** - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.drill.exec.server.options; - -import java.util.Set; - -import javax.validation.ConstraintViolation; - -import org.apache.drill.common.exceptions.LogicalPlanParsingException; -import org.apache.drill.common.logical.data.LogicalOperator; -import org.apache.drill.common.logical.data.LogicalOperatorBase; - -public class SetOptionException extends LogicalPlanParsingException{ - static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SetOptionException.class); - - public SetOptionException() { - super(); - - } - - public SetOptionException(LogicalOperator operator, Set> violations) { - super(operator, violations); - - } - - public SetOptionException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) { - super(message, cause, enableSuppression, writableStackTrace); - - } - - public SetOptionException(String message, Throwable cause) { - super(message, cause); - - } - - public SetOptionException(String message) { - super(message); - - } - - public SetOptionException(Throwable cause) { - super(cause); - - } - - -} 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 2d41740c94a..cdf96ff3afd 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 @@ -24,8 +24,11 @@ import java.util.concurrent.ConcurrentMap; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Maps; +import org.apache.calcite.sql.SqlLiteral; import org.apache.commons.collections.IteratorUtils; import org.apache.drill.common.config.DrillConfig; +import org.apache.drill.common.exceptions.UserException; import org.apache.drill.exec.ExecConstants; import org.apache.drill.exec.compile.ClassTransformer; import org.apache.drill.exec.compile.QueryClassLoader; @@ -35,9 +38,6 @@ import org.apache.drill.exec.store.sys.PStoreConfig; import org.apache.drill.exec.store.sys.PStoreProvider; import org.apache.drill.exec.util.AssertionUtil; -import org.apache.calcite.sql.SqlLiteral; - -import com.google.common.collect.Maps; public class SystemOptionManager extends BaseOptionManager { private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(SystemOptionManager.class); @@ -123,9 +123,17 @@ public class SystemOptionManager extends BaseOptionManager { } private final PStoreConfig config; + + // persistent store for options that have been changed from default + // NOTE: CRUD operations must use lowercase keys private PStore options; + private SystemOptionAdmin admin; + + // all known options; also contains default values + // NOTE: CRUD operations must use lowercase keys private final ConcurrentMap knownOptions = Maps.newConcurrentMap(); + private final PStoreProvider provider; public SystemOptionManager(final DrillConfig config, final PStoreProvider provider) { @@ -141,15 +149,43 @@ public SystemOptionManager init() throws IOException { return this; } + /** + * Get the {@link OptionValue} from the persistent store. + * + * @param name name of the option + * @return the option value of present; null otherwise + */ + private OptionValue getFromOptions(final String name) { + return options.get(name.toLowerCase()); + } + + /** + * Gets the {@link OptionValidator} from the known validators. + * + * @param name name of the option + * @return the associated validator + * @throws UserException - if the validator is not found + */ + private OptionValidator getFromKnown(final String name) { + final OptionValidator validator = knownOptions.get(name.toLowerCase()); + if (validator == null) { + throw UserException.validationError() + .message("Unknown option %s", name) + .build(logger); + } + return validator; + } + @Override public Iterator iterator() { final Map buildList = Maps.newHashMap(); - for(OptionValidator v : knownOptions.values()){ - buildList.put(v.getOptionName(), v.getDefault()); + // populate the default options + for (final Map.Entry v : knownOptions.entrySet()) { + buildList.put(v.getKey(), v.getValue().getDefault()); } - for(Map.Entry v : options){ - final OptionValue value = v.getValue(); - buildList.put(value.name, value); + // override if changed + for (final Map.Entry v : options) { + buildList.put(v.getKey(), v.getValue()); } return buildList.values().iterator(); } @@ -157,28 +193,19 @@ public Iterator iterator() { @Override public OptionValue getOption(final String name) { // check local space - final OptionValue v = options.get(name); - if(v != null){ - return v; + final OptionValue value = getFromOptions(name); + if (value != null) { + return value; } // otherwise, return default. - OptionValidator validator = knownOptions.get(name); - if(validator == null) { - return null; - } else { - return validator.getDefault(); - } + final OptionValidator validator = getFromKnown(name); + return validator.getDefault(); } @Override public OptionValue getDefault(final String name) { - final OptionValidator validator = knownOptions.get(name); - if(validator == null) { - return null; - } else { - return validator.getDefault(); - } + return getFromKnown(name).getDefault(); } @Override @@ -189,15 +216,16 @@ public void setOption(final OptionValue value) { } private void setOptionInternal(final OptionValue value) { - if (!value.equals(knownOptions.get(value.name))) { - options.put(value.name, value); + final String name = value.name; + if (getFromOptions(name) == null && value.equals(getFromKnown(name).getDefault())) { + return; // if the option is not overridden, ignore setting option to default } + options.put(name.toLowerCase(), value); } - @Override public void setOption(final String name, final SqlLiteral literal, final OptionType type) { - assert type == OptionValue.OptionType.SYSTEM || type == OptionValue.OptionType.SESSION; + assert type == OptionValue.OptionType.SYSTEM; final OptionValue v = admin.validate(name, literal, type); setOptionInternal(v); } @@ -218,51 +246,51 @@ public OptionAdmin getAdmin() { } private class SystemOptionAdmin implements OptionAdmin { + public SystemOptionAdmin() { - for(OptionValidator v : VALIDATORS) { - knownOptions.put(v.getOptionName(), v); + // register all options with a lower case name + for (final OptionValidator v : VALIDATORS) { + final String name = v.getOptionName().toLowerCase(); + if (!name.equals(v.getOptionName())) { + logger.warn("Registering option with a lower case name `{}`", name); + } + knownOptions.put(name, v); } - for(Entry v : options) { - final OptionValue value = v.getValue(); - final OptionValidator defaultValidator = knownOptions.get(v.getKey()); + // if necessary, deprecate and replace options from persistent store + for (final Entry v : options) { + final String name = v.getKey(); + final OptionValidator defaultValidator = knownOptions.get(name.toLowerCase()); if (defaultValidator == null) { // deprecated option, delete. - options.delete(value.name); - logger.warn("Deleting deprecated option `{}`.", value.name); + options.delete(name); + logger.warn("Deleting deprecated option `{}`", name); + } else { + if (!name.equals(defaultValidator.getOptionName().toLowerCase())) { + // for backwards compatibility <= 1.1, rename to lower case + final OptionValue value = v.getValue(); + options.delete(name); + options.put(name.toLowerCase(), value); + logger.warn("Changing option name to lower case `{}`", name); + } } } } - @Override - public void registerOptionType(final OptionValidator validator) { - if (null != knownOptions.putIfAbsent(validator.getOptionName(), validator)) { - throw new IllegalArgumentException("Only one option is allowed to be registered with name: " - + validator.getOptionName()); - } - } - @Override public OptionValidator getValidator(final String name) { - return knownOptions.get(name); + return getFromKnown(name); } @Override - public void validate(final OptionValue v) throws SetOptionException { - final OptionValidator validator = knownOptions.get(v.name); - if (validator == null) { - throw new SetOptionException("Unknown option " + v.name); - } + public void validate(final OptionValue v) { + final OptionValidator validator = getFromKnown(v.name); validator.validate(v); } @Override - public OptionValue validate(final String name, final SqlLiteral value, final OptionType optionType) - throws SetOptionException { - final OptionValidator validator = knownOptions.get(name); - if (validator == null) { - throw new SetOptionException("Unknown option: " + name); - } + public OptionValue validate(final String name, final SqlLiteral value, final OptionType optionType) { + final OptionValidator validator = getFromKnown(name); return validator.validate(value, optionType); } } 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 b8597b72469..d6e206b6463 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 @@ -17,13 +17,11 @@ */ package org.apache.drill.exec.server.options; -import java.io.IOException; import java.math.BigDecimal; import java.util.HashSet; import java.util.Set; -import com.fasterxml.jackson.databind.ObjectMapper; -import org.apache.drill.common.exceptions.ExpressionParsingException; +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; @@ -32,7 +30,7 @@ import org.apache.calcite.util.NlsString; public class TypeValidators { -// private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TypeValidators.class); + private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TypeValidators.class); public static class PositiveLongValidator extends LongValidator { private final long max; @@ -43,11 +41,12 @@ public PositiveLongValidator(String name, long max, long def) { } @Override - public void validate(OptionValue v) throws ExpressionParsingException { + public void validate(OptionValue v) { super.validate(v); - if (v.num_val > max || v.num_val < 0) { - throw new ExpressionParsingException(String.format("Option %s must be between %d and %d.", getOptionName(), 0, - max)); + if (v.num_val > max || v.num_val < 1) { + throw UserException.validationError() + .message(String.format("Option %s must be between %d and %d.", getOptionName(), 1, max)) + .build(logger); } } } @@ -59,14 +58,16 @@ public PowerOfTwoLongValidator(String name, long max, long def) { } @Override - public void validate(OptionValue v) throws ExpressionParsingException { + public void validate(OptionValue v) { super.validate(v); if (!isPowerOfTwo(v.num_val)) { - throw new ExpressionParsingException(String.format("Option %s must be a power of two.", getOptionName())); + throw UserException.validationError() + .message(String.format("Option %s must be a power of two.", getOptionName())) + .build(logger); } } - private boolean isPowerOfTwo(long num) { + private static boolean isPowerOfTwo(long num) { return (num & (num - 1)) == 0; } } @@ -82,14 +83,14 @@ public RangeDoubleValidator(String name, double min, double max, double def) { } @Override - public void validate(OptionValue v) throws ExpressionParsingException { + public void validate(OptionValue v) { super.validate(v); if (v.float_val > max || v.float_val < min) { - throw new ExpressionParsingException(String.format("Option %s must be between %f and %f.", - getOptionName(), min, max)); + throw UserException.validationError() + .message(String.format("Option %s must be between %f and %f.", getOptionName(), min, max)) + .build(logger); } } - } public static class BooleanValidator extends TypeValidator { @@ -127,11 +128,12 @@ public RangeLongValidator(String name, long min, long max, long def) { } @Override - public void validate(OptionValue v) throws ExpressionParsingException { + public void validate(OptionValue v) { super.validate(v); if (v.num_val > max || v.num_val < min) { - throw new ExpressionParsingException(String.format("Option %s must be between %d and %d.", - getOptionName(), min, max)); + throw UserException.validationError() + .message(String.format("Option %s must be between %d and %d.", getOptionName(), min, max)) + .build(logger); } } } @@ -150,39 +152,12 @@ public EnumeratedStringValidator(String name, String def, String... values) { } @Override - public void validate(final OptionValue v) throws ExpressionParsingException { + public void validate(final OptionValue v) { super.validate(v); if (!valuesSet.contains(v.string_val.toLowerCase())) { - throw new ExpressionParsingException(String.format("Option %s must be one of: %s", getOptionName(), valuesSet)); - } - } - } - - /** - * Validator for POJO passed in as JSON string - */ - public static class JsonStringValidator extends StringValidator { - - private static final ObjectMapper mapper = new ObjectMapper(); - private final Class clazz; - - public JsonStringValidator(final String name, final Class clazz, final String def) { - super(name, def); - this.clazz = clazz; - validateJson(def, clazz); - } - - @Override - public void validate(final OptionValue v) throws ExpressionParsingException { - super.validate(v); - validateJson(v.string_val, clazz); - } - - private static void validateJson(final String jsonString, final Class clazz) { - try { - mapper.readValue(jsonString, clazz); - } catch (IOException e) { - throw new ExpressionParsingException("Invalid JSON string (" + jsonString + ") for class " + clazz.getName(), e); + throw UserException.validationError() + .message(String.format("Option %s must be one of: %s.", getOptionName(), valuesSet)) + .build(logger); } } } @@ -203,19 +178,19 @@ public OptionValue getDefault() { } @Override - public OptionValue validate(final SqlLiteral value, final OptionType optionType) - throws ExpressionParsingException { + public OptionValue validate(final SqlLiteral value, final OptionType optionType) { final OptionValue op = getPartialValue(getOptionName(), optionType, value); validate(op); return op; } @Override - public void validate(final OptionValue v) throws ExpressionParsingException { + public void validate(final OptionValue v) { if (v.kind != kind) { - throw new ExpressionParsingException(String.format( - "Option %s must be of type %s but you tried to set to %s.", - getOptionName(), kind.name(), v.kind.name())); + throw UserException.validationError() + .message(String.format("Option %s must be of type %s but you tried to set to %s.", getOptionName(), + kind.name(), v.kind.name())) + .build(logger); } } } @@ -252,8 +227,9 @@ private static OptionValue getPartialValue(final String name, final OptionType t 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(String.format("Drill doesn't support set option expressions with literals of type %s.", typeName)) + .build(logger); } } } 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 2c0afe416b3..8f9589dec08 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 @@ -20,11 +20,10 @@ import com.fasterxml.jackson.annotation.JsonSubTypes; import com.fasterxml.jackson.annotation.JsonSubTypes.Type; import com.fasterxml.jackson.annotation.JsonTypeInfo; -import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import org.apache.drill.common.exceptions.DrillRuntimeException; -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.proto.CoordinationProtos.DrillbitEndpoint; import org.apache.drill.exec.server.options.OptionManager; @@ -81,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.DOUBLE, OptionValue.createString(OptionType.SESSION, name, def)); + super(name, OptionValue.Kind.STRING, OptionValue.createString(OptionType.SESSION, name, def)); assert ttl > 0; this.ttl = ttl; } @@ -97,15 +96,19 @@ public boolean isShortLived() { } @Override - public void validate(final OptionValue v) throws ExpressionParsingException { + public void validate(final OptionValue v) { if (v.type != OptionType.SESSION) { - throw new ExpressionParsingException("Controls can be set only at SESSION level."); + throw UserException.validationError() + .message("Controls can be set only at SESSION level.") + .build(logger); } final String jsonString = v.string_val; try { validateControlsString(jsonString); } catch (final IOException e) { - throw new ExpressionParsingException("Invalid control options string (" + jsonString + ").", e); + throw UserException.validationError() + .message(String.format("Invalid controls option string (%s) due to %s.", jsonString, e.getMessage())) + .build(logger); } } } 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 c522da88647..e1ab73f239a 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 @@ -18,6 +18,8 @@ package org.apache.drill.exec.server; import org.apache.drill.BaseTestQuery; +import org.apache.drill.common.exceptions.UserException; +import org.apache.drill.exec.ExecConstants; import org.junit.Test; public class TestOptions extends BaseTestQuery{ @@ -31,11 +33,16 @@ public void testDrillbits() throws Exception{ @Test public void testOptions() throws Exception{ test( - "select * from sys.options;" + + "select * from sys.options;" + "ALTER SYSTEM set `planner.disable_exchanges` = true;" + "select * from sys.options;" + "ALTER SESSION set `planner.disable_exchanges` = true;" + "select * from sys.options;" - ); + ); + } + + @Test(expected = UserException.class) + public void checkException() throws Exception { + test(String.format("ALTER session SET `%s` = '%s';", ExecConstants.SLICE_TARGET, "fail")); } }