impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3208: max row size option
Date Thu, 17 Aug 2017 21:55:32 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3208: max_row_size option

Patch Set 10:


Starting with my first high level question as I may be missing something
File common/thrift/PlanNodes.thrift:

PS10, Line 503:   4: optional i64 max_row_buffer_size
              : }
maybe this will become clear to me as I review more code, but why does this have to go in
this struct? Does it really be different for different nodes? I think it'd be easier to reason
about if you didn't have to think about this extra per-node resource knob.
File fe/src/main/java/org/apache/impala/planner/

PS10, Line 251:     // Analytic uses a single buffer size.
Why? It looks like every other node has the same behavior - can this just do the same? Then
we could avoid making the max row buffer bytes a new per-node option (i.e. it'd be the same
File fe/src/main/java/org/apache/impala/planner/

PS10, Line 66: setMemEstimateBytes(MIN_PER_HOST_MEM_ESTIMATE_BYTES)
             :       .setMinReservationBytes(0)
I think you could give this last line another tab, for good measure

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic70f6dddbcef124bb4b329ffa2e42a74a1826570
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message