GEODE-2936: Refactoring OrderByComparator and updating OrderByComparatorJUnitTest
This closes #580 Project: http://git-wip-us.apache.org/repos/asf/geode/repo Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/06b44db8 Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/06b44db8 Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/06b44db8 Branch: refs/heads/feature/GEM-1483 Commit: 06b44db8ccf9016899ae79c663147a0829293a94 Parents: 6267809 Author: Jinmei Liao <[email protected]> Authored: Wed Jun 7 16:10:56 2017 -0700 Committer: Ken Howe <[email protected]> Committed: Wed Jul 26 15:27:08 2017 -0700 ---------------------------------------------------------------------- .../query/internal/CompiledSortCriterion.java | 6 +- .../cache/query/internal/OrderByComparator.java | 208 ++++++++----------- .../internal/OrderByComparatorUnmapped.java | 6 +- .../query/functional/StructSetOrResultsSet.java | 13 +- .../internal/OrderByComparatorJUnitTest.java | 69 +++--- 5 files changed, 134 insertions(+), 168 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/geode/blob/06b44db8/geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledSortCriterion.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledSortCriterion.java b/geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledSortCriterion.java index d49b4a5..4d2be62 100644 --- a/geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledSortCriterion.java +++ b/geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledSortCriterion.java @@ -56,7 +56,7 @@ public class CompiledSortCriterion extends AbstractCompiledValue { * evaluates sort criteria in order by clause */ public Object evaluate(Object data, ExecutionContext context) { - Object value = null; + Object value; if (this.columnIndex > 0) { value = ((Object[]) data)[this.columnIndex]; } else if (this.columnIndex == 0) { @@ -113,7 +113,7 @@ public class CompiledSortCriterion extends AbstractCompiledValue { } private CompiledValue getReconstructedExpression(String projAttribStr, ExecutionContext context) - throws AmbiguousNameException, TypeMismatchException, NameResolutionException { + throws TypeMismatchException, NameResolutionException { List<CompiledValue> expressions = PathUtils.collectCompiledValuesInThePath(expr, context); StringBuilder tempBuff = new StringBuilder(); ListIterator<CompiledValue> listIter = expressions.listIterator(expressions.size()); @@ -180,7 +180,7 @@ public class CompiledSortCriterion extends AbstractCompiledValue { } boolean mapExpressionToProjectionField(List projAttrs, ExecutionContext context) - throws AmbiguousNameException, TypeMismatchException, NameResolutionException { + throws TypeMismatchException, NameResolutionException { boolean mappedColumn = false; this.originalCorrectedExpression = expr; if (projAttrs != null) { http://git-wip-us.apache.org/repos/asf/geode/blob/06b44db8/geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java b/geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java index 59eb493..04469cb 100644 --- a/geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java +++ b/geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java @@ -15,7 +15,6 @@ package org.apache.geode.cache.query.internal; import java.util.Comparator; -import java.util.Iterator; import java.util.List; import org.apache.geode.cache.query.FunctionDomainException; @@ -28,9 +27,8 @@ import org.apache.geode.internal.cache.VMCachedDeserializable; import org.apache.geode.pdx.internal.PdxString; /** - * A generic comparator class which compares two Object/StructImpl according to their sort criterion - * specified in order by clause - * + * A generic comparator class which compares two Object/StructImpl according to their sort criteria + * specified in order by clause. */ public class OrderByComparator implements Comparator { private final ObjectType objType; @@ -45,78 +43,44 @@ public class OrderByComparator implements Comparator { } /** - * Yogesh : This methods evaluates sort criteria and returns a ArrayList of Object[] arrays of - * evaluated criteria + * This method evaluates sort criteria and returns an ArrayList of Object[] arrays of the + * evaluated criteria. * - * @param value - * @return Object[] + * @param value the criteria to be evaluated. + * @return an Object array of Object arrays of the evaluated criteria. */ protected Object[] evaluateSortCriteria(Object value) { - - CompiledSortCriterion csc; Object[] array = null; if (orderByAttrs != null) { array = new Object[orderByAttrs.size()]; - Iterator orderiter = orderByAttrs.iterator(); int i = 0; - while (orderiter.hasNext()) { - csc = (CompiledSortCriterion) orderiter.next(); - Object[] arr = new Object[2]; - arr[0] = csc.evaluate(value, context); - arr[1] = Boolean.valueOf(csc.getCriterion()); + for (CompiledSortCriterion csc : orderByAttrs) { + Object[] arr = {csc.evaluate(value, context), csc.getCriterion()}; array[i++] = arr; } - } return array; } + /** + * This method evaluates sort criteria and returns the resulting integer value of comparing the + * two objects passed into it based on these criteria. + * + * @param value1 the first object to be compared. + * @param value2 the second object to be compared. + * @return a negative integer, zero, or a positive integer if the first argument is less than, + * equal to, or greater than the second, based on the evaluated sort criteria. + */ protected int evaluateSortCriteria(Object value1, Object value2) { int result = -1; - CompiledSortCriterion csc; if (orderByAttrs != null) { - Iterator orderiter = orderByAttrs.iterator(); - while (orderiter.hasNext()) { - csc = (CompiledSortCriterion) orderiter.next(); + for (CompiledSortCriterion csc : orderByAttrs) { Object sortCriteriaForValue1 = csc.evaluate(value1, context); Object sortCriteriaForValue2 = csc.evaluate(value2, context); - - if (sortCriteriaForValue1 == null || sortCriteriaForValue2 == null) { - if (sortCriteriaForValue1 == null) { - result = (sortCriteriaForValue2 == null ? 0 : -1); - } else { - result = 1; - } - } else if (sortCriteriaForValue1 == QueryService.UNDEFINED - || sortCriteriaForValue2 == QueryService.UNDEFINED) { - if (sortCriteriaForValue1 == QueryService.UNDEFINED) { - result = (sortCriteriaForValue2 == QueryService.UNDEFINED ? 0 : -1); - } else { - result = 1; - } - } else { - if (sortCriteriaForValue1 instanceof Number && sortCriteriaForValue2 instanceof Number) { - double diff = ((Number) sortCriteriaForValue1).doubleValue() - - ((Number) sortCriteriaForValue2).doubleValue(); - result = diff > 0 ? 1 : diff < 0 ? -1 : 0; - } else { - if (sortCriteriaForValue1 instanceof PdxString - && sortCriteriaForValue2 instanceof String) { - sortCriteriaForValue2 = new PdxString((String) sortCriteriaForValue2); - } else if (sortCriteriaForValue2 instanceof PdxString - && sortCriteriaForValue1 instanceof String) { - sortCriteriaForValue1 = new PdxString((String) sortCriteriaForValue1); - } - result = ((Comparable) sortCriteriaForValue1).compareTo(sortCriteriaForValue2); - } - - } - - if (result == 0) { - continue; - } else { - if (Boolean.valueOf(csc.getCriterion())) { - result = (result * (-1)); + result = compareHelperMethod(sortCriteriaForValue1, sortCriteriaForValue2); + if (result != 0) { + if (csc.getCriterion()) { + result *= -1; } break; } @@ -137,89 +101,35 @@ public class OrderByComparator implements Comparator { * Comparator. */ public int compare(Object obj1, Object obj2) { - int result = -1; + int result; if (obj1 == null && obj2 == null) { return 0; } assert !(obj1 instanceof VMCachedDeserializable || obj2 instanceof VMCachedDeserializable); - if ((this.objType.isStructType() && obj1 instanceof Object[] && obj2 instanceof Object[]) - || !this.objType.isStructType()) { // obj1 instanceof Object && obj2 - // instanceof Object){ - if ((result = evaluateSortCriteria(obj1, obj2)) != 0) { + || !this.objType.isStructType()) { + if (((result = evaluateSortCriteria(obj1, obj2)) != 0) && (orderByAttrs != null)) { return result; } - - QueryObserver observer = QueryObserverHolder.getInstance(); - if (observer != null) { - observer.orderByColumnsEqual(); + if (QueryObserverHolder.getInstance() != null) { + QueryObserverHolder.getInstance().orderByColumnsEqual(); } - // The comparable fields are equal, so we check if the overall keys are - // equal or not + // Comparable fields are equal - check if overall keys are equal if (this.objType.isStructType()) { int i = 0; for (Object o1 : (Object[]) obj1) { Object o2 = ((Object[]) obj2)[i++]; - - // Check for null value. - if (o1 == null || o2 == null) { - if (o1 == null) { - if (o2 == null) { - continue; - } - return -1; - } else { - return 1; - } - } else if (o1 == QueryService.UNDEFINED || o2 == QueryService.UNDEFINED) { - if (o1 == QueryService.UNDEFINED) { - if (o2 == QueryService.UNDEFINED) { - continue; - } - return -1; - } else { - return 1; - } - } - - if (o1 instanceof Comparable) { - final int rslt; - if (o1 instanceof Number && o2 instanceof Number) { - double diff = ((Number) o1).doubleValue() - ((Number) o2).doubleValue(); - rslt = diff > 0 ? 1 : diff < 0 ? -1 : 0; - } else { - if (o1 instanceof PdxString && o2 instanceof String) { - o2 = new PdxString((String) o2); - } else if (o2 instanceof PdxString && o1 instanceof String) { - o1 = new PdxString((String) o1); - } - rslt = ((Comparable) o1).compareTo(o2); - } - if (rslt == 0) { - continue; - } else { - return rslt; - } - } else if (!o1.equals(o2)) { - return -1; + result = compareHelperMethod(o1, o2); + if (result != 0) { + return result; } } return 0; } else { - if (obj1 instanceof PdxString && obj2 instanceof String) { - obj2 = new PdxString((String) obj2); - } else if (obj2 instanceof PdxString && obj1 instanceof String) { - obj1 = new PdxString((String) obj1); - } - - if (obj1 instanceof Comparable) { - return ((Comparable) obj1).compareTo(obj2); - } else { - return obj1.equals(obj2) ? 0 : -1; - } + return compareTwoStrings(obj1, obj2); } } - return -1; + throw new ClassCastException(); // throw exception if args can't be compared w/this comparator } void addEvaluatedSortCriteria(Object row, ExecutionContext context) @@ -228,4 +138,56 @@ public class OrderByComparator implements Comparator { // No op } + private int compareHelperMethod(Object obj1, Object obj2) { + if (obj1 == null || obj2 == null) { + return compareIfOneOrMoreNull(obj1, obj2); + } else if (obj1 == QueryService.UNDEFINED || obj2 == QueryService.UNDEFINED) { + return compareIfOneOrMoreQueryServiceUndefined(obj1, obj2); + } else { + return compareTwoObjects(obj1, obj2); + } + } + + private int compareIfOneOrMoreNull(Object obj1, Object obj2) { + if (obj1 == null) { + return obj2 == null ? 0 : -1; + } else { + return 1; + } + } + + private int compareIfOneOrMoreQueryServiceUndefined(Object obj1, Object obj2) { + if (obj1 == QueryService.UNDEFINED) { + return obj2 == QueryService.UNDEFINED ? 0 : -1; + } else { + return 1; + } + } + + private int compareTwoObjects(Object obj1, Object obj2) { + if (obj1 instanceof Number && obj2 instanceof Number) { + return compareTwoNumbers(obj1, obj2); + } else { + return compareTwoStrings(obj1, obj2); + } + } + + private int compareTwoNumbers(Object obj1, Object obj2) { + Number num1 = (Number) obj1; + Number num2 = (Number) obj2; + return Double.compare(num1.doubleValue(), num2.doubleValue()); + } + + private int compareTwoStrings(Object obj1, Object obj2) { + if (obj1 instanceof PdxString && obj2 instanceof String) { + obj2 = new PdxString((String) obj2); + } else if (obj2 instanceof PdxString && obj1 instanceof String) { + obj1 = new PdxString((String) obj1); + } + if (obj1 instanceof Comparable) { + return ((Comparable) obj1).compareTo(obj2); + } else { + return obj1.equals(obj2) ? 0 : -1; + } + } } http://git-wip-us.apache.org/repos/asf/geode/blob/06b44db8/geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparatorUnmapped.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparatorUnmapped.java b/geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparatorUnmapped.java index db31df9..ee2caa0 100644 --- a/geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparatorUnmapped.java +++ b/geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparatorUnmapped.java @@ -14,13 +14,13 @@ */ package org.apache.geode.cache.query.internal; -import it.unimi.dsi.fastutil.objects.Object2ObjectOpenCustomHashMap; - import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; +import it.unimi.dsi.fastutil.objects.Object2ObjectOpenCustomHashMap; + import org.apache.geode.cache.query.FunctionDomainException; import org.apache.geode.cache.query.NameResolutionException; import org.apache.geode.cache.query.QueryInvocationTargetException; @@ -113,7 +113,7 @@ public class OrderByComparatorUnmapped extends OrderByComparator { @Override protected Object[] evaluateSortCriteria(Object row) { - return (Object[]) orderByMap.get(row); + return orderByMap.get(row); } http://git-wip-us.apache.org/repos/asf/geode/blob/06b44db8/geode-core/src/test/java/org/apache/geode/cache/query/functional/StructSetOrResultsSet.java ---------------------------------------------------------------------- diff --git a/geode-core/src/test/java/org/apache/geode/cache/query/functional/StructSetOrResultsSet.java b/geode-core/src/test/java/org/apache/geode/cache/query/functional/StructSetOrResultsSet.java index fa5b3cd..b328cfe 100755 --- a/geode-core/src/test/java/org/apache/geode/cache/query/functional/StructSetOrResultsSet.java +++ b/geode-core/src/test/java/org/apache/geode/cache/query/functional/StructSetOrResultsSet.java @@ -18,7 +18,9 @@ */ package org.apache.geode.cache.query.functional; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; @@ -93,8 +95,8 @@ public class StructSetOrResultsSet { public void CompareQueryResultsWithoutAndWithIndexes(Object[][] r, int len, boolean checkOrder, String queries[]) { - Collection coll1 = null; - Collection coll2 = null; + Collection coll1; + Collection coll2; for (int j = 0; j < len; j++) { checkSelectResultTypes((SelectResults) r[j][0], (SelectResults) r[j][1], queries[j]); checkResultSizes((SelectResults) r[j][0], (SelectResults) r[j][1], queries[j]); @@ -113,7 +115,7 @@ public class StructSetOrResultsSet { public void compareExternallySortedQueriesWithOrderBy(String[] queries, Object[][] baseResults) throws Exception { for (int i = 0; i < queries.length; i++) { - Query q = null; + Query q; try { String query = queries[i]; int indexOfOrderBy = query.indexOf("order "); @@ -124,7 +126,7 @@ public class StructSetOrResultsSet { int unorderedResultsSize = ((SelectResults) baseResults[i][1]).size(); if (unorderedResultsSize == 0) { fail( - "The test results size is 0 , it possibly is not validating anything. rewrite the test"); + "The test results size is 0. It may not be validating anything. Please rewrite the test."); } Wrapper wrapper = getOrderByComparatorAndLimitForQuery(queries[i], unorderedResultsSize); if (wrapper.validationLevel != ValidationLevel.NONE) { @@ -230,7 +232,6 @@ public class StructSetOrResultsSet { @Override public int compare(Struct o1, Struct o2) { return obc.compare(o1.getFieldValues(), o2.getFieldValues()); - } }; } http://git-wip-us.apache.org/repos/asf/geode/blob/06b44db8/geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java ---------------------------------------------------------------------- diff --git a/geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java b/geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java index 4d5174e..6c365cd 100644 --- a/geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java @@ -14,12 +14,15 @@ */ package org.apache.geode.cache.query.internal; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.lang.reflect.Field; import java.util.Collection; +import java.util.List; import org.junit.After; import org.junit.Before; @@ -32,9 +35,9 @@ import org.apache.geode.cache.Region; import org.apache.geode.cache.query.CacheUtils; import org.apache.geode.cache.query.Query; import org.apache.geode.cache.query.QueryInvalidException; -import org.apache.geode.cache.query.QueryService; import org.apache.geode.cache.query.data.Portfolio; import org.apache.geode.cache.query.data.Position; +import org.apache.geode.cache.query.internal.types.StructTypeImpl; import org.apache.geode.test.junit.categories.IntegrationTest; @Category(IntegrationTest.class) @@ -43,7 +46,6 @@ public class OrderByComparatorJUnitTest { @Before public void setUp() throws java.lang.Exception { CacheUtils.startCache(); - } @After @@ -53,18 +55,13 @@ public class OrderByComparatorJUnitTest { @Test public void testOrderByComparatorUnmapped() throws Exception { - String queries[] = { - "SELECT distinct ID, description, createTime FROM /portfolio1 pf1 where ID > 0 order by ID desc, pkid desc ",}; Object r[][] = new Object[queries.length][2]; - QueryService qs; - qs = CacheUtils.getQueryService(); Position.resetCounter(); - // Create Regions + // Create Regions Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class); - for (int i = 0; i < 10; i++) { r1.put(i + "", new Portfolio(i)); } @@ -85,7 +82,6 @@ public class OrderByComparatorJUnitTest { assertTrue(base instanceof SortedStructSet); SortedStructSet sss = (SortedStructSet) base; assertTrue(sss.comparator() instanceof OrderByComparatorUnmapped); - } catch (Exception e) { e.printStackTrace(); fail(q.getQueryString()); @@ -95,16 +91,12 @@ public class OrderByComparatorJUnitTest { @Test public void testOrderByComparatorMapped() throws Exception { - String queries[] = { - "SELECT distinct ID, description, createTime, pkid FROM /portfolio1 pf1 where ID > 0 order by ID desc, pkid desc ",}; Object r[][] = new Object[queries.length][2]; - QueryService qs; - qs = CacheUtils.getQueryService(); Position.resetCounter(); - // Create Regions + // Create Regions Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class); for (int i = 0; i < 10; i++) { @@ -128,7 +120,6 @@ public class OrderByComparatorJUnitTest { SortedStructSet sss = (SortedStructSet) base; assertFalse(sss.comparator() instanceof OrderByComparatorUnmapped); assertTrue(sss.comparator() instanceof OrderByComparator); - } catch (Exception e) { e.printStackTrace(); fail(q.getQueryString()); @@ -138,14 +129,9 @@ public class OrderByComparatorJUnitTest { @Test public void testUnsupportedOrderByForPR() throws Exception { - String unsupportedQueries[] = - {"select distinct p.status from /portfolio1 p order by p.status, p.ID", - - }; + {"select distinct p.status from /portfolio1 p order by p.status, p.ID"}; Object r[][] = new Object[unsupportedQueries.length][2]; - QueryService qs; - qs = CacheUtils.getQueryService(); Position.resetCounter(); // Create Regions PartitionAttributesFactory paf = new PartitionAttributesFactory(); @@ -158,8 +144,7 @@ public class OrderByComparatorJUnitTest { } for (int i = 0; i < unsupportedQueries.length; i++) { - Query q = null; - + Query q; CacheUtils.getLogger().info("Executing query: " + unsupportedQueries[i]); q = CacheUtils.getQueryService().newQuery(unsupportedQueries[i]); try { @@ -173,17 +158,12 @@ public class OrderByComparatorJUnitTest { @Test public void testSupportedOrderByForRR() throws Exception { - String unsupportedQueries[] = - {"select distinct p.status from /portfolio1 p order by p.status, p.ID", - - }; + {"select distinct p.status from /portfolio1 p order by p.status, p.ID"}; Object r[][] = new Object[unsupportedQueries.length][2]; - QueryService qs; - qs = CacheUtils.getQueryService(); Position.resetCounter(); - // Create Regions + // Create Regions Region r1 = CacheUtils.createRegion("portfolio1", Portfolio.class); for (int i = 0; i < 50; i++) { @@ -191,13 +171,11 @@ public class OrderByComparatorJUnitTest { } for (int i = 0; i < unsupportedQueries.length; i++) { - Query q = null; - + Query q; CacheUtils.getLogger().info("Executing query: " + unsupportedQueries[i]); q = CacheUtils.getQueryService().newQuery(unsupportedQueries[i]); try { r[i][0] = q.execute(); - } catch (QueryInvalidException qe) { qe.printStackTrace(); fail(qe.toString()); @@ -205,4 +183,29 @@ public class OrderByComparatorJUnitTest { } } + // The following tests cover edge cases in OrderByComparator. + @Test + public void testCompareTwoNulls() throws Exception { + assertThat(createComparator().compare(null, null)).isEqualTo(0); + } + + @Test + public void testCompareTwoObjectArrays() throws Exception { + String[] arrString1 = {"elephants"}; + String[] arrString2 = {"elephant"}; + assertThat(createComparator().compare(arrString1, arrString2)).isEqualTo(1); + } + + @Test + public void testCompareThrowsClassCastException() throws Exception { + String testString = "elephant"; + int testInt = 159; + assertThatThrownBy(() -> createComparator().compare(testString, testInt)) + .isInstanceOf(ClassCastException.class); + } + + private OrderByComparator createComparator() throws Exception { + StructTypeImpl objType = new StructTypeImpl(); + return new OrderByComparator(null, objType, null); + } }
