Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14859 )
Change subject: IMPALA-4224: execute separate join builds fragments ...................................................................... Patch Set 40: Code-Review+1 (5 comments) Went through the java part and tests. Only found some potential beautifications. I also went through the c++ part in a past PS, it seemed ok though I didn't dive into it deep enough. http://gerrit.cloudera.org:8080/#/c/14859/38/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/14859/38/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@304 PS38, Line 304: Preconditions.checkState(sink_ instanceof JoinBuildSink); Check in the loops looks a bit weird to me - I would prefer to check it outside even if it makes the code longer. http://gerrit.cloudera.org:8080/#/c/14859/38/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@407 PS38, Line 407: Preconditions.checkState(perInstanceResourceProfile_.isValid()); : Preconditions.checkState(perBackendResourceProfile_.isValid()); : Preconditions.checkArgument(perInstanceInitialMemReservationTotalClaims_ > -1); : Preconditions.checkArgument(perBackendInitialMemReservationTotalClaims_ > -1); : Preconditions.checkArgument(producedRuntimeFiltersMemReservationBytes_ > -1); : Preconditions.checkArgument(consumedGlobalRuntimeFiltersMemReservationBytes_ > -1); I think that this is not the best place for these checks - we combine two invariants: 1. toThrift() should be only called after computeResourceProfile() 2. all of these resource estimates should be filled at the end of computeResourceProfile() Note that there is a comment around line 67 that describes a state machine with a function called finalize() which doesn't exist. It would be nice to update the comment. http://gerrit.cloudera.org:8080/#/c/14859/40/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/14859/40/fe/src/main/java/org/apache/impala/planner/PlanNode.java@476 PS40, Line 476: // ExchangeNodes and separated join builds) to have no children. : if (this instanceof ExchangeNode) { : msg.num_children = 0; : return; : } else if (this instanceof JoinNode && ((JoinNode)this).hasSeparateBuild()) { : // The join build is in a separate fragment so only the left child should be : // serialized. : msg.num_children = 1; : Preconditions.checkState(children_.size() == 2); : children_.get(0).treeToThriftHelper(container); : } else { : msg.num_children = children_.size(); : for (PlanNode child: children_) { : child.treeToThriftHelper(container); : } : } Can't we generalize this by checking if fragment_ is the same as in the child? http://gerrit.cloudera.org:8080/#/c/14859/40/fe/src/main/java/org/apache/impala/planner/ResourceProfile.java File fe/src/main/java/org/apache/impala/planner/ResourceProfile.java: http://gerrit.cloudera.org:8080/#/c/14859/40/fe/src/main/java/org/apache/impala/planner/ResourceProfile.java@157 PS40, Line 157: combine Is sum() still useful after creating combine? Are there case where specifically want buffer sizes to be undefined? http://gerrit.cloudera.org:8080/#/c/14859/40/fe/src/main/java/org/apache/impala/planner/ResourceProfile.java@172 PS40, Line 172: returns the max We return invalid instead of max if I understand correctly -- To view, visit http://gerrit.cloudera.org:8080/14859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4403c8e62d9c13854e7830602ee613f8efc80c58 Gerrit-Change-Number: 14859 Gerrit-PatchSet: 40 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 13 Feb 2020 12:40:39 +0000 Gerrit-HasComments: Yes
