[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/970#discussion_r145764765
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java
 ---
@@ -331,21 +320,21 @@ public int next() {
 return count;
   }
 
-  private int getPercentFilled() {
-int filled = 0;
-for (final ValueVector v : primitiveVectors) {
-  filled = Math.max(filled, v.getAccessor().getValueCount() * 100 / 
v.getValueCapacity());
-  if (v instanceof VariableWidthVector) {
-filled = Math.max(filled, ((VariableWidthVector) 
v).getCurrentSizeInBytes() * 100 / ((VariableWidthVector) v).getByteCapacity());
-  }
-  // TODO - need to re-enable this
-//  if (v instanceof RepeatedFixedWidthVector) {
-//filled = Math.max(filled, ((RepeatedFixedWidthVector) 
v).getAccessor().getGroupCount() * 100)
+//  private int getPercentFilled() {
--- End diff --

Delete?


---

[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/970#discussion_r145764035
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java 
---
@@ -208,17 +208,17 @@ public boolean wasAllPartitionsPruned() {
 return this.wasAllPartitionsPruned;
   }
 
-  private static String commonPath(final List statuses) {
-if (statuses == null || statuses.isEmpty()) {
-  return "";
-}
-
-final List files = Lists.newArrayList();
-for (final FileStatus status : statuses) {
-  files.add(status.getPath().toString());
-}
-return commonPathForFiles(files);
-  }
+//  private static String commonPath(final List statuses) {
--- End diff --

Could we delete this instead of comment it out?


---

[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/970#discussion_r145766449
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java
 ---
@@ -725,21 +724,24 @@ public void runTestAndValidate(String selection, 
String validationSelection, Str
 try {
   deleteTableIfExists(outputFile);
   test("use dfs_test.tmp");
-  //test("ALTER SESSION SET `planner.add_producer_consumer` = false");
+//test("ALTER SESSION SET `planner.add_producer_consumer` = false");
--- End diff --

Can we delete this?


---

[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/970#discussion_r145766019
  
--- Diff: exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java 
---
@@ -371,6 +373,16 @@ public static void testNoResult(String query, 
Object... args) throws Exception {
 testNoResult(1, query, args);
   }
 
+  public static void alterSession(String key, Object value) throws 
Exception {
--- End diff --

There is the setSessionOption method on line 487 that already does 
something similar. Could we delete those methods and their usages in favor of 
this one?


---

[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[ilooner]

Github user ilooner commented on a diff in the pull request:

https://github.com/apache/drill/pull/970#discussion_r145762743
  
--- Diff: 
contrib/storage-hive/core/src/test/java/org/apache/drill/exec/hive/TestHiveStorage.java
 ---
@@ -74,10 +73,8 @@ public void testNativeScanWhenNoColumnIsRead() throws 
Exception {
   .baselineValues(200l)
   .go();
 } finally {
-  final OperatorFixture.TestOptionSet testOptionSet = new 
OperatorFixture.TestOptionSet();
-  test(String.format("alter session set `%s` = %s",
-  ExecConstants.HIVE_OPTIMIZE_SCAN_WITH_NATIVE_READERS,
-  
Boolean.toString(testOptionSet.getDefault(ExecConstants.HIVE_OPTIMIZE_SCAN_WITH_NATIVE_READERS).bool_val)));
+  test(String.format("alter session reset `%s`",
--- End diff --

Minor nitpick. The String.format is not necessary here. BaseTestQuery has a 
test method that does the String.format for you.


---

[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/970#discussion_r144924587
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java
 ---
@@ -84,14 +82,7 @@
   private RecordReader recordReader;
   private DrillParquetRecordMaterializer recordMaterializer;
   private int recordCount;
-  private List primitiveVectors;
   private OperatorContext operatorContext;
--- End diff --

Variable was not used.


---

[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/970#discussion_r144924491
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java
 ---
@@ -184,25 +183,26 @@ public void testAllScalarTypes() throws Exception {
 
 try {
   // read all of the types with the complex reader
-  test(String.format("alter session set %s = true", 
ExecConstants.PARQUET_NEW_RECORD_READER));
+  alterSession(ExecConstants.PARQUET_NEW_RECORD_READER, true);
--- End diff --

Good point. Here, I was not changing the semantics of the test; rather just 
ensuring that the tests are deterministic in doing what they already did. 
Presumably the test author knew which reader to use for which test. In fact, 
the fact that the tests now pass indicates that the proper reader is used (or, 
at least, the wrong reader is not used...)


---

[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[sachouche]

Github user sachouche commented on a diff in the pull request:

https://github.com/apache/drill/pull/970#discussion_r143017778
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet2/DrillParquetReader.java
 ---
@@ -84,14 +82,7 @@
   private RecordReader recordReader;
   private DrillParquetRecordMaterializer recordMaterializer;
   private int recordCount;
-  private List primitiveVectors;
   private OperatorContext operatorContext;
--- End diff --

Can you explain the reason you are removing the existing logic?


---

[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[sachouche]

Github user sachouche commented on a diff in the pull request:

https://github.com/apache/drill/pull/970#discussion_r143020963
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java
 ---
@@ -184,25 +183,26 @@ public void testAllScalarTypes() throws Exception {
 
 try {
   // read all of the types with the complex reader
-  test(String.format("alter session set %s = true", 
ExecConstants.PARQUET_NEW_RECORD_READER));
+  alterSession(ExecConstants.PARQUET_NEW_RECORD_READER, true);
--- End diff --

Paul,

- I noticed the key PARQUET_NEW_RECORD_READER is erroneous
- There are currently two readers
   o The old one is used when nested data is used as it can handle all 
parquet use-cases
   o The new reader only deals with Flat parquet data types
- We might want to rename the keys as the new reader cannot always be 
substituted with the old one 



---

[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...

[paul-rogers]

GitHub user paul-rogers opened a pull request:

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

DRILL-5832: Migrate OperatorFixture to use SystemOptionManager rather than 
mock

The `OperatorFixture` provides structure for testing individual operators 
and other "sub-operator" bits of code. To do that, the framework provides mock 
network-free and server-free versions of the fragment context and operator 
context. As part of the mock, the `OperatorFixture` provides a mock version of 
the system option manager that provides a simple test-only implementation of an 
option set.

With the recent major changes to the system option manager, this mock 
implementation has drifted out of sync with the system option manager. Rather 
than upgrading the mock implementation, this change uses the system option 
manager directly – but configured for no ZK or file persistence of options.

The key reason for this change is that the system option manager has 
implemented a sophisticated way to handle option defaults; it is better to 
leverage that than to provide a mock implementation.

Work is divided into a series of commits. The first is just routine code 
cleanup.

### Options Manager Updates

The second commit introduces three changes to the options manager:

* Provide a test-only constructor for the system option manager that 
“persists” options to memory only.
* Provide classic `getType(String name)` methods for the option manager 
with types `Long`, `Double`, `Boolean` and `String`. This allows clients to 
provide only a name, not an option validator, to get an option value.
* In the `setLocalValue(String name, Object value)` method, allow values 
that are `int`s (mapped to `long`s) or `float`s (mapped to `double`s). This 
saves having to remember to write things like 10L or 1.0D in test code.
* Since we are now using the system option manager for the test framework, 
the `BaseOptionSet` class is no longer needed and was removed.

### Use System Option Manager for Tests

The next commit deals with replacing the ad-hoc `TestOptionSet` with the 
`SystemOptionManager` class.

* The `SystemOptionManager` is extended with a test-only constructor that 
uses an in-memory “persistent” store to avoid polluting the disk or ZK with 
test-only settings.
* The `OperatorFixtureBuilder` class is extended to gather a set of 
name/value pairs for options to set. These are set in the `OperatorFixture` 
once the system option manager is created. This mechanism uses the generic type 
mechanism to allow the values to be any valid Java type, rather than just 
strings. That is:

```
OperatorFixtureBuilder builder = …
builder.systemOption("myOption", 10);
builder.systemOption("yourOption", "yourValue");
…
```

### Rename FixtureBuilder

Drill now has two “fixtures”: `ClusterFixture` and `OperatorFixture`. 
The class `FixtureBuilder` is confusing: which fixture does it build? To 
resolve this, the class was renamed to `ClusterFixtureBuilder` to be consistent 
with other fixture builders.

### Use `ALTER SESSION RESET`

Many tests set a session option, then reset the option back to the 
defaults. Much code did this the hard way:

* Get the default value of the option from the code
* Set the session option using this default.

In removing the `TestOptionSet` class, we removed a method used to get the 
default. We need an alternative.

As it turns out, there is a cleaner way to accomplish the goal. Rather than 
forcing the option to the default value explicitly, we can just use the `ALTER 
SESSION RESET` command.

A new method, `resetSystemOption`, was added to `BaseTestQuery` and to 
`ClientFixture`. For tests that don’t use these classes, special one-off 
solutions were added.

This change has the added benefit of being more accurate. In the options 
system, an option set to the (presumed) default value is not the same as an 
unset option that automatically reverts to the default. Using `ALTER SESSION 
RESET` therefore puts the system back into the true "default" state where as 
explicitly setting the default does not. (And, trying to explicitly set the 
default can mask bugs, as described next.)

### `TestParquetWriter` and DRILL-5833

When testing the fixes, testing revealed a random failure in 
`TestParquetWriter` due to inconsistent resetting of session options. As it 
turns out, failure to reset the option to select the “new” Parquet reader 
caused the `testDecimal()` test sometimes use the “old” version. When the 
“old” version was used, it hit a long-standing bug in the way the code 
reads Decimal9 and Decimal18 types.

This bug was fixed, code visited during the investigation was cleaned up, 
the tests modified to always reset system options to the default value, and the 
failing