[GitHub] geode pull request #591: GEODE-3073: Renamed OrderByComparatorUnmapped to Or...

[asfgit]

Github user asfgit closed the pull request at:

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


---
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 #591: GEODE-3073: Renamed OrderByComparatorUnmapped to Or...

[YehEmily]

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

https://github.com/apache/geode/pull/591#discussion_r123854670
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledSelect.java
 ---
@@ -925,79 +917,89 @@ private SelectResults 
applyProjectionOnCollection(SelectResults resultSet,
 
   private SelectResults prepareEmptyResultSet(ExecutionContext context, 
boolean ignoreOrderBy)
   throws TypeMismatchException, AmbiguousNameException {
-// if no projection attributes or '*'as projection attribute
-// & more than one/RunTimeIterator then create a StrcutSet.
-// If attribute is null or '*' & only one RuntimeIterator then create a
-// ResultSet.
-// If single attribute is present without alias name , then create
-// ResultSet
-// Else if more than on attribute or single attribute with alias is
-// present then return a StrcutSet
-// create StructSet which will contain root objects of all iterators in
-// from clause
+// If no projection attributes or '*' as projection attribute & more 
than one/RunTimeIterator
+// then create a StructSet.
+// If attribute is null or '*' & only one RuntimeIterator then create 
a ResultSet.
+// If single attribute is present without alias name, then create 
ResultSet.
+// Else if more than on attribute or single attribute with alias is 
present then return a
+// StructSet.
+// Create StructSet which will contain root objects of all iterators 
in from clause.
 
 ObjectType elementType = this.cachedElementTypeForOrderBy != null
 ? this.cachedElementTypeForOrderBy : prepareResultType(context);
 SelectResults results;
-if (this.distinct || !this.count) {
-  if (this.orderByAttrs != null) {
-boolean nullValuesAtStart = !((CompiledSortCriterion) 
orderByAttrs.get(0)).getCriterion();
-if (elementType.isStructType()) {
-  if (ignoreOrderBy) {
-results = this.distinct ? new LinkedStructSet((StructTypeImpl) 
elementType)
-: new SortedResultsBag(elementType, nullValuesAtStart);
 
-  } else {
-OrderByComparator comparator = this.hasUnmappedOrderByCols
-? new OrderByComparatorUnmapped(this.orderByAttrs, 
(StructTypeImpl) elementType,
-context)
-: new OrderByComparator(this.orderByAttrs, 
(StructTypeImpl) elementType, context);
-results = this.distinct ? new SortedStructSet(comparator, 
(StructTypeImpl) elementType)
-: new SortedStructBag(comparator, (StructType) 
elementType, nullValuesAtStart);
+if (!this.distinct && this.count) {
+  // Shobhit: If it's a 'COUNT' query and no End processing required 
Like for 'DISTINCT'
+  // we can directly keep count in ResultSet and ResultBag is good 
enough for that.
+  countStartQueryResult = 0;
+  return new ResultsBag(new ObjectTypeImpl(Integer.class), 1, 
context.getCachePerfStats());
+}
 
-  }
-} else {
-  if (ignoreOrderBy) {
-results =
-this.distinct ? new LinkedResultSet() : new 
SortedResultsBag(nullValuesAtStart);
+// Potential edge-case: Could this be non-null but empty?
+boolean nullValuesAtStart = orderByAttrs != null && 
!orderByAttrs.get(0).getCriterion();
+OrderByComparator comparator;
+switch (convertToStringCase(elementType, ignoreOrderBy)) {
+  case "UNORDERED DISTINCT STRUCT":
+return new StructSet((StructType) elementType);
+  case "UNORDERED DISTINCT RESULTS":
+return new ResultsSet(elementType);
+  case "UNORDERED INDISTINCT STRUCT":
+return new StructBag((StructType) elementType, 
context.getCachePerfStats());
+  case "UNORDERED INDISTINCT RESULTS":
+return new ResultsBag(elementType, context.getCachePerfStats());
+
+  case "ORDERED DISTINCT STRUCT IGNORED":
+return new LinkedStructSet((StructTypeImpl) elementType);
+  case "ORDERED INDISTINCT STRUCT IGNORED":
+return new SortedResultsBag(elementType, nullValuesAtStart);
+  case "ORDERED DISTINCT STRUCT UNIGNORED":
--- End diff --

Good suggestion - thanks! @PurelyApplied and I worked together to get rid 
of the Strings, and we're now using enums. You should be able to see these 
changes starting at around line 920 of `CompiledSelect` of the latest commit. 
We're using a naming convention that I think follows your suggestion (instead 
of `UNORDERED_DISTINCT_STRUCT_IGNORED`, for example, we just use 
`UNORDERED_DISTINCT_STRUCT`).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub 

[GitHub] geode pull request #591: GEODE-3073: Renamed OrderByComparatorUnmapped to Or...

[jhuynh1]

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

https://github.com/apache/geode/pull/591#discussion_r123388223
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledSelect.java
 ---
@@ -925,79 +917,89 @@ private SelectResults 
applyProjectionOnCollection(SelectResults resultSet,
 
   private SelectResults prepareEmptyResultSet(ExecutionContext context, 
boolean ignoreOrderBy)
   throws TypeMismatchException, AmbiguousNameException {
-// if no projection attributes or '*'as projection attribute
-// & more than one/RunTimeIterator then create a StrcutSet.
-// If attribute is null or '*' & only one RuntimeIterator then create a
-// ResultSet.
-// If single attribute is present without alias name , then create
-// ResultSet
-// Else if more than on attribute or single attribute with alias is
-// present then return a StrcutSet
-// create StructSet which will contain root objects of all iterators in
-// from clause
+// If no projection attributes or '*' as projection attribute & more 
than one/RunTimeIterator
+// then create a StructSet.
+// If attribute is null or '*' & only one RuntimeIterator then create 
a ResultSet.
+// If single attribute is present without alias name, then create 
ResultSet.
+// Else if more than on attribute or single attribute with alias is 
present then return a
+// StructSet.
+// Create StructSet which will contain root objects of all iterators 
in from clause.
 
 ObjectType elementType = this.cachedElementTypeForOrderBy != null
 ? this.cachedElementTypeForOrderBy : prepareResultType(context);
 SelectResults results;
-if (this.distinct || !this.count) {
-  if (this.orderByAttrs != null) {
-boolean nullValuesAtStart = !((CompiledSortCriterion) 
orderByAttrs.get(0)).getCriterion();
-if (elementType.isStructType()) {
-  if (ignoreOrderBy) {
-results = this.distinct ? new LinkedStructSet((StructTypeImpl) 
elementType)
-: new SortedResultsBag(elementType, nullValuesAtStart);
 
-  } else {
-OrderByComparator comparator = this.hasUnmappedOrderByCols
-? new OrderByComparatorUnmapped(this.orderByAttrs, 
(StructTypeImpl) elementType,
-context)
-: new OrderByComparator(this.orderByAttrs, 
(StructTypeImpl) elementType, context);
-results = this.distinct ? new SortedStructSet(comparator, 
(StructTypeImpl) elementType)
-: new SortedStructBag(comparator, (StructType) 
elementType, nullValuesAtStart);
+if (!this.distinct && this.count) {
+  // Shobhit: If it's a 'COUNT' query and no End processing required 
Like for 'DISTINCT'
+  // we can directly keep count in ResultSet and ResultBag is good 
enough for that.
+  countStartQueryResult = 0;
+  return new ResultsBag(new ObjectTypeImpl(Integer.class), 1, 
context.getCachePerfStats());
+}
 
-  }
-} else {
-  if (ignoreOrderBy) {
-results =
-this.distinct ? new LinkedResultSet() : new 
SortedResultsBag(nullValuesAtStart);
+// Potential edge-case: Could this be non-null but empty?
+boolean nullValuesAtStart = orderByAttrs != null && 
!orderByAttrs.get(0).getCriterion();
+OrderByComparator comparator;
+switch (convertToStringCase(elementType, ignoreOrderBy)) {
+  case "UNORDERED DISTINCT STRUCT":
+return new StructSet((StructType) elementType);
+  case "UNORDERED DISTINCT RESULTS":
+return new ResultsSet(elementType);
+  case "UNORDERED INDISTINCT STRUCT":
+return new StructBag((StructType) elementType, 
context.getCachePerfStats());
+  case "UNORDERED INDISTINCT RESULTS":
+return new ResultsBag(elementType, context.getCachePerfStats());
+
+  case "ORDERED DISTINCT STRUCT IGNORED":
+return new LinkedStructSet((StructTypeImpl) elementType);
+  case "ORDERED INDISTINCT STRUCT IGNORED":
+return new SortedResultsBag(elementType, nullValuesAtStart);
+  case "ORDERED DISTINCT STRUCT UNIGNORED":
--- End diff --

I think the ignoreOrderBy flag is a bit misleading, otherwise UNORDERED 
DISTINCT STRUCT should technically be the same as ORDERED DISTINCT STRUCT 
IGNORED (since ignoreOrderby is true).  It's almost like a "sort at data 
structure" level or delay the sort until later flag.  

If you decide to keep the naming, would it make sense to not have UNIGNORED 
but rather a no string.  So ORDERED DISTINCT STRUCT  == ORDERED DISTINCT STRUCT 
UNIGNORED


---
If your project is set up for it, you can reply to 

[GitHub] geode pull request #591: GEODE-3073: Renamed OrderByComparatorUnmapped to Or...

[PurelyApplied]

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

https://github.com/apache/geode/pull/591#discussion_r122849064
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledSelect.java
 ---
@@ -941,17 +940,16 @@ private SelectResults 
prepareEmptyResultSet(ExecutionContext context, boolean ig
 SelectResults results;
 if (this.distinct || !this.count) {
--- End diff --

This could stand to be flattened as well, although it will be more 
difficult that the above.  I could go either way on this chunk.


---
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 #591: GEODE-3073: Renamed OrderByComparatorUnmapped to Or...

[PurelyApplied]

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

https://github.com/apache/geode/pull/591#discussion_r122848112
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/cache/query/internal/CompiledSelect.java
 ---
@@ -1415,11 +1413,7 @@ public boolean evaluateCq(ExecutionContext context) 
throws FunctionDomainExcepti
 // add UNDEFINED to results only for NOT EQUALS queries
 if (this.whereClause.getType() == COMPARISON) {
   int operator = ((Filter) this.whereClause).getOperator();
-  if ((operator != TOK_NE && operator != TOK_NE_ALT)) {
-return false;
-  } else {
-return true;
-  }
+  return !(operator != TOK_NE && operator != TOK_NE_ALT);
--- End diff --

That's a lot of negatives.  `return operator == TOK_NE || operator == 
TOK_NE_ALT` read better, in my opinion.


---
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 #591: GEODE-3073: Renamed OrderByComparatorUnmapped to Or...

[YehEmily]

GitHub user YehEmily opened a pull request:

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

GEODE-3073: Renamed OrderByComparatorUnmapped to OrderByComparatorMapped

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

`OrderByComparatorUnmapped.java` actually uses mapping, which is why it 
should really be called `OrderByComparatorMapped.java`. Some refactoring was 
also done (similar to the refactoring done in [pull request 
#580](https://github.com/apache/geode/pull/580), although to a lesser extent).

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-3073-2

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

https://github.com/apache/geode/pull/591.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 #591


commit b915804d857fd3efd7fefe6af45a6a8c44745bed
Author: YehEmily 
Date:   2017-06-16T20:41:21Z

GEODE-3073: Renamed OrderByComparatorUnmapped to OrderByComparatorMapped 
and refactored the code a bit.




---
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.
---