Tim Armstrong has posted comments on this change. Change subject: IMPALA-3208: max_row_size option ......................................................................
Patch Set 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/7629/11//COMMIT_MSG Commit Message: PS11, Line 18: The default value is 512KB. I picked that number because it doesn't : increase minimum reservations *too* much but should be large enough : for almost all reasonable workloads. > what's the advantage of picking that below the default buffer size? Oh, I g Yeah exactly, it makes a big difference for small buffers (e.g. 64kb). http://gerrit.cloudera.org:8080/#/c/7629/11/be/src/exec/partitioned-aggregation-node.h File be/src/exec/partitioned-aggregation-node.h: Line 741: resource_profile_.max_row_buffer_size * 2; > if we're going to use the resource profile to compute this, any reason not Yeah these computations are redundant with the planner. I could just rip them out but in some ways it seemed nice to have this code closer to the implementation as a way to document and enforce that the planner's calculation matches the backend's logic. http://gerrit.cloudera.org:8080/#/c/7629/11/be/src/service/query-options.cc File be/src/service/query-options.cc: PS11, Line 522: Spillable > should that say "Min spillable"? Done http://gerrit.cloudera.org:8080/#/c/7629/11/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 270: 60: optional i64 max_row_size = 524288; > doesn't have to be in this commit, but it'd be nice to document the interac I expanded the comment here slightly. I think there's a distinction between the meaning of the option, which we should document clearly here, and the way it is implemented for different operators, which should be documented somewhere else. http://gerrit.cloudera.org:8080/#/c/7629/11/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: PS11, Line 323: max-size buffers > let's spell that out a bit more: Done http://gerrit.cloudera.org:8080/#/c/7629/11/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java File fe/src/main/java/org/apache/impala/planner/HashJoinNode.java: PS11, Line 235: max-size buffers > same Done -- To view, visit http://gerrit.cloudera.org:8080/7629 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic70f6dddbcef124bb4b329ffa2e42a74a1826570 Gerrit-PatchSet: 11 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
