impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5648: fix count(*) mem estimate regression
Date Thu, 24 Aug 2017 00:25:40 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5648: fix count(*) mem estimate regression

Patch Set 3:

File fe/src/main/java/org/apache/impala/planner/

Line 131:   // overhead of MemPools, RowBatches, etc. This is only used if there is no data
> You say this is only used if there is no data being scanned, but I don't se
Fair point. I reworded the comment. I think just setting a floor on the memory consumption
is simplest. It mostly won't matter that much, it's just that outputting 0 looks "wrong".

Line 136:   private static final long MIN_MEMORY_ESTIMATE = 1 * 1024 * 1024;
> final static (for consistency)
I'd never noticed that we used the other way. "final static" sounds wrong to me. Looks both
crop up. I'll make this file consistent at least.

  tarmstrong@tarmstrong-box:~/Impala/incubator-impala$ git grep 'static final' | wc -l
  tarmstrong@tarmstrong-box:~/Impala/incubator-impala$ git   grep 'final static' | wc -l

Line 1046:         if ((slot.getColumn() == null ||
> How about this instead:

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf5c2316bef2afae54a94245c715534ed294f286
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message