impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
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:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7783/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

Line 131:   // overhead of MemPools, RowBatches, etc. This is only used if there is no data
being
> 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
  232
  tarmstrong@tarmstrong-box:~/Impala/incubator-impala$ git   grep 'final static' | wc -l
  176


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


-- 
To view, visit http://gerrit.cloudera.org:8080/7783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf5c2316bef2afae54a94245c715534ed294f286
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message