drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ilooner <...@git.apache.org>
Subject [GitHub] drill pull request #923: DRILL-5723: Added System Internal Options That can ...
Date Mon, 11 Sep 2017 17:11:30 GMT
Github user ilooner commented on a diff in the pull request:

    --- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/server/TestOptions.java ---
    @@ -56,7 +56,7 @@ public void checkChangedColumn() throws Exception {
         test("ALTER session SET `%s` = %d;", SLICE_TARGET,
    -        .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND type = 'SESSION'",
    +        .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND optionScope =
    --- End diff --
    Yeah we are in a tough spot because **type** was not well defined previously, so the tests
implicitly assumed **type** represented where an option was set. Now that we have settled
on a definition for **type**, which is the set of scopes where an option can be set, we have
deviated from the meaning **type** was given in the tests. One possible way out of this situation
is to change the definition of **type** and **optionScope** again by swapping their meanings:
    * **type**: Would become where an option was set.
    * **optionScope**: Would become the set of scopes where an option could be set.
    This would minimize the changes required to the unit tests. It's hard to say how it would
impact other user's scripts though because **type** was treated inconsistently in the code
base, so I'm not sure how someone could have used the **type** information productively except
to write unit tests, which verified incorrect behavior.
    Long story short, I'll swap the definitions of **type** and **optionScope**. I think that
would minimize the impact on unit tests and users, but we cannot provide any guarantees for
users who depended on type when it was inconsistently defined.


View raw message