impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3208: max row size option
Date Mon, 21 Aug 2017 21:50:59 GMT
Dan Hecht 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 guess the reason
is it puts a limit on the savings the scaled down buffer size optimization?


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 to just get the
reservation value directly from there rather than recomputing?


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"?


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 interaction of all the
query options that affect memory reservations. that should probably go in the docs, but may
also help as a comment in the code somewhere.


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:

... large enough to hold the maximum-sized row ...

or similar.


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


-- 
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message