Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/23219 )
Change subject: IMPALA-13437 (part 2): Implement cost-based tuple cache placement ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/23219/8/fe/src/main/java/org/apache/impala/planner/TupleCacheCostBasedPolicy.java File fe/src/main/java/org/apache/impala/planner/TupleCacheCostBasedPolicy.java: http://gerrit.cloudera.org:8080/#/c/23219/8/fe/src/main/java/org/apache/impala/planner/TupleCacheCostBasedPolicy.java@121 PS8, Line 121: new Double new Double() has been deprecated since Java 9, maybe use Double.valueOf() instead. https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Double.html http://gerrit.cloudera.org:8080/#/c/23219/8/fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java File fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java: http://gerrit.cloudera.org:8080/#/c/23219/8/fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java@260 PS8, Line 260: Preconditions.checkState(isEligible(), : "TupleCacheInfo only has cost information if it is cache eligible."); : Preconditions.checkState(finalized_, "TupleCacheInfo not finalized"); It seems we can create a private method for all these checks in multiple methods to reduce the duplication and easy to maintain. private void checkFinalizedAndEligible() { Preconditions.checkState(isEligible(), "TupleCacheInfo only has this information if it is cache eligible."); Preconditions.checkState(finalized_, "TupleCacheInfo not finalized"); } public long getEstimatedSerializedSizePerNode() { checkFinalizedAndEligible(); return estimatedSerializedSizePerNode_; } http://gerrit.cloudera.org:8080/#/c/23219/8/fe/src/main/java/org/apache/impala/planner/TupleCacheInfo.java@316 PS8, Line 316: thisPlanNode.getNumNodes() Do we wanna assert the thisPlanNode.getNumNodes() always larger than 0 here? Or handle that case? http://gerrit.cloudera.org:8080/#/c/23219/8/fe/src/main/java/org/apache/impala/planner/TupleCachePlanner.java File fe/src/main/java/org/apache/impala/planner/TupleCachePlanner.java: http://gerrit.cloudera.org:8080/#/c/23219/8/fe/src/main/java/org/apache/impala/planner/TupleCachePlanner.java@38 PS8, Line 38: * The current algorithm is to add a TupleCacheNode at every eligible location. This will : * need to be refined with cost calculations later. We should update the comments for TupleCacheCostBasedPolicy and TupleCacheAllEligiblePolicy -- To view, visit http://gerrit.cloudera.org:8080/23219 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifc6e7b95621a7937d892511dc879bf7c8da07cdc Gerrit-Change-Number: 23219 Gerrit-PatchSet: 8 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Tue, 09 Sep 2025 01:39:01 +0000 Gerrit-HasComments: Yes
