[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/geode/pull/580


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[YehEmily]

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

https://github.com/apache/geode/pull/580#discussion_r129695770
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,54 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @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++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 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());
   }
 }
   }
 
+  // The following tests cover edge cases in OrderByComparator.
+  @Test
+  public void testCompareTwoNulls() throws Exception {
+assert (createComparator().compare(null, null) == 0);
--- End diff --

Thanks for catching this! I've fixed it and updated the PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[kirklund]

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

https://github.com/apache/geode/pull/580#discussion_r129693040
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,54 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @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++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 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());
   }
 }
   }
 
+  // The following tests cover edge cases in OrderByComparator.
+  @Test
+  public void testCompareTwoNulls() throws Exception {
+assert (createComparator().compare(null, null) == 0);
--- End diff --

"assert" should be used in product code instead of tests. These are the 
optional assertions that are enabled when running a Java product with -ea 
(enable assertions).

You should use assertThat instead:
```java
assertThat(createComparator().compare(null, null).isEqualTo(0);
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[kirklund]

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

https://github.com/apache/geode/pull/580#discussion_r129693128
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,54 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @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++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 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());
   }
 }
   }
 
+  // The following tests cover edge cases in OrderByComparator.
+  @Test
+  public void testCompareTwoNulls() throws Exception {
+assert (createComparator().compare(null, null) == 0);
+  }
+
+  @Test
+  public void testCompareTwoObjectArrays() throws Exception {
+String[] arrString1 = {"elephant"};
+String[] arrString2 = {"elephants"};
+assert (createComparator().compare(arrString2, arrString1) == 1);
--- End diff --

Same as previous comment. Please use assertThat.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[YehEmily]

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

https://github.com/apache/geode/pull/580#discussion_r129399451
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,58 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @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++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 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());
   }
 }
   }
 
+  /**
+   * Tests three cases that were not originally covered by the original 
tests in this class. (To
+   * confirm that these tests cover all cases in OrderByComparator, run 
them with coverage.)
+   */
+  @Test
+  public void testCompareMethodEdgeCases() throws Exception {
--- End diff --

Thanks for the feedback! I've updated the PR!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[YehEmily]

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

https://github.com/apache/geode/pull/580#discussion_r129399374
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,58 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @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++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 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());
   }
 }
   }
 
+  /**
+   * Tests three cases that were not originally covered by the original 
tests in this class. (To
--- End diff --

All good points - I've fixed this and updated the PR!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[pdxrunner]

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

https://github.com/apache/geode/pull/580#discussion_r129395826
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/cache/query/internal/OrderByComparatorJUnitTest.java
 ---
@@ -173,36 +157,58 @@ public void testUnsupportedOrderByForPR() throws 
Exception {
 
   @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++) {
   r1.put(new Portfolio(i), new Portfolio(i));
 }
 
 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());
   }
 }
   }
 
+  /**
+   * Tests three cases that were not originally covered by the original 
tests in this class. (To
--- End diff --

This comment probably doesn't need to be a javadoc, and the note about 
coverage isn't needed. I'd make this comment say something like, "The following 
tests cover edge cases in OrderByComparator".


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[YehEmily]

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

https://github.com/apache/geode/pull/580#discussion_r122067285
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java
 ---
@@ -228,4 +139,55 @@ void addEvaluatedSortCriteria(Object row, 
ExecutionContext context)
 // 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) {
--- End diff --

Oh no, that's very true! I'll fix this and update the PR!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[jaredjstewart]

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

https://github.com/apache/geode/pull/580#discussion_r122066276
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java
 ---
@@ -228,4 +139,55 @@ void addEvaluatedSortCriteria(Object row, 
ExecutionContext context)
 // 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) {
--- End diff --

I see it was not introduced by your changes, but the subtraction on 177 is 
not a safe way to compare doubles as it can result in under/overflow.  For 
example, consider the result of `compareTwoNumbers( Double.MAX_VALUE, -1d)`.  
Here's a safer way to do it:

```
  private int compareTwoNumbers(Object obj1, Object obj2) {
Number number1 = (Number) obj1;
Number number2 = (Number) obj2 ;
return Double.compare(number1.doubleValue(), number2.doubleValue());
  }
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[YehEmily]

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

https://github.com/apache/geode/pull/580#discussion_r122055572
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/cache/query/internal/OrderByComparator.java
 ---
@@ -45,78 +44,50 @@ public OrderByComparator(List 
orderByAttrs, ObjectType ob
   }
 
   /**
-   * 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();
+  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());
+  while (orderIter.hasNext()) {
--- End diff --

Fixed, and updating the PR now - thank you!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #580: GEODE-2936: Refactoring OrderByComparator

[YehEmily]

GitHub user YehEmily opened a pull request:

https://github.com/apache/geode/pull/580

GEODE-2936: Refactoring OrderByComparator

[View the JIRA ticket 
here.](https://issues.apache.org/jira/browse/GEODE-2936)

`OrderByComparator.java` contained a lot of redundant and confusing code, 
as well as somewhat misleading comments (for example, the `compare` method was 
supposed to throw a `ClassCastException` if the parameters given could not be 
compared using this comparator, but it didn't. This has been fixed). Code that 
is shared by both the `compare` and `evaluateSortCriteria` methods has been 
moved to helper methods to improve readability and reduce redundancy.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in 
the commit message?

- [ ] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?

- [ ] Is your initial contribution a single, squashed commit?

- [ ] Does `gradlew build` run cleanly?

- [ ] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and
submit an update to your PR as soon as possible. If you need help, please 
send an
email to dev@geode.apache.org.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/YehEmily/geode GEODE-2936-2

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/geode/pull/580.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #580


commit 3ff6d0a2b4f097947e337f5567b641aeadfb0e22
Author: Jinmei Liao 
Date:   2017-06-07T23:10:56Z

GEODE-2936: Refactoring OrderByComparator to reduce redundant code and 
improve readability.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---