Details
-
Improvement
-
Status: Closed
-
Minor
-
Resolution: Fixed
-
None
-
None
Description
Within a session, currently we set configuration options and it would be very useful to have a 'reset' command to reset the value of an option to its default system value:
ALTER SESSION RESET <option name>
If we don't want to add a new keyword for RESET, we could potentially overload the SET command and allow the user to set to the 'default' value.
Attachments
Issue Links
- depends upon
-
CALCITE-823 Parser support to reset an option
- Closed
Activity
Propose syntax:
ALTER SESSION RESET `option name`
ALTER SYSTEM RESET `option name`
ALTER SESSION RESET ALL
ALTER SYSTEM RESET ALL
GitHub user sudheeshkatkam opened a pull request:
https://github.com/apache/drill/pull/159
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
@jacques-n @vkorukanti please review.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sudheeshkatkam/drill DRILL-1065
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/159.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #159
commit f785c3ba4d50bfe5a7ff77a8d37b3589bbe1c1cf
Author: Sudheesh Katkam <skatkam@maprtech.com>
Date: 2015-09-15T23:13:23Z
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
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39644014
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java —
@@ -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); + }+
{ // No scope mentioned assumed SESSION + type = OptionType.SESSION; + }
+ final String scope = option.getScope();
+ final OptionValue.OptionType type;
+ if (scope == null)else
Unknown macro: { 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)
Unknown macro: { + // 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); } + }+
{ + throw UserException.validationError() + .message("Drill does not support multi-part identifier for an option name (%s).", + nameIdentifier.toString()) + .build(logger); + }
+ // Currently, we use one part identifiers.
+ final SqlIdentifier nameIdentifier = option.getName();
+ if (!nameIdentifier.isSimple())
+ final String name = nameIdentifier.getSimple();
+ if (value != null)
else
{ - throw new ValidationException("Sql options can only be literals."); + } else { // RESET option
+ if ("ALL".equals(name)) {
— End diff –
.equalsIgnoreCase(...)
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-141176948
I worked around the fallback structure of option managers, which is wrong. I will update the pull request soon.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39796579
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/InMemoryOptionManager.java —
@@ -35,6 +37,20 @@
}
@Override
+ public void deleteOption(final String name, final OptionValue.OptionType type) {
+ throw UserException.unsupportedError()
— End diff –
Why throw a UserException here ? is the error message relevant to the user ? do we even expect this method to be called ?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39796601
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/InMemoryOptionManager.java —
@@ -35,6 +37,20 @@
}
@Override
+ public void deleteOption(final String name, final OptionValue.OptionType type)
+
+ @Override
+ public void deleteAllOptions(final OptionValue.OptionType type) {
+ throw UserException.unsupportedError()
— End diff –
same here
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39797379
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java —
@@ -46,6 +47,33 @@ public SessionOptionManager(final OptionManager systemOptions, final UserSession
}
@Override
+ public void deleteOption(final String name, final OptionValue.OptionType type) {
+ if (type == OptionValue.OptionType.SESSION) {
+ try
catch (final IllegalArgumentException e) {
+ throw UserException.validationError()
— End diff –
you can just use:
```
throw UserException.validationError(e).build(logger);
```
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39798636
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java —
@@ -46,6 +47,33 @@ public SessionOptionManager(final OptionManager systemOptions, final UserSession
}
@Override
+ public void deleteOption(final String name, final OptionValue.OptionType type) {
+ if (type == OptionValue.OptionType.SESSION) {
+ try { // ensure option exists
— End diff –
You only ensure the option exists when `type == SESSION`. How about SYSTEM, is it done by the underlying fallback OptionManager ?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39798736
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java —
@@ -46,6 +47,33 @@ public SessionOptionManager(final OptionManager systemOptions, final UserSession
}
@Override
+ public void deleteOption(final String name, final OptionValue.OptionType type) {
+ if (type == OptionValue.OptionType.SESSION) {
+ try
catch (final IllegalArgumentException e)
{ + throw UserException.validationError() + .message(e.getMessage()) + .build(logger); + } + options.remove(name);
+ shortLivedOptions.remove(name);
+ } else {
+ fallback.deleteOption(name, type);
— End diff –
so if I change an option both at the session and system levels, then I reset the same option at the system level it will leave it changed at the session level. Is this expected ?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39799014
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java —
@@ -46,6 +47,33 @@ public SessionOptionManager(final OptionManager systemOptions, final UserSession
}
@Override
+ public void deleteOption(final String name, final OptionValue.OptionType type) {
+ if (type == OptionValue.OptionType.SESSION) {
+ try
catch (final IllegalArgumentException e)
{ + throw UserException.validationError() + .message(e.getMessage()) + .build(logger); + } + options.remove(name);
+ shortLivedOptions.remove(name);
+ } else
+ }
+
+ @Override
+ public void deleteAllOptions(final OptionValue.OptionType type) {
+ if (type == OptionValue.OptionType.SESSION)
else {
+ fallback.deleteAllOptions(type);
— End diff –
same here
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39799140
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java —
@@ -241,6 +243,31 @@ public void setOption(final OptionValue value) {
}
@Override
+ public void deleteOption(final String name, OptionType type) {
+ assert type == OptionType.SYSTEM;
+ try
catch (final IllegalArgumentException e) {
+ throw UserException.validationError()
— End diff –
you can just use:
```
throw UserException.validationError(e).build(logger);
```
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39803317
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/InMemoryOptionManager.java —
@@ -35,6 +37,20 @@
}
@Override
+ public void deleteOption(final String name, final OptionValue.OptionType type) {
+ throw UserException.unsupportedError()
— End diff –
Removing this in my new implementation.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39803386
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java —
@@ -46,6 +47,33 @@ public SessionOptionManager(final OptionManager systemOptions, final UserSession
}
@Override
+ public void deleteOption(final String name, final OptionValue.OptionType type) {
+ if (type == OptionValue.OptionType.SESSION) {
+ try { // ensure option exists
— End diff –
Yes, the fallback for a SessionOptionManager is a SystemOptionManager.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39803498
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java —
@@ -46,6 +47,33 @@ public SessionOptionManager(final OptionManager systemOptions, final UserSession
}
@Override
+ public void deleteOption(final String name, final OptionValue.OptionType type) {
+ if (type == OptionValue.OptionType.SESSION) {
+ try
catch (final IllegalArgumentException e)
{ + throw UserException.validationError() + .message(e.getMessage()) + .build(logger); + } + options.remove(name);
+ shortLivedOptions.remove(name);
+ } else {
+ fallback.deleteOption(name, type);
— End diff –
I'd say yes. Session options always override system options (default or not).
Also, adding:
+ support for
SET `option name` RESET `option name` RESET ALL
as discussed here with an assumption that the scope is SESSION.
+ allow for compound identifiers for option names
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39804235
— Diff: pom.xml —
@@ -1227,7 +1227,7 @@
<dependency>
<groupId>org.apache.calcite</groupId>
<artifactId>calcite-core</artifactId>
- <version>1.4.0-drill-r2</version>
+ <version>1.4.0-drill-r3</version>-
- End diff –
-
can you add to the commit message that you bumped the calcite version
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39804781
— Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java —
@@ -46,19 +48,124 @@ public void testOptions() throws Exception{
@Test
public void checkValidationException() throws Exception
@Test // DRILL-3122
public void checkChangedColumn() throws Exception
+
+ @Test
+ public void setAndResetSessionOption() throws Exception
+
+ @Test
+ public void setAndResetSystemOption() throws Exception {
— End diff –
please add a test where both SYSTEM and SESSION options are changed, and confirm the reset is working as expected.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-141251928
I have added a note to the SessionOptionManager that the effects of deleting a short lived option are undefined because that might require slightly extensive changes, which is out of scope for this JIRA.
@jacques-n please review the changes in CompoundIdentifierConverter class.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39984380
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java —
@@ -107,7 +117,7 @@ public SqlNode visitChild(
}
SqlNode newOperand = operand.accept(CompoundIdentifierConverter.this);
enableComplex = localEnableComplex;
- if (newOperand != operand) {
+ if (! newOperand.equalsDeep(operand, false)) {-
- End diff –
-
Is it necessary to call equalsDeep()? If the expression has an identifier which is rewritten by CompoundIdentifierConverter, is it true that the new expression would be different in reference from then original one?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39996054
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java —
@@ -49,45 +53,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); + }+
{ // No scope mentioned assumed SESSION + type = OptionType.SESSION; + }
+ final String scope = option.getScope();
+ final OptionValue.OptionType type;
+ if (scope == null)else
Unknown macro: { 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,-
- End diff –
-
you should reuse `context.getOptions()` as a local variable
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39996293
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java —
@@ -49,45 +53,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); + }+
{ // No scope mentioned assumed SESSION + type = OptionType.SESSION; + }
+ final String scope = option.getScope();
+ final OptionValue.OptionType type;
+ if (scope == null)else
Unknown macro: { 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)
Unknown macro: { + // 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())-
- End diff –
-
this is not important, but you can just pass nameIdentifier and .toString() will be called implicitly
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39996432
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java —
@@ -161,6 +171,7 @@ public SqlNode visitChild(
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));
— End diff –
can you add a comment explaining why this rule is needed ?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39996569
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -102,6 +119,29 @@ public void setOption(OptionValue value) {
}
@Override
+ public void deleteOption(final String name, final OptionType type) {
+ try
catch (final IllegalArgumentException e)
{ + throw UserException.validationError(e) + .build(logger); + } +
+ // fallback if unable to delete locally
+ if (!deleteLocalOption(name, type)) {
— End diff –
you should call `name.toLowerCase()` here to make sure implementation will always be case insensitive.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39996854
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -83,14 +84,30 @@ public OptionValue getOption(final String name) {
*/
abstract boolean setLocalOption(OptionValue value);
+ /**
+ * Deletes the option with given name for this manager without falling back.
+ *
+ * @param type option type
+ * @return true iff the option was successfully deleted
— End diff –
this javadoc can have multiple meanings: successfully deleted could either mean the implementation couldn't find the option, or it doesn't support the option type. It should actually return false if it doesn't support the option type
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39996879
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -83,14 +84,30 @@ public OptionValue getOption(final String name) {
*/
abstract boolean setLocalOption(OptionValue value);
+ /**
+ * Deletes the option with given name for this manager without falling back.
+ *
+ * @param type option type
+ * @return true iff the option was successfully deleted
+ */
+ abstract boolean deleteLocalOptions(OptionType type);
+
+ /**
+ * Deletes the option with given name for this manager without falling back.
+ *
+ * @param name option name
+ * @param type option type
+ * @return true iff the option was successfully deleted
— End diff –
same here
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39996968
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -83,14 +84,30 @@ public OptionValue getOption(final String name) {
*/
abstract boolean setLocalOption(OptionValue value);
+ /**
+ * Deletes the option with given name for this manager without falling back.
+ *
+ * @param type option type
+ * @return true iff the option was successfully deleted
— End diff –
actually it's less confusing here because we only pass an `OptionType` to the method but it's more prevalent for `deleteLocalOption()`
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39997032
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -83,14 +84,30 @@ public OptionValue getOption(final String name) {
*/
abstract boolean setLocalOption(OptionValue value);
+ /**
+ * Deletes the option with given name for this manager without falling back.
+ *
+ * @param type option type
+ * @return true iff the option was successfully deleted
+ */
+ abstract boolean deleteLocalOptions(OptionType type);
— End diff –
can you rename this to `deleteAllLocalOptions` ?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39997204
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/InMemoryOptionManager.java —
@@ -54,12 +56,32 @@ boolean setLocalOption(final OptionValue value)
+ @Override
+ boolean deleteLocalOptions(final OptionType type) {
+ if (supportsOption(type))
else
{ + return false; + }+ }
+
+ @Override
+ boolean deleteLocalOption(final String name, final OptionType type) {
+ if (supportsOption(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 supportsOption(OptionType type);-
- End diff –
-
you should probably rename this to `supportsOptionType` because we are effectively checking if the option type is supported. This will help the method be self explanatory
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39997250
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionManager.java —
@@ -31,8 +33,33 @@
void setOption(OptionValue value);
/**
+ * Deletes the option. Unfortunately, the type is required given the fallback structure of option managers.
— End diff –
why unfortunately ?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39997680
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java —
@@ -20,14 +20,21 @@
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
{@link OptionValidator#isShortLived}
+ * 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) are
+ * undefined.-
- End diff –
-
can you please elaborate on the undefined effects of deleting short lived options ?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39997731
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java —
@@ -241,6 +243,30 @@ public void setOption(final OptionValue value) {
}
@Override
+ public void deleteOption(final String name, OptionType type) {
+ assert type == OptionType.SYSTEM;
— End diff –
can you please add an error message to the assertion ?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39997752
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java —
@@ -241,6 +243,30 @@ public void setOption(final OptionValue value) {
}
@Override
+ public void deleteOption(final String name, OptionType type) {
+ assert type == OptionType.SYSTEM;
+ try
catch (final IllegalArgumentException e)
{ + throw UserException.validationError(e) + .build(logger); + } + options.delete(name.toLowerCase());
+ }
+
+ @Override
+ public void deleteAllOptions(OptionType type) {
+ assert type == OptionType.SYSTEM;
— End diff –
same here
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39997945
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java —
@@ -241,6 +243,30 @@ public void setOption(final OptionValue value) {
}
@Override
+ public void deleteOption(final String name, OptionType type) {
+ assert type == OptionType.SYSTEM;
+ try { // ensure option exists
— End diff –
you can extract this try block as a static method `ensureOptionExists`, this same code is duplicated in multiple places
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39998097
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java —
@@ -80,7 +80,7 @@
- @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));-
- End diff –
-
can you add a comment explaining why this needs to be a SYSTEM rather than a SESSION option ? someone else might revert this change inadvertently.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39998161
— Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java —
@@ -46,19 +48,207 @@ public void testOptions() throws Exception{
@Test
public void checkValidationException() throws Exception
@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,-
- End diff –
-
you can remove `String.format` here too
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39998259
— Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java —
@@ -46,19 +48,207 @@ public void testOptions() throws Exception{
@Test
public void checkValidationException() throws Exception
@Test // DRILL-3122
public void checkChangedColumn() throws Exception
+
+ @Test
+ public void setAndResetSessionOption() throws Exception
+
+ @Test
+ public void setAndResetSystemOption() throws Exception
+
+ @Test
+ public void resetAllSessionOptions() throws Exception {
— End diff –
resetting all SYSTEM options is not covered here
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r39998405
— Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java —
@@ -46,19 +48,207 @@ public void testOptions() throws Exception{
@Test
public void checkValidationException() throws Exception
@Test // DRILL-3122
public void checkChangedColumn() throws Exception
+
+ @Test
+ public void setAndResetSessionOption() throws Exception
+
+ @Test
+ public void setAndResetSystemOption() throws Exception
+
+ @Test
+ public void resetAllSessionOptions() throws Exception {
— End diff –
also change SESSION and SYSTEM options then reset all SESSION options
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40010220
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java —
@@ -49,45 +53,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); + }+
{ // No scope mentioned assumed SESSION + type = OptionType.SESSION; + }
+ final String scope = option.getScope();
+ final OptionValue.OptionType type;
+ if (scope == null)else
Unknown macro: { 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,-
- End diff –
-
This is not a change I introduced (just difference in spacing), but I'll address this issue.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40010391
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java —
@@ -161,6 +171,7 @@ public SqlNode visitChild(
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));
— End diff –
There is already a comment explaining the rules, and why they are needed. What more?
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40012463
— Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java —
@@ -46,19 +48,124 @@ public void testOptions() throws Exception{
@Test
public void checkValidationException() throws Exception
@Test // DRILL-3122
public void checkChangedColumn() throws Exception
+
+ @Test
+ public void setAndResetSessionOption() throws Exception
+
+ @Test
+ public void setAndResetSystemOption() throws Exception {
— End diff –
changeSessionAndNotSystem and changeSystemAndNotSession are testing this.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40014413
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java —
@@ -161,6 +171,7 @@ public SqlNode visitChild(
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));
— End diff –
the comment doesn't explain why this specific rule has been added. It just explains how the rules work in general.
Why do we need this specific rule, what happens if I remove it ?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40014502
— Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java —
@@ -46,19 +48,124 @@ public void testOptions() throws Exception{
@Test
public void checkValidationException() throws Exception
@Test // DRILL-3122
public void checkChangedColumn() throws Exception
+
+ @Test
+ public void setAndResetSessionOption() throws Exception
+
+ @Test
+ public void setAndResetSystemOption() throws Exception {
— End diff –
thanks for adding those unit tests
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40020795
— Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java —
@@ -46,19 +48,207 @@ public void testOptions() throws Exception{
@Test
public void checkValidationException() throws Exception
@Test // DRILL-3122
public void checkChangedColumn() throws Exception
+
+ @Test
+ public void setAndResetSessionOption() throws Exception
+
+ @Test
+ public void setAndResetSystemOption() throws Exception
+
+ @Test
+ public void resetAllSessionOptions() throws Exception {
— End diff –
changeSessionAndNotSystem is testing this.
Also, we cannot test resetting all system options because the unit tests have a "before" method that changes a system option.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40020971
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java —
@@ -80,7 +80,7 @@
- @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));-
- End diff –
-
I don't think this requires a comment in code, since this applies for all option validators.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40021824
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java —
@@ -241,6 +243,30 @@ public void setOption(final OptionValue value) {
}
@Override
+ public void deleteOption(final String name, OptionType type) {
+ assert type == OptionType.SYSTEM;
+ try { // ensure option exists
— End diff –
I left this as is because the callers could add context and handle the exception differently.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40021955
— Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java —
@@ -46,19 +48,207 @@ public void testOptions() throws Exception{
@Test
public void checkValidationException() throws Exception
@Test // DRILL-3122
public void checkChangedColumn() throws Exception
+
+ @Test
+ public void setAndResetSessionOption() throws Exception
+
+ @Test
+ public void setAndResetSystemOption() throws Exception
+
+ @Test
+ public void resetAllSessionOptions() throws Exception {
— End diff –
my bad, I meant: we change the same option at the SESSION and SYSTEM level then we reset it at the SESSION level and we make sure it's still changed at the SYSTEM level.
`changeSessionAndNotSystem` kind of does that but with a `RESET ALL` and we need to make sure `ALTER SESSION RESET A` also works fine
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40022217
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java —
@@ -20,14 +20,21 @@
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
{@link OptionValidator#isShortLived}
+ * 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) are
+ * undefined.-
- End diff –
-
Should I add a TODO and create a ticket?
(personally I don't think this is worth a JIRA yet)
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40022907
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionManager.java —
@@ -31,8 +33,33 @@
void setOption(OptionValue value);
/**
+ * Deletes the option. Unfortunately, the type is required given the fallback structure of option managers.
— End diff –
Unfortunately because we assume fallback structure of option managers
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40022994
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControls.java —
@@ -80,7 +80,7 @@
- @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));-
- End diff –
-
Ok, I see, `TypeValidator` always expects SYSTEM OptionValues
Should we add an assertion to TypeValidator to make sure this doesn't happen again ?
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40023540
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SessionOptionManager.java —
@@ -20,14 +20,21 @@
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
{@link OptionValidator#isShortLived}
+ * 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) are
+ * undefined.-
- End diff –
-
No, I meant could you explain what you mean by undefined effects. I think you mean if we set a short lived session, for example we inject an exception, then try to delete it, 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.
Just improve the comment to explain what you mean
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40026355
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java —
@@ -161,6 +171,7 @@ public SqlNode visitChild(
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));
— End diff –
I see the confusion. We now allow option names to be compound identifiers. Let me add this to the JIRA and the commit message.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40027080
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -102,6 +119,29 @@ public void setOption(OptionValue value) {
}
@Override
+ public void deleteOption(final String name, final OptionType type) {
+ try
catch (final IllegalArgumentException e)
{ + throw UserException.validationError(e) + .build(logger); + } +
+ // fallback if unable to delete locally
+ if (!deleteLocalOption(name, type)) {
— End diff –
Implementations of OptionManager are required to be case insensitive to option names (mentioned in the OptionManager class docs). Calling ```name.toLowerCase()``` in this one place is insufficient to ensure implementation will always be case insensitive.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40037509
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -83,14 +84,30 @@ public OptionValue getOption(final String name) {
*/
abstract boolean setLocalOption(OptionValue value);
+ /**
+ * Deletes the option with given name for this manager without falling back.
+ *
+ * @param type option type
+ * @return true iff the option was successfully deleted
— End diff –
I will clarify the docs in both places.
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40101415
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java —
@@ -107,7 +117,7 @@ public SqlNode visitChild(
}
SqlNode newOperand = operand.accept(CompoundIdentifierConverter.this);
enableComplex = localEnableComplex;
- if (newOperand != operand) {
+ if (! newOperand.equalsDeep(operand, false)) {-
- End diff –
-
I'm a little confused by this change. Operands are expected to be immutable. As such, identity comparison should be sufficient. Are you trying to avoid excessive rewrites?
Github user jacques-n commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-142325962
As an aside, I'm really excited about this commit. Should make things much easier for end users. We should probably create a new jira to update the docs to remove escaping for all the options.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40101930
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java —
@@ -107,7 +117,7 @@ public SqlNode visitChild(
}
SqlNode newOperand = operand.accept(CompoundIdentifierConverter.this);
enableComplex = localEnableComplex;
- if (newOperand != operand) {
+ if (! newOperand.equalsDeep(operand, false)) {-
- End diff –
-
I reverted this change. Will post a new patch soon.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-142342234
We use "exec" as the first part of many option names (e.g. ```exec.min_hash_table_size```), and unfortunately, "exec" is a keyword, and the parser throws an error (```... PARSE ERROR: Encountered "SET exec" ...```). We still have to escape keywords.
Github user jacques-n commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-142454472
Let's fix this as part of the patch. Since we don't actually support EXEC (as far as I can remember), let's make this work nicely by removing it as a reserved word. (I think there is an unreserved reserved word list in the parser). @julianhyde may be able to help point us in the write direction.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-142477062
(commenting here and not on the commit)
There are various ```getOption(...)``` convenient methods in the OptionManager interface with typed validators as parameters (like ```StringValidator```) which return the value with that type (like ```String```).
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-143294316
Hey @julianhyde, looks like the [keyword](https://github.com/apache/incubator-calcite/blob/master/core/src/main/codegen/templates/Parser.jj#L5094) list can be extended but non-reserved keywords cannot be. Should I open a JIRA?
Github user julianhyde commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-143310179
Yes, open a JIRA.
We don't need NonReservedKeyWord() and CommonNonReservedKeyWord() to be separate anymore, so you can generate code into whichever one is most convenient.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-143367166
Here's the list of keywords that we need to add to the non-reserved list: ["EXEC", "JOIN", "WINDOW", "LARGE", "PARTITION", "OLD", "COLUMN"].
Given Calcite's [note](https://github.com/apache/incubator-calcite/blob/master/core/src/main/codegen/templates/Parser.jj#L5114) about such a list and the [standard](http://savage.net.au/SQL/sql-2003-2.bnf.html#xref-keywords), I am against making this change. There are a lot of unit test failures (PARSE ERROR) in TestWindowFrame and TestWindowFunctions with this change.
@julianhyde What are the implications of adding a keyword to the non-reserved list (say "JOIN")?
Also the option "store.parquet.block-size" has a "-" in it which is not allowed. So do we are escape the word? Or change the name?
Github user jacques-n commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-143477295
Okay, let's skip this for now.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-143511508
@sudheeshkatkam can you please rebase, and create a separate JIRA for removing exec from reserved words.
+1, LGTM except `CompoundIdentifierConverter.java` I don't have enough knowledge to review this class. @jacques-n can you please review the change in this file ?
Thanks
Github user jacques-n commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-143511727
The CompoundIdentifierConverter.java changes look sound. There is no reason to identifier expansion for the purpose of SetOptions. +1
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-144591640
Rebased, squashed, and edited the commit message. Also, see DRILL-3875(https://issues.apache.org/jira/browse/DRILL-3875).
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40873420
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java —
@@ -179,6 +181,7 @@ public void validate(final OptionValue v) {
public TypeValidator(final String name, final Kind kind, final OptionValue defValue) {
super(name);
+ checkArgument(defValue.type == OptionType.SYSTEM, "Default value must be SYSTEM type.");
— End diff –
Can you modify all of these to return UserException? Maybe create a static method ideally reporting the specific option and value as additional UserException context.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40873825
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java —
@@ -179,6 +181,7 @@ public void validate(final OptionValue v) {
public TypeValidator(final String name, final Kind kind, final OptionValue defValue) {
super(name);
+ checkArgument(defValue.type == OptionType.SYSTEM, "Default value must be SYSTEM type.");
— End diff –
This check is when a TypeValidator (static instance) is created, so developers know not to create a validator with non SYSTEM level option type. Nothing to do with usage. Right?
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40873899
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java —
@@ -179,6 +181,7 @@ public void validate(final OptionValue v) {
public TypeValidator(final String name, final Kind kind, final OptionValue defValue) {
super(name);
+ checkArgument(defValue.type == OptionType.SYSTEM, "Default value must be SYSTEM type.");
— End diff –
Are all of these? I may have just missed the context.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40874245
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java —
@@ -179,6 +181,7 @@ public void validate(final OptionValue v) {
public TypeValidator(final String name, final Kind kind, final OptionValue defValue) {
super(name);
+ checkArgument(defValue.type == OptionType.SYSTEM, "Default value must be SYSTEM type.");
— End diff –
I am confused.. what does "these" refer to? I do not see any other precondition checks.
That change was addressing Hakim's comment [here](https://github.com/apache/drill/pull/159/files#diff-767c81796fcc968c436bd67655218f1aR83). The issue was when ControlsOptionValidator was being registered with the SystemOptionManager, the default option was being stored within SystemOptionManager as a SESSION option.
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40875088
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -102,6 +125,29 @@ public void setOption(OptionValue value) {
}
@Override
+ public void deleteOption(final String name, final OptionType type) {
+ try
catch (final IllegalArgumentException e) {
+ throw UserException.validationError(e)
+ .build(logger);
— End diff –
Can we simply fix the lower level exception here. We should be throwing UserException (which mean we wouldn't have to catch here).
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40875160
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java —
@@ -179,6 +181,7 @@ public void validate(final OptionValue v) {
public TypeValidator(final String name, final Kind kind, final OptionValue defValue) {
super(name);
+ checkArgument(defValue.type == OptionType.SYSTEM, "Default value must be SYSTEM type.");
— End diff –
There are three other checkArguments in this changeset. However, I think they also refer to dev mistakes instead of user errors. Probably fine as is.
Github user jacques-n commented on the pull request:
https://github.com/apache/drill/pull/159#issuecomment-144601466
Looks good. One small change above. Otherwise +1.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40877406
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -102,6 +125,29 @@ public void setOption(OptionValue value) {
}
@Override
+ public void deleteOption(final String name, final OptionType type) {
+ try
catch (final IllegalArgumentException e) {
+ throw UserException.validationError(e)
+ .build(logger);
— End diff –
Initially, I made that change in the ```getValidator``` function, but it seemed like UserException was one level too deep (if that makes sense). This way the calling function can do whatever it wants with the exception e.g. wrap it in a UserException for the user, or ignore it in case of a unit test.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40878478
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -102,6 +125,29 @@ public void setOption(OptionValue value) {
}
@Override
+ public void deleteOption(final String name, final OptionType type) {
+ try
catch (final IllegalArgumentException e) {
+ throw UserException.validationError(e)
+ .build(logger);
— End diff –
if the `getValidator` fails, can the caller actually handle the situation differently from just failing the query ?
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40878882
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -102,6 +125,29 @@ public void setOption(OptionValue value) {
}
@Override
+ public void deleteOption(final String name, final OptionType type) {
+ try
catch (final IllegalArgumentException e) {
+ throw UserException.validationError(e)
+ .build(logger);
— End diff –
The function call does not have to be within the context of a query e.g. unit tests can use ```SystemOptionManager.getValidator(...)```. But callers, within the context of a query, will wrap the exception in a UserException.
Github user jacques-n commented on a diff in the pull request:
https://github.com/apache/drill/pull/159#discussion_r40880950
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java —
@@ -102,6 +125,29 @@ public void setOption(OptionValue value) {
}
@Override
+ public void deleteOption(final String name, final OptionType type) {
+ try
catch (final IllegalArgumentException e) {
+ throw UserException.validationError(e)
+ .build(logger);
— End diff –
My main issue here is that we have an unfriendly message. One of the things that we should be doing is creating a user friendly message at the point of user exception creation. If you think it was too deep one level deeper, then you should make a friendly user message at this level. Trusting that the system level exception message is meaningful to the user should be avoided.
provided ='reset' to reset an option.