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 <[email protected]> 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); }
