Uploaded image for project: 'Apache Drill'
  1. Apache Drill
  2. DRILL-1065

Provide a reset command to reset an option to its default value

Details

    • Improvement
    • Status: Closed
    • Minor
    • Resolution: Fixed
    • None
    • 1.3.0
    • Execution - Flow
    • None

    Description

      Within a session, currently we set configuration options and it would be very useful to have a 'reset' command to reset the value of an option to its default system value:
      ALTER SESSION RESET <option name>

      If we don't want to add a new keyword for RESET, we could potentially overload the SET command and allow the user to set to the 'default' value.

      Attachments

        Issue Links

          Activity

            sudheeshkatkam Sudheesh Katkam added a comment - - edited

            provided ='reset' to reset an option.

            sudheeshkatkam Sudheesh Katkam added a comment - - edited provided ='reset' to reset an option.
            jnadeau Jacques Nadeau added a comment -

            Propose syntax:

            ALTER SESSION RESET `option name`
            ALTER SYSTEM RESET `option name`

            ALTER SESSION RESET ALL
            ALTER SYSTEM RESET ALL

            jnadeau Jacques Nadeau added a comment - Propose syntax: ALTER SESSION RESET `option name` ALTER SYSTEM RESET `option name` ALTER SESSION RESET ALL ALTER SYSTEM RESET ALL
            githubbot ASF GitHub Bot added a comment -

            GitHub user sudheeshkatkam opened a pull request:

            https://github.com/apache/drill/pull/159

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

            + Support for "SET option = value" statement (assumes scope as SESSION)
            + Better error messages in SetOptionHandler
            + Changes in CompoundIdentifierConverter

            • update when rewritten operand is not deeply equal to the original operand
            • Added Override annotations
              + Default ExecutionControls option value should be at SYSTEM level

            @jacques-n @vkorukanti please review.

            You can merge this pull request into a Git repository by running:

            $ git pull https://github.com/sudheeshkatkam/drill DRILL-1065

            Alternatively you can review and apply these changes as the patch at:

            https://github.com/apache/drill/pull/159.patch

            To close this pull request, make a commit to your master/trunk branch
            with (at least) the following in the commit message:

            This closes #159


            commit f785c3ba4d50bfe5a7ff77a8d37b3589bbe1c1cf
            Author: Sudheesh Katkam <skatkam@maprtech.com>
            Date: 2015-09-15T23:13:23Z

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

            + Support for "SET option = value" statement (assumes scope as SESSION)
            + Better error messages in SetOptionHandler
            + Changes in CompoundIdentifierConverter

            • update when rewritten operand is not deeply equal to the original operand
            • Added Override annotations
              + Default ExecutionControls option value should be at SYSTEM level

            githubbot ASF GitHub Bot added a comment - GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/159 DRILL-1065 : Support for ALTER ... RESET statement + Support for "SET option = value" statement (assumes scope as SESSION) + Better error messages in SetOptionHandler + Changes in CompoundIdentifierConverter update when rewritten operand is not deeply equal to the original operand Added Override annotations + Default ExecutionControls option value should be at SYSTEM level @jacques-n @vkorukanti please review. You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-1065 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/159.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #159 commit f785c3ba4d50bfe5a7ff77a8d37b3589bbe1c1cf Author: Sudheesh Katkam <skatkam@maprtech.com> Date: 2015-09-15T23:13:23Z DRILL-1065 : Support for ALTER ... RESET statement + Support for "SET option = value" statement (assumes scope as SESSION) + Better error messages in SetOptionHandler + Changes in CompoundIdentifierConverter update when rewritten operand is not deeply equal to the original operand Added Override annotations + Default ExecutionControls option value should be at SYSTEM level
            githubbot ASF GitHub Bot added a comment -

            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

              Unknown macro: { switch (scope.toLowerCase()) { - case "session": - type = OptionValue.OptionType.SESSION; - break; - case "system": - type = OptionValue.OptionType.SYSTEM; - break; -// case "query": -// type = OptionValue.OptionType.QUERY; -// break; - default: - throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM."); + case "session": + type = OptionType.SESSION; + break; + case "system": + type = OptionType.SYSTEM; + break; + default: + throw UserException.validationError() + .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope) + .build(logger); } + }
            • if (type == OptionType.SYSTEM) {
            • // If the user authentication is enabled, make sure the user who is trying to change the system option has
            • // administrative privileges.
            • if (context.isUserAuthenticationEnabled() &&
            • !ImpersonationUtil.hasAdminPrivileges(
            • context.getQueryUserName(),
            • context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
            • context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { - throw UserException.permissionError() - .message("Not authorized to change SYSTEM options.") - .build(logger); - }

              + if (type == OptionType.SYSTEM)

              Unknown macro: { + // If the user authentication is enabled, make sure the user who is trying to change the system option has + // administrative privileges. + if (context.isUserAuthenticationEnabled() && + !ImpersonationUtil.hasAdminPrivileges( + context.getQueryUserName(), + context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val, + context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { + throw UserException.permissionError() + .message("Not authorized to change SYSTEM options.") + .build(logger); } + }

              +
              + // Currently, we use one part identifiers.
              + final SqlIdentifier nameIdentifier = option.getName();
              + if (!nameIdentifier.isSimple())

              { + throw UserException.validationError() + .message("Drill does not support multi-part identifier for an option name (%s).", + nameIdentifier.toString()) + .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(...)

            githubbot ASF GitHub Bot added a comment - 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 Unknown macro: { switch (scope.toLowerCase()) { - case "session": - type = OptionValue.OptionType.SESSION; - break; - case "system": - type = OptionValue.OptionType.SYSTEM; - break; -// case "query": -// type = OptionValue.OptionType.QUERY; -// break; - default: - throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM."); + case "session": + type = OptionType.SESSION; + break; + case "system": + type = OptionType.SYSTEM; + break; + default: + throw UserException.validationError() + .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope) + .build(logger); } + } if (type == OptionType.SYSTEM) { // If the user authentication is enabled, make sure the user who is trying to change the system option has // administrative privileges. if (context.isUserAuthenticationEnabled() && !ImpersonationUtil.hasAdminPrivileges( context.getQueryUserName(), context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val, context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { - throw UserException.permissionError() - .message("Not authorized to change SYSTEM options.") - .build(logger); - } + if (type == OptionType.SYSTEM) Unknown macro: { + // If the user authentication is enabled, make sure the user who is trying to change the system option has + // administrative privileges. + if (context.isUserAuthenticationEnabled() && + !ImpersonationUtil.hasAdminPrivileges( + context.getQueryUserName(), + context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val, + context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { + throw UserException.permissionError() + .message("Not authorized to change SYSTEM options.") + .build(logger); } + } + + // Currently, we use one part identifiers. + final SqlIdentifier nameIdentifier = option.getName(); + if (!nameIdentifier.isSimple()) { + throw UserException.validationError() + .message("Drill does not support multi-part identifier for an option name (%s).", + nameIdentifier.toString()) + .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(...)
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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 ?

            githubbot ASF GitHub Bot added a comment - 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 ?
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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);
            ```

            githubbot ASF GitHub Bot added a comment - 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); ```
            githubbot ASF GitHub Bot added a comment -

            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 ?

            githubbot ASF GitHub Bot added a comment - 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 ?
            githubbot ASF GitHub Bot added a comment -

            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 ?

            githubbot ASF GitHub Bot added a comment - 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 ?
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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);
            ```

            githubbot ASF GitHub Bot added a comment - 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); ```
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

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

            githubbot ASF GitHub Bot added a comment - 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).
            sudheeshkatkam Sudheesh Katkam added a comment - - edited

            Also, adding:
            + support for

            SET `option name`
            RESET `option name`
            RESET ALL
            

            as discussed here with an assumption that the scope is SESSION.

            + allow for compound identifiers for option names

            sudheeshkatkam Sudheesh Katkam added a comment - - edited Also, adding: + support for SET `option name` RESET `option name` RESET ALL as discussed here with an assumption that the scope is SESSION. + allow for compound identifiers for option names
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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?

            githubbot ASF GitHub Bot added a comment - 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?
            githubbot ASF GitHub Bot added a comment -

            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

              Unknown macro: { switch (scope.toLowerCase()) { - case "session": - type = OptionValue.OptionType.SESSION; - break; - case "system": - type = OptionValue.OptionType.SYSTEM; - break; -// case "query": -// type = OptionValue.OptionType.QUERY; -// break; - default: - throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM."); + case "session": + type = OptionType.SESSION; + break; + case "system": + type = OptionType.SYSTEM; + break; + default: + throw UserException.validationError() + .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope) + .build(logger); } + }
            • if (type == OptionType.SYSTEM) {
            • // If the user authentication is enabled, make sure the user who is trying to change the system option has
            • // administrative privileges.
            • if (context.isUserAuthenticationEnabled() &&
            • !ImpersonationUtil.hasAdminPrivileges(
            • context.getQueryUserName(),
            • context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
            • context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { - throw UserException.permissionError() - .message("Not authorized to change SYSTEM options.") - .build(logger); - }

              + if (type == OptionType.SYSTEM) {
              + // If the user authentication is enabled, make sure the user who is trying to change the system option has
              + // administrative privileges.
              + if (context.isUserAuthenticationEnabled() &&
              + !ImpersonationUtil.hasAdminPrivileges(
              + context.getQueryUserName(),
              + context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,

                • End diff –

            you should reuse `context.getOptions()` as a local variable

            githubbot ASF GitHub Bot added a comment - 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 Unknown macro: { switch (scope.toLowerCase()) { - case "session": - type = OptionValue.OptionType.SESSION; - break; - case "system": - type = OptionValue.OptionType.SYSTEM; - break; -// case "query": -// type = OptionValue.OptionType.QUERY; -// break; - default: - throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM."); + case "session": + type = OptionType.SESSION; + break; + case "system": + type = OptionType.SYSTEM; + break; + default: + throw UserException.validationError() + .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope) + .build(logger); } + } if (type == OptionType.SYSTEM) { // If the user authentication is enabled, make sure the user who is trying to change the system option has // administrative privileges. if (context.isUserAuthenticationEnabled() && !ImpersonationUtil.hasAdminPrivileges( context.getQueryUserName(), context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val, context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { - throw UserException.permissionError() - .message("Not authorized to change SYSTEM options.") - .build(logger); - } + if (type == OptionType.SYSTEM) { + // If the user authentication is enabled, make sure the user who is trying to change the system option has + // administrative privileges. + if (context.isUserAuthenticationEnabled() && + !ImpersonationUtil.hasAdminPrivileges( + context.getQueryUserName(), + context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val, End diff – you should reuse `context.getOptions()` as a local variable
            githubbot ASF GitHub Bot added a comment -

            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

              Unknown macro: { switch (scope.toLowerCase()) { - case "session": - type = OptionValue.OptionType.SESSION; - break; - case "system": - type = OptionValue.OptionType.SYSTEM; - break; -// case "query": -// type = OptionValue.OptionType.QUERY; -// break; - default: - throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM."); + case "session": + type = OptionType.SESSION; + break; + case "system": + type = OptionType.SYSTEM; + break; + default: + throw UserException.validationError() + .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope) + .build(logger); } + }
            • if (type == OptionType.SYSTEM) {
            • // If the user authentication is enabled, make sure the user who is trying to change the system option has
            • // administrative privileges.
            • if (context.isUserAuthenticationEnabled() &&
            • !ImpersonationUtil.hasAdminPrivileges(
            • context.getQueryUserName(),
            • context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
            • context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { - throw UserException.permissionError() - .message("Not authorized to change SYSTEM options.") - .build(logger); - }

              + if (type == OptionType.SYSTEM)

              Unknown macro: { + // If the user authentication is enabled, make sure the user who is trying to change the system option has + // administrative privileges. + if (context.isUserAuthenticationEnabled() && + !ImpersonationUtil.hasAdminPrivileges( + context.getQueryUserName(), + context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val, + context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { + throw UserException.permissionError() + .message("Not authorized to change SYSTEM options.") + .build(logger); } + }

              +
              + // Currently, we use one part identifiers.
              + final SqlIdentifier nameIdentifier = option.getName();
              + if (!nameIdentifier.isSimple()) {
              + throw UserException.validationError()
              + .message("Drill does not support multi-part identifier for an option name (%s).",
              + nameIdentifier.toString())

                • End diff –

            this is not important, but you can just pass nameIdentifier and .toString() will be called implicitly

            githubbot ASF GitHub Bot added a comment - 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 Unknown macro: { switch (scope.toLowerCase()) { - case "session": - type = OptionValue.OptionType.SESSION; - break; - case "system": - type = OptionValue.OptionType.SYSTEM; - break; -// case "query": -// type = OptionValue.OptionType.QUERY; -// break; - default: - throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM."); + case "session": + type = OptionType.SESSION; + break; + case "system": + type = OptionType.SYSTEM; + break; + default: + throw UserException.validationError() + .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope) + .build(logger); } + } if (type == OptionType.SYSTEM) { // If the user authentication is enabled, make sure the user who is trying to change the system option has // administrative privileges. if (context.isUserAuthenticationEnabled() && !ImpersonationUtil.hasAdminPrivileges( context.getQueryUserName(), context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val, context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { - throw UserException.permissionError() - .message("Not authorized to change SYSTEM options.") - .build(logger); - } + if (type == OptionType.SYSTEM) Unknown macro: { + // If the user authentication is enabled, make sure the user who is trying to change the system option has + // administrative privileges. + if (context.isUserAuthenticationEnabled() && + !ImpersonationUtil.hasAdminPrivileges( + context.getQueryUserName(), + context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val, + context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { + throw UserException.permissionError() + .message("Not authorized to change SYSTEM options.") + .build(logger); } + } + + // Currently, we use one part identifiers. + final SqlIdentifier nameIdentifier = option.getName(); + if (!nameIdentifier.isSimple()) { + throw UserException.validationError() + .message("Drill does not support multi-part identifier for an option name (%s).", + nameIdentifier.toString()) End diff – this is not important, but you can just pass nameIdentifier and .toString() will be called implicitly
            githubbot ASF GitHub Bot added a comment -

            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 ?

            githubbot ASF GitHub Bot added a comment - 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 ?
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

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

            githubbot ASF GitHub Bot added a comment - 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()`
            githubbot ASF GitHub Bot added a comment -

            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` ?

            githubbot ASF GitHub Bot added a comment - 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` ?
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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 ?

            githubbot ASF GitHub Bot added a comment - 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 ?
            githubbot ASF GitHub Bot added a comment -

            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 ?

            githubbot ASF GitHub Bot added a comment - 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 ?
            githubbot ASF GitHub Bot added a comment -

            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 ?

            githubbot ASF GitHub Bot added a comment - 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 ?
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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

              Unknown macro: { switch (scope.toLowerCase()) { - case "session": - type = OptionValue.OptionType.SESSION; - break; - case "system": - type = OptionValue.OptionType.SYSTEM; - break; -// case "query": -// type = OptionValue.OptionType.QUERY; -// break; - default: - throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM."); + case "session": + type = OptionType.SESSION; + break; + case "system": + type = OptionType.SYSTEM; + break; + default: + throw UserException.validationError() + .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope) + .build(logger); } + }
            • if (type == OptionType.SYSTEM) {
            • // If the user authentication is enabled, make sure the user who is trying to change the system option has
            • // administrative privileges.
            • if (context.isUserAuthenticationEnabled() &&
            • !ImpersonationUtil.hasAdminPrivileges(
            • context.getQueryUserName(),
            • context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,
            • context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { - throw UserException.permissionError() - .message("Not authorized to change SYSTEM options.") - .build(logger); - }

              + if (type == OptionType.SYSTEM) {
              + // If the user authentication is enabled, make sure the user who is trying to change the system option has
              + // administrative privileges.
              + if (context.isUserAuthenticationEnabled() &&
              + !ImpersonationUtil.hasAdminPrivileges(
              + context.getQueryUserName(),
              + context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val,

                • End diff –

            This is not a change I introduced (just difference in spacing), but I'll address this issue.

            githubbot ASF GitHub Bot added a comment - 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 Unknown macro: { switch (scope.toLowerCase()) { - case "session": - type = OptionValue.OptionType.SESSION; - break; - case "system": - type = OptionValue.OptionType.SYSTEM; - break; -// case "query": -// type = OptionValue.OptionType.QUERY; -// break; - default: - throw new ValidationException("Invalid OPTION scope. Scope must be SESSION or SYSTEM."); + case "session": + type = OptionType.SESSION; + break; + case "system": + type = OptionType.SYSTEM; + break; + default: + throw UserException.validationError() + .message("Invalid OPTION scope %s. Scope must be SESSION or SYSTEM.", scope) + .build(logger); } + } if (type == OptionType.SYSTEM) { // If the user authentication is enabled, make sure the user who is trying to change the system option has // administrative privileges. if (context.isUserAuthenticationEnabled() && !ImpersonationUtil.hasAdminPrivileges( context.getQueryUserName(), context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val, context.getOptions().getOption(ExecConstants.ADMIN_USER_GROUPS_KEY).string_val)) { - throw UserException.permissionError() - .message("Not authorized to change SYSTEM options.") - .build(logger); - } + if (type == OptionType.SYSTEM) { + // If the user authentication is enabled, make sure the user who is trying to change the system option has + // administrative privileges. + if (context.isUserAuthenticationEnabled() && + !ImpersonationUtil.hasAdminPrivileges( + context.getQueryUserName(), + context.getOptions().getOption(ExecConstants.ADMIN_USERS_KEY).string_val, End diff – This is not a change I introduced (just difference in spacing), but I'll address this issue.
            githubbot ASF GitHub Bot added a comment -

            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?

            githubbot ASF GitHub Bot added a comment - 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?
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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 ?

            githubbot ASF GitHub Bot added a comment - 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 ?
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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)

            githubbot ASF GitHub Bot added a comment - 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)
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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 ?

            githubbot ASF GitHub Bot added a comment - 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 ?
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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?

            githubbot ASF GitHub Bot added a comment - 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?
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

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

            githubbot ASF GitHub Bot added a comment - 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```).
            githubbot ASF GitHub Bot added a comment -

            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?

            githubbot ASF GitHub Bot added a comment - 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?
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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?

            githubbot ASF GitHub Bot added a comment - 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?
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

            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

            githubbot ASF GitHub Bot added a comment - 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
            githubbot ASF GitHub Bot added a comment -

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

            githubbot ASF GitHub Bot added a comment - 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 ).
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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?

            githubbot ASF GitHub Bot added a comment - 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?
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

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

            githubbot ASF GitHub Bot added a comment - 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).
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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 ?

            githubbot ASF GitHub Bot added a comment - 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 ?
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            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.

            githubbot ASF GitHub Bot added a comment - 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.
            githubbot ASF GitHub Bot added a comment -

            Github user asfgit closed the pull request at:

            https://github.com/apache/drill/pull/159

            githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/drill/pull/159
            khfaraaz Khurram Faraaz added a comment -

            Verified.

            khfaraaz Khurram Faraaz added a comment - Verified.

            People

              sudheeshkatkam Sudheesh Katkam
              amansinha100 Aman Sinha
              Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: