[Impala-ASF-CR] IMPALA-3360: Codegen inserting into runtime filters

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3360: Codegen inserting into runtime filters
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8029/2/be/src/exec/filter-context.cc
File be/src/exec/filter-context.cc:

Line 217: 
> You're right, we should be able to cross-compile Insert() and substitute in
So I've been working on this, but it turns out to be tricky. I uploaded a new 
patch, but it doesn't fully work.

One issue is that the function returned by ScalarExpr::GetCodegendComputeFn() 
can't be used as a drop-in replacement for ScalarExprEvaluator::GetValue() as 
they have different return types (AnyVal and void* respectively). I solved this 
with ScalarExpr::GetCodegendComputePtrFn(), but it means keeping a lot of the 
IRBuilder code that's here.

The other issue is how to deal with making the type argument to GetHashValue() 
a constant. As far as I can tell, we don't already have any tools for directly 
replacing arguments to functions.

The latest version of the patch defines a function GetType(), called in 
FilterContext::Insert(), that I replace with ReplaceCallSitesWithValue(), but 
this doesn't work (in particular, the values to be hashed that are passed to 
GetHashValue() appear to be corrupted, and from dumping the IR the branching 
isn't eliminated anyways), and I'm not sure if its even reasonable to expect it 
to work since we don't use ReplaceCallSitesWithValue() in that way anywhere 
else.

I also considered making something like RawValue::CodegenGetHashValue() which 
would take the type and return a codegen'd function that can be used to replace 
GetHashValue() with ReplaceCallSites() (ie. the codegen'd function would still 
also take a type argument but would ignore it), but this would be a bunch more 
IRBuilder (more even than the original version of the patch, I think), so I 
wanted to see if that approach makes sense before doing it.


http://gerrit.cloudera.org:8080/#/c/8029/2/be/src/util/bloom-filter.h
File be/src/util/bloom-filter.h:

Line 135:   // Same as Insert(), but skips the CPU check and assumes that AVX 
is not available.
> naming nit: We actually need AVX2 -- AVX won't cut it
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3360: Codegen inserting into runtime filters

[Thomas Tauber-Marshall (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8029

to look at the new patch set (#3).

Change subject: IMPALA-3360: Codegen inserting into runtime filters
..

IMPALA-3360: Codegen inserting into runtime filters

This patch codegens PhjBuilder::InsertRuntimeFilters() and
FilterContext::Insert().

This allows us to unroll the loop over all the filters in
PhjBuilder::ProcessBuildBatch(), eliminate the branch on type that
happens in RawValue::GetHashValue(), and eliminate the AVX check
that happens in BloomFilter::Insert().

Testing:
- Ran existing runtime filter tests.
- Ran perf tests locally (all avg. over three runs):
  - Four way self join on tpch_parquet.lineitem. Should be a good case
for this as there's several large hash join build sides that will
benefit from the codegen. Total query running time improved ~7%
(from 16.07s to 14.91s).
  - Single join of tpch_parquet.lineitem against a selectively
filtered tpch_parquet.lineitem. Should be a bad case for this
patch, as the build side of the join is very small. Total query
running time regressed by about ~2% (from 0.73s to 0.75s) due to
an increase in codegen time (from 295ms to 309ms for the fragment
containing the hash join).

Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/exec/CMakeLists.txt
M be/src/exec/filter-context.cc
M be/src/exec/filter-context.h
M be/src/exec/partitioned-hash-join-builder-ir.cc
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-evaluator.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M be/src/util/CMakeLists.txt
A be/src/util/bloom-filter-ir.cc
M be/src/util/bloom-filter.h
15 files changed, 244 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8029/3
-- 
To view, visit http://gerrit.cloudera.org:8080/8029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5932: Improve the transitive closure computation performance in value transfer graph

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-5932: Improve the transitive closure computation 
performance in value transfer graph
..


Patch Set 1:

(4 comments)

nice

http://gerrit.cloudera.org:8080/#/c/8098/1//COMMIT_MSG
Commit Message:

Line 7: IMPALA-5932: Improve the transitive closure computation performance in 
value transfer graph
Shrink to fit


Line 9: This patch implements Floyd-Warshall algorithm for the transitive
the Floyd-Warshall algorithm


Line 10: closure computation in value transfer graph, replacing the existing N^4
closure computation for the value transfer graph


http://gerrit.cloudera.org:8080/#/c/8098/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

Line 2703: if (!valueTransfer_[p[i]][p[j]]) continue;
Comment that this optimization works because our graphs are typically sparse


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change.

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..


Patch Set 5: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8035/5/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java:

Line 23: import org.apache.hadoop.classification.InterfaceStability.Unstable;
These don't look pretty, since we own this now I guess they could disappear 
(along with the @Private @Unstable annotations below).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5932: Improve the transitive closure computation performance in value transfer graph

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8098

Change subject: IMPALA-5932: Improve the transitive closure computation 
performance in value transfer graph
..

IMPALA-5932: Improve the transitive closure computation performance in value 
transfer graph

This patch implements Floyd-Warshall algorithm for the transitive
closure computation in value transfer graph, replacing the existing N^4
brute force algorithm.
The performance improvement depends on the size and structure of the
value transfer graph. On a random graph with 800 slots and 2800 edges it
is 43X faster itself. And the "Equivalence classes computed" event in
the runtime profile becomes 21X faster.
This computation is covered by the existing tests, which verifies the
equivalency of the new and the old value transfer graphs.

Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
1 file changed, 7 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/8098/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8098
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change.

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java:

Line 64:   private final Map> 
queueAcls;
> I don't think it's worth changing this one otherwise I'll have to modify a 
That's fine.  We may even want extra ACLs sometime in the future - I can see a 
case for CANCEL_OTHERS


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java:

Line 30: public class AllocationConfigurationException extends Exception {
> We still use the resource parsing code that throws this, e.g. in FairSchedu
Fair enough.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java:

Line 103:   public void serviceInit(Configuration conf) throws Exception {
> Yeah you're right, though I'm on the fence about how much we should doctor 
Can you file a JIRA?  This is actually a well compartmentalized ramp-up or 
beginner task (that even someone from outside Cloudera could do)


Line 223: Map minSharePreemptionTimeouts = new HashMap<>();
> I think it may be worth leaving this code as-is even if it's doing extra wo
Seems fine.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java:

Line 70: Pattern pattern = Pattern.compile("(\\d+)(\\.\\d*)?\\s*" + units);
> Agreed, though I don't think it's worth spending time improving this code. 
Nope.  I have zero insight into how often this is called.  Obviously if it ends 
up being called every time we need to check a memory reservation it might be a 
problem, but that seems very unlikely.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java:

Line 57:   
Is it worth fixing the trailing spaces so future diffs don't give the linter 
hives?


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java:

Line 25: return DEFAULT_POLICY;
> Per my other comments, I'd prefer not to change the parsing code too much y
Seems fine.


http://gerrit.cloudera.org:8080/#/c/8035/4/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java
File fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java:

Line 124: new Configuration().get(HADOOP_SECURITY_AUTH_TO_LOCAL, 
"DEFAULT"));
> I'm actually not sure why the testResolvePrincipalName() was working before
If I had to guess, I'd guess that kerberos tests were flaky and got disabled or 
marked xfailed.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

[Matthew Jacobs (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8035

to look at the new patch set (#5).

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..

IMPALA-5920: Remove admission control dependency on YARN RM jar

Impala's admission controller relies on the YARN
fair-scheduler.xml for configuration. That configuration is
loaded using YARN directly (ie. as a library by the
frontend). In Hadoop 3, a number of changes were made to the
YARN resourcemanager which break Impala. While we eventually
want to rethink the admission control configuration
(IMPALA-4159), in the meantime we at least should avoid
using unsupported YARN APIs.

This patch removes the fe dependency on the YARN artifact
'hadoop-yarn-server-resourcemanager' which contains private
APIs and isn't meant to be used as a library.

A subset of the required code has been added in
'common/yarn-extras', taken from Hadoop 2.6.0 in CDH, e.g.
see [1]. The code is added in packages 'org.apache.impala.*'
instead of 'org.apache.yarn.*'. Some code could be copied
as-is, those files are marked with the comment:
//YARNUTIL: VANILLA
Files that required some modifications are marked with:
//YARNUTIL: MODIFIED
Or, if all code except a dummy interface could be added:
//YARNUTIL: DUMMY IMPL

The goal the 'yarn-extras' is to make Impala's handling of
the AC configuration self-sufficient, i.e. it shouldn't
matter what version of Hadoop exists. As-is, this was tested
and found to work when Hadoop 2.6 is installed. Because
the yarn-extras/pom.xml still references hadoop-common,
hadoop-yarn-common, and hadoop-yarn-api, additional testing
will be required to ensure Impala using yarn-extras works
when installed along side Hadoop 3.

That testing for Hadoop 3 will be done later. Future changes
will make any other changes required for existing code to
work when Hadoop 3 is installed.

Testing:
* Ran existing tests on master.

1: 
https://www.cloudera.com/documentation/enterprise/release-notes/topics/cm_vd_cdh_package_tarball_512.html

Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
---
M CMakeLists.txt
A common/yarn-extras/CMakeLists.txt
A common/yarn-extras/README.txt
A common/yarn-extras/pom.xml
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/resource/ResourceWeights.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FSQueueType.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementPolicy.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/utils/BuilderUtils.java
M fe/CMakeLists.txt
M fe/pom.xml
M fe/src/main/java/org/apache/impala/util/RequestPoolService.java
M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java
18 files changed, 1,690 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/8035/5
-- 
To view, visit http://gerrit.cloudera.org:8080/8035
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..


Patch Set 4:

(13 comments)

Thanks, Zach - some good observations. I made a bunch of the changes for 
additional cleanup that you suggested, but some I held off on because I don't 
wanna make diffing the more complex classes harder later. We can always choose 
to simplify this further later, if we want. Let me know if you feel strongly 
about it.

http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java:

Line 61:   private final float queueMaxAMShareDefault;
> Impala doesn't make use of YARN's concept of ApplicationMasters.  Why not k
Done


Line 64:   private final Map> 
queueAcls;
> Useless (just SUBMIT_APPLICATIONS)
I don't think it's worth changing this one otherwise I'll have to modify a 
bunch of code that I don't wanna bother adding extra tests for.


Line 69:   private final Map minSharePreemptionTimeouts;
> Unused by Impala
Done


Line 74:   private final Map fairSharePreemptionTimeouts;
> Also unused by Impala
Done


Line 80:   private final Map fairSharePreemptionThresholds;
> Also unused
Done


Line 82:   private final Map schedulingPolicies;
> The only policy is DEFAULT_POLICY so this is also not needed.
Done


Line 84:   private final SchedulingPolicy defaultSchedulingPolicy;
> Ditto
Done


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java:

Line 30: public class AllocationConfigurationException extends Exception {
> The only point of this class is capturing configuration exceptions and thro
We still use the resource parsing code that throws this, e.g. in 
FairSchedulerConfiguration. I don't think we should change the API of those 
functions.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java:

Line 103:   public void serviceInit(Configuration conf) throws Exception {
> 103-165 duplicate code and functionality provided by impala.util.FileWatchS
Yeah you're right, though I'm on the fence about how much we should doctor this 
right now given we know this works and that we don't have great automated 
end-to-end test coverage.

Unless you feel strongly about cleaning up this now, I'd rather leave this as a 
cleanup opportunity for later (ramp up JIRA?) when we can add some additional 
tests which include modifying the allocation file.


Line 223: Map minSharePreemptionTimeouts = new HashMap<>();
> Preemption configuration does nothing in Impala; could also be killed.
I think it may be worth leaving this code as-is even if it's doing extra work. 
The AllocationConfiguration will ignore the things we don't care about, but 
leaving this as-is will make diff-ing this later much easier.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java:

Line 70: Pattern pattern = Pattern.compile("(\\d+)(\\.\\d*)?\\s*" + units);
> This seems a pretty inefficient way to look up resources. This would be a c
Agreed, though I don't think it's worth spending time improving this code. I'd 
rather not make diffing harder later. Do you feel strongly?


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java:

Line 25: return DEFAULT_POLICY;
> This whole class is pretty useless and in Impala's case, only serves to thr
Per my other comments, I'd prefer not to change the parsing code too much yet, 
in which case I think it'll just be worth keeping this dummy impl here for now.


http://gerrit.cloudera.org:8080/#/c/8035/4/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java
File fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java:

Line 124: new 

[Impala-ASF-CR] IMPALA-5941: Fix Metastore schema creation in create-test-configuration.sh

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5941: Fix Metastore schema creation in 
create-test-configuration.sh
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 1952: 
> what is the purpose of this? I think we should avoid shared_from_this() unl
Since the Thrift classes expect a boost::shared_ptr, we need to pass in a 
boost::shared_ptr to the interface of 
ImpalaHiveServer2ServiceProcessor, etc.

shared_from_this() is basically a shared_ptr version of the 'this' pointer. So 
this looks like the cleanest way to do this.


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS1, Line 123: thrift_
> is that the thrift impala internal service port (as opposed to to krpc veri
It's the thrift internal service port. I renamed it to thrift_be_port.


Line 999:   /// Init() were <= 0.
> can you comment on who owns these? even better, if this class owns them, us
Changed to scoped_ptr.


PS1, Line 1002: ptr since krpc is on the way, how about thrift_be_server_? this goes away once 
Yes, that's right. I've renamed it.


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

PS1, Line 86: boost::shared_ptr impala_server
> why do we need shared ownership of this thing? who else needs to keep it al
Yes, they are a consequence of the Thrift interface expecting a 
boost::shared_ptr.

The Thrift classes are:
ImpalaServiceProcessor, ImpalaHiveServer2ServiceProcessor, etc.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new patch set (#2).

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..

IMPALA-4786: Clean up how ImpalaServers are created

ImpalaServer had to be created via an awkward CreateImpalaServer()
method that took a lot of arguments. This patch refactors that code (in
anticipation of some KRPC changes) to follow a more normal lifecycle:

1. ImpalaServer* server = new ImpalaServer(ExecEnv*)
2. RETURN_IF_ERROR(server->Init()) // for error-returning init operations
3. RETURN_IF_ERROR(server->Start())
4. server->Join()

Also add ExecEnv::Init(), and move calls to ExecEnv::StartServices() to
ImpalaServer::StartServices(). This captures a dependency that KRPC will
rely on - where initialization of both ExecEnv and ImpalaServer need to
happen before services are started.

This sets up a clean-up of InProcessImpalaServer, which is too heavy for
the work that it does. That work is deferred to a follow-on patch.

This is a slightly cleaned up version of Henry's abandoned patch:
https://gerrit.cloudera.org/#/c/7673/

Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
---
M be/src/exprs/expr-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/scheduler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/impalad-main.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/hdfs-util-test.cc
10 files changed, 123 insertions(+), 137 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/8076/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change.

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..


Patch Set 4:

(17 comments)

Looks good but I have a few comments - there is a lot more that could be 
pruned.  Let me know which if any you might pursue and I can give a +1

http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java:

Line 61:   private final float queueMaxAMShareDefault;
Impala doesn't make use of YARN's concept of ApplicationMasters.  Why not kill 
this.


Line 64:   private final Map> 
queueAcls;
Useless (just SUBMIT_APPLICATIONS)


Line 69:   private final Map minSharePreemptionTimeouts;
Unused by Impala


Line 74:   private final Map fairSharePreemptionTimeouts;
Also unused by Impala


Line 80:   private final Map fairSharePreemptionThresholds;
Also unused


Line 82:   private final Map schedulingPolicies;
The only policy is DEFAULT_POLICY so this is also not needed.


Line 84:   private final SchedulingPolicy defaultSchedulingPolicy;
Ditto


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java:

Line 30: public class AllocationConfigurationException extends Exception {
The only point of this class is capturing configuration exceptions and 
throwing, but if you remove most of the throws, maybe this class is not 
necessary either.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java:

Line 103:   public void serviceInit(Configuration conf) throws Exception {
103-165 duplicate code and functionality provided by 
impala.util.FileWatchService (minus the ALLOC_RELOAD_WAIT_MS check); we only 
needed this code before because this is the way yarn worked internally, but it 
would be better to have just one implementation here - maybe switch this to use 
FileWatchService.


Line 171:* classpath, but loaded like a regular File.
This seems like unnecessary functionality that we could deprecate if desired.


Line 223: Map minSharePreemptionTimeouts = new HashMap<>();
Preemption configuration does nothing in Impala; could also be killed.


Line 226: Map> queueAcls = new 
HashMap<>();
Impala only uses a single queueACL, SUBMIT_APPLICATIONS.  We could pull the 
entire QueueACL class into impala to get rid of one more Yarnish dependency.


Line 323: } else if 
("defaultQueueSchedulingPolicy".equals(element.getTagName())
Here's the useless code and exception throwing.


Line 533:   public interface Listener {
This can probably go away too.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java:

Line 70: Pattern pattern = Pattern.compile("(\\d+)(\\.\\d*)?\\s*" + units);
This seems a pretty inefficient way to look up resources. This would be a 
candidate for memoizing if this is called often with the same 'units'.


http://gerrit.cloudera.org:8080/#/c/8035/4/common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java
File 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java:

Line 25: return DEFAULT_POLICY;
This whole class is pretty useless and in Impala's case, only serves to throw 
exceptions when the SchedulingPolicy can't be parsed as a valid policy.

Since that is now impossible, maybe just remove the class?


http://gerrit.cloudera.org:8080/#/c/8035/4/fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java
File fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java:

Line 124: new Configuration().get(HADOOP_SECURITY_AUTH_TO_LOCAL, 
"DEFAULT"));
Not clear to me why this is needed now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: 

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#7).

Change subject: IMPALA-5895: clean up runtime profile lifecycle
..

IMPALA-5895: clean up runtime profile lifecycle

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 450 insertions(+), 416 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/8069/7
-- 
To view, visit http://gerrit.cloudera.org:8080/8069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5895: clean up runtime profile lifecycle
..


Patch Set 6:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:

PS6, Line 476: Open
> Open()
Done


PS6, Line 480: Initialized
> previous comment you say "created". is it a synonym (and if so, let's commo
Done


http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/exec/scan-node.cc
File be/src/exec/scan-node.cc:

PS6, Line 73: We
> "we" seems ambiguous. is this talking about subclasses as well or just this
Done


http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/exec/scan-node.h
File be/src/exec/scan-node.h:

Line 87:   virtual Status Prepare(RuntimeState* state) WARN_UNUSED_RESULT;
> nit: i think we usually have newline between method comments
Done


http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/runtime/data-stream-recvr.cc
File be/src/runtime/data-stream-recvr.cc:

PS6, Line 267: profile_
> is that still the expected one now that profile is the child?
It looks like the child_profile was added recently as part of IMPALA-5773. I 
think it was just an oversight that this pointed to profile_ instead of 
child_profile. Seems more consistent to have everything in the same profile.


http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/util/periodic-counter-updater.h
File be/src/util/periodic-counter-updater.h:

PS6, Line 73: the counter
> what is "the counter"?
Clarified that it's 'buckets'.


http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

PS6, Line 330: with the index corresponding
 :   /// to the value
> how is the index computed from the src_counter value?  is the src_counter v
Done


PS6, Line 336: AddBucketingCounters
> the other Add functions are given a 'name'. why doesn't this one need a nam
Added a comment with further explanation. These "bucketing" counters are only 
used in one place in HdfsScanNode. The counters doesn't actually show up in the 
profile, they're processed and added as a string in the profile.

This is seriously wonky but it doesn't seem worth tackling right now.


PS6, Line 383: owned by counter_map_
> but then the counter_map_ comment seems to say that they are owned by the p
Comment was inaccurate. Change just to say that they also appear in counter_map.


PS6, Line 391: typedef std::map 
TimeSeriesCounterMap;
 :   TimeSeriesCounterMap time_series_counter_map_;
> comment
Done


PS6, Line 398: counter_child_map_
> child_counter_map_?
Done


PS6, Line 479: non-locked
> that sounds like they aren't protected by the lock.  Maybe "non-locking"? O
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 1952: handler = shared_from_this();
what is the purpose of this? I think we should avoid shared_from_this() unless 
it's really the clearest way to do something. could you elaborate on what this 
is all about?


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS1, Line 123: be_port
is that the thrift impala internal service port (as opposed to to krpc verison 
of that service), or something else?


Line 999:   /// Init() were <= 0.
can you comment on who owns these? even better, if this class owns them, use 
scoped_ptr to make that explicit (we just don't want to use shared_ptr, and 
shared ownership, generally unless it really makes sense. but scoped_ptr, which 
implies single ownership, is fine.


PS1, Line 1002: be_server_
since krpc is on the way, how about thrift_be_server_? this goes away once we 
fully switch ImpalaInternalService to krpc, is that right?


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

PS1, Line 86: boost::shared_ptr impala_server
why do we need shared ownership of this thing? who else needs to keep it alive 
beyond this function scope? (maybe it is needed, but let's reason through that).

Oh, maybe this (and the one in ImpalaServer shared_from_this) is a consequence 
of ImpalaServer being the thing that implements the thrift interface and thrift 
requires the shared_ptr?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5941: Fix Metastore schema creation in create-test-configuration.sh

[David Knupp (Code Review)]

David Knupp has posted comments on this change.

Change subject: IMPALA-5941: Fix Metastore schema creation in 
create-test-configuration.sh
..


Patch Set 3:

Started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1234/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3360: Codegen inserting into runtime filters

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3360: Codegen inserting into runtime filters
..


Patch Set 2:

> (4 comments)
 > 
 > Looks good to me minus a couple of things. Not sure if Dan wants to
 > take a look

I looked at the header changes and they look fine. Tim, feel free to do the +2 
once all comments are addressed.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

[Matthew Jacobs (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8035

to look at the new patch set (#4).

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..

IMPALA-5920: Remove admission control dependency on YARN RM jar

Impala's admission controller relies on the YARN
fair-scheduler.xml for configuration. That configuration is
loaded using YARN directly (ie. as a library by the
frontend). In Hadoop 3, a number of changes were made to the
YARN resourcemanager which break Impala. While we eventually
want to rethink the admission control configuration
(IMPALA-4159), in the meantime we at least should avoid
using unsupported YARN APIs.

This patch removes the fe dependency on the YARN artifact
'hadoop-yarn-server-resourcemanager' which contains private
APIs and isn't meant to be used as a library.

A subset of the required code has been added in
'common/yarn-extras', taken from Hadoop 2.6.0 in CDH, e.g.
see [1]. The code is added in packages 'org.apache.impala.*'
instead of 'org.apache.yarn.*'. Some code could be copied
as-is, those files are marked with the comment:
//YARNUTIL: VANILLA
Files that required some modifications are marked with:
//YARNUTIL: MODIFIED
Or, if all code except a dummy interface could be added:
//YARNUTIL: DUMMY IMPL

The goal the 'yarn-extras' is to make Impala's handling of
the AC configuration self-sufficient, i.e. it shouldn't
matter what version of Hadoop exists. As-is, this was tested
and found to work when Hadoop 2.6 is installed. Because
the yarn-extras/pom.xml still references hadoop-common,
hadoop-yarn-common, and hadoop-yarn-api, additional testing
will be required to ensure Impala using yarn-extras works
when installed along side Hadoop 3.

That testing for Hadoop 3 will be done later. Future changes
will make any other changes required for existing code to
work when Hadoop 3 is installed.

Testing:
* Ran existing tests on master.

1: 
https://www.cloudera.com/documentation/enterprise/release-notes/topics/cm_vd_cdh_package_tarball_512.html

Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
---
M CMakeLists.txt
A common/yarn-extras/CMakeLists.txt
A common/yarn-extras/README.txt
A common/yarn-extras/pom.xml
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/resource/ResourceWeights.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfigurationException.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FSQueueType.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/FairSchedulerConfiguration.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementPolicy.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/QueuePlacementRule.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/SchedulingPolicy.java
A 
common/yarn-extras/src/main/java/org/apache/impala/yarn/server/utils/BuilderUtils.java
M fe/CMakeLists.txt
M fe/pom.xml
M fe/src/main/java/org/apache/impala/util/RequestPoolService.java
M fe/src/test/java/org/apache/impala/util/TestRequestPoolService.java
18 files changed, 1,805 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/8035/4
-- 
To view, visit http://gerrit.cloudera.org:8080/8035
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8035/3/common/yarn-extras/pom.xml
File common/yarn-extras/pom.xml:

Line 23:   org.apache.impala
> I think we should move the implementing classes here to "org.apache.impala"
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5895: clean up runtime profile lifecycle
..


Patch Set 6:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:

PS6, Line 476: Open
Open()


PS6, Line 480: Initialized
previous comment you say "created". is it a synonym (and if so, let's 
common-ize).


http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/exec/scan-node.cc
File be/src/exec/scan-node.cc:

PS6, Line 73: We
"we" seems ambiguous. is this talking about subclasses as well or just this 
class? the header comment seems to imply the former, but then at least some 
subclasses also do this, right?


http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/exec/scan-node.h
File be/src/exec/scan-node.h:

Line 87:   virtual Status Prepare(RuntimeState* state) WARN_UNUSED_RESULT;
nit: i think we usually have newline between method comments


http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/runtime/data-stream-recvr.cc
File be/src/runtime/data-stream-recvr.cc:

PS6, Line 267: profile_
is that still the expected one now that profile is the child?


http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/util/periodic-counter-updater.h
File be/src/util/periodic-counter-updater.h:

PS6, Line 73: the counter
what is "the counter"?


http://gerrit.cloudera.org:8080/#/c/8069/6/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

PS6, Line 330: with the index corresponding
 :   /// to the value
how is the index computed from the src_counter value?  is the src_counter value 
used as the index directly, or is it scaled in some way? i guess i'm asking if 
the bucket width is 1 or can it be > 1?


PS6, Line 336: AddBucketingCounters
the other Add functions are given a 'name'. why doesn't this one need a name?


PS6, Line 383: owned by counter_map_
but then the counter_map_ comment seems to say that they are owned by the 
profile. which is it?


PS6, Line 391: typedef std::map 
TimeSeriesCounterMap;
 :   TimeSeriesCounterMap time_series_counter_map_;
comment


PS6, Line 398: counter_child_map_
child_counter_map_?


PS6, Line 479: non-locked
that sounds like they aren't protected by the lock.  Maybe "non-locking"? Or 
say "Internal implementations of the Add*Counter() ..."


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5941: Fix Metastore schema creation in create-test-configuration.sh

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5941: Fix Metastore schema creation in 
create-test-configuration.sh
..


Patch Set 3:

Ran full dataload + tests and everything worked. I think this is ready to merge.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 346: /// Reference to the server's query options
I think this is kosher because the server keeps references around to the 
session. I also considered making a copy for simplicity.


Line 392: TQueryOptions query_options();
This is violating our style guide two-fold.

First of all, it should be QueryOptions(), which I'll change in the next 
iteration. Second of all, now we've got two functions in the stuct, and it 
should be a class. Is it worth it?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Re-apply: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change.

Change subject: Re-apply: IMPALA-5589: change "set" in impala-shell to show 
empty string for unset query options
..


Patch Set 1:

This is a re-run of http://gerrit.cloudera.org:8080/7886. The interesting thing 
is https://gerrit.cloudera.org/#/c/8070.

(I'm happy to collapse the two changes if that's preferred.)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I29f5d8ab874cb1338077f16019a9537766cac0c4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Philip Zeyliger (Code Review)]

Hello Impala Public Jenkins, Bharath Vissapragada, Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8070

to look at the new patch set (#7).

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET =""' will now unset an option within the session,
reverting it to its default state.

This change became necessary when "SET" started returning an empty
string for unset options which don't have a default. The test
infrastructure (impala_test_suite.py) resets options to what it thinks
is its defaults, and, when this broke, some ASAN builds started to fail,
presumably due to a timing issue with how we re-use connections between
tests.

Previously, SessionState copied over the default options from the server
when the session was created and then mutated that. To support unsetting
options at the session layer, this change keeps a pointer to the default
server settings, keeps separately the mutations, and overlays the
options each time they're requested.

Because "set key=''" is ambiguous between "set to the empty string" and
"unset", it's now impossible to set to the empty string, at the session
layer, an option that is configured at a previous layer. In practice,
this is just debug_action and request_pool. debug_action is essentially
an internal tool. For request_pool, this means that setting the default
request_pool via impalad command line is now a bad idea, but that
doesn't seem worse than before.

Testing:
* Added a simple test that triggered this side-effect without this code.
  Specifically, "impala-python infra/python/env/bin/py.test 
tests/metadata/test_set.py -s"
  with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
  defaulted and non-defaulted values.
* Added a custom cluster test that checks that overlays are
  working against
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
---
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
M tests/custom_cluster/test_admission_controller.py
A tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
14 files changed, 194 insertions(+), 18 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/70/8070/7
-- 
To view, visit http://gerrit.cloudera.org:8080/8070
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] Re-apply: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

[Philip Zeyliger (Code Review)]

Philip Zeyliger has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8096

Change subject: Re-apply: IMPALA-5589: change "set" in impala-shell to show 
empty string for unset query options
..

Re-apply: IMPALA-5589: change "set" in impala-shell to show empty string for 
unset query options

(Re-applies reverted commit 387bde0639ffd8ef580ccbf727152954e62bacbe.
The commit broke ASAN tests due to a race in how test infrastructure
re-uses connections. The fix for that is in an adjacent commit.)

When converting TQueryOptions to a map, we now convert
unset options to the empty string. Within TQueryOptions we have optional
options (like mt_dop or compression_codec) with no default specified. In
this case, the user was seeing 0 for numeric types and the first enum
option for enumeration types (e.g., "NONE" in the compression case).
This was confusing as the implementation handles this "null" case
differently (e.g., using SNAPPY as the default codec in the case
reported in the JIRA).

When running "set" in impala-shell, the difference is as
follows:

-BUFFER_POOL_LIMIT: [0]
+BUFFER_POOL_LIMIT: []
-COMPRESSION_CODEC: [NONE]
+COMPRESSION_CODEC: []
-MT_DOP: [0]
+MT_DOP: []
-RESERVATION_REQUEST_TIMEOUT: [0]
+RESERVATION_REQUEST_TIMEOUT: []
-SEQ_COMPRESSION_MODE: [0]
+SEQ_COMPRESSION_MODE: []
-V_CPU_CORES: [0]
+V_CPU_CORES: []

Obviously, the empty string is a valid value for a string-typed option, where
it will be impossible to tell the difference between "unset" and "set to empty
string." Today, there are two string-typed options: debug_string defaults to ""
and request_pool has no default. An alternative would have been to use
a special token like "_unset" or to introduce a new field in the beeswax
Thrift ConfigVariable struct. I think the empty string approach
is clearest.

The other users of this state, which I believe are HiveServer2's OpenSession()
call and HiveServer2's response to a "SET" query are affected. They
benefit from the same fix, and a new test has been added to test_hs2.py.

I did a mild refactoring in the HS2 tests to write a helper method
for the very common pattern of excecuting a query.

Testing:
* Manual testing with impala-shell
* Modified impala-shell tests to check this explicitly for one case.
* Modified HS2 test to check this as well as the SET k=v statement,
  which looked otherwise untested.

Change-Id: I29f5d8ab874cb1338077f16019a9537766cac0c4
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_hs2.py
M tests/shell/test_shell_commandline.py
7 files changed, 106 insertions(+), 60 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/96/8096/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I29f5d8ab874cb1338077f16019a9537766cac0c4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-4939, IMPALA-4940: Decimal V2 multiplication

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4939, IMPALA-4940: Decimal V2 multiplication
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7438/5/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

Line 203:   RESULT_T result = value / multiplier;
> Another idea: there are a finite number of possible delta_scale values.  It
After trying different approaches, this one produced the best results: 
http://github.mtv.cloudera.com/Taras/Impala/commit/950892a48e555ba24dcca772851d251ae3ecb17f

Before: 66.13s
After: 55.63s

So, a small improvement. Still not that great: without the multiplication 
patch, it took 8.26s


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I37ad6232d7953bd75c18dc86e665b2b501a1ebe1
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4671: Replace kudu::ServicePool with one that uses 
Impala threads
..


Patch Set 1:

> Why not make a separate implementation of a service pool, rather
 > than modify Kudu's? See 
 > https://github.com/henryr/Impala/commit/1e1810dad63b821d3d530d09debf47e3a8f4a570
 > for an example of how that could work. Then you have a much better
 > chance of integrating the service pool with Impala's observability
 > subsystems (again, there are examples in that commit).

Thanks for the suggestion and the code pointer, Henry!
The way I see it, there are some pros and cons to having a parallel ServicePool 
implementation.
Pros:
* Better observability. (Huge win)
* We avoid modifying more Kudu code.


Cons:
* We need to periodically check against kudu::ServicePool if there were any 
bugs and re-implement the fix on our side. (Not too bad, but still a task)
* This might be obsolete after we standardize on a utils library between Impala 
and Kudu. (But this may take a while to happen)
* Requires more time to implement and review.

Given these, I'm still leaning towards your suggestion of having an Impala 
native ServicePool. Let me go through your patch before I make a call.

In the mean time, other reviewers should feel free to weigh in.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb75c91d4c754f136bcb4b51dd66e2d933b14b35
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5920: Remove admission control dependency on YARN RM jar

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5920: Remove admission control dependency on YARN RM jar
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8035/3/common/yarn-extras/pom.xml
File common/yarn-extras/pom.xml:

Line 23:   org.apache.impala
I think we should move the implementing classes here to "org.apache.impala" as 
well, as opposed to leaving them in org.apache.hadoop. This is a fairly 
mechanical change, but it'll make it explicit that we've copied these over, and 
it'll avoid the unfortunate bugs we'd see if two copies of one of the classes 
appeared on the CLASSPATH, and we didn't know which one was active.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7efdd8ebea298836ca2a82c0a4ae037ac9285bcf
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2636: HS2 GetTables() returns TABLE TYPE as TABLE for VIEW

[sandeep akinapelli (Code Review)]

sandeep akinapelli has posted comments on this change.

Change subject: IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for 
VIEW
..


Patch Set 2:

(13 comments)

Addressed review comments.

http://gerrit.cloudera.org:8080/#/c/7353/1/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

Line 62:   private static final String TABLE_TYPE_VIEW = "VIEW";
> In any case I think returning "VIEW" is better, but it would be good to doc
Done


Line 62:   private static final String TABLE_TYPE_VIEW = "VIEW";
> Please check if this is consistent with what Hive returns. Does Hive return
Hive does return VIRTUAL_VIEW if "hive.server2.table.type.mapping" is set to 
"hive". I am assuming we only want to represent two types table and view. (also 
we are not distinguishing between managed table and external table)


Line 63:   private static final String TABLE_TYPE_INDEX_TABLE = "INDEX";
> Why not just "INDEX"?
Yep. Index is probably simpler to understand.


PS1, Line 221: taleTypes
> typo
Done


PS1, Line 221: type
> types
Done


Line 298: if (table.getMetaStoreTable() != null) {
> Why this null check? Loaded tables should really have this set.
I ran this locally and sometime I get null, not sure why!


Line 321: TableType tType;
> single line and space after "if"
Done


Line 324: try {
> Is this really needed? If so, please Inline into L326.
not absolutely. extra precaution since coming through thrift as a string. 
Inlined..


Line 328: }
> indent case by 2
Done


Line 338: return defaultTableType;
> move this up so it's clear the switch block cannot throw
Done


Line 495: if (tableTypes != null && !tableTypes.isEmpty()) {
> This should be fixed as well.
good catch. fixed.


http://gerrit.cloudera.org:8080/#/c/7353/1/fe/src/test/java/org/apache/impala/service/FrontendTest.java
File fe/src/test/java/org/apache/impala/service/FrontendTest.java:

Line 221:   public void TestUnloadedView() throws ImpalaException {
> We should test two cases:
Done


Line 226: "create view %s.test_view as select * from 
functional.alltypes", dbName));
> No need to test tables here
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I90616388e6181cf342b3de389af940214ed46428
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: sandeep akinapelli 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2636: HS2 GetTables() returns TABLE TYPE as TABLE for VIEW

[sandeep akinapelli (Code Review)]

sandeep akinapelli has uploaded a new patch set (#2).

Change subject: IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for 
VIEW
..

IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for VIEW

Added code to read the table type from metastore table but defaults to
"TABLE" if the metastore table is not loaded.
After the change, GetTabletypes also returns "VIEW" apart from "TABLE"
Changed unit and jdbc testcases for GetTableTypes.
Added new Frontend test for reading views.

Change-Id: I90616388e6181cf342b3de389af940214ed46428
---
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/service/FrontendTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
3 files changed, 107 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/7353/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7353
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I90616388e6181cf342b3de389af940214ed46428
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: sandeep akinapelli 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-5860: upgrade to LLVM 3.9.1

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5860: upgrade to LLVM 3.9.1
..


Patch Set 9: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida873ddb15e393b0bd37486db24add8a32f43ad0
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5860: upgrade to LLVM 3.9.1

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5860: upgrade to LLVM 3.9.1
..


IMPALA-5860: upgrade to LLVM 3.9.1

LLVM made a few API changes:
* Misc minor changes to function and type signatures
* The CloneFunction() API changed semantics (http://reviews.llvm.org/D18628)

Needed to fix a few new clang-tidy warnings.

Testing:
Ran core and ASAN tests.

Perf:
Ran single node TPC-H and targeted perf with scale factor 60. Both
improved on average.

+--+---+-++++
| Workload | File Format   | Avg (s) | Delta(Avg) | GeoMean(s) | 
Delta(GeoMean) |
+--+---+-++++
| TPCH(60) | parquet / none / none | 17.82   | -5.01% | 11.64  | -4.23% 
|
+--+---+-++++

+--+--+---++-++++-+---+
| Workload | Query| File Format   | Avg(s) | Base Avg(s) | 
Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
+--+--+---++-++++-+---+
| TPCH(60) | TPCH-Q1  | parquet / none / none | 27.97  | 27.59   |   +1.36% 
  |   0.39%|   0.41%| 1   | 5 |
| TPCH(60) | TPCH-Q20 | parquet / none / none | 5.81   | 5.78|   +0.44% 
  |   0.73%|   0.21%| 1   | 5 |
| TPCH(60) | TPCH-Q21 | parquet / none / none | 62.98  | 62.98   |   +0.01% 
  |   5.56%|   1.07%| 1   | 5 |
| TPCH(60) | TPCH-Q15 | parquet / none / none | 8.45   | 8.46|   -0.20% 
  |   0.40%|   0.38%| 1   | 5 |
| TPCH(60) | TPCH-Q4  | parquet / none / none | 5.57   | 5.59|   -0.41% 
  |   0.43%|   0.80%| 1   | 5 |
| TPCH(60) | TPCH-Q6  | parquet / none / none | 3.16   | 3.17|   -0.45% 
  |   0.78%|   1.70%| 1   | 5 |
| TPCH(60) | TPCH-Q5  | parquet / none / none | 7.41   | 7.47|   -0.92% 
  |   0.71%|   1.06%| 1   | 5 |
| TPCH(60) | TPCH-Q9  | parquet / none / none | 33.45  | 33.78   |   -0.99% 
  |   1.15%|   0.85%| 1   | 5 |
| TPCH(60) | TPCH-Q11 | parquet / none / none | 2.00   | 2.03|   -1.34% 
  |   1.71%|   2.24%| 1   | 5 |
| TPCH(60) | TPCH-Q2  | parquet / none / none | 4.71   | 4.79|   -1.60% 
  |   1.49%|   1.95%| 1   | 5 |
| TPCH(60) | TPCH-Q18 | parquet / none / none | 46.48  | 47.71   |   -2.58% 
  |   1.04%|   0.38%| 1   | 5 |
| TPCH(60) | TPCH-Q14 | parquet / none / none | 5.85   | 6.02|   -2.84% 
  |   0.44%|   0.70%| 1   | 5 |
| TPCH(60) | TPCH-Q22 | parquet / none / none | 6.51   | 6.76|   -3.71% 
  |   2.29%|   2.42%| 1   | 5 |
| TPCH(60) | TPCH-Q19 | parquet / none / none | 7.27   | 7.63|   -4.69% 
  |   1.33%|   0.78%| 1   | 5 |
| TPCH(60) | TPCH-Q10 | parquet / none / none | 13.19  | 13.84   |   -4.73% 
  |   0.42%|   1.44%| 1   | 5 |
| TPCH(60) | TPCH-Q13 | parquet / none / none | 21.95  | 23.12   |   -5.03% 
  |   0.25%|   1.19%| 1   | 5 |
| TPCH(60) | TPCH-Q16 | parquet / none / none | 5.29   | 5.57|   -5.04% 
  |   0.85%|   0.78%| 1   | 5 |
| TPCH(60) | TPCH-Q7  | parquet / none / none | 42.05  | 44.33   |   -5.16% 
  |   2.07%|   2.28%| 1   | 5 |
| TPCH(60) | TPCH-Q12 | parquet / none / none | 19.77  | 21.00   |   -5.87% 
  |   8.14%|   5.09%| 1   | 5 |
| TPCH(60) | TPCH-Q3  | parquet / none / none | 11.46  | 12.32   |   -6.94% 
  |   0.76%|   0.53%| 1   | 5 |
| TPCH(60) | TPCH-Q17 | parquet / none / none | 40.09  | 49.28   |   
-18.64%  |   2.09%|   0.67%| 1   | 5 |
| TPCH(60) | TPCH-Q8  | parquet / none / none | 10.63  | 13.47   | I 
-21.08%  | * 12.34% * | * 21.09% * | 1   | 5 |
+--+--+---++-++++-+---+

+---+---+-++++
| Workload  | File Format   | Avg (s) | Delta(Avg) | GeoMean(s) 
| Delta(GeoMean) |
+---+---+-++++
| TARGETED-PERF(60) | parquet / none / none | 22.38   | -1.24% | 4.17   
| +0.81% |

[Impala-ASF-CR] IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4671: Replace kudu::ServicePool with one that uses 
Impala threads
..


Patch Set 1:

Why not make a separate implementation of a service pool, rather than modify 
Kudu's? See 
https://github.com/henryr/Impala/commit/1e1810dad63b821d3d530d09debf47e3a8f4a570
 for an example of how that could work. Then you have a much better chance of 
integrating the service pool with Impala's observability subsystems (again, 
there are examples in that commit).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb75c91d4c754f136bcb4b51dd66e2d933b14b35
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8094

Change subject: IMPALA-4671: Replace kudu::ServicePool with one that uses 
Impala threads
..

IMPALA-4671: Replace kudu::ServicePool with one that uses Impala threads

Kudu's ServicePool uses Kudu's Thread class to service RPC requests.
While this works well, the threads it creates aren't monitored by
Impala's ThreadMgr subsystem.

Eventually we'd like to standardise on one thread class between
projects, but for now it would be useful to reimplement ServicePool
in terms of our thread class.

The reactor threads and acceptor threads will still be Kudu threads,
but those are less likely to do substantial work, so having them
missing from monitoring isn't a big problem.

This patch just changes kudu::Thread to impala::Thread and changes
the container pointer type from scoped_refptr to unique_ptr.

Testing: Ported it on top of https://gerrit.cloudera.org/#/c/8023/
and verified that the Service pool works as expected.

Change-Id: Ibb75c91d4c754f136bcb4b51dd66e2d933b14b35
---
M be/src/kudu/rpc/service_pool.cc
M be/src/kudu/rpc/service_pool.h
2 files changed, 12 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/8094/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8094
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibb75c91d4c754f136bcb4b51dd66e2d933b14b35
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5599: Fix for mis-use of TimestampValue

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5599: Fix for mis-use of TimestampValue
..


Patch Set 2:

(15 comments)

Do you plan to take care of the other cases noted in the jira?  Okay to do it 
in a follow on commit but wondering the plan.

A unit test would be good for testing this kind of code.  You can take a look 
at the various *-test.cc files be/src/*/ for examples.

http://gerrit.cloudera.org:8080/#/c/8084/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS2, Line 921: UnixMillis()
for consistency in these values, how about setting this to 'now_us / 
MICROS_PER_MILLI'?


PS2, Line 1146:  Precision::Second
seems okay to print milliseconds here


PS2, Line 1845: , Precision::Second
here too


PS2, Line 1924:  Precision::Second
and here


http://gerrit.cloudera.org:8080/#/c/8084/2/be/src/util/time.cc
File be/src/util/time.cc:

Line 32: static std::string TimepointToString(const 
chrono::system_clock::time_point& t,
for static things not defined in header, it's helpful to include a short 
function comment for them at the function definition.


PS2, Line 46:   std::string s(buf);
:   return s;
return string(buf);

(and you don't the namespace in the cc files. you can also take a look at 
names.h and see how it's included to pull in the "common" names to .cc files).


Line 53: 
nit: consider removing blank lines here and line 68 to make more code fit on 
screen, since it doesn't seem like these blanks help readability.


Line 65: // 1-second precision or unknown unit. Return empty string.
nice to document (and prove) that invariant with a dcheck:
DCHECK_EQ(p, Precision::Second);


PS2, Line 89:   chrono::system_clock::time_point 
t(chrono::duration_cast(
: chrono::seconds(s)));
:   return t;
could consider combing these like suggested at line 46-47 (but given how long 
the type name is, use your judgement).


PS2, Line 95: system_clock
the docs for chrono::system_clock::time_point claim that the epoch is 
unspecified, so we can we be sure this works everywhere? it does seem likely to 
always use unix epoch, but technically it doesn't have to.


PS2, Line 101: chrono::microseconds
given that the "to" type is the same as the "from" type, why is the 
duration_cast needed?


http://gerrit.cloudera.org:8080/#/c/8084/2/be/src/util/time.h
File be/src/util/time.h:

PS2, Line 72: Precision
"precision" seems kind of generic. How about calling that TimePrecision?


PS2, Line 79: Unix timestamp
I think we usually refer to this as "Unix time", and timestamp often invokes 
the thought of our timestamp type, so how about we say:
... the input Unix time, specified in seconds since the Unix epoch, to a ...


PS2, Line 81: p
nit: in header comments to distinguish variables, we usually use single quotes: 
'p'


PS2, Line 85: Precision p=Precision::Second
normally we should avoid default arguments, but in this case it does seem 
reasonable to assume the output precision should be the same as the input, so 
I'm okay with that. but please put a space around the = just like in other 
parts of the code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 5:

Following through on the layering (process options, pool defaults, session 
options, and request options), there are some complexities.

When I changed SetQueryOption(option, "") to reset the mask bit, a new test 
found that server process options weren't surviving a reset cycle. What's going 
on is that SessionState.default_query_options is initialized to 
ImpalaServer.default_query_options, and that resolution is never re-applied. 
So, when we "reset", we don't handily have a reference to what to reset it to.

I'm planning to rename session->default_query_options to 
session->session_query_options, and resolve against the server defaults every 
time.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5538: Use explicit catalog versions for deleted objects

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5538: Use explicit catalog versions for deleted objects
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7731/5/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:

PS5, Line 87: If true, this item was deleted
> How about:
Actually, the parenthesis I suggested doesn't clarify.  Maybe that part should 
say: (since the latest version of every still-present topic will be included)

or something like that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I93cb7a033dc8f0d3e0339394b36affe14523274c
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#6).

Change subject: IMPALA-5895: clean up runtime profile lifecycle
..

IMPALA-5895: clean up runtime profile lifecycle

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 442 insertions(+), 417 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/8069/6
-- 
To view, visit http://gerrit.cloudera.org:8080/8069
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5860: upgrade to LLVM 3.9.1

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5860: upgrade to LLVM 3.9.1
..


Patch Set 9: Code-Review+2

Missed a couple of clang warnings.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida873ddb15e393b0bd37486db24add8a32f43ad0
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5860: upgrade to LLVM 3.9.1

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5860: upgrade to LLVM 3.9.1
..


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1233/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida873ddb15e393b0bd37486db24add8a32f43ad0
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5860: upgrade to LLVM 3.9.1

[Tim Armstrong (Code Review)]

Hello Impala Public Jenkins, Dan Hecht,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7974

to look at the new patch set (#9).

Change subject: IMPALA-5860: upgrade to LLVM 3.9.1
..

IMPALA-5860: upgrade to LLVM 3.9.1

LLVM made a few API changes:
* Misc minor changes to function and type signatures
* The CloneFunction() API changed semantics (http://reviews.llvm.org/D18628)

Needed to fix a few new clang-tidy warnings.

Testing:
Ran core and ASAN tests.

Perf:
Ran single node TPC-H and targeted perf with scale factor 60. Both
improved on average.

+--+---+-++++
| Workload | File Format   | Avg (s) | Delta(Avg) | GeoMean(s) | 
Delta(GeoMean) |
+--+---+-++++
| TPCH(60) | parquet / none / none | 17.82   | -5.01% | 11.64  | -4.23% 
|
+--+---+-++++

+--+--+---++-++++-+---+
| Workload | Query| File Format   | Avg(s) | Base Avg(s) | 
Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
+--+--+---++-++++-+---+
| TPCH(60) | TPCH-Q1  | parquet / none / none | 27.97  | 27.59   |   +1.36% 
  |   0.39%|   0.41%| 1   | 5 |
| TPCH(60) | TPCH-Q20 | parquet / none / none | 5.81   | 5.78|   +0.44% 
  |   0.73%|   0.21%| 1   | 5 |
| TPCH(60) | TPCH-Q21 | parquet / none / none | 62.98  | 62.98   |   +0.01% 
  |   5.56%|   1.07%| 1   | 5 |
| TPCH(60) | TPCH-Q15 | parquet / none / none | 8.45   | 8.46|   -0.20% 
  |   0.40%|   0.38%| 1   | 5 |
| TPCH(60) | TPCH-Q4  | parquet / none / none | 5.57   | 5.59|   -0.41% 
  |   0.43%|   0.80%| 1   | 5 |
| TPCH(60) | TPCH-Q6  | parquet / none / none | 3.16   | 3.17|   -0.45% 
  |   0.78%|   1.70%| 1   | 5 |
| TPCH(60) | TPCH-Q5  | parquet / none / none | 7.41   | 7.47|   -0.92% 
  |   0.71%|   1.06%| 1   | 5 |
| TPCH(60) | TPCH-Q9  | parquet / none / none | 33.45  | 33.78   |   -0.99% 
  |   1.15%|   0.85%| 1   | 5 |
| TPCH(60) | TPCH-Q11 | parquet / none / none | 2.00   | 2.03|   -1.34% 
  |   1.71%|   2.24%| 1   | 5 |
| TPCH(60) | TPCH-Q2  | parquet / none / none | 4.71   | 4.79|   -1.60% 
  |   1.49%|   1.95%| 1   | 5 |
| TPCH(60) | TPCH-Q18 | parquet / none / none | 46.48  | 47.71   |   -2.58% 
  |   1.04%|   0.38%| 1   | 5 |
| TPCH(60) | TPCH-Q14 | parquet / none / none | 5.85   | 6.02|   -2.84% 
  |   0.44%|   0.70%| 1   | 5 |
| TPCH(60) | TPCH-Q22 | parquet / none / none | 6.51   | 6.76|   -3.71% 
  |   2.29%|   2.42%| 1   | 5 |
| TPCH(60) | TPCH-Q19 | parquet / none / none | 7.27   | 7.63|   -4.69% 
  |   1.33%|   0.78%| 1   | 5 |
| TPCH(60) | TPCH-Q10 | parquet / none / none | 13.19  | 13.84   |   -4.73% 
  |   0.42%|   1.44%| 1   | 5 |
| TPCH(60) | TPCH-Q13 | parquet / none / none | 21.95  | 23.12   |   -5.03% 
  |   0.25%|   1.19%| 1   | 5 |
| TPCH(60) | TPCH-Q16 | parquet / none / none | 5.29   | 5.57|   -5.04% 
  |   0.85%|   0.78%| 1   | 5 |
| TPCH(60) | TPCH-Q7  | parquet / none / none | 42.05  | 44.33   |   -5.16% 
  |   2.07%|   2.28%| 1   | 5 |
| TPCH(60) | TPCH-Q12 | parquet / none / none | 19.77  | 21.00   |   -5.87% 
  |   8.14%|   5.09%| 1   | 5 |
| TPCH(60) | TPCH-Q3  | parquet / none / none | 11.46  | 12.32   |   -6.94% 
  |   0.76%|   0.53%| 1   | 5 |
| TPCH(60) | TPCH-Q17 | parquet / none / none | 40.09  | 49.28   |   
-18.64%  |   2.09%|   0.67%| 1   | 5 |
| TPCH(60) | TPCH-Q8  | parquet / none / none | 10.63  | 13.47   | I 
-21.08%  | * 12.34% * | * 21.09% * | 1   | 5 |
+--+--+---++-++++-+---+

+---+---+-++++
| Workload  | File Format   | Avg (s) | Delta(Avg) | GeoMean(s) 
| Delta(GeoMean) |
+---+---+-++++
| TARGETED-PERF(60) | 

[Impala-ASF-CR] IMPALA-3360: Codegen inserting into runtime filters

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3360: Codegen inserting into runtime filters
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8029/2/be/src/exec/filter-context.cc
File be/src/exec/filter-context.cc:

Line 217: Status FilterContext::CodegenInsert(
> I'm still learning here. I don't see a loop here. What makes this ineligibl
You're right, we should be able to cross-compile Insert() and substitute in 
GetValue(), GetHashValue() and BloomFilter::Insert().


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5860: upgrade to LLVM 3.9.1

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5860: upgrade to LLVM 3.9.1
..


Patch Set 8: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1232/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida873ddb15e393b0bd37486db24add8a32f43ad0
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No