Github user ilooner commented on a diff in the pull request:
https://github.com/apache/drill/pull/923#discussion_r138132892
--- 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,
ExecConstants.SLICE_TARGET_DEFAULT);
testBuilder()
- .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND
type = 'SESSION'", SLICE_TARGET)
+ .sqlQuery("SELECT status FROM sys.options WHERE name = '%s' AND
optionScope = 'SESSION'", SLICE_TARGET)
--- 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.
---
