DRILL-1065: Support for ALTER ... RESET statement #159

sudheeshkatkam · 2015-09-15T23:27:24Z

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

sudheeshkatkam · 2015-09-16T15:24:43Z

exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java

-    }else{
-      throw new ValidationException("Sql options can only be literals.");
+    } else { // RESET option
+      if ("ALL".equals(name)) {


.equalsIgnoreCase(...)

sudheeshkatkam · 2015-09-17T18:26:47Z

I worked around the fallback structure of option managers, which is wrong. I will update the pull request soon.

adeneche · 2015-09-17T20:45:20Z

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()


Why throw a UserException here ? is the error message relevant to the user ? do we even expect this method to be called ?

Removing this in my new implementation.

adeneche · 2015-09-17T22:03:26Z

exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java

+  }
+
+  @Test
+  public void setAndResetSystemOption() throws Exception {


please add a test where both SYSTEM and SESSION options are changed, and confirm the reset is working as expected.

changeSessionAndNotSystem and changeSystemAndNotSession are testing this.

thanks for adding those unit tests

sudheeshkatkam · 2015-09-17T22:15:11Z

I have added a note to the SessionOptionManager that the effects of deleting a short lived option are undefined because that requires slightly extensive changes, which is out of scope for this JIRA.

@jacques-n please review the changes in CompoundIdentifierConverter class.

jinfengni · 2015-09-21T15:28:04Z

...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)) {


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?

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?

I reverted this change. Will post a new patch soon.

jacques-n · 2015-09-22T15:34:44Z

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.

sudheeshkatkam · 2015-09-22T16:33:54Z

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.

jacques-n · 2015-09-22T23:44:08Z

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.

sudheeshkatkam · 2015-09-23T03:03:18Z

(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).

sudheeshkatkam · 2015-09-25T17:24:21Z

Hey @julianhyde, looks like the keyword list can be extended but non-reserved keywords cannot be. Should I open a JIRA?

julianhyde · 2015-09-25T18:06:42Z

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.

sudheeshkatkam · 2015-09-25T22:24:55Z

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 about such a list and the standard, 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?

jacques-n · 2015-09-26T18:16:34Z

Okay, let's skip this for now.

adeneche · 2015-09-27T01:25:39Z

@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

jacques-n · 2015-09-27T01:33:53Z

The CompoundIdentifierConverter.java changes look sound. There is no reason to do identifier expansion for the purpose of SetOptions. +1

sudheeshkatkam · 2015-10-01T01:39:48Z

Rebased, squashed, and edited the commit message. Also, see DRILL-3875.

jacques-n · 2015-10-01T02:03:02Z

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.");


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.

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?

Are all of these? I may have just missed the context.

I am confused.. what does "these" refer to? I do not see any other precondition checks.

That change was addressing Hakim's comment here. The issue was when ControlsOptionValidator was being registered with the SystemOptionManager, the default option was being stored within SystemOptionManager as a SESSION option.

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.

jacques-n · 2015-10-01T02:48:04Z

Looks good. One small change above. Otherwise +1.

+ Support for "SET option = value" statement (assumes scope as SESSION) + Bump Calcite version to include CALCITE-823 (Parser support for "ALTER ... RESET" statement). This commit includes a breaking change: SqlSetOption#getName now returns a SqlIdentifier rather than a String => option names are multi-part identifiers, and do not require escaping + Add rule in CompoundIdentifierConverter (+ Override annotations) + Improve error messages in SetOptionHandler + Add documentation (CompoundIdentifierConverter, OptionValue, SessionOptionManager, SystemOptionManager) - Does not include support for deleting short lived options + Default ExecutionControls option value should be at SYSTEM level + Change asserts to preconditions in SystemOptionManager + Add a precondition to TypeValidator's ctor to ensure default values are set at SYSTEM level + Address Hakim's and Jinfeng's review comments

sudheeshkatkam reviewed Sep 16, 2015
View reviewed changes

adeneche reviewed Sep 17, 2015
View reviewed changes

sudheeshkatkam force-pushed the DRILL-1065 branch from f785c3b to b0133af Compare September 17, 2015 21:59

adeneche reviewed Sep 17, 2015
View reviewed changes

jinfengni reviewed Sep 21, 2015
View reviewed changes

sudheeshkatkam force-pushed the DRILL-1065 branch from 0f97e40 to 5c9dfef Compare September 22, 2015 22:11

sudheeshkatkam force-pushed the DRILL-1065 branch from 5c9dfef to 62625c3 Compare October 1, 2015 01:35

jacques-n reviewed Oct 1, 2015
View reviewed changes

Sudheesh Katkam added 2 commits October 1, 2015 15:28

DRILL-1065: Addressing Jacques' comments (1)

8802d83

sudheeshkatkam force-pushed the DRILL-1065 branch from 028c983 to 8802d83 Compare October 1, 2015 22:28

asfgit closed this in 401119f Oct 2, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRILL-1065: Support for ALTER ... RESET statement #159

DRILL-1065: Support for ALTER ... RESET statement #159

sudheeshkatkam commented Sep 15, 2015

sudheeshkatkam Sep 16, 2015

sudheeshkatkam commented Sep 17, 2015

adeneche Sep 17, 2015

sudheeshkatkam Sep 17, 2015

adeneche Sep 17, 2015

sudheeshkatkam Sep 21, 2015

adeneche Sep 21, 2015

sudheeshkatkam commented Sep 17, 2015

jinfengni Sep 21, 2015

jacques-n Sep 22, 2015

sudheeshkatkam Sep 22, 2015

jacques-n commented Sep 22, 2015

sudheeshkatkam commented Sep 22, 2015

jacques-n commented Sep 22, 2015

sudheeshkatkam commented Sep 23, 2015

sudheeshkatkam commented Sep 25, 2015

julianhyde commented Sep 25, 2015

sudheeshkatkam commented Sep 25, 2015

jacques-n commented Sep 26, 2015

adeneche commented Sep 27, 2015

jacques-n commented Sep 27, 2015

sudheeshkatkam commented Oct 1, 2015

jacques-n Oct 1, 2015

sudheeshkatkam Oct 1, 2015

jacques-n Oct 1, 2015

sudheeshkatkam Oct 1, 2015

jacques-n Oct 1, 2015

jacques-n commented Oct 1, 2015

DRILL-1065: Support for ALTER ... RESET statement #159

DRILL-1065: Support for ALTER ... RESET statement #159

Conversation

sudheeshkatkam commented Sep 15, 2015

Choose a reason for hiding this comment

sudheeshkatkam commented Sep 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudheeshkatkam commented Sep 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacques-n commented Sep 22, 2015

sudheeshkatkam commented Sep 22, 2015

jacques-n commented Sep 22, 2015

sudheeshkatkam commented Sep 23, 2015

sudheeshkatkam commented Sep 25, 2015

julianhyde commented Sep 25, 2015

sudheeshkatkam commented Sep 25, 2015

jacques-n commented Sep 26, 2015

adeneche commented Sep 27, 2015

jacques-n commented Sep 27, 2015

sudheeshkatkam commented Oct 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacques-n commented Oct 1, 2015