[GitHub] drill pull request #970: DRILL-5832: Migrate OperatorFixture to use SystemOp...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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 deci