[drill] 03/03: Changes following second code review

[bo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

boaz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/drill.git

commit fc1eacda8b181934eea811e5f228fff697f94549
Author: Ben-Zvi <bb...@mapr.com>
AuthorDate: Tue Jan 8 18:40:54 2019 -0800

    Changes following second code review
---
 .../exec/physical/impl/common/ChainedHashTable.java      | 16 ++++++++--------
 .../exec/physical/impl/common/HashTableTemplate.java     |  7 +++++--
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/ChainedHashTable.java b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/ChainedHashTable.java
index baed508..e7abd98 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/ChainedHashTable.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/ChainedHashTable.java
@@ -130,7 +130,7 @@ public class ChainedHashTable {
   private RecordBatch incomingProbe;
   private final RecordBatch outgoing;
 
-  private enum setupWork {DO_BUILD, DO_PROBE, CHECK_BOTH_NULLS};
+  private enum SetupWork {DO_BUILD, DO_PROBE, CHECK_BOTH_NULLS};
 
   public ChainedHashTable(HashTableConfig htConfig, FragmentContext context, BufferAllocator allocator,
                           RecordBatch incomingBuild, RecordBatch incomingProbe, RecordBatch outgoing) {
@@ -223,12 +223,12 @@ public class ChainedHashTable {
     // (used by Hash-Join to avoid creating a long hash-table chain of null keys, which can lead to useless O(n^2) work on that chain.)
     // The logic is: Nulls match on build, and don't match on probe. Note that this logic covers outer joins as well.
     setupIsKeyMatchInternal(cgInner, bothKeysNullIncomingBuildMapping, bothKeysNullHtableMapping, keyExprsBuild,
-        htConfig.getComparators(), htKeyFieldIds, setupWork.CHECK_BOTH_NULLS);
+        htConfig.getComparators(), htKeyFieldIds, SetupWork.CHECK_BOTH_NULLS);
     // generate code for isKeyMatch(), setValue(), getHash() and outputRecordKeys()
     setupIsKeyMatchInternal(cgInner, KeyMatchIncomingBuildMapping, KeyMatchHtableMapping, keyExprsBuild,
-        htConfig.getComparators(), htKeyFieldIds, setupWork.DO_BUILD);
+        htConfig.getComparators(), htKeyFieldIds, SetupWork.DO_BUILD);
     setupIsKeyMatchInternal(cgInner, KeyMatchIncomingProbeMapping, KeyMatchHtableProbeMapping, keyExprsProbe,
-        htConfig.getComparators(), htKeyFieldIds, setupWork.DO_PROBE);
+        htConfig.getComparators(), htKeyFieldIds, SetupWork.DO_PROBE);
 
     setupSetValue(cgInner, keyExprsBuild, htKeyFieldIds);
     if (outgoing != null) {
@@ -249,9 +249,9 @@ public class ChainedHashTable {
   }
 
   private void setupIsKeyMatchInternal(ClassGenerator<HashTable> cg, MappingSet incomingMapping, MappingSet htableMapping,
-      LogicalExpression[] keyExprs, List<Comparator> comparators, TypedFieldId[] htKeyFieldIds, setupWork work) {
+      LogicalExpression[] keyExprs, List<Comparator> comparators, TypedFieldId[] htKeyFieldIds, SetupWork work) {
 
-    boolean checkIfBothNulls = work == setupWork.CHECK_BOTH_NULLS;
+    boolean checkIfBothNulls = work == SetupWork.CHECK_BOTH_NULLS;
 
     // Regular key matching may return false in the middle (i.e., some pair of columns did not match), and true only if all matched;
     // but "both nulls" check returns the opposite logic (i.e., true when one pair of nulls is found, need check no more)
@@ -277,7 +277,7 @@ public class ChainedHashTable {
 
       JConditional jc;
 
-      if ( work != setupWork.DO_BUILD ) {  // BUILD runs this logic in a separate method - areBothKeysNull()
+      if ( work != SetupWork.DO_BUILD ) {  // BUILD runs this logic in a separate method - areBothKeysNull()
         // codegen for the special case when both columns are null (i.e., return early with midPointResult)
         if (comparators.get(i) == Comparator.EQUALS
             && left.isOptional() && right.isOptional()) {
@@ -298,7 +298,7 @@ public class ChainedHashTable {
       }
     }
 
-    // All key expressions compared equal, so return TRUE
+    // All key expressions compared the same way, so return the appropriate final result
     cg.getEvalBlock()._return(finalResult);
   }
 
diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashTableTemplate.java b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashTableTemplate.java
index a064449..1d83239 100644
--- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashTableTemplate.java
+++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/HashTableTemplate.java
@@ -230,8 +230,11 @@ public abstract class HashTableTemplate implements HashTable {
       return isKeyMatchInternalBuild(incomingRowIdx, currentIdxWithinBatch);
     }
 
-    // This method should only be called following a "false" return from isKeyMatch()
-    // It returns the next index in the hash chain (if any) so that next call to isKeyMatch() would compare the next link
+    // This method should only be used in an "iterator like" next() fashion, to traverse a hash table chain looking for a match.
+    // Starting from the first element (i.e., index) in the chain, _isKeyMatch()_ should be called on that element; if "false" is returned,
+    // then this method should be called to return the (index to the) next element in the chain (or an EMPTY_SLOT to terminate), and then
+    // _isKeyMatch()_ should be called on that next element; and so on until a match is found - where the loop is exited with the found result.
+    // (This was not implemented as a real Java iterator as each index may point to another BatchHolder).
     private int nextLinkInHashChain(int currentIndex) {
       return links.getAccessor().get(currentIndex & BATCH_MASK);
     }