This is an automated email from the ASF dual-hosted git repository.
stigahuang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
The following commit(s) were added to refs/heads/master by this push:
new 04608452d IMPALA-13226: Rename TupleCacheInfo.finalize() to
finalizeHash()
04608452d is described below
commit 04608452d37edc9256e368bec69b23c9e989b443
Author: stiga-huang <[email protected]>
AuthorDate: Tue Jul 16 16:32:40 2024 +0800
IMPALA-13226: Rename TupleCacheInfo.finalize() to finalizeHash()
TupleCacheInfo.finalize() unintentionally overwrites Object.finalize()
which is called by the JVM garbage collector when garbage collection
determines that there are no more references to the object. Usually the
finalize method is overrided to dispose of system resources or to
perform other cleanup.
TupleCacheInfo.finalize() is not meant to be used during GC. We'd better
use another method name to avoid confusion. This patch renames it to
finalizeHash(). Also fixed some stale comments.
Change-Id: I657c4f14b074b7c16dc7d126b0c8b5083b8f19c6
Reviewed-on: http://gerrit.cloudera.org:8080/21588
Reviewed-by: Impala Public Jenkins <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>
---
.../org/apache/impala/planner/PlanFragment.java | 2 +-
.../java/org/apache/impala/planner/PlanNode.java | 4 ++--
.../org/apache/impala/planner/TupleCacheInfo.java | 13 +++++------
.../apache/impala/planner/TupleCacheInfoTest.java | 26 ++++++++++------------
4 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/fe/src/main/java/org/apache/impala/planner/PlanFragment.java
b/fe/src/main/java/org/apache/impala/planner/PlanFragment.java
index 1c7f68a5f..696562ba6 100644
--- a/fe/src/main/java/org/apache/impala/planner/PlanFragment.java
+++ b/fe/src/main/java/org/apache/impala/planner/PlanFragment.java
@@ -100,7 +100,7 @@ public class PlanFragment extends TreeNode<PlanFragment> {
// exchange node or join node to which this fragment sends its output
private PlanNode destNode_;
- // created in finalize() or set in setSink()
+ // created in finalizeExchanges() or set in setSink()
private DataSink sink_;
// specification of the partition of the input of this fragment;
diff --git a/fe/src/main/java/org/apache/impala/planner/PlanNode.java
b/fe/src/main/java/org/apache/impala/planner/PlanNode.java
index 62c1a108d..3f95e8b8c 100644
--- a/fe/src/main/java/org/apache/impala/planner/PlanNode.java
+++ b/fe/src/main/java/org/apache/impala/planner/PlanNode.java
@@ -1331,7 +1331,7 @@ abstract public class PlanNode extends TreeNode<PlanNode>
{
// If we are already ineligible, there is no need to process the Thrift
if (!tupleCacheInfo_.isEligible()) {
- tupleCacheInfo_.finalize();
+ tupleCacheInfo_.finalizeHash();
return;
}
@@ -1343,7 +1343,7 @@ abstract public class PlanNode extends TreeNode<PlanNode>
{
initThrift(msg, serialCtx);
toThrift(msg, serialCtx);
tupleCacheInfo_.hashThrift(msg);
- tupleCacheInfo_.finalize();
+ tupleCacheInfo_.finalizeHash();
}
/**
diff --git a/fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
b/fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
index b02ee322e..63f867cb2 100644
--- a/fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
+++ b/fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java
@@ -40,7 +40,6 @@ import org.apache.thrift.protocol.TBinaryProtocol;
import com.google.common.base.Preconditions;
import com.google.common.hash.Hasher;
-import com.google.common.hash.HashCode;
import com.google.common.hash.Hashing;
/**
@@ -81,8 +80,8 @@ import com.google.common.hash.Hashing;
* Anything hashed will have a representation incorporated into the trace.
*
* This accumulates information from various sources, then it is finalized and
cannot
- * be modified further. The hash key and hash trace cannot be accessed until
finalize()
- * is called.
+ * be modified further. The hash key and hash trace cannot be accessed until
+ * finalizeHash() is called.
*/
public class TupleCacheInfo {
// Keep track of the reasons why a location in the plan is ineligible. This
may be
@@ -108,13 +107,13 @@ public class TupleCacheInfo {
private final IdGenerator<SlotId> translatedSlotIdGenerator_ =
SlotId.createGenerator();
- // These fields accumulate partial results until finalize() is called.
+ // These fields accumulate partial results until finalizeHash() is called.
private Hasher hasher_ = Hashing.murmur3_128().newHasher();
// The hash trace keeps a human-readable record of the items hashed into the
cache key.
private StringBuilder hashTraceBuilder_ = new StringBuilder();
- // When finalize() is called, these final values are filled in and the
hasher and
+ // When finalizeHash() is called, these final values are filled in and the
hasher and
// hash trace builder are destroyed.
private boolean finalized_ = false;
private String finalizedHashTrace_ = null;
@@ -152,9 +151,9 @@ public class TupleCacheInfo {
/**
* Finish accumulating information and calculate the final hash value and
* hash trace. This must be called before accessing the hash or hash trace.
- * No further modifications can be made after calling finalize().
+ * No further modifications can be made after calling finalizeHash().
*/
- public void finalize() {
+ public void finalizeHash() {
finalizedHashString_ = hasher_.hash().toString();
hasher_ = null;
finalizedHashTrace_ = hashTraceBuilder_.toString();
diff --git a/fe/src/test/java/org/apache/impala/planner/TupleCacheInfoTest.java
b/fe/src/test/java/org/apache/impala/planner/TupleCacheInfoTest.java
index e1a798b4e..8059b3f02 100644
--- a/fe/src/test/java/org/apache/impala/planner/TupleCacheInfoTest.java
+++ b/fe/src/test/java/org/apache/impala/planner/TupleCacheInfoTest.java
@@ -23,9 +23,7 @@ import static org.junit.Assert.assertTrue;
import org.apache.impala.analysis.DescriptorTable;
import org.apache.impala.analysis.SlotDescriptor;
-import org.apache.impala.analysis.SlotId;
import org.apache.impala.analysis.TupleDescriptor;
-import org.apache.impala.analysis.TupleId;
import org.apache.impala.catalog.PrimitiveType;
import org.apache.impala.catalog.ScalarType;
import org.apache.impala.thrift.TUniqueId;
@@ -42,11 +40,11 @@ public class TupleCacheInfoTest {
// This test doesn't need a DescriptorTable, so it just sets it to null.
TupleCacheInfo info1 = new TupleCacheInfo(null);
info1.hashThrift(new TUniqueId(1L, 2L));
- info1.finalize();
+ info1.finalizeHash();
TupleCacheInfo info2 = new TupleCacheInfo(null);
info2.hashThrift(new TUniqueId(1L, 2L));
- info2.finalize();
+ info2.finalizeHash();
assertEquals(info1.getHashTrace(), "TUniqueId(hi:1, lo:2)");
assertEquals(info1.getHashTrace(), info2.getHashTrace());
@@ -60,17 +58,17 @@ public class TupleCacheInfoTest {
// This test doesn't need a DescriptorTable, so it just sets it to null.
TupleCacheInfo child1 = new TupleCacheInfo(null);
child1.hashThrift(new TUniqueId(1L, 2L));
- child1.finalize();
+ child1.finalizeHash();
TupleCacheInfo child2 = new TupleCacheInfo(null);
child2.hashThrift(new TUniqueId(3L, 4L));
- child2.finalize();
+ child2.finalizeHash();
TupleCacheInfo parent = new TupleCacheInfo(null);
parent.mergeChild(child1);
parent.mergeChild(child2);
parent.hashThrift(new TUniqueId(5L, 6L));
- parent.finalize();
+ parent.finalizeHash();
assertEquals(parent.getHashTrace(),
"TUniqueId(hi:1, lo:2)TUniqueId(hi:3, lo:4)TUniqueId(hi:5, lo:6)");
@@ -84,13 +82,13 @@ public class TupleCacheInfoTest {
// Child 1 is eligible
TupleCacheInfo child1 = new TupleCacheInfo(null);
child1.hashThrift(new TUniqueId(1L, 2L));
- child1.finalize();
+ child1.finalizeHash();
assertTrue(child1.isEligible());
// Child 2 is ineligible
TupleCacheInfo child2 = new TupleCacheInfo(null);
child2.setIneligible(TupleCacheInfo.IneligibilityReason.NOT_IMPLEMENTED);
- child2.finalize();
+ child2.finalizeHash();
assertTrue(!child2.isEligible());
TupleCacheInfo parent = new TupleCacheInfo(null);
@@ -98,9 +96,9 @@ public class TupleCacheInfoTest {
// Still eligible after adding child1 without child2
assertTrue(parent.isEligible());
parent.mergeChild(child2);
- // It is allowed to check eligibility before finalize()
+ // It is allowed to check eligibility before finalizeHash()
assertTrue(!parent.isEligible());
- parent.finalize();
+ parent.finalizeHash();
assertTrue(!parent.isEligible());
}
@@ -129,7 +127,7 @@ public class TupleCacheInfoTest {
TupleCacheInfo child1 = new TupleCacheInfo(descTbl);
child1.hashThrift(new TUniqueId(1L, 2L));
child1.registerTuple(tuple1.getId());
- child1.finalize();
+ child1.finalizeHash();
assertEquals(child1.getLocalTupleId(tuple1.getId()).asInt(), 0);
assertEquals(child1.getLocalSlotId(t1slot.getId()).asInt(), 0);
String child1ExpectedHashTrace = "TUniqueId(hi:1, lo:2)" +
@@ -145,7 +143,7 @@ public class TupleCacheInfoTest {
TupleCacheInfo child2 = new TupleCacheInfo(descTbl);
child2.hashThrift(new TUniqueId(1L, 2L));
child2.registerTuple(tuple2.getId());
- child2.finalize();
+ child2.finalizeHash();
// Note: we expect the id's to be translated to local ids, so even though
this is
// tuple 2 and slot 2, this will still have TupleId=0 and SlotId=0. In
fact, at this
// point the only difference between child1 and child2 is the TUniqueId.
@@ -160,7 +158,7 @@ public class TupleCacheInfoTest {
TupleCacheInfo parent = new TupleCacheInfo(descTbl);
parent.mergeChild(child2);
parent.mergeChild(child1);
- parent.finalize();
+ parent.finalizeHash();
// Tuple1 = second index
// Tuple2 = first index
