You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by sudheeshkatkam <gi...@git.apache.org> on 2015/09/16 01:27:26 UTC
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
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 <sk...@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
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 {
thrownException.expect(new UserExceptionMatcher(VALIDATION));
- test(String.format("ALTER session SET `%s` = '%s';", ExecConstants.SLICE_TARGET, "fail"));
+ test("ALTER session SET `%s` = '%s';", SLICE_TARGET, "fail");
}
@Test // DRILL-3122
public void checkChangedColumn() throws Exception {
- test(String.format("ALTER session SET `%s` = %d;", ExecConstants.SLICE_TARGET,
+ test(String.format("ALTER session SET `%s` = %d;", SLICE_TARGET,
ExecConstants.SLICE_TARGET_DEFAULT));
testBuilder()
- .sqlQuery(String.format("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", ExecConstants.SLICE_TARGET))
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
.unOrdered()
.baselineColumns("status")
.baselineValues("DEFAULT")
.build()
.run();
}
+
+ @Test
+ public void setAndResetSessionOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+
+ // change option
+ test("SET `%s` = %d;", SLICE_TARGET, 10);
+ // check changed
+ test("SELECT status, type, name FROM sys.options WHERE type = 'SESSION';");
+ testBuilder()
+ .sqlQuery("SELECT num_val FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .baselineColumns("num_val")
+ .baselineValues((long) 10)
+ .build()
+ .run();
+
+ // reset option
+ test("RESET `%s`;", SLICE_TARGET);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+ }
+
+ @Test
+ public void setAndResetSystemOption() throws Exception {
--- End diff --
please add a test where both _SYSTEM_ and _SESSION_ options are changed, and confirm the reset is working as expected.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 {
thrownException.expect(new UserExceptionMatcher(VALIDATION));
- test(String.format("ALTER session SET `%s` = '%s';", ExecConstants.SLICE_TARGET, "fail"));
+ test("ALTER session SET `%s` = '%s';", SLICE_TARGET, "fail");
}
@Test // DRILL-3122
public void checkChangedColumn() throws Exception {
- test(String.format("ALTER session SET `%s` = %d;", ExecConstants.SLICE_TARGET,
+ test(String.format("ALTER session SET `%s` = %d;", SLICE_TARGET,
--- End diff --
you can remove `String.format` here too
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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```).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 { // ensure option exists
+ SystemOptionManager.getValidator(name);
+ } catch (final IllegalArgumentException e) {
+ throw UserException.validationError()
+ .message(e.getMessage())
+ .build(logger);
+ }
+ options.remove(name);
+ shortLivedOptions.remove(name);
+ } else {
+ fallback.deleteOption(name, type);
--- 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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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()`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 { // ensure option exists
+ SystemOptionManager.getValidator(name);
+ } catch (final IllegalArgumentException e) {
+ throw UserException.validationError()
+ .message(e.getMessage())
+ .build(logger);
+ }
+ options.remove(name);
+ shortLivedOptions.remove(name);
+ } else {
+ fallback.deleteOption(name, type);
+ }
+ }
+
+ @Override
+ public void deleteAllOptions(final OptionValue.OptionType type) {
+ if (type == OptionValue.OptionType.SESSION) {
+ options.clear();
+ shortLivedOptions.clear();
+ } else {
+ fallback.deleteAllOptions(type);
--- End diff --
same here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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
+ * set at the session level only apply to queries that you run during the current Drill connection. Session level
+ * settings override system level settings.
+ *
+ * NOTE that currently, the effects of deleting a short lived option (see {@link OptionValidator#isShortLived}) are
+ * undefined.
--- End diff --
can you please elaborate on the _undefined_ effects of deleting short lived options ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jinfengni <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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
+ * set at the session level only apply to queries that you run during the current Drill connection. Session level
+ * settings override system level settings.
+ *
+ * NOTE that currently, the effects of deleting a short lived option (see {@link OptionValidator#isShortLived}) are
+ * undefined.
--- End diff --
Should I add a TODO and create a ticket?
(personally I don't think this is worth a JIRA yet)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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 {
thrownException.expect(new UserExceptionMatcher(VALIDATION));
- test(String.format("ALTER session SET `%s` = '%s';", ExecConstants.SLICE_TARGET, "fail"));
+ test("ALTER session SET `%s` = '%s';", SLICE_TARGET, "fail");
}
@Test // DRILL-3122
public void checkChangedColumn() throws Exception {
- test(String.format("ALTER session SET `%s` = %d;", ExecConstants.SLICE_TARGET,
+ test(String.format("ALTER session SET `%s` = %d;", SLICE_TARGET,
ExecConstants.SLICE_TARGET_DEFAULT));
testBuilder()
- .sqlQuery(String.format("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", ExecConstants.SLICE_TARGET))
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
.unOrdered()
.baselineColumns("status")
.baselineValues("DEFAULT")
.build()
.run();
}
+
+ @Test
+ public void setAndResetSessionOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+
+ // change option
+ test("SET `%s` = %d;", SLICE_TARGET, 10);
+ // check changed
+ test("SELECT status, type, name FROM sys.options WHERE type = 'SESSION';");
+ testBuilder()
+ .sqlQuery("SELECT num_val FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .baselineColumns("num_val")
+ .baselineValues((long) 10)
+ .build()
+ .run();
+
+ // reset option
+ test("RESET `%s`;", SLICE_TARGET);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+ }
+
+ @Test
+ public void setAndResetSystemOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("status")
+ .baselineValues("DEFAULT")
+ .build()
+ .run();
+
+ // change option
+ test("ALTER system SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+ // check changed
+ testBuilder()
+ .sqlQuery("SELECT bool_val FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("bool_val")
+ .baselineValues(true)
+ .build()
+ .run();
+
+ // reset option
+ test("ALTER system RESET `%s`;", ENABLE_VERBOSE_ERRORS_KEY);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("status")
+ .baselineValues("DEFAULT")
+ .build()
+ .run();
+ }
+
+ @Test
+ public void resetAllSessionOptions() throws Exception {
--- 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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) {
return options.values();
}
+ @Override
+ boolean deleteLocalOptions(final OptionType type) {
+ if (supportsOption(type)) {
+ options.clear();
+ return true;
+ } 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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 {
thrownException.expect(new UserExceptionMatcher(VALIDATION));
- test(String.format("ALTER session SET `%s` = '%s';", ExecConstants.SLICE_TARGET, "fail"));
+ test("ALTER session SET `%s` = '%s';", SLICE_TARGET, "fail");
}
@Test // DRILL-3122
public void checkChangedColumn() throws Exception {
- test(String.format("ALTER session SET `%s` = %d;", ExecConstants.SLICE_TARGET,
+ test(String.format("ALTER session SET `%s` = %d;", SLICE_TARGET,
ExecConstants.SLICE_TARGET_DEFAULT));
testBuilder()
- .sqlQuery(String.format("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", ExecConstants.SLICE_TARGET))
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
.unOrdered()
.baselineColumns("status")
.baselineValues("DEFAULT")
.build()
.run();
}
+
+ @Test
+ public void setAndResetSessionOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+
+ // change option
+ test("SET `%s` = %d;", SLICE_TARGET, 10);
+ // check changed
+ test("SELECT status, type, name FROM sys.options WHERE type = 'SESSION';");
+ testBuilder()
+ .sqlQuery("SELECT num_val FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .baselineColumns("num_val")
+ .baselineValues((long) 10)
+ .build()
+ .run();
+
+ // reset option
+ test("RESET `%s`;", SLICE_TARGET);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+ }
+
+ @Test
+ public void setAndResetSystemOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("status")
+ .baselineValues("DEFAULT")
+ .build()
+ .run();
+
+ // change option
+ test("ALTER system SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+ // check changed
+ testBuilder()
+ .sqlQuery("SELECT bool_val FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("bool_val")
+ .baselineValues(true)
+ .build()
+ .run();
+
+ // reset option
+ test("ALTER system RESET `%s`;", ENABLE_VERBOSE_ERRORS_KEY);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("status")
+ .baselineValues("DEFAULT")
+ .build()
+ .run();
+ }
+
+ @Test
+ public void resetAllSessionOptions() throws Exception {
--- End diff --
resetting all _SYSTEM_ options is not covered here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 {
thrownException.expect(new UserExceptionMatcher(VALIDATION));
- test(String.format("ALTER session SET `%s` = '%s';", ExecConstants.SLICE_TARGET, "fail"));
+ test("ALTER session SET `%s` = '%s';", SLICE_TARGET, "fail");
}
@Test // DRILL-3122
public void checkChangedColumn() throws Exception {
- test(String.format("ALTER session SET `%s` = %d;", ExecConstants.SLICE_TARGET,
+ test(String.format("ALTER session SET `%s` = %d;", SLICE_TARGET,
ExecConstants.SLICE_TARGET_DEFAULT));
testBuilder()
- .sqlQuery(String.format("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", ExecConstants.SLICE_TARGET))
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
.unOrdered()
.baselineColumns("status")
.baselineValues("DEFAULT")
.build()
.run();
}
+
+ @Test
+ public void setAndResetSessionOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+
+ // change option
+ test("SET `%s` = %d;", SLICE_TARGET, 10);
+ // check changed
+ test("SELECT status, type, name FROM sys.options WHERE type = 'SESSION';");
+ testBuilder()
+ .sqlQuery("SELECT num_val FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .baselineColumns("num_val")
+ .baselineValues((long) 10)
+ .build()
+ .run();
+
+ // reset option
+ test("RESET `%s`;", SLICE_TARGET);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+ }
+
+ @Test
+ public void setAndResetSystemOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("status")
+ .baselineValues("DEFAULT")
+ .build()
+ .run();
+
+ // change option
+ test("ALTER system SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+ // check changed
+ testBuilder()
+ .sqlQuery("SELECT bool_val FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("bool_val")
+ .baselineValues(true)
+ .build()
+ .run();
+
+ // reset option
+ test("ALTER system RESET `%s`;", ENABLE_VERBOSE_ERRORS_KEY);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("status")
+ .baselineValues("DEFAULT")
+ .build()
+ .run();
+ }
+
+ @Test
+ public void resetAllSessionOptions() throws Exception {
--- 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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 {
+ SystemOptionManager.getValidator(name); // ensure the option exists
+ } 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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by julianhyde <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 { // ensure option exists
+ getValidator(name);
+ } catch (final IllegalArgumentException e) {
+ throw UserException.validationError()
--- End diff --
you can just use:
```
throw UserException.validationError(e).build(logger);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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 {
thrownException.expect(new UserExceptionMatcher(VALIDATION));
- test(String.format("ALTER session SET `%s` = '%s';", ExecConstants.SLICE_TARGET, "fail"));
+ test("ALTER session SET `%s` = '%s';", SLICE_TARGET, "fail");
}
@Test // DRILL-3122
public void checkChangedColumn() throws Exception {
- test(String.format("ALTER session SET `%s` = %d;", ExecConstants.SLICE_TARGET,
+ test(String.format("ALTER session SET `%s` = %d;", SLICE_TARGET,
ExecConstants.SLICE_TARGET_DEFAULT));
testBuilder()
- .sqlQuery(String.format("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", ExecConstants.SLICE_TARGET))
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
.unOrdered()
.baselineColumns("status")
.baselineValues("DEFAULT")
.build()
.run();
}
+
+ @Test
+ public void setAndResetSessionOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+
+ // change option
+ test("SET `%s` = %d;", SLICE_TARGET, 10);
+ // check changed
+ test("SELECT status, type, name FROM sys.options WHERE type = 'SESSION';");
+ testBuilder()
+ .sqlQuery("SELECT num_val FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .baselineColumns("num_val")
+ .baselineValues((long) 10)
+ .build()
+ .run();
+
+ // reset option
+ test("RESET `%s`;", SLICE_TARGET);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+ }
+
+ @Test
+ public void setAndResetSystemOption() throws Exception {
--- End diff --
changeSessionAndNotSystem and changeSystemAndNotSession are testing this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 {
thrownException.expect(new UserExceptionMatcher(VALIDATION));
- test(String.format("ALTER session SET `%s` = '%s';", ExecConstants.SLICE_TARGET, "fail"));
+ test("ALTER session SET `%s` = '%s';", SLICE_TARGET, "fail");
}
@Test // DRILL-3122
public void checkChangedColumn() throws Exception {
- test(String.format("ALTER session SET `%s` = %d;", ExecConstants.SLICE_TARGET,
+ test(String.format("ALTER session SET `%s` = %d;", SLICE_TARGET,
ExecConstants.SLICE_TARGET_DEFAULT));
testBuilder()
- .sqlQuery(String.format("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", ExecConstants.SLICE_TARGET))
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
.unOrdered()
.baselineColumns("status")
.baselineValues("DEFAULT")
.build()
.run();
}
+
+ @Test
+ public void setAndResetSessionOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+
+ // change option
+ test("SET `%s` = %d;", SLICE_TARGET, 10);
+ // check changed
+ test("SELECT status, type, name FROM sys.options WHERE type = 'SESSION';");
+ testBuilder()
+ .sqlQuery("SELECT num_val FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .baselineColumns("num_val")
+ .baselineValues((long) 10)
+ .build()
+ .run();
+
+ // reset option
+ test("RESET `%s`;", SLICE_TARGET);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+ }
+
+ @Test
+ public void setAndResetSystemOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("status")
+ .baselineValues("DEFAULT")
+ .build()
+ .run();
+
+ // change option
+ test("ALTER system SET `%s` = %b;", ENABLE_VERBOSE_ERRORS_KEY, true);
+ // check changed
+ testBuilder()
+ .sqlQuery("SELECT bool_val FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("bool_val")
+ .baselineValues(true)
+ .build()
+ .run();
+
+ // reset option
+ test("ALTER system RESET `%s`;", ENABLE_VERBOSE_ERRORS_KEY);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SYSTEM'", ENABLE_VERBOSE_ERRORS_KEY)
+ .unOrdered()
+ .baselineColumns("status")
+ .baselineValues("DEFAULT")
+ .build()
+ .run();
+ }
+
+ @Test
+ public void resetAllSessionOptions() throws Exception {
--- End diff --
also change _SESSION_ and _SYSTEM_ options then reset all _SESSION_ options
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 { // ensure option exists
+ SystemOptionManager.getValidator(name);
+ } catch (final IllegalArgumentException e) {
+ throw UserException.validationError()
--- End diff --
you can just use:
```
throw UserException.validationError(e).build(logger);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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);
+ }
+
+ final String scope = option.getScope();
+ final OptionValue.OptionType type;
+ if (scope == null) { // No scope mentioned assumed SESSION
+ type = OptionType.SESSION;
+ } else {
switch (scope.toLowerCase()) {
- case "session":
- type = OptionValue.OptionType.SESSION;
- break;
- case "system":
- type = OptionValue.OptionType.SYSTEM;
- break;
-// case "query":
-// type = OptionValue.OptionType.QUERY;
-// break;
- default:
- throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM.");
+ case "session":
+ type = OptionType.SESSION;
+ break;
+ case "system":
+ type = OptionType.SYSTEM;
+ break;
+ default:
+ throw UserException.validationError()
+ .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope)
+ .build(logger);
}
+ }
- if (type == OptionType.SYSTEM) {
- // If the user authentication is enabled, make sure the user who is trying to change the system option has
- // administrative privileges.
- if (context.isUserAuthenticationEnabled() &&
- !ImpersonationUtil.hasAdminPrivileges(
- context.getQueryUserName(),
- context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
- context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) {
- throw UserException.permissionError()
- .message("Not authorized to change SYSTEM options.")
- .build(logger);
- }
+ if (type == OptionType.SYSTEM) {
+ // If the user authentication is enabled, make sure the user who is trying to change the system option has
+ // administrative privileges.
+ if (context.isUserAuthenticationEnabled() &&
+ !ImpersonationUtil.hasAdminPrivileges(
+ context.getQueryUserName(),
+ context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
--- End diff --
you should reuse `context.getOptions()` as a local variable
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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 {
+ SystemOptionManager.getValidator(name); // ensure the option exists
+ } 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 { // ensure option exists
+ getValidator(name);
+ } 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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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) {
+ throw UserException.unsupportedError()
+ .message("This manager does not support deleting an option.")
+ .build(logger);
+ }
+
+ @Override
+ public void deleteAllOptions(final OptionValue.OptionType type) {
+ throw UserException.unsupportedError()
--- End diff --
same here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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 { // ensure option exists
+ SystemOptionManager.getValidator(name);
+ } catch (final IllegalArgumentException e) {
+ throw UserException.validationError()
+ .message(e.getMessage())
+ .build(logger);
+ }
+ options.remove(name);
+ shortLivedOptions.remove(name);
+ } else {
+ fallback.deleteOption(name, type);
--- End diff --
I'd say yes. Session options always override system options (default or not).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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 {
+ SystemOptionManager.getValidator(name); // ensure the option exists
+ } 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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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` ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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_ ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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);
+ }
+
+ final String scope = option.getScope();
+ final OptionValue.OptionType type;
+ if (scope == null) { // No scope mentioned assumed SESSION
+ type = OptionType.SESSION;
+ } else {
switch (scope.toLowerCase()) {
- case "session":
- type = OptionValue.OptionType.SESSION;
- break;
- case "system":
- type = OptionValue.OptionType.SYSTEM;
- break;
-// case "query":
-// type = OptionValue.OptionType.QUERY;
-// break;
- default:
- throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM.");
+ case "session":
+ type = OptionType.SESSION;
+ break;
+ case "system":
+ type = OptionType.SYSTEM;
+ break;
+ default:
+ throw UserException.validationError()
+ .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope)
+ .build(logger);
}
+ }
- if (type == OptionType.SYSTEM) {
- // If the user authentication is enabled, make sure the user who is trying to change the system option has
- // administrative privileges.
- if (context.isUserAuthenticationEnabled() &&
- !ImpersonationUtil.hasAdminPrivileges(
- context.getQueryUserName(),
- context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
- context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) {
- throw UserException.permissionError()
- .message("Not authorized to change SYSTEM options.")
- .build(logger);
- }
+ if (type == OptionType.SYSTEM) {
+ // If the user authentication is enabled, make sure the user who is trying to change the system option has
+ // administrative privileges.
+ if (context.isUserAuthenticationEnabled() &&
+ !ImpersonationUtil.hasAdminPrivileges(
+ context.getQueryUserName(),
+ context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
+ context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) {
+ throw UserException.permissionError()
+ .message("Not authorized to change SYSTEM options.")
+ .build(logger);
}
+ }
+
+ // Currently, we use one part identifiers.
+ final SqlIdentifier nameIdentifier = option.getName();
+ if (!nameIdentifier.isSimple()) {
+ throw UserException.validationError()
+ .message("Drill does not support multi-part identifier for an option name (%s).",
+ nameIdentifier.toString())
--- End diff --
this is not important, but you can just pass nameIdentifier and .toString() will be called implicitly
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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 {
+ SystemOptionManager.getValidator(name); // ensure the option exists
+ } 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 {
+ SystemOptionManager.getValidator(name); // ensure the option exists
+ } 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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 {
+ SystemOptionManager.getValidator(name); // ensure the option exists
+ } 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.*
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 {
thrownException.expect(new UserExceptionMatcher(VALIDATION));
- test(String.format("ALTER session SET `%s` = '%s';", ExecConstants.SLICE_TARGET, "fail"));
+ test("ALTER session SET `%s` = '%s';", SLICE_TARGET, "fail");
}
@Test // DRILL-3122
public void checkChangedColumn() throws Exception {
- test(String.format("ALTER session SET `%s` = %d;", ExecConstants.SLICE_TARGET,
+ test(String.format("ALTER session SET `%s` = %d;", SLICE_TARGET,
ExecConstants.SLICE_TARGET_DEFAULT));
testBuilder()
- .sqlQuery(String.format("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", ExecConstants.SLICE_TARGET))
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
.unOrdered()
.baselineColumns("status")
.baselineValues("DEFAULT")
.build()
.run();
}
+
+ @Test
+ public void setAndResetSessionOption() throws Exception {
+ // check unchanged
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+
+ // change option
+ test("SET `%s` = %d;", SLICE_TARGET, 10);
+ // check changed
+ test("SELECT status, type, name FROM sys.options WHERE type = 'SESSION';");
+ testBuilder()
+ .sqlQuery("SELECT num_val FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .baselineColumns("num_val")
+ .baselineValues((long) 10)
+ .build()
+ .run();
+
+ // reset option
+ test("RESET `%s`;", SLICE_TARGET);
+ // check reverted
+ testBuilder()
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'", SLICE_TARGET)
+ .unOrdered()
+ .expectsEmptyResultSet()
+ .build()
+ .run();
+ }
+
+ @Test
+ public void setAndResetSystemOption() throws Exception {
--- End diff --
thanks for adding those unit tests
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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);
+ }
+
+ final String scope = option.getScope();
+ final OptionValue.OptionType type;
+ if (scope == null) { // No scope mentioned assumed SESSION
+ type = OptionType.SESSION;
+ } else {
switch (scope.toLowerCase()) {
- case "session":
- type = OptionValue.OptionType.SESSION;
- break;
- case "system":
- type = OptionValue.OptionType.SYSTEM;
- break;
-// case "query":
-// type = OptionValue.OptionType.QUERY;
-// break;
- default:
- throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM.");
+ case "session":
+ type = OptionType.SESSION;
+ break;
+ case "system":
+ type = OptionType.SYSTEM;
+ break;
+ default:
+ throw UserException.validationError()
+ .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope)
+ .build(logger);
}
+ }
- if (type == OptionType.SYSTEM) {
- // If the user authentication is enabled, make sure the user who is trying to change the system option has
- // administrative privileges.
- if (context.isUserAuthenticationEnabled() &&
- !ImpersonationUtil.hasAdminPrivileges(
- context.getQueryUserName(),
- context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
- context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) {
- throw UserException.permissionError()
- .message("Not authorized to change SYSTEM options.")
- .build(logger);
- }
+ if (type == OptionType.SYSTEM) {
+ // If the user authentication is enabled, make sure the user who is trying to change the system option has
+ // administrative privileges.
+ if (context.isUserAuthenticationEnabled() &&
+ !ImpersonationUtil.hasAdminPrivileges(
+ context.getQueryUserName(),
+ context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
--- End diff --
This is not a change I introduced (just difference in spacing), but I'll address this issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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 {
+ SystemOptionManager.getValidator(name); // ensure the option exists
+ } 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/159
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by jacques-n <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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);
+ }
+
+ final String scope = option.getScope();
+ final OptionValue.OptionType type;
+ if (scope == null) { // No scope mentioned assumed SESSION
+ type = OptionType.SESSION;
+ } else {
switch (scope.toLowerCase()) {
- case "session":
- type = OptionValue.OptionType.SESSION;
- break;
- case "system":
- type = OptionValue.OptionType.SYSTEM;
- break;
-// case "query":
-// type = OptionValue.OptionType.QUERY;
-// break;
- default:
- throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM.");
+ case "session":
+ type = OptionType.SESSION;
+ break;
+ case "system":
+ type = OptionType.SYSTEM;
+ break;
+ default:
+ throw UserException.validationError()
+ .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope)
+ .build(logger);
}
+ }
- if (type == OptionType.SYSTEM) {
- // If the user authentication is enabled, make sure the user who is trying to change the system option has
- // administrative privileges.
- if (context.isUserAuthenticationEnabled() &&
- !ImpersonationUtil.hasAdminPrivileges(
- context.getQueryUserName(),
- context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
- context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) {
- throw UserException.permissionError()
- .message("Not authorized to change SYSTEM options.")
- .build(logger);
- }
+ if (type == OptionType.SYSTEM) {
+ // If the user authentication is enabled, make sure the user who is trying to change the system option has
+ // administrative privileges.
+ if (context.isUserAuthenticationEnabled() &&
+ !ImpersonationUtil.hasAdminPrivileges(
+ context.getQueryUserName(),
+ context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
+ context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) {
+ throw UserException.permissionError()
+ .message("Not authorized to change SYSTEM options.")
+ .build(logger);
}
+ }
+
+ // Currently, we use one part identifiers.
+ final SqlIdentifier nameIdentifier = option.getName();
+ if (!nameIdentifier.isSimple()) {
+ throw UserException.validationError()
+ .message("Drill does not support multi-part identifier for an option name (%s).",
+ nameIdentifier.toString())
+ .build(logger);
+ }
+ final String name = nameIdentifier.getSimple();
+ if (value != null) { // SET option
final OptionValue optionValue = createOptionValue(name, type, (SqlLiteral) value);
context.getOptions().setOption(optionValue);
- }else{
- throw new ValidationException("Sql options can only be literals.");
+ } else { // RESET option
+ if ("ALL".equals(name)) {
--- End diff --
.equalsIgnoreCase(...)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-1065: Support for ALTER ... RESET statem...
Posted by adeneche <gi...@git.apache.org>.
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
+ * set at the session level only apply to queries that you run during the current Drill connection. Session level
+ * settings override system level settings.
+ *
+ * NOTE that currently, the effects of deleting a short lived option (see {@link OptionValidator#isShortLived}) are
+ * undefined.
--- 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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---