Skip to content

Commit

Permalink
DRILL-4332: Makes vector comparison order stable in test framework
Browse files Browse the repository at this point in the history 
In the test framework, a vector is a map of <String, Object>. When comparing
actual values with baseline, the comparison is made column by column, but
a HashMap key ordering is not guaranteed, and the ordering actually changed
between Java7 and Java8 in Oracle/OpenJDK.

Replacing HashMap with TreeMap which has a guaranteed ordering by design.

Small update by jason during merge, fixed test failure on JDK 7 due to map key ordering,
just replaced two more uses of HashMap with TreeMap.

Closes #389
  • Loading branch information
@laurentgo @jaltekruse
laurentgo authored and jaltekruse committed Mar 8, 2016
1 parent 80316f3 commit 447b093
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 31 deletions.
59 changes: 29 additions & 30 deletions exec/java-exec/src/test/java/org/apache/drill/DrillTestWrapper.java
View file
Expand Up @@ -29,6 +29,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;

import org.apache.drill.common.expression.SchemaPath;
import org.apache.drill.common.types.TypeProtos;
Expand Down Expand Up @@ -157,7 +158,6 @@ private void compareHyperVectors(Map<String, HyperVectorValueIterator> expectedR
}

private void compareMergedVectors(Map<String, List<Object>> expectedRecords, Map<String, List<Object>> actualRecords) throws Exception {

for (String s : actualRecords.keySet()) {
assertNotNull("Unexpected extra column " + s + " returned by query.", expectedRecords.get(s));
assertEquals("Incorrect number of rows returned by query.", expectedRecords.get(s).size(), actualRecords.get(s).size());
Expand Down Expand Up @@ -209,7 +209,7 @@ private String printNearbyRecords(Map<String, List<Object>> expectedRecords, Map
private Map<String, HyperVectorValueIterator> addToHyperVectorMap(List<QueryDataBatch> records, RecordBatchLoader loader,
BatchSchema schema) throws SchemaChangeException, UnsupportedEncodingException {
// TODO - this does not handle schema changes
Map<String, HyperVectorValueIterator> combinedVectors = new HashMap<>();
Map<String, HyperVectorValueIterator> combinedVectors = new TreeMap<>();

long totalRecords = 0;
QueryDataBatch batch;
Expand Down Expand Up @@ -255,7 +255,7 @@ private Map<String, HyperVectorValueIterator> addToHyperVectorMap(List<QueryData
private Map<String, List<Object>> addToCombinedVectorResults(List<QueryDataBatch> records, RecordBatchLoader loader,
BatchSchema schema) throws SchemaChangeException, UnsupportedEncodingException {
// TODO - this does not handle schema changes
Map<String, List<Object>> combinedVectors = new HashMap<>();
Map<String, List<Object>> combinedVectors = new TreeMap<>();

long totalRecords = 0;
QueryDataBatch batch;
Expand Down Expand Up @@ -398,38 +398,37 @@ public void compareMergedOnHeapVectors() throws Exception {
Map<String, List<Object>> expectedSuperVectors;

try {
BaseTestQuery.test(testOptionSettingQueries);
actual = BaseTestQuery.testRunAndReturn(queryType, query);
BaseTestQuery.test(testOptionSettingQueries);
actual = BaseTestQuery.testRunAndReturn(queryType, query);

checkNumBatches(actual);
checkNumBatches(actual);

// To avoid extra work for test writers, types can optionally be inferred from the test query
addTypeInfoIfMissing(actual.get(0), testBuilder);
// To avoid extra work for test writers, types can optionally be inferred from the test query
addTypeInfoIfMissing(actual.get(0), testBuilder);

actualSuperVectors = addToCombinedVectorResults(actual, loader, schema);
actualSuperVectors = addToCombinedVectorResults(actual, loader, schema);

// If baseline data was not provided to the test builder directly, we must run a query for the baseline, this includes
// the cases where the baseline is stored in a file.
if (baselineRecords == null) {
BaseTestQuery.test(baselineOptionSettingQueries);
expected = BaseTestQuery.testRunAndReturn(baselineQueryType, testBuilder.getValidationQuery());
expectedSuperVectors = addToCombinedVectorResults(expected, loader, schema);
} else {
// data is built in the TestBuilder in a row major format as it is provided by the user
// translate it here to vectorized, the representation expected by the ordered comparison
expectedSuperVectors = new HashMap<>();
expected = new ArrayList<>();
for (String s : baselineRecords.get(0).keySet()) {
expectedSuperVectors.put(s, new ArrayList<>());
}
for (Map<String, Object> m : baselineRecords) {
for (String s : m.keySet()) {
expectedSuperVectors.get(s).add(m.get(s));
// If baseline data was not provided to the test builder directly, we must run a query for the baseline, this includes
// the cases where the baseline is stored in a file.
if (baselineRecords == null) {
BaseTestQuery.test(baselineOptionSettingQueries);
expected = BaseTestQuery.testRunAndReturn(baselineQueryType, testBuilder.getValidationQuery());
expectedSuperVectors = addToCombinedVectorResults(expected, loader, schema);
} else {
// data is built in the TestBuilder in a row major format as it is provided by the user
// translate it here to vectorized, the representation expected by the ordered comparison
expectedSuperVectors = new TreeMap<>();
expected = new ArrayList<>();
for (String s : baselineRecords.get(0).keySet()) {
expectedSuperVectors.put(s, new ArrayList<>());
}
for (Map<String, Object> m : baselineRecords) {
for (String s : m.keySet()) {
expectedSuperVectors.get(s).add(m.get(s));
}
}
}
}

compareMergedVectors(expectedSuperVectors, actualSuperVectors);
compareMergedVectors(expectedSuperVectors, actualSuperVectors);
} catch (Exception e) {
throw new Exception(e.getMessage() + "\nFor query: " + query , e);
} finally {
Expand Down Expand Up @@ -510,7 +509,7 @@ protected void addToMaterializedResults(List<Map<String, Object>> materializedRe
logger.debug("reading batch with " + loader.getRecordCount() + " rows, total read so far " + totalRecords);
totalRecords += loader.getRecordCount();
for (int j = 0; j < loader.getRecordCount(); j++) {
HashMap<String, Object> record = new HashMap<>();
Map<String, Object> record = new TreeMap<>();
for (VectorWrapper<?> w : loader) {
Object obj = w.getValueVector().getAccessor().getObject(j);
if (obj != null) {
Expand Down
2 changes: 1 addition & 1 deletion exec/java-exec/src/test/java/org/apache/drill/TestFrameworkTest.java
View file
Expand Up @@ -321,7 +321,7 @@ public void testCSVVerificationOfOrder_checkFailure() throws Throwable {
.build().run();
} catch (Exception ex) {
assertThat(ex.getMessage(), CoreMatchers.containsString(
"at position 0 column '`first_name`' mismatched values, expected: Jewel(String) but received Peggy(String)"));
"at position 0 column '`employee_id`' mismatched values, expected: 12(String) but received 16(String)"));
// this indicates successful completion of the test
return;
}
Expand Down

0 comments on commit 447b093

Please sign in to comment.