[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] PREVIEW IMPALA-2521: Add clustered hint to insert statements

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: PREVIEW IMPALA-2521: Add clustered hint to insert statements
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:

Line 269:* clustered/noclustered plan hint. The clustering re-order the 
data in 'inputFragment'
Better to be specific: Say that it adds a sort node that orders on the 
partition columns of the target table.


Line 272:   // TODO: this function shares some code with 
QueryStmt::createSortTupleInfo(). However
You could move most of the code into SortInfo.createSortTupleInfo(List 
resultExprs);


Line 309: sortTupleDesc.setIsMaterialized(true);
To indicate that this tuple is a physical tuple needed during runtime execution 
and not a "virtual" tuple like e.g. the tuple of an inline view (which never 
gets materialized).


Line 324: // TODO Why is this needed here but not in QueryStmt.java? If it 
is omitted, the query
Before plan generation we call QueryStmt.materializeRequiredSlots() to make 
sure all slots needed to evaluate that QueryStmt are marked as materialized. 
However, we have no such mechanism here because we are adding a plan node that 
does not come from a SQL clause, so we must mark the slots as materialized 
manually.


http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:

Line 144: rootFragment = distributedPlanner.addClusteringFragment(
1. we should also do this for single-node plans, so adding the clustering 
doesn't really belong in the DistributedPlanner
2. we are not adding a new fragment, so I'd rephrase this addClusteringNode() 
or similar


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4253: impala-server.backends.client-cache.total-clients shows negative value

[Juan Yu (Code Review)]

Juan Yu has posted comments on this change.

Change subject: IMPALA-4253: impala-server.backends.client-cache.total-clients 
shows negative value
..


Patch Set 2: Code-Review+2

(1 comment)

Carry Dan's +2

http://gerrit.cloudera.org:8080/#/c/4668/1/be/src/runtime/client-cache.cc
File be/src/runtime/client-cache.cc:

PS1, Line 95: if (metrics_enabled_) {
> the "if succeeded" explains this, so you could remove this sentence, which 
Done


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

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

[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-2905: Move QueryResultSet implementations into separate 
module
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-2905: Move QueryResultSet implementations into separate 
module
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4736/2//COMMIT_MSG
Commit Message:

Line 7: IMPALA-2905: Move QueryResultSet implementations into separate module
> Is this the right JIRA? IMPALA-2905 is fixed.
It was suggested in the 2905 review, where half the work was done. Didn't see a 
lot of point making a new JIRA.


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

PS2, Line 500: (
> nit: extraneous parens.
Done


http://gerrit.cloudera.org:8080/#/c/4736/2/be/src/service/query-result-set.h
File be/src/service/query-result-set.h:

Line 27: #include 
> what for?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4253: impala-server.backends.client-cache.total-clients shows negative value

[Juan Yu (Code Review)]

Hello Sailesh Mukil, Dan Hecht,

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

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

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

Change subject: IMPALA-4253: impala-server.backends.client-cache.total-clients 
shows negative value
..

IMPALA-4253: impala-server.backends.client-cache.total-clients shows negative 
value

Fixed double decrement in case a cached connection is broken
and cannot be re-created.

Change-Id: Ic9e28055cb232cdb543c4c9f05a558ab0f73f777
---
M be/src/runtime/client-cache.cc
1 file changed, 5 insertions(+), 1 deletion(-)


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

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

[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module

[Henry Robinson (Code Review)]

Hello Alex Behm, Dan Hecht,

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

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

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

Change subject: IMPALA-2905: Move QueryResultSet implementations into separate 
module
..

IMPALA-2905: Move QueryResultSet implementations into separate module

This mostly mechanical change moves the definition and implementation of
the Beeswax and HS2-specific result sets into their own module. Result
sets are now uniformly created by one of two factory methods, so the
implementation is decoupled from the client.

Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32
---
M be/src/service/CMakeLists.txt
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
A be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
5 files changed, 503 insertions(+), 419 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 

[Impala-ASF-CR] Add search / sort to HTML tables for metrics and threads

[Henry Robinson (Code Review)]

Henry Robinson has submitted this change and it was merged.

Change subject: Add search / sort to HTML tables for metrics and threads
..


Add search / sort to HTML tables for metrics and threads

Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f
Reviewed-on: http://gerrit.cloudera.org:8080/4743
Tested-by: Internal Jenkins
Reviewed-by: Henry Robinson 
---
M www/metric_group.tmpl
M www/thread-group.tmpl
2 files changed, 67 insertions(+), 39 deletions(-)

Approvals:
  Henry Robinson: Looks good to me, approved
  Internal Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Internal Jenkins

[Impala-ASF-CR] Buffer pool: Add basic counters to buffer pool client

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: Buffer pool: Add basic counters to buffer pool client
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a5a57b7cfccf67ee498e68964f1e077075ee325
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Buffer pool: Add basic counters to buffer pool client

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: Buffer pool: Add basic counters to buffer pool client
..


Buffer pool: Add basic counters to buffer pool client

Change-Id: I9a5a57b7cfccf67ee498e68964f1e077075ee325
Reviewed-on: http://gerrit.cloudera.org:8080/4714
Reviewed-by: Tim Armstrong 
Tested-by: Internal Jenkins
---
A be/src/bufferpool/buffer-pool-counters.h
M be/src/bufferpool/buffer-pool-test.cc
M be/src/bufferpool/buffer-pool.cc
M be/src/bufferpool/buffer-pool.h
M be/src/bufferpool/reservation-tracker-test.cc
5 files changed, 102 insertions(+), 32 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9a5a57b7cfccf67ee498e68964f1e077075ee325
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] Remove Llama dependency

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: Remove Llama dependency
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Add search / sort to HTML tables for metrics and threads

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: Add search / sort to HTML tables for metrics and threads
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Internal Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4123: Fast bit unpacking
..


IMPALA-4123: Fast bit unpacking

Adds utility functions for fast unpacking of batches of bit-packed
values. These support reading batches of any number of values provided
that the start of the batch is aligned to a byte boundary. Callers that
want to read smaller batches that don't align to byte boundaries will
need to implement their own buffering.

The unpacking code uses only portable C++ and no SIMD intrinsics, but is
fairly efficient because unpacking a full batch of 32 values compiles
down to 32-bit loads, shifts by constants, masks by constants, bitwise
ors when a value straddles 32-bit words and stores. Further speedups
should be possible using SIMD intrinsics.

Testing:
Added unit tests for unpacking, exhaustively covering different
bitwidths with additional test dimensions (memory alignment, various
input sizes, etc).

Tested under ASAN to ensure the bit unpacking doesn't read past the end
of buffers.

Perf:
Added microbenchmark that shows on average an 8-9x speedup over the
existing BitReader code.

Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Reviewed-on: http://gerrit.cloudera.org:8080/4494
Reviewed-by: Tim Armstrong 
Tested-by: Internal Jenkins
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/bswap-benchmark.cc
M be/src/exprs/expr-test.cc
A be/src/testutil/mem-util.h
M be/src/util/CMakeLists.txt
A be/src/util/bit-packing-test.cc
A be/src/util/bit-packing.h
A be/src/util/bit-packing.inline.h
M be/src/util/bit-stream-utils.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/openssl-util-test.cc
13 files changed, 929 insertions(+), 84 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4123: Fast bit unpacking
..


Patch Set 15: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3342: Add thread counters to monitor plan fragment execution

[anujphadke (Code Review)]

anujphadke has uploaded a new patch set (#6).

Change subject: IMPALA-3342: Add thread counters to monitor plan fragment 
execution
..

IMPALA-3342: Add thread counters to monitor plan fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to measure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 17 insertions(+), 26 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] PREVIEW IMPALA-2521: Add clustered hint to insert statements

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: PREVIEW IMPALA-2521: Add clustered hint to insert statements
..


Patch Set 1:

In a first round I'd like to ask for feedback on the overall approach and the 
open TODO's in the code. The new method in DistributedPlanner.java is currenlty 
still very verbosly commented, mostly because I tried to understand the control 
flow. I can remove most of the comments in the next iteration, but I'd like to 
receive feedback if any of them are wrong.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2905: Move QueryResultSet implementations into separate 
module
..


Patch Set 2: Code-Review+2

(1 comment)

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

PS2, Line 500: (
nit: extraneous parens.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: Yes

[Impala-ASF-CR] PREVIEW IMPALA-2521: Add clustered hint to insert statements

[Lars Volker (Code Review)]

Lars Volker has uploaded a new change for review.

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

Change subject: PREVIEW IMPALA-2521: Add clustered hint to insert statements
..

PREVIEW IMPALA-2521: Add clustered hint to insert statements

This change introduces a clustered/noclustered hint for insert
statements. Specifying this hint adds an additional sort node to the
distributed plan, just before the table sink. This has the effect that
data will be clustered by its partition prior to writing partitions,
which therefore can be written sequentially.

Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14
---
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
5 files changed, 155 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 

[Impala-ASF-CR] IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()
..


Patch Set 4:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

PS4, Line 72:  If overridden in subclass, must also
:   /// call superclass's Codegen().
does it matter whether this happens first or last?


http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exec/hash-join-node.cc
File be/src/exec/hash-join-node.cc:

Line 148:   }
why not do this in ExecNode::Prepare() to avoid having to do the same thing in 
every derived class?


http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exec/partitioned-hash-join-builder.h
File be/src/exec/partitioned-hash-join-builder.h:

PS4, Line 88: whether the codegen was
:   /// enabled 
we only call this if codegen is enabled, right?  So I'm not sure what this part 
means (maybe just delete it).


http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:

Line 409:   // TODO: fix this
not for this change, but i think this timestamp stuff may have been fixed by 
moving things out of the IR.


http://gerrit.cloudera.org:8080/#/c/4651/4/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

PS4, Line 161: 'codegen_'.
public method comments shouldn't really talk about private members. how about 
"... for this fragment instance" or something like that.


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

Line 532
why is it okay to always enable codegen now, whereas before we were so careful 
to only enable it based on this heuristic?


http://gerrit.cloudera.org:8080/#/c/4651/4/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:

Line 80: vector.get_value('exec_option')['disable_codegen'] = 1
this doesn't invalidate any of the preexisting test cases?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I207566bc9f4c6a159271ecdbc4bbdba3d78c6651
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-2905: Move QueryResultSet implementations into separate 
module
..


Patch Set 2: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4736/2//COMMIT_MSG
Commit Message:

Line 7: IMPALA-2905: Move QueryResultSet implementations into separate module
Is this the right JIRA? IMPALA-2905 is fixed.


http://gerrit.cloudera.org:8080/#/c/4736/2/be/src/service/query-result-set.h
File be/src/service/query-result-set.h:

Line 27: #include 
what for?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Alex Behm 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4120: Incorrect results with LEAD() analytic function

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4120: Incorrect results with LEAD() analytic function
..


Patch Set 1:

(4 comments)

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

PS1, Line 18: AggFnEvaluatorthese
?


http://gerrit.cloudera.org:8080/#/c/4740/1/be/src/exec/analytic-eval-node.cc
File be/src/exec/analytic-eval-node.cc:

PS1, Line 389: AggFnEvaluator::GetValue(evaluators_, fn_ctxs_, curr_tuple_, 
result_tuple,
 :   cur_tuple_pool);
wrt my comment in AggFnEvaluator, what if we didn't change GetValue() and 
instead we called a helper fn here that moved any backing memory from 
result_tuple into cur_tuple_pool, e.g.

// Copies memory backing Strings (in analytic result slots) from result_tuple 
into memory allocated from cur_tuple_pool.
RelocateStringMemory(result_tuple, cur_tuple_pool);

(or something like that)


http://gerrit.cloudera.org:8080/#/c/4740/1/be/src/exprs/agg-fn-evaluator.cc
File be/src/exprs/agg-fn-evaluator.cc:

PS1, Line 499: v = StringVal::null();
this means the allocation failed, right? should we be propagating errors? seems 
like that'd lead to incorrect results under low-memory conditions.


http://gerrit.cloudera.org:8080/#/c/4740/1/be/src/exprs/agg-fn-evaluator.h
File be/src/exprs/agg-fn-evaluator.h:

PS1, Line 149:   /// Puts the finalized value from Tuple* src in Tuple* dst 
just as Finalize() does.
 :   /// 'pool' is the MemPool used for allocating memory for the 
finalized value (e.g.
 :   /// StringVal). However, unlike Finalize(), GetValue() does 
not clean up state in src.
 :   /// GetValue() can be called repeatedly with the same src. 
Only used internally for
 :   /// analytic fn builtins.
 :   void GetValue(FunctionContext* agg_fn_ctx, Tuple* src, Tuple* 
dst, MemPool* pool);
I see how this and the below changes to the GetValue() path (all the way 
through SerializeOrFinalize) fix the issue, but I'm worried it's going to be 
more confusing when the interface isn't symmetrical, e.g. other Serialize and 
Finalize can't allocate mem out of pool even though it wouldn't be crazy for 
that to be allowed.

What if the caller handled copying GetValue memory instead of having GetValue() 
handle the string copy? We could have a helper fn somewhere (maybe just in 
AggFnEvaluator, or maybe there's a better place) which moved backing memory 
into a specified pool. Does that make sense?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I85bb1745232d8dd383a6047c86019c6378ab571f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2916: Add warning to query profile if debug build

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-2916: Add warning to query profile if debug build
..


Patch Set 1:

(2 comments)

Thanks for the review, please see PS2.

http://gerrit.cloudera.org:8080/#/c/4588/1/be/src/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:

PS1, Line 88: WARNING
> Let's not use such a generic key. How about "Debug Mode Warning" ?
Done. I made the key all uppercase, so it becomes very easy to spot. Let me 
know if you think it's too much.


PS1, Line 88: "WARNING", "This profile has been created by a DEBUG build. If "
:   "you are looking for performance related information, we 
suggest to use a RELEASE "
:   "build instead."
> Just a suggestion to make it snappier:
Thanks, done.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2916: Add warning to query profile if debug build

[Lars Volker (Code Review)]

Lars Volker has uploaded a new patch set (#2).

Change subject: IMPALA-2916: Add warning to query profile if debug build
..

IMPALA-2916: Add warning to query profile if debug build

Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37
---
M be/src/service/query-exec-state.cc
1 file changed, 4 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Henry Robinson 

[Impala-ASF-CR] Add search / sort to HTML tables for metrics and threads

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new change for review.

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

Change subject: Add search / sort to HTML tables for metrics and threads
..

Add search / sort to HTML tables for metrics and threads

Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f
---
M www/metric_group.tmpl
M www/thread-group.tmpl
2 files changed, 67 insertions(+), 39 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If069ce6a9eae00bacaa30605d23bea72f29e5c4f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 

[Impala-ASF-CR] Buffer pool: Add basic counters to buffer pool client

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: Buffer pool: Add basic counters to buffer pool client
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9a5a57b7cfccf67ee498e68964f1e077075ee325
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
..


Patch Set 7:

(26 comments)

Next round over the code.

http://gerrit.cloudera.org:8080/#/c/4414/7/be/src/service/frontend.cc
File be/src/service/frontend.cc:

Line 62: "value should be a comma separated list of hostnames or IP 
addresses.");
are ports optional or mandatory?


http://gerrit.cloudera.org:8080/#/c/4414/7/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:

Line 358:   1: required list values
Why not a list of TExpr that are expected to be literals? Seems more future 
proof.


Line 368: // the type parameter.
which type parameter?


http://gerrit.cloudera.org:8080/#/c/4414/7/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:

Line 398:   14: optional list distribute_by;
for consistency let's remove trailing ";"


http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

Line 1033: // class doesn't inherit from CreateTableStmt. 
whitespace


Line 1047: // Used for creating external Kudu tables for which the schema is 
loaded from Kudu.
There seem to be more uses of this production, so this comment could be 
misleading. Maybe generalize to something like
"Used for creating tables where the schema is inferred externally, e.g., from 
an Avro schema, Kudu table or query statement."


Line 1112: // or one RANGE clause
typo: clauses


http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java:

Line 93:   void setIsPrimaryKey() { isPrimaryKey_ = true; }
do we need this?


Line 191:   
Preconditions.checkState(!colDefsByColName.containsKey(colDef.getColName()));
can check return value of put()


http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java:

Line 208:* Analyzes and checks table properties which are common for both 
managed and external
typo: common to


Line 255: "PARTITIONED BY cannot be used with an Kudu table.");
typo: a Kudu table

this also needs to be checked for managed tables right?


Line 273: AnalysisUtils.throwIfNullOrEmpty(getPrimaryKeyColumnDefs(),
Shouldn't this check hasPrimaryKey()?


Line 284: "zero. Given number of replicas is: " + r.toString() + 
".'");
remove trailing .' or add the opening single-quote


Line 318:   private boolean hasPrimaryKey() {
Isn't it enough to check primaryKeyColDefs_ in tableDef_?


http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/DistributeParam.java
File fe/src/main/java/org/apache/impala/analysis/DistributeParam.java:

Line 105: for (String colName: colNames_) {
we could specify the same distribution column multiple times


Line 127: throw new AnalysisException("Split values cannot be 
NULL");
do we have a test for this?


Line 223: colNames_.addAll(colNames);
do we need toLower()?


http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java
File fe/src/main/java/org/apache/impala/analysis/TableDef.java:

Line 69:   // Populated during analysis.
Authoritative list of primary key column definitions populated during analysis.


Line 177: fqTableName_.analyze();
Do you know if Kudu has more permissive or more restrictive constraints on what 
strings can be used as table/column names? I'd be surprised if HMS and Kudu 
were identical in that respect. Better to file a JIRA and leave that 
investigation/fix out of this patch.


Line 181: if (analyzer.dbContainsTable(getTblName().getDb(), getTbl(), 
Privilege.CREATE)
Are we going to check Sentry privs for Kudu tables? Also ok to defer this fix, 
but let's not forget.


Line 220:* Analyzes the primary key columns. Primary keys are only 
supported for Kudu
Replace the second sentence with a brief description what this checks. It does 
not check the format and succeeds if no primary keys are given, so there is 
nothing Kudu specific here (that is checked in CreateTableStmt).


Line 234:   StringBuilder columnDefStr = new StringBuilder();
Not used?


http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

Line 1117: if (db != null && params.cascade) dropTablesFromKudu(db);
I think it might be a good idea to do this under the metastoreDdlLock_ as well 
to ensure that the Kudu and HMS table deletions are atomic.


http://gerrit.cloudera.org:8080/#/c/4414/7/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

Line 47: 

[Impala-ASF-CR] Remove Llama dependency

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: Remove Llama dependency
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4739/1/common/thrift/metrics.json
File common/thrift/metrics.json:

PS1, Line 573: 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
> I don't have any way to easily test it. These metrics no longer exist so I 
Looks like we (I) have removed metrics pretty recently anyhow. I couldn't 
remember whether this file constituted any kind of public interface that we 
couldn't remove things from. You should be good to go.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Remove Llama dependency

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: Remove Llama dependency
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4739/1/common/thrift/metrics.json
File common/thrift/metrics.json:

PS1, Line 573: 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
> Does anything break in (e.g.) Cloudera Manager if you remove this?
I don't have any way to easily test it. These metrics no longer exist so I 
can't see how anything downstream could be doing anything with them.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4230: ASF policy issues from 2.7.0 rc3.

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4230: ASF policy issues from 2.7.0 rc3.
..


Patch Set 5: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I30ff77d7ac28ce67511c200764fba19ae69922e0
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Impala-3342: Adding thread counters to measure time spent during plan fragment execution

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: Impala-3342: Adding thread counters to measure time spent 
during plan fragment execution
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4633/5//COMMIT_MSG
Commit Message:

Line 7: Impala-3342: Adding thread counters to measure time spent during plan
The formatting is still weird.


http://gerrit.cloudera.org:8080/#/c/4633/5/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

Line 347: <<< HEAD
Bad rebase?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4024: Add "system" database and expose Impala metrics as a table

[Tim Armstrong (Code Review)]

Tim Armstrong has abandoned this change.

Change subject: IMPALA-4024: Add "system" database and expose Impala metrics as 
a table
..


Abandoned

I need to make time to polish this up and put out a version for review.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I7adbeb45220c468e43b424d70c30b952f6cec2cd
Gerrit-PatchSet: 22
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Kathy Sun 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4123: Fast bit unpacking
..


Patch Set 15:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-packing-test.cc
File be/src/util/bit-packing-test.cc:

Line 132: 
> No seed is the same as a single default seed for this type.
Oh ok, this should be ok too then.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4123: Fast bit unpacking
..


Patch Set 15: Code-Review+2

Carry +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking

[Tim Armstrong (Code Review)]

Hello Jim Apple,

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

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

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

Change subject: IMPALA-4123: Fast bit unpacking
..

IMPALA-4123: Fast bit unpacking

Adds utility functions for fast unpacking of batches of bit-packed
values. These support reading batches of any number of values provided
that the start of the batch is aligned to a byte boundary. Callers that
want to read smaller batches that don't align to byte boundaries will
need to implement their own buffering.

The unpacking code uses only portable C++ and no SIMD intrinsics, but is
fairly efficient because unpacking a full batch of 32 values compiles
down to 32-bit loads, shifts by constants, masks by constants, bitwise
ors when a value straddles 32-bit words and stores. Further speedups
should be possible using SIMD intrinsics.

Testing:
Added unit tests for unpacking, exhaustively covering different
bitwidths with additional test dimensions (memory alignment, various
input sizes, etc).

Tested under ASAN to ensure the bit unpacking doesn't read past the end
of buffers.

Perf:
Added microbenchmark that shows on average an 8-9x speedup over the
existing BitReader code.

Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/bswap-benchmark.cc
M be/src/exprs/expr-test.cc
A be/src/testutil/mem-util.h
M be/src/util/CMakeLists.txt
A be/src/util/bit-packing-test.cc
A be/src/util/bit-packing.h
A be/src/util/bit-packing.inline.h
M be/src/util/bit-stream-utils.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/openssl-util-test.cc
13 files changed, 929 insertions(+), 84 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..

IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

This is to help with IMPALA-4277 to make it easier to build against
Hadoop/Hive distributions where the directory layout doesn't exactly
match our current CDH dependencies, or where we may want to
temporarily override a version without making a source change.

Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
---
M bin/impala-config.sh
M buildall.sh
M cmake_modules/FindHDFS.cmake
M common/thrift/CMakeLists.txt
4 files changed, 27 insertions(+), 21 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3725 Support Kudu UPSERT in Impala
..


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4047/6/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

PS6, Line 734: RESULT = new InsertStmt(w, table, false, null, null, query, 
null, false, true); :}
nit: can you add some newlines to make this spaced like the case above?


http://gerrit.cloudera.org:8080/#/c/4047/6/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:

PS6, Line 75: all non-partition columns in Hive
:   // order.
doesn't quite make sense for the Kudu code. Can you indicate how it's handled 
in that case?


PS6, Line 342: setTargetTable
This does more than sets the table. analyzeTargetTable() would be more accurate.


http://gerrit.cloudera.org:8080/#/c/4047/6/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

PS6, Line 869: ParsesOk("with t as (select 1) upsert into x with t as 
(select 2) select * from t");
 : ParsesOk("with t(c1) as (select 1) " +
 : "upsert into x with t(c2) as (select 2) select * from 
t");
This removes the insert test cases, I assume you didn't mean to change these 
lines


http://gerrit.cloudera.org:8080/#/c/4047/6/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test:

PS6, Line 272: VARCHAR(20)), false),
 : (1, 'known', 1, 0, cast('a' as VARCHAR
why varchars?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4299: add buildall.sh option to start test cluster

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4299: add buildall.sh option to start test cluster
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0ab3461f8ff3de49b3f28a0dc22fa0a6d5569da5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2789: More compact mem layout with null bits at the end.

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2789: More compact mem layout with null bits at the end.
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4673/7/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:

PS7, Line 575: // (in this case the tuple contains only a nullable double)
 : // define void @SetNotNull({ i8, double }* %tuple) {
 : // entry:
 : //   %null_byte_ptr = getelementptr inbounds { i8, double }* 
%tuple, i32 0, i32 0
 : //   %null_byte = load i8* %null_byte_ptr
 : //   %0 = and i8 %null_byte, -2
 : //   store i8 %0, i8* %null_byte_ptr
 : //   ret void
this looks outdated.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6510c75d841bddafa6638f1bd2ac6731a7053f6
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3920: TotalStorageWaitTime counter not populated for fragments with Kudu scan node

[anujphadke (Code Review)]

Hello Matthew Jacobs, Dan Hecht,

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

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

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

Change subject: IMPALA-3920: TotalStorageWaitTime counter not populated for 
fragments with Kudu scan node
..

IMPALA-3920: TotalStorageWaitTime counter not populated for fragments
with Kudu scan node

Currently we do not start the TotalStorageWaitTime timer in the
kudu-scanner. This patch replaces the kudu_read_timer with the
TotalStorageWaitTime which measures the intended time.

Change-Id: If0c793930799fdcaff53e705f94b52cadac2f53a
---
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scan-node.h
M be/src/exec/kudu-scanner.cc
3 files changed, 2 insertions(+), 7 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0c793930799fdcaff53e705f94b52cadac2f53a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
..


Patch Set 5:

(8 comments)

Responses to comments. Starting next round on code.

http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

Line 195:   msClient.alter_table(msTable_.getDbName(), 
msTable_.getTableName(), msTable_);
> Good point. We can potentially avoid this by checking if there has been som
Right, most of the time there will be no changes. I'm not only worried about 
the perf, but all the extra weird states we can be in when doing this extra RPC 
to modify the HMS.


Line 216:   String colName = ToSqlUtils.getIdentSql(colSchema.getName());
> Hm, can you explain why this is the case given that we will use this name t
We don't parse the column names in the HMS backend table and neither does Hive. 
The backticks are only needed to tell our parser to interpret a token as an 
identifier, but the parser is never invoked for column names from the HMS.

I recommend trying this out with some Impala keywords to see what happens.


Line 223: numClusteringCols_ = primaryKeyColumnNames_.size();
> Actually, I removed this and left a TODO. There are a few places that inter
Yea, there's definitely some inconsistency here.


http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

Line 1151: // the metadata must be fetched from the Hive Metastore.
> In order to drop a table from Kudu we need the Kudu table name which is sto
Makes sense now that you've explained, please leave a brief comment.


Line 1172:   if (!KuduTable.isKuduTable(msTable)) continue;
> Well, it's an easy check we can do to avoid RPC calls to Kudu for dropping 
Got it, thanks!


http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

Line 85:   kudu.createTable(kuduTableName, schema, tableOpts);
> I see what you're saying but I don't think this will work. We need to be ab
Agree. Can you file a JIRA against Kudu? Thanks!


Line 178:   if (kudu.tableExists(tableName)) {
> I get it. See my comment above and let me know what you think.
I agree with you.


Line 214: String colName = ToSqlUtils.getIdentSql(colSchema.getName());
> I'd appreciate it if you could explain why is it wrong to use this here and
Explained in other comment, let me know if still unclear :)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Impala-3342: Adding thread counters to measure time spent during plan fragment execution

[anujphadke (Code Review)]

anujphadke has posted comments on this change.

Change subject: Impala-3342: Adding thread counters to measure time spent 
during plan fragment execution
..


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4633/2//COMMIT_MSG
Commit Message:

PS2, Line 7: Impala-3342 Adding thread counters to measure time spent during 
plan
   : fragment execution
> Please fix the formatting of the msg.
Done


PS2, Line 11: meausure
> measure
Done


PS2, Line 13: hdfs/kudu scanner and in a blocking join
> Why is this worth calling out? Doesn't this measure all exec nodes?
This change replaces every instance of the total_cpu_timer. Replacing this 
timer in those 2 places and adding the THREAD_COUNTERS which get aggregated in 
the plan-fragment-executor. 
Adding thread counters in every thread would bulk up the profile.


Line 14: 
> What does the profile look like? Would be helpful to see what the plan frag
Fragment F01:
  Instance c64650f62b67d849:ff790c360002 
(host=anuj-OptiPlex-9020:22000):(Total: 31.008ms, non-child: 0.000ns, % 
non-child: 0.00%)
Hdfs split stats (:<# splits>/): 0:29/3.60 GB 
MemoryUsage(500.000ms): 57.78 MB, 129.77 MB, 137.79 MB, 129.79 MB, 
129.79 MB, 121.79 MB, 153.78 MB, 121.79 MB, 129.79 MB, 129.79 MB, 129.79 MB, 
129.79 MB, 97.79 MB, 137.79 MB, 137.79 MB, 129.79 MB, 121.79 MB, 105.79 MB, 
105.89 MB, 121.79 MB, 129.79 MB, 121.79 MB, 137.79 MB, 113.79 MB, 113.79 MB, 
137.79 MB, 121.79 MB, 129.79 MB, 129.79 MB, 113.79 MB, 105.86 MB, 97.76 MB, 
113.76 MB, 105.76 MB, 73.72 MB
ThreadUsage(500.000ms): 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 
6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 5, 5, 5, 4
 - AverageThreadTokens: 5.86 
 - BloomFilterBytes: 0
 - PeakMemoryUsage: 169.83 MB (178079056)
 - PerHostPeakMemUsage: 1.17 GB (1256325736)
 - PlanFragmentThreadInvoluntaryContextSwitches: 4.32K (4321)
 - PlanFragmentThreadTotalWallClockTime: 1m42s
   - PlanFragmentThreadSysTime: 206.195ms
   - PlanFragmentThreadUserTime: 41s769ms
 - PlanFragmentThreadVoluntaryContextSwitches: 35.57K (35570)
 - PrepareTime: 30.293ms
 - RowsProduced: 30.00M (2795)
 - TotalNetworkReceiveTime: 0.000ns
 - TotalNetworkSendTime: 2s396ms
 - TotalStorageWaitTime: 920.765ms
CodeGen:(Total: 41.328ms, non-child: 41.328ms, % non-child: 100.00%)
   - CodegenTime: 518.571us
   - CompileTime: 3.161ms
   - LoadTime: 0.000ns
   - ModuleBitcodeSize: 1.90 MB (1996720)
   - NumFunctions: 9 (9)
   - NumInstructions: 113 (113)
   - OptimizationTime: 7.696ms
   - PrepareTime: 30.095ms
DataStreamSender (dst_id=5):(Total: 16s230ms, non-child: 16s230ms, % 
non-child: 100.00%)
   - BytesSent: 428.18 MB (448979535)
   - NetworkThroughput(*): 236.47 MB/sec
   - OverallThroughput: 26.38 MB/sec


http://gerrit.cloudera.org:8080/#/c/4633/2/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

Line 213:   ADD_COUNTER(profile(), PER_HOST_PEAK_MEM_COUNTER, TUnit::BYTES);
> Let's create the new counter here along with the other ones so that the set
Done


http://gerrit.cloudera.org:8080/#/c/4633/2/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 241:   ///Fragment thread counters
> The comment isn't too helpful. Maybe:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Yonghyun Hwang
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) Follow Apache Project Branding Requirements

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Follow Apache Project Branding Requirements
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4683/1/bylaws.html
File bylaws.html:

PS1, Line 134: 
 : 
 : 
 : 
> I don't quite understand this change - is all the content going in the mast
This is a separate issue that I threw in here to get this one page to have the 
same width for the body text as the other pages.


http://gerrit.cloudera.org:8080/#/c/4683/1/downloads.html
File downloads.html:

PS1, Line 176: 
 : 
 : 
 : Apache Impala is an effort undergoing incubation at the 
Apache Software Foundation
 : (ASF), sponsored by the Apache Incubator PMC. Incubation is 
required of all newly
 : accepted projects until a further review indicates that the 
infrastructure,
 : communications, and decision making process have stabilized 
in a manner consistent
 : with other successful ASF projects. While incubation status 
is not necessarily a
 : reflection of the completeness or stability of the code, it 
does indicate that the
 :   project has yet to be fully endorsed by the ASF.
 : 
 : Apache Impala, Impala, Apache, the Apache feather logo, 
and the Apache Impala
 : project logo are either registered trademarks or trademarks 
of The Apache Software
 : Foundation in the United States and other countries.
 : 
 :   
> Can you try and keep the indentation consistent for s? It's hard to se
I'm not thrilled with the indentation for these pages, either. 
https://issues.cloudera.org/browse/IMPALA-4307


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I263790d7c9aba7bfc57de7c16a367fe5204c49ff
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) Follow Apache Project Branding Requirements

[Jim Apple (Code Review)]

Jim Apple has uploaded a new patch set (#2).

Change subject: Follow Apache Project Branding Requirements
..

Follow Apache Project Branding Requirements

See: http://www.apache.org/foundation/marks/pmcs.html

Specifically:

1. Add TM to logo

2. Add links back to ASF

3. Add trademark notice to footers

Change-Id: I263790d7c9aba7bfc57de7c16a367fe5204c49ff
---
M bylaws.html
M community.html
M downloads.html
M img/impala-logo.png
M impala-docs.html
M index.html
M overview.html
7 files changed, 186 insertions(+), 53 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I263790d7c9aba7bfc57de7c16a367fe5204c49ff
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 

[Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
..


Patch Set 7:

(29 comments)

http://gerrit.cloudera.org:8080/#/c/4414/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:

Line 1350: // CTAS in an external Kudu table
What's the rationale for not allowing this?


Line 1716: AnalyzesOk("create table tab (x int, y string, primary key(x, 
y)) " +
Can you add a brief comment explaining the table layout for this one?


Line 1717: "distribute by hash(x) into 3 buckets, range(y) split rows " 
+
can we distribute by range, hash?


Line 1723: AnalyzesOk("create table tab (a int, b int, c int, d int, 
primary key (a, b, c))" +
What happens if I specify PARTITION BY for a Kudu table?


Line 1739: AnalysisError(String.format("create table tab (x int) distribute 
by hash (x) " +
What about specifying the other params in tblproperties like the distribute by 
and split rows.


Line 1748: AnalysisError("create table tab (x int primary key, primary 
key(x)) stored " +
Add negative test where two ColumnDefs are declared as PK.


Line 1762: AnalysisError("create table tab (a int, b int, c int, d int, 
primary key(a, b, c)) " +
Do we have tests somewhere for checking which types are supported with Kudu? We 
should make sure that:
* you can create a table with all supported types (and same for the specific 
clauses like primary key, distributed by, etc.)
* you cannot create tables with unsupported types like TIMESTAMP/DECIMAL and 
nested types (should fail gracefully with "not supported")
* or alternatively, if we want to defer the type checks to Kudu (and not bake 
it into Impala analysis), then we should document that somewhere


Line 1772: // No float split keys
Even if the column type is float? If so, might want to test that.


Line 1810: // DISTRIBUTE BY is required for managed tables.
Primary key is also required, do we have a test?


Line 1812: "Table partitioning must be specified for managed Kudu 
tables.");
Let's rephrase this as "Table distribution must be specified ..."


PS6, Line 1828: AnalysisError("create external table t tblproperties (" +
Not that it makes any sense, but hat happens with:

create external table t (primary key()) ...


Line 1863:   public void TestCreateAvroTest() {
Add a test with some of the Kudu-specific clauses and STORED AS AVRO


Line 2822:   public void TestShowFiles() throws AnalysisException {
DO we have a TODO/JIRA somewhere to go through all DDL/SHOW commands and make 
them behave properly for Kudu?


http://gerrit.cloudera.org:8080/#/c/4414/6/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

Line 1632
Why remove this? It will break my setup :)


Line 2235: ParsesOk("CREATE TABLE foo (i INT PRIMARY KEY, PRIMARY KEY(i)) 
STORED AS KUDU");
Add case like this:
CREATE TABLE foo (i INT PRIMARY KEY, j PRIMARY KEY) STORED AS KUDU


Line 2541: ParsesOk("CREATE TABLE Foo TBLPROPERTIES ('a'='b', 'c'='d') AS 
SELECT * from bar");
nit: let's be consistent with how we chop up string literals across lines 
(space at end of previous line xor beginning of next line)


http://gerrit.cloudera.org:8080/#/c/4414/6/fe/src/test/java/org/apache/impala/catalog/HdfsStorageDescriptorTest.java
File fe/src/test/java/org/apache/impala/catalog/HdfsStorageDescriptorTest.java:

Line 58: "org.apache.impala.hive.serde.ParquetInputFormat",
revert


http://gerrit.cloudera.org:8080/#/c/4414/6/testdata/bin/generate-schema-statements.py
File testdata/bin/generate-schema-statements.py:

Line 203:   if row_format and file_format == 'text':
I think row_format is also valid for sequence files (maybe we're not using it 
though)


http://gerrit.cloudera.org:8080/#/c/4414/6/testdata/datasets/functional/functional_schema_template.sql
File testdata/datasets/functional/functional_schema_template.sql:

Line 823: distribute by range(id) split rows ((1003), (1007)) stored as kudu;
ah so much better


http://gerrit.cloudera.org:8080/#/c/4414/6/testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test
File 
testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test:

Line 218: create table kudu_table_clone like functional_kudu.alltypes
Why can't we check this in analysis?


http://gerrit.cloudera.org:8080/#/c/4414/6/testdata/workloads/functional-query/queries/QueryTest/kudu_create.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_create.test:

Line 3: # This test file contains several cases for what basically amount to 
analysis errors,
Should mention that some of this behavior is intentional and why.


Line 11: ImpalaRuntimeException: Type TIMESTAMP is not supported in Kudu
Do we get the same message 

[Impala-ASF-CR] Remove Llama dependency

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: Remove Llama dependency
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4739/2/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

Line 92:   21: optional i16 v_cpu_cores
> add some TODO here
Done


Line 108:   25: optional i64 rm_initial_mem = 0
> add same TODO here
Done


http://gerrit.cloudera.org:8080/#/c/4739/2/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:

Line 137:   V_CPU_CORES,
> add same TODO
Done


Line 153:   RM_INITIAL_MEM,
> add same TODO
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Remove Llama dependency

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: Remove Llama dependency
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables

[Dimitris Tsirogiannis (Code Review)]

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables
..


Patch Set 5:

(41 comments)

http://gerrit.cloudera.org:8080/#/c/4414/5//COMMIT_MSG
Commit Message:

Line 45:"KEY" is expected to be common enough that the ident style
> Might also want to mention that "key" is used for nested map types, so it's
Done


Line 51:on the HMS database and table name. If the database is "default",
> Does this last regarding "default" make the code easier or more complicated
Not sure why the special case. Removing it unless someone objects.


http://gerrit.cloudera.org:8080/#/c/4414/5/be/src/service/frontend.cc
File be/src/service/frontend.cc:

Line 61: DEFINE_string(kudu_master_addrs, "", "Specifies the default Kudu 
master(s). The given "
> Can we make this more consistent with out existing options? For example:
Done


http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java:

Line 176:   static List toColumnNames(Collection columnDefs) 
{
> colDefs
Done


Line 184:   static Map 
mapByColumnNames(Collection colDefs) {
> comment that colDefs is assumed have no duplicate names, or alternatively d
Done


http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java:

Line 99:   "does not support (%s) file format. Supported formats are: 
(%s)",
> the (%s) file format.
Done


Line 100:   createStmt_.getFileFormat().toString().replace("_", ""),
> what's with this weird "_" replacement?
I believe the intention was to make the file format name from THdfsFileFormat 
consistent with the file format name as specified in a "STORED AS" clause. 
RC_FILE is RCFILE, TEXT is TEXTFILE. So, as you can tell we aren't very 
consistent here. That's also the reason for hardcoding the values in L101. Left 
a TODO in the CatalogObjects.thrift for now if that's ok with you.


Line 101:   "PARQUET, TEXTFILE, KUDU"));
> use SUPPORTED_INSERT_FORMATS instead of hardcoding
See comment above.


http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java:

Line 80:   private void setColumnDefs(List columnDefs) {
> nit: colDefs everywhere
Done


Line 95:   Map getTblProperties() { return 
tableDef_.getTblProperties(); }
> is there any significance to not adding public/private to some of these met
Casey started using the default access modifier that essentially gives public 
access only to classes in the same package. In theory, we should be using the 
most restrictive access modifier, so I kind of like this. For sure the codebase 
is not consistent with using this rule, so if you think we shouldn't be doing 
this I'll make them public.


Line 192:   "Only Kudu tables can use DISTRIBUTE BY clause.");
> the DISTRIBUTE BY clause
Done


http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/analysis/TableDef.java
File fe/src/main/java/org/apache/impala/analysis/TableDef.java:

Line 43:  * Represents the table parameters in a CREATE TABLE statement.
> Let's be more precise and list the clauses that this captures.
Done


http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:

Line 97:   // Table name in Kudu storage engine. It may not neccessarily be the 
same as the
> the Kudu storage engine
Done


Line 101:   // 1. For managed tables, 'kuduTableName_' is prefixed with 
'__IMPALA__' to
> Can we make the prefix 'impala::' instead?
Sure no prob. Done


Line 154:* are loaded from HMS. The function also updates the table schema 
in HMS.
> Add a comment about why we also update the HMS. My understanding is that we
Done


Line 195:   msClient.alter_table(msTable_.getDbName(), 
msTable_.getTableName(), msTable_);
> This means every invalidate/refresh will do an extra alter to HMS. Are we c
Good point. We can potentially avoid this by checking if there has been some 
change in the schema. At this point, I am not so terribly worried about this. 
I'll keep a note to actually profile it and see if it's worth optimizing that 
path.


Line 202:* Loads the schema from the Kudu table including column 
definitions and primary key
> mention that it loads the schema into this table as well as the HMS table
Done


Line 216:   String colName = ToSqlUtils.getIdentSql(colSchema.getName());
> getIdentSql() may add backticks, I don't think we want that here
Hm, can you explain why this is the case given that we will use this name to 
populate the columns in 

[Impala-ASF-CR] IMPALA-3823: Add timer to measure Parquet footer reads

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-3823: Add timer to measure Parquet footer reads
..


Patch Set 8: Code-Review+1

(11 comments)

http://gerrit.cloudera.org:8080/#/c/4371/8/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:

PS8, Line 204: Contrary to 
"Unlike"


PS8, Line 204: a 
remove


PS8, Line 206: The average is stored in the 'value_' member of the base class.
Referring to member variables in a class description is usually a sign the text 
is leaking implementation details. Do you mean to say that value() returns the 
average?


Line 209:   SummaryStatsCounter(TUnit::type unit, int32_t total_num_values,
Comment what this constructor is used for (is it when merging two SSCounters 
together?)


PS8, Line 216: total_num_values
can total_num_values be 0?


PS8, Line 247: This keeps track of t
Remove - just "The total..." is sufficient.


PS8, Line 255: SpinLock counter_lock_;
any reason not to call this lock_ ?


http://gerrit.cloudera.org:8080/#/c/4371/8/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

PS8, Line 664: /* Print all SummaryStatsCounters as following:
 : : (Avg:  ; Min:  ; Max:  ;
 : Number of samples: ) */
Comment style?


PS8, Line 669: are
is


PS8, Line 1019: { min_ = new_value; }
remove parentheses for single-line if statements.


http://gerrit.cloudera.org:8080/#/c/4371/8/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

PS8, Line 317: \
remove


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf87bad90037dd0cea63b10c537382ec0f980cbf
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2916: Add warning to query profile if debug build

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-2916: Add warning to query profile if debug build
..


Patch Set 1:

(2 comments)

This will be useful to have.

http://gerrit.cloudera.org:8080/#/c/4588/1/be/src/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:

PS1, Line 88: WARNING
Let's not use such a generic key. How about "Debug Mode Warning" ?


PS1, Line 88: "WARNING", "This profile has been created by a DEBUG build. If "
:   "you are looking for performance related information, we 
suggest to use a RELEASE "
:   "build instead."
Just a suggestion to make it snappier:

"Query profile created while running a DEBUG build of Impala. Use RELEASE 
builds to measure query performance."


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I85ce4d4a5624382203e6b2c8f5b96d04c4482f37
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4720/2/bin/impala-config.sh
File bin/impala-config.sh:

PS2, Line 333: exanded
"expanded"


PS2, Line 335: ${HADOOP_HOME}/share/hadoop/tools/lib/*
Just checking, but does this need updating too?


PS2, Line 404: ${HADOOP_HOME}/lib/native/
There are more here.


PS2, Line 424: ${HADOOP_HOME}/lib/native
And here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Remove Llama dependency

[Tim Armstrong (Code Review)]

Hello Dan Hecht,

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

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

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

Change subject: Remove Llama dependency
..

Remove Llama dependency

This change prevents us from depending on LLAMA to build.

Note that the LLAMA MiniKDC is left in - it is a test
utility that does not depend on LLAMA itself.
IMPALA-4292 tracks cleaning this up.

Testing:
Ran a private build to verify that all tests pass.

Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0
---
M bin/bootstrap_toolchain.py
M bin/generate_minidump_collection_testdata.py
M bin/impala-config.sh
M bin/start-impala-cluster.py
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M common/thrift/generate_metrics.py
M common/thrift/metrics.json
M infra/deploy/deploy.py
9 files changed, 6 insertions(+), 43 deletions(-)


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

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

[Impala-ASF-CR](asf-site) Follow Apache Project Branding Requirements

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: Follow Apache Project Branding Requirements
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4683/1/bylaws.html
File bylaws.html:

PS1, Line 134: 
 : 
 : 
 : 
I don't quite understand this change - is all the content going in the masthead 
or navbar div now?


http://gerrit.cloudera.org:8080/#/c/4683/1/downloads.html
File downloads.html:

PS1, Line 176: 
 : 
 : 
 : Apache Impala is an effort undergoing incubation at the 
Apache Software Foundation
 : (ASF), sponsored by the Apache Incubator PMC. Incubation is 
required of all newly
 : accepted projects until a further review indicates that the 
infrastructure,
 : communications, and decision making process have stabilized 
in a manner consistent
 : with other successful ASF projects. While incubation status 
is not necessarily a
 : reflection of the completeness or stability of the code, it 
does indicate that the
 :   project has yet to be fully endorsed by the ASF.
 : 
 : Apache Impala, Impala, Apache, the Apache feather logo, 
and the Apache Impala
 : project logo are either registered trademarks or trademarks 
of The Apache Software
 : Foundation in the United States and other countries.
 : 
 :   
Can you try and keep the indentation consistent for s? It's hard to see 
where this lines up relative to the navbar div above.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I263790d7c9aba7bfc57de7c16a367fe5204c49ff
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4080, IMPALA-3638: Introduce ExecNode::Codegen()
..


Patch Set 4:

Do we still plan to make the ExecNodes' Codegen() be static (in a future 
patch), or has that plan changed?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I207566bc9f4c6a159271ecdbc4bbdba3d78c6651
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Charlie Helin (Code Review)]

Charlie Helin has posted comments on this change.

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..


Patch Set 1: -Code-Review

> Did a grep for HADOOP_HOME, and this might need to be changed too:
 > https://github.com/apache/incubator-impala/blob/75a857c0ceb6a58691ff45f0df8d88586edc8acd/buildall.sh#L357

Yes here it seems like ${HADOOP_HOME}\lib should categorically be substituted 
by ${HADOOP_LIB_DIR}. But regarding this line it also looks like we need to 
parameterize HADOOP LZO?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3920: TotalStorageWaitTime counter not populated for fragments with Kudu scan node

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3920: TotalStorageWaitTime counter not populated for 
fragments with Kudu scan node
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If0c793930799fdcaff53e705f94b52cadac2f53a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..


Patch Set 1:

Did a grep for HADOOP_HOME, and this might need to be changed too:
https://github.com/apache/incubator-impala/blob/75a857c0ceb6a58691ff45f0df8d88586edc8acd/buildall.sh#L357

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3504: UDF for current timestamp in UTC

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3504: UDF for current timestamp in UTC
..


Patch Set 8:

(1 comment)

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

PS8, Line 842:   
query_ctx->__set_utc_timestamp_string(TimestampValue::UtcTime().DebugString());
 :   
query_ctx->__set_now_string(TimestampValue::LocalTime().DebugString());
it would be better if NOW() and UTC_TIMESTAMP() return the same instance in 
time (represented in different timezones).  The current implementation will 
return slightly different instances in time.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5ee6ee192aa469f77c711f27ad324696a42004d1
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Youwei Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Youwei Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4216: Test became flaky: TestTpchMemLimitError.test low mem limit q20

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4216: Test became flaky: 
TestTpchMemLimitError.test_low_mem_limit_q20
..


Patch Set 1:

But why do we no longer get the MEM_LIMIT_EXCEEDED error status returned by 
this query?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I471a90c8e3bdecb4fed08fbae3436c50b8618471
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4720/1/bin/impala-config.sh
File bin/impala-config.sh:

Line 305: export 
IMPALA_LLAMA_MINIKDC_VERSION=${IMPALA_LLAMA_MINIKDC_VERSION:-1.0.0}
> This is actually separate from Llama - a test utility we stole from an old 
I'm also going to post a separate patch to remove Llama (needed to do some 
testing there to make sure it didn't break anything).


PS1, Line 341: $HADOOP_HOME/bin
> If HADOOP_INCLUDE_DIR and HADOOP_LIB_DIR point to a different path than HAD
I think if they were set to incompatible versions it might cause a problem 
since we'd be building against the wrong version of Hadoop (e.g Hadoop 2 API 
versus Hadoop 3 API). I think it's reasonable to assume that it won't be set up 
to point to incompatible versions.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4720/1/bin/impala-config.sh
File bin/impala-config.sh:

PS1, Line 341: $HADOOP_HOME/bin
If HADOOP_INCLUDE_DIR and HADOOP_LIB_DIR point to a different path than 
HADOOP_HOME/(include)|(lib)/ , wouldn't this cause an issue?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR](hadoop-next) IMPALA-4277: bump Hadoop component versions except for Hadoop itself

[Charlie Helin (Code Review)]

Charlie Helin has posted comments on this change.

Change subject: IMPALA-4277: bump Hadoop component versions except for Hadoop 
itself
..


Patch Set 1:

(1 comment)

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

PS1, Line 16: This builds if I manually put hive_metastore.thrift in the right 
place
> We don't have the special component tarballs for CDH6.x, so I just did this
Ohh I see that looks better!


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0be6bbe76d929ceaeb2fa9ac4ebba1820c4dab7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: hadoop-next
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Remove Llama dependency

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new change for review.

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

Change subject: Remove Llama dependency
..

Remove Llama dependency

This change prevents us from depending on LLAMA to build.

Note that the LLAMA MiniKDC is left in - it is a test
utility that does not depend on LLAMA itself.
IMPALA-4292 tracks cleaning this up.

Testing:
Ran a private build to verify that all tests pass.

Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0
---
M bin/bootstrap_toolchain.py
M bin/generate_minidump_collection_testdata.py
M bin/impala-config.sh
M bin/start-impala-cluster.py
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M common/thrift/generate_metrics.py
M common/thrift/metrics.json
M infra/deploy/deploy.py
9 files changed, 6 insertions(+), 43 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If2e5e21d8047097d56062ded11b0832a1d397fe0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4720/1/bin/impala-config.sh
File bin/impala-config.sh:

Line 305: export 
IMPALA_LLAMA_MINIKDC_VERSION=${IMPALA_LLAMA_MINIKDC_VERSION:-1.0.0}
> Should this not be removed, in C6 we won't have llama?
This is actually separate from Llama - a test utility we stole from an old 
version of the Llama codebase and have kept around. Not quite sure what we're 
going to do about it but there's a JIRA IMPALA-4292.

It also doesn't affect the final build bits.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Charlie Helin (Code Review)]

Charlie Helin has posted comments on this change.

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4720/1/bin/impala-config.sh
File bin/impala-config.sh:

Line 305: export 
IMPALA_LLAMA_MINIKDC_VERSION=${IMPALA_LLAMA_MINIKDC_VERSION:-1.0.0}
Should this not be removed, in C6 we won't have llama?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Impala-3342 Adding thread counters to measure time spent during plan fragment execution

[anujphadke (Code Review)]

anujphadke has uploaded a new patch set (#3).

Change subject: Impala-3342 Adding thread counters to measure time spent during 
plan fragment execution
..

Impala-3342 Adding thread counters to measure time spent during plan
fragment execution

This change removes the use of total_cpu_timer which incorrectly
monitors the CPU time. Adding THREAD_COUNTERS to meausure the user
and sys time in plan fragment execution. This also accounts for the
time spent in the hdfs/kudu scanner and in a blocking join.

Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/kudu-scan-node.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/plan-fragment-executor.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
7 files changed, 17 insertions(+), 28 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifa88aa6f3371fa42d11ecc122f43c7d83623c300
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Yonghyun Hwang

[Impala-ASF-CR](hadoop-next) IMPALA-4277: bump Hadoop component versions except for Hadoop itself

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4277: bump Hadoop component versions except for Hadoop 
itself
..

IMPALA-4277: bump Hadoop component versions except for Hadoop itself

The Hive version bump required some minor changes to deal with renames
and added function arguments. The other components required no changes
to build the frontend against their API (I haven't been able to test
the JNI backend stuff).

IMPALA-4172 is blocking the hadoop version bump.

This builds if I manually put hive_metastore.thrift in the right place
and set SKIP_TOOLCHAIN_BOOTSTRAP=false. Tests can't run since we don't
have the special tarballs available that are used for the test cluster.

I manually constructed the Hive build dependency by taking
hive_metastore.thrift and putting it at
$CDH_COMPONENTS_HOME/hive-2.1.0-cdh6.x-SNAPSHOT/src/metastore/if/hive_metastore.thrift

The Hive API changes were:

* org.apache.hive.service.cli moved to org.apache.hive.service.rpc
* MetaStoreUtils.validateName() and
  MetaStoreUtils.updatePartitionStatsFast() have additional context
  arguments.
* HiveConf.ConfVars.METASTORE_BATCH_RETRIEVE_TABLE_PARTITION_MAX
  was renamed to METASTORE_BATCH_RETRIEVE_OBJECTS_MAX

Change-Id: Ia0be6bbe76d929ceaeb2fa9ac4ebba1820c4dab7
---
M bin/impala-config.sh
M common/thrift/TCLIService.thrift
M common/thrift/cli_service.thrift
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CreateDataSrcStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java
M fe/src/main/java/org/apache/impala/analysis/DropDataSrcStmt.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/analysis/TableName.java
M fe/src/main/java/org/apache/impala/analysis/TypeDef.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/service/FrontendTest.java
16 files changed, 38 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia0be6bbe76d929ceaeb2fa9ac4ebba1820c4dab7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: hadoop-next
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR](hadoop-next) IMPALA-4277: bump Hadoop component versions except for Hadoop itself

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4277: bump Hadoop component versions except for Hadoop 
itself
..


Patch Set 1:

(3 comments)

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

PS1, Line 16: This builds if I manually put hive_metastore.thrift in the right 
place
> I agree with the question above. Would it not be better to use the env vari
See https://gerrit.cloudera.org/#/c/4720/


PS1, Line 16: This builds if I manually put hive_metastore.thrift in the right 
place
> Why does this need to manually be put in the right place?
We don't have the special component tarballs for CDH6.x, so I just did this to 
validate that it can build.


Line 23: 
> Since there are only a few API changes, I think it would be good if you cou
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0be6bbe76d929ceaeb2fa9ac4ebba1820c4dab7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: hadoop-next
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4123: Fast bit unpacking
..

IMPALA-4123: Fast bit unpacking

Adds utility functions for fast unpacking of batches of bit-packed
values. These support reading batches of any number of values provided
that the start of the batch is aligned to a byte boundary. Callers that
want to read smaller batches that don't align to byte boundaries will
need to implement their own buffering.

The unpacking code uses only portable C++ and no SIMD intrinsics, but is
fairly efficient because unpacking a full batch of 32 values compiles
down to 32-bit loads, shifts by constants, masks by constants, bitwise
ors when a value straddles 32-bit words and stores. Further speedups
should be possible using SIMD intrinsics.

Testing:
Added unit tests for unpacking, exhaustively covering different
bitwidths with additional test dimensions (memory alignment, various
input sizes, etc).

Tested under ASAN to ensure the bit unpacking doesn't read past the end
of buffers.

Perf:
Added microbenchmark that shows on average an 8-9x speedup over the
existing BitReader code.

Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/bswap-benchmark.cc
M be/src/exprs/expr-test.cc
A be/src/testutil/mem-util.h
M be/src/util/CMakeLists.txt
A be/src/util/bit-packing-test.cc
A be/src/util/bit-packing.h
A be/src/util/bit-packing.inline.h
M be/src/util/bit-stream-utils.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/openssl-util-test.cc
13 files changed, 929 insertions(+), 84 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4123: Fast bit unpacking

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4123: Fast bit unpacking
..

IMPALA-4123: Fast bit unpacking

Adds utility functions for fast unpacking of batches of bit-packed
values. These support reading batches of any number of values provided
that the start of the batch is aligned to a byte boundary. Callers that
want to read smaller batches that don't align to byte boundaries will
need to implement their own buffering.

The unpacking code uses only portable C++ and no SIMD intrinsics, but is
fairly efficient because unpacking a full batch of 32 values compiles
down to 32-bit loads, shifts by constants, masks by constants, bitwise
ors when a value straddles 32-bit words and stores. Further speedups
should be possible using SIMD intrinsics.

Testing:
Added unit tests for unpacking, exhaustively covering different
bitwidths with additional test dimensions (memory alignment, various
input sizes, etc).

Tested under ASAN to ensure the bit unpacking doesn't read past the end
of buffers.

Perf:
Added microbenchmark that shows on average an 8-9x speedup over the
existing BitReader code.

Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-packing-benchmark.cc
M be/src/benchmarks/bswap-benchmark.cc
M be/src/exprs/expr-test.cc
A be/src/testutil/mem-util.h
M be/src/util/CMakeLists.txt
A be/src/util/bit-packing-test.cc
A be/src/util/bit-packing.h
A be/src/util/bit-packing.inline.h
M be/src/util/bit-stream-utils.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/openssl-util-test.cc
13 files changed, 928 insertions(+), 84 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR](hadoop-next) IMPALA-4277: temporary hack to avoid compile errors for HDFS block location API

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4277: temporary hack to avoid compile errors for HDFS 
block location API
..


Patch Set 1: Code-Review+2

(1 comment)

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

Line 15: hadoop-3.0.0-alpha1-cdh6.x-SNAPSHOT/lib/native/libgplcompression.a
> I just did it by hand to verify. For Cauldron I'm hoping to just use these 
Ok, would be worth checking (if you haven't already) if with both the patches 
and the right environment variables, everything builds correctly.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ice765b1be62c0c3d1b99282d8c652dd35d3d766d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: hadoop-next
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4277: allow overriding of Hive/Hadoop versions/locations

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4277: allow overriding of Hive/Hadoop versions/locations
..


Patch Set 1:

Is this necessary for the master branch? Once we move permanently to C6, we 
wouldn't need these optional settings, am I right?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7da10e38f9c4309f2d193dc25f14a6ea308c9639
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR](hadoop-next) IMPALA-4277: temporary hack to avoid compile errors for HDFS block location API

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4277: temporary hack to avoid compile errors for HDFS 
block location API
..


Patch Set 1:

(1 comment)

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

Line 15: hadoop-3.0.0-alpha1-cdh6.x-SNAPSHOT/lib/native/libgplcompression.a
> Is there a temporary script that does this copy of the minimum files? Or do
I just did it by hand to verify. For Cauldron I'm hoping to just use these 
environment variables to point it to the lib/include directories 
https://gerrit.cloudera.org/#/c/4720/


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ice765b1be62c0c3d1b99282d8c652dd35d3d766d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: hadoop-next
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR](hadoop-next) IMPALA-4277: temporary hack to avoid compile errors for HDFS block location API

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4277: temporary hack to avoid compile errors for HDFS 
block location API
..


Patch Set 1:

(1 comment)

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

Line 15: hadoop-3.0.0-alpha1-cdh6.x-SNAPSHOT/lib/native/libgplcompression.a
Is there a temporary script that does this copy of the minimum files? Or does 
it have to be done manually every time it needs to be built with cauldron?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ice765b1be62c0c3d1b99282d8c652dd35d3d766d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: hadoop-next
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR](hadoop-next) IMPALA-4277: bump Hadoop component versions except for Hadoop itself

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4277: bump Hadoop component versions except for Hadoop 
itself
..


Patch Set 1:

(2 comments)

Sorry for the delay.

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

PS1, Line 16: This builds if I manually put hive_metastore.thrift in the right 
place
Why does this need to manually be put in the right place?


Line 23: 
Since there are only a few API changes, I think it would be good if you could 
add a link here (if one exists) to the function signatures that changed, or 
just list them out.
It will be good to refer back to this in the case of any issues related to 
these new APIs we see in testing in the future.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0be6bbe76d929ceaeb2fa9ac4ebba1820c4dab7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: hadoop-next
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Charlie Helin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] review comments 1

[Henry Robinson (Code Review)]

Henry Robinson has abandoned this change.

Change subject: review comments 1
..


Abandoned

Mistakenly pushed.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I3d1094dffd6b0341f8554283c3b77c6a970ca7ec
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 

[Impala-ASF-CR] review comments 1

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new change for review.

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

Change subject: review comments 1
..

review comments 1

Change-Id: I3d1094dffd6b0341f8554283c3b77c6a970ca7ec
---
M be/src/runtime/coordinator.cc
1 file changed, 0 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3d1094dffd6b0341f8554283c3b77c6a970ca7ec
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 

[Impala-ASF-CR] IMPALA-2905: Move QueryResultSet implementations into separate module

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new change for review.

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

Change subject: IMPALA-2905: Move QueryResultSet implementations into separate 
module
..

IMPALA-2905: Move QueryResultSet implementations into separate module

This mostly mechanical change moves the definition and implementation of
the Beeswax and HS2-specific result sets into their own module. Result
sets are now uniformly created by one of two factory methods, so the
implementation is decoupled from the client.

Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32
---
M be/src/service/CMakeLists.txt
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
A be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
5 files changed, 504 insertions(+), 419 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6ab883b62d3ec7012240edf8d56889349e7c0e32
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 

[Impala-ASF-CR] IMPALA-4299: add buildall.sh option to start test cluster

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new change for review.

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

Change subject: IMPALA-4299: add buildall.sh option to start test cluster
..

IMPALA-4299: add buildall.sh option to start test cluster

A previous commit "IMPALA-4259: build Impala without any test
cluster setup" altered some undocumented side-effects of
buildall.sh.

Previously the following commands reconfigured and restarted the test
cluster. It worked because buildall.sh unconditionally regenerated
the test cluster configs.

  ./buildall.sh -notests && ./testdata/bin/run-all.sh
  ./buildall.sh -noclean -notests && ./testdata/bin/run-all.sh

Instead of restoring the old behaviour and continuing to encourage
mixing use of low and high level scripts like testdata/bin/run-all.sh
as part of the "standard" workflow, this commit adds another
high-level option to buildall.sh, -start_minicluster, that
accomplishes the high-level task of restarting a minicluster with
fresh configs. The above commands can be replaced with:

  ./buildall.sh -notests -start_minicluster
  ./buildall.sh -notests -noclean -start_minicluster

Change-Id: I0ab3461f8ff3de49b3f28a0dc22fa0a6d5569da5
---
M buildall.sh
1 file changed, 14 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0ab3461f8ff3de49b3f28a0dc22fa0a6d5569da5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4230: ASF policy issues from 2.7.0 rc3.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4230: ASF policy issues from 2.7.0 rc3.
..


Patch Set 5:

Tim, Taras, any other concerns?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I30ff77d7ac28ce67511c200764fba19ae69922e0
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4270: Gracefully fail unsupported queries with mt dop > 0.

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4270: Gracefully fail unsupported queries with mt_dop > 
0.
..


IMPALA-4270: Gracefully fail unsupported queries with mt_dop > 0.

MT_DOP > 0 is only supported for plans without distributed joins
or table sinks. Adds validation to fail unsupported queries
gracefully in planning.

For scans in queries that are executable with MT_DOP > 0 we either
use the optimized MT scan node BE implementation (only Parquet), or
we use the conventional scan node with num_scanner_threads=1.

TODO: Still need to add end-to-end tests.

Change-Id: I91a60ea7b6e3ae4ee44be856615ddd3cd0af476d
Reviewed-on: http://gerrit.cloudera.org:8080/4677
Reviewed-by: Alex Behm 
Tested-by: Internal Jenkins
---
M be/src/exec/exec-node.cc
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/PlannerContext.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
A 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
10 files changed, 450 insertions(+), 34 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Alex Behm: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I91a60ea7b6e3ae4ee44be856615ddd3cd0af476d
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 

[Impala-ASF-CR] IMPALA-4270: Gracefully fail unsupported queries with mt dop > 0.

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4270: Gracefully fail unsupported queries with mt_dop > 
0.
..


Patch Set 6: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I91a60ea7b6e3ae4ee44be856615ddd3cd0af476d
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No