date:20170920

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5416: Fix an impala-shell command recursion bug
..


Patch Set 5:

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

It seems to be related to IMPALA-5920. I will do a rebasing again.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I453af2d4694d47e184031cb07ecd2af259ba20f3
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5416: Fix an impala-shell command recursion bug

2017-09-20 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5416: Fix an impala-shell command recursion bug
..


Patch Set 5: Verified-1

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I453af2d4694d47e184031cb07ecd2af259ba20f3
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] Remove unused MemPool::peak allocated bytes

2017-09-20 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: Remove unused MemPool::peak_allocated_bytes_
..


Patch Set 1:

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

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

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

[Impala-ASF-CR] Remove unused MemPool::peak allocated bytes

Dan Hecht has posted comments on this change.

Change subject: Remove unused MemPool::peak_allocated_bytes_
..


Patch Set 1: Code-Review+2

Nice!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99eba01869914c1d1e0a6ed0cab039d904fecc02
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: anujphadke 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4863/IMPALA-5311: Correctly account the file type and compression codec

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4863/IMPALA-5311: Correctly account the file type and 
compression codec
..


Patch Set 11: Code-Review+2

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

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

[Impala-ASF-CR] IMPALA-4863/IMPALA-5311: Correctly account the file type and compression codec

2017-09-20 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4863/IMPALA-5311: Correctly account the file type and 
compression codec
..


Patch Set 11:

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

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

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

[Impala-ASF-CR] IMPALA-5966: Fix the result file location of PlannerTest

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5966: Fix the result file location of PlannerTest
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8113/1/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java:

Line 90:   private final String testDir_ = 
"functional-planner/queries/PlannerTest";
> also do the same for testDir_ while you are here
Done


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

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

[Impala-ASF-CR] IMPALA-5966: Fix the result file location of PlannerTest

Tianyi Wang has uploaded a new patch set (#2).

Change subject: IMPALA-5966: Fix the result file location of PlannerTest
..

IMPALA-5966: Fix the result file location of PlannerTest

The Plannertest result files should be written to
$IMPALA_FE_TEST_LOGS_DIR/PlannerTest, but instead they are written to
$IMPALA_FE_TEST_LOGS_DIR with "PlannerTest" as the prefix of their
filenames. This patch fixes this bug and refactores surrounding code a
little, replacing some Strings with Paths.

Change-Id: I20005bd0421e1bb9f3eedbb003c97d92a8faf110
---
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
1 file changed, 9 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I20005bd0421e1bb9f3eedbb003c97d92a8faf110
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

2017-09-20 Thread Tim Wood (Code Review)

Tim Wood has posted comments on this change.

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 1:

@mmokhtar
Results of elapsed-time measurements of partial and full (this commit) TPC-DS 
suite run:
Partial: 54.36 sec.  
https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/301/
Full:232.79 sec.  
https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/297/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4863/IMPALA-5311: Correctly account the file type and compression codec

Hello Impala Public Jenkins, Tim Armstrong,

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

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

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

Change subject: IMPALA-4863/IMPALA-5311: Correctly account the file type and 
compression codec
..

IMPALA-4863/IMPALA-5311: Correctly account the file type and compression codec

If a scan range is skipped at runtime the scan node skips reading
the range and never figures out the underlying compression codec used
to compress the files. In such a scenario we default the compression
codec to NONE which can be misleading. This change marks these files
as filtered in the scan node profile

e.g. - File Formats: TEXT/NONE:364 TEXT/NONE(Skipped):1460

Change-Id: I797916505f62e568f4159e07099481b8ff571da2
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M 
testdata/workloads/functional-query/queries/QueryTest/hdfs_scanner_profile.test
M tests/query_test/test_scanners.py
7 files changed, 71 insertions(+), 20 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I797916505f62e568f4159e07099481b8ff571da2
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke

[Impala-ASF-CR] IMPALA-5966: Fix the result file location of PlannerTest

Alex Behm has posted comments on this change.

Change subject: IMPALA-5966: Fix the result file location of PlannerTest
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8113/1/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java:

Line 90:   private final String testDir_ = 
"functional-planner/queries/PlannerTest";
also do the same for testDir_ while you are here


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

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

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

2017-09-20 Thread Zach Amsden (Code Review)

Zach Amsden has posted comments on this change.

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


Patch Set 5:

Have we ever tried using SSE/AVX for multiplication?  It should be possible to 
avoid using int256_t altogether if we can do four 64-bit multiplies in 
parallel.  Are FMA instructions available for integer? (I have the manual in 
front of me but have to run and no time to look it up).  We should be able to 
do the division the same way with another round of multiplication.

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

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

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 8:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7805/8/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS8, Line 67: SetNonExistOptions)
> SetNonExistentOptions.
Done


Line 124:   {MAKE_KEYDEF(max_row_size),  {1, I64_MAX}},
> Should add DEFAULT_SPILLABLE_BUFFER_SIZE and MIN_SPILLABLE_BUFFER_SIZE. Tho
They are in SetSpecialOptions.


Line 175:   // ENTRIES(TPrefetchMode, (None)(HT_BUCKET)) expands to
> Can you add blank lines after the macro definitions? It's a bit hard to see
Done


PS8, Line 219: err
> It might be clearer to call these functions something like TestError() and 
Done.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

Tianyi Wang has uploaded a new patch set (#9).

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..

IMPALA-5425: Add test for validating input when setting query options

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 251 insertions(+), 153 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/9
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] Remove unused MemPool::peak allocated bytes

anujphadke has posted comments on this change.

Change subject: Remove unused MemPool::peak_allocated_bytes_
..


Patch Set 1: Code-Review+1

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

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

[Impala-ASF-CR] Remove unused MemPool::peak allocated bytes

Tim Armstrong has uploaded a new change for review.

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

Change subject: Remove unused MemPool::peak_allocated_bytes_
..

Remove unused MemPool::peak_allocated_bytes_

The value is not used for anything but there is code devoted to updating
it and testing the value.

Testing:
Ran mem-pool-test to confirm it still works.

Change-Id: I99eba01869914c1d1e0a6ed0cab039d904fecc02
---
M be/src/runtime/mem-pool-test.cc
M be/src/runtime/mem-pool.cc
M be/src/runtime/mem-pool.h
3 files changed, 2 insertions(+), 32 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-4863/IMPALA-5311: Correctly account the file type and compression codec

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4863/IMPALA-5311: Correctly account the file type and 
compression codec
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7245/9/testdata/workloads/functional-query/queries/QueryTest/hdfs_scanner_profile.test
File 
testdata/workloads/functional-query/queries/QueryTest/hdfs_scanner_profile.test:

PS9, Line 13: 608
Is this number stable? Or does it depend on how scan ranges and assigned to 
backends?


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

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

[Impala-ASF-CR] IMPALA-5966: Fix the result file location of PlannerTest

Tianyi Wang has uploaded a new change for review.

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

Change subject: IMPALA-5966: Fix the result file location of PlannerTest
..

IMPALA-5966: Fix the result file location of PlannerTest

The Plannertest result files should be written to
$IMPALA_FE_TEST_LOGS_DIR/PlannerTest, but instead they are written to
$IMPALA_FE_TEST_LOGS_DIR with "PlannerTest" as the prefix of their
filenames. This patch fixes this bug.

Change-Id: I20005bd0421e1bb9f3eedbb003c97d92a8faf110
---
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
1 file changed, 6 insertions(+), 9 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 8:

(4 comments)

I'm generally ok with this patch. I think there are places where there's a 
trade-off between using macros/lambdas and repeating ourselves. My general 
feeling is that there wouldn't be a net improvement to the code by changing 
that, but I'm ok with whatever Lars and Tianyi figure out.

http://gerrit.cloudera.org:8080/#/c/7805/8/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS8, Line 67: SetNonExistOptions)
SetNonExistentOptions.


Line 124:   {MAKE_KEYDEF(max_row_size),  {1, I64_MAX}},
Should add DEFAULT_SPILLABLE_BUFFER_SIZE and MIN_SPILLABLE_BUFFER_SIZE. Those 
might require some additional logic since they're restricted to power-of-twos.


Line 175:   // ENTRIES(TPrefetchMode, (None)(HT_BUCKET)) expands to
Can you add blank lines after the macro definitions? It's a bit hard to see 
which comment goes with which macro currently.


PS8, Line 219: err
It might be clearer to call these functions something like TestError() and 
TestOk(). I don't feel strongly though - this seems readable enough to me.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

Matthew Jacobs has posted comments on this change.

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


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG
Commit Message:

PS9, Line 29: For request_pool, this means that setting the default
: request_pool via impalad command line is now a bad idea
> Okay. What is the right way for the user to do this (so that we can documen
To do what? To have a default pool? That should rely on using the 'default' 
rule.


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

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

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

2017-09-20 Thread Thomas Tauber-Marshall (Code Review)

Thomas Tauber-Marshall has posted comments on this change.

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


Patch Set 5:

(1 comment)

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

Line 225:   Constant* expr_type_arg = codegen->ConstantToGVPtr(
> Yeah, I have no idea why it doesn't work. The types definitely line up or i
So I've been playing around with this more, and I have a theory. I noticed that 
it works if I only replace 'GetType' and not 'GetValue', so the problem may 
actually be with 'GetValue'.

I think that the value that's being returned by the function generated by 
GetCodegendComputePtrFn() is stack allocated (because ToNativePtr() is called 
on 'result' which then calls CreateEntryBlockAlloca, which allocates stack 
space).

Basically, GetCodegendComputePtrFn() is trying to create a codegend copy of 
ScalarExprEvaluator::GetValue() but its not currently doing the part where the 
value is stored in 'result_' so that a pointer to it can be passed back.

One solution would be to figure out how to write IRBuilder for storing the 
value in 'result_' (or else cross compile ScalarExprEvaluator::GetValue() and 
sub things in), but that's potentially complicated. It may also be worth just 
going back to the original version of this patch to eliminate the perf cost 
from having to store the value in 'result_'.

Or maybe you had something in mind with your other comment about not needing to 
create a whole new function that would solve this problem as well.


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

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

[Impala-ASF-CR] IMPALA-5954: Set DO NOT UPDATE STATS in alterTable() RPCs to HMS.

2017-09-20 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5954: Set DO_NOT_UPDATE_STATS in alterTable() RPCs to 
HMS.
..


Patch Set 2: Code-Review+1

(2 comments)

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

Line 800:   // HMS requires this param for stats changes to take effect.
> I checked the Hive/HMS code carefully and partition-level stat are not affe
Thanks for explaining. I agree.


PS1, Line 822: }
 : // HMS requires this param for stats changes to take effect.
> Setting this flag is required for for stats changes to take effect (table o
Sorry, I got confused. I now see how all this works.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I05c63315579d66efeaa4c65105372ae68820ab2f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-HasComments: Yes

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

Dan Hecht has posted comments on this change.

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


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG
Commit Message:

PS9, Line 29: For request_pool, this means that setting the default
: request_pool via impalad command line is now a bad idea
> I don't have data to back this up, but I'd guess that some folks are probab
Okay. What is the right way for the user to do this (so that we can document 
that as the "workaround")?  Or do we think that this setup is not useful?


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

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

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

2017-09-20 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..


Patch Set 5:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

2017-09-20 Thread Lars Volker (Code Review)

Lars Volker has posted comments on this change.

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..


Patch Set 5: Code-Review+2

Rebased, carrying Tim's +2.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4863/IMPALA-5311: Correctly account the file type and compression codec

anujphadke has posted comments on this change.

Change subject: IMPALA-4863/IMPALA-5311: Correctly account the file type and 
compression codec
..


Patch Set 8:

Ran the core tests and the exhaustive tests with the change and they passed.

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

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

[Impala-ASF-CR] IMPALA-5250: Unify decompressor output length semantics

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5250: Unify decompressor output_length semantics
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8030/3//COMMIT_MSG
Commit Message:

Line 33: 
> Add a separate section for testing. Mention the tests you added and the per
Done


http://gerrit.cloudera.org:8080/#/c/8030/1/be/src/util/decompress-test.cc
File be/src/util/decompress-test.cc:

Line 196:   int64_t* output_len, uint8_t** output, bool 
output_preallocated) {
> Your interpretation is correct.
A test cases is added.


http://gerrit.cloudera.org:8080/#/c/8030/1/be/src/util/decompress.cc
File be/src/util/decompress.cc:

Line 426: 
> Agree this in/out pattern is not great and we should consider your proposal
All output_length will be set to 0 if the decompression fails now.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd42942b169921a7eb53940c3762bc45bb82a993
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5250: Unify decompressor output length semantics

Hello Alex Behm,

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

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

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

Change subject: IMPALA-5250: Unify decompressor output_length semantics
..

IMPALA-5250: Unify decompressor output_length semantics

This patch makes the semantics of the output_length parameter in
Codec::ProcessBlock to be the same across all codecs. In existing code
different decompressor treats output_length differently:
1. SnappyDecompressor needs output_length to be greater than or equal
   to the actual decompressed length, but it does not set it to the
   actual decompressed length after decompression.
2. SnappyBlockDecompressor and Lz4Decompressor require output_length to
   be exactly the same as the actual decompressed length, otherwise
   decompression fails.
3. Other decompressors need output_length to be greater than or equal to
   the actual decompressed length and will set it to actual decompressed
   length if oversized.
This inconsistency leads to a bug where the error message is
undeterministic when the compressed block is corrupted. This patch makes
all decompressor behave like a modified version of 3:
Output_length should be greater than or equal to the actual decompressed
length and it will be set to actual decompressed length if oversized. A
decompression failure sets it to 0.
Lz4Decompressor will use the "safe" instead of the "fast" decompression
function, for the latter is insecure with corrupted data and requires
the decompressed length to be known.

Testing: A testcase is added checking that the decompressors can handle
an oversized output buffer correctly. A regression test for the exact
case described in IMPALA-5250 is also added. A benchmark is run on a
16-node cluster testing the performance impact of the LZ4Decompressor
change and no performance regression is found.

Change-Id: Ifd42942b169921a7eb53940c3762bc45bb82a993
---
M be/src/util/codec.h
M be/src/util/decompress-test.cc
M be/src/util/decompress.cc
3 files changed, 74 insertions(+), 45 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifd42942b169921a7eb53940c3762bc45bb82a993
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

2017-09-20 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..


Patch Set 4: Verified-1

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4863/IMPALA-5311: Correctly account the file type and compression codec

Hello Impala Public Jenkins, Tim Armstrong,

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

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

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

Change subject: IMPALA-4863/IMPALA-5311: Correctly account the file type and 
compression codec
..

IMPALA-4863/IMPALA-5311: Correctly account the file type and compression codec

If a scan range is skipped at runtime the scan node skips reading
the range and never figures out the underlying compression codec used
to compress the files. In such a scenario we default the compression
codec to NONE which can be misleading. This change marks these files
as filtered in the scan node profile

e.g. - File Formats: TEXT/NONE:364 TEXT/NONE(Skipped):1460

Change-Id: I797916505f62e568f4159e07099481b8ff571da2
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M 
testdata/workloads/functional-query/queries/QueryTest/hdfs_scanner_profile.test
M tests/query_test/test_scanners.py
7 files changed, 71 insertions(+), 20 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I797916505f62e568f4159e07099481b8ff571da2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke

[Impala-ASF-CR] IMPALA-5250: Unify decompressor output length semantics

Alex Behm has posted comments on this change.

Change subject: IMPALA-5250: Unify decompressor output_length semantics
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8030/3//COMMIT_MSG
Commit Message:

Line 33: 
Add a separate section for testing. Mention the tests you added and the perf 
validation you did.


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

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

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

2017-09-20 Thread David Knupp (Code Review)

David Knupp has posted comments on this change.

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 1:

(1 comment)

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

Line 11: Data set constructed in mini-cluster using 
incubator-impala/bin/buildall.sh -testdata
> I think this sentence is awkward. "incubator-impala" happens to be what the
mikeb pointed out to me that a searchable string, like the previous commits 
title -- "IMPALA-5376: Loads all TPC-DS tables" -- is preferred. URL's can 
change.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3516: Avoid writing to /tmp in testing

Alex Behm has posted comments on this change.

Change subject: IMPALA-3516: Avoid writing to /tmp in testing
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8047/3/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java:

Line 763: FileWriter fw = new FileWriter(outDir_ + testFile + ".test");
> Under the same JIRA or not?
New JIRA. Be sure to add a link to the new JIRA on the old JIRA.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e503eb7d333c1b89dc8aea87cf30504838c44f9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3516: Avoid writing to /tmp in testing

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-3516: Avoid writing to /tmp in testing
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8047/3/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java:

Line 763: FileWriter fw = new FileWriter(outDir_ + testFile + ".test");
> Agree. Tianyi, can you make the changes?
Under the same JIRA or not?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e503eb7d333c1b89dc8aea87cf30504838c44f9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5250: Unify decompressor output length semantics

Tianyi Wang has uploaded a new patch set (#3).

Change subject: IMPALA-5250: Unify decompressor output_length semantics
..

IMPALA-5250: Unify decompressor output_length semantics

This patch makes the semantics of the output_length parameter in
Codec::ProcessBlock to be the same across all codecs. In existing code
different decompressor treats output_length differently:
1. SnappyDecompressor needs output_length to be greater than or equal
   to the actual decompressed length, but it does not set it to the
   actual decompressed length after decompression.
2. SnappyBlockDecompressor and Lz4Decompressor require output_length to
   be exactly the same as the actual decompressed length, otherwise
   decompression fails.
3. Other decompressors need output_length to be greater than or equal to
   the actual decompressed length and will set it to actual decompressed
   length if oversized.
This inconsistency leads to a bug where the error message is
undeterministic when the compressed block is corrupted. This patch makes
all decompressor behave like a modified version of 3:
Output_length should be greater than or equal to the actual decompressed
length and it will be set to actual decompressed length if oversized. A
decompression failure sets it to 0.
A testcase is added checking that decompressors can handle an oversized
output buffer correctly.
Lz4Decompressor will use the "safe" instead of the "fast" decompression
function, for the latter is insecure with corrupted data and requires
the decompressed length to be known. A benchmark is run on a 16-node
cluster and no performance impact is found.

Change-Id: Ifd42942b169921a7eb53940c3762bc45bb82a993
---
M be/src/util/codec.h
M be/src/util/decompress-test.cc
M be/src/util/decompress.cc
3 files changed, 74 insertions(+), 45 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifd42942b169921a7eb53940c3762bc45bb82a993
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tianyi Wang

[Impala-ASF-CR] IMPALA-3516: Avoid writing to /tmp in testing

Alex Behm has posted comments on this change.

Change subject: IMPALA-3516: Avoid writing to /tmp in testing
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8047/3/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java:

Line 763: FileWriter fw = new FileWriter(outDir_ + testFile + ".test");
> I think its better to switch outDir_ to a "Path" type to avoid these kind o
Agree. Tianyi, can you make the changes?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e503eb7d333c1b89dc8aea87cf30504838c44f9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5954: Set DO NOT UPDATE STATS in alterTable() RPCs to HMS.

Alex Behm has posted comments on this change.

Change subject: IMPALA-5954: Set DO_NOT_UPDATE_STATS in alterTable() RPCs to
HMS.
..

Patch Set 1:

(2 comments)

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

Line 800:
partition.putToParameters(MetastoreShim.statsGeneratedViaStatsTaskParam());
> Are partition level stats also affected, just wanted to make sure that we d
I checked the Hive/HMS code carefully and partition-level stat are not
affected. Feel free to look yourself. The problematic stats-wiping function is
MetastoreUtils.updateTableStatsFast().

While at some level I'd love to pass DO_NOT_UPDATE_STATS in all RPC to the HMS,
I feel like being too defensive is confusing to readers. Adding the flag here
will actually add the table property to the partition which is also weird.

PS1, Line 822: Pair statsTaskParam =
MetastoreShim.statsGeneratedViaStatsTaskParam();
: msTbl.putToParameters(statsTaskParam.first,
statsTaskParam.second);
> This looks redundant and is overwritten anyway right?
Setting this flag is required for for stats changes to take effect (table or
partition). Why do you way this is redundant? It is different than the
DO_NOT_UPDATE_STATS flag.

Added comments.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I05c63315579d66efeaa4c65105372ae68820ab2f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5954: Set DO NOT UPDATE STATS in alterTable() RPCs to HMS.

Alex Behm has uploaded a new patch set (#2).

Change subject: IMPALA-5954: Set DO_NOT_UPDATE_STATS in alterTable() RPCs to 
HMS.
..

IMPALA-5954: Set DO_NOT_UPDATE_STATS in alterTable() RPCs to HMS.

This improves an existing workaround for HIVE-12730 to avoid
having the HMS wipe stats as a side-effect of an alterTable()
RPC. The new workaround uses DO_NOT_UPDATE_STATS which is
the recommended way of telling the HMS not to recompute/reset
statistics on its own.

Testing:
- core/hdfs run passed

Change-Id: I05c63315579d66efeaa4c65105372ae68820ab2f
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
1 file changed, 9 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I05c63315579d66efeaa4c65105372ae68820ab2f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada

[Impala-ASF-CR] IMPALA-5416: Fix an impala-shell command recursion bug

2017-09-20 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5416: Fix an impala-shell command recursion bug
..


Patch Set 5:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I453af2d4694d47e184031cb07ecd2af259ba20f3
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5416: Fix an impala-shell command recursion bug

Alex Behm has posted comments on this change.

Change subject: IMPALA-5416: Fix an impala-shell command recursion bug
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I453af2d4694d47e184031cb07ecd2af259ba20f3
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

2017-09-20 Thread David Knupp (Code Review)

David Knupp has posted comments on this change.

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 1:

(1 comment)

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

Line 11: Data set constructed in mini-cluster using 
incubator-impala/bin/buildall.sh -testdata
I think this sentence is awkward. "incubator-impala" happens to be what the 
directory is called on your system. I suspect that most people have renamed 
their local directories to "Impala." A better choice would be 
${IMPALA_HOME}/bin/etc...

I also think it might be helpful to reference this patch, although I'm not sure 
which link preferable. Yo might want to solicit opinions on that.
https://github.com/apache/incubator-impala/commit/f1558957
http://gerrit.cloudera.org:8080/6877


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5416: Fix an impala-shell command recursion bug

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5416: Fix an impala-shell command recursion bug
..


Patch Set 5:

Code is rebased. Please build again.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I453af2d4694d47e184031cb07ecd2af259ba20f3
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5416: Fix an impala-shell command recursion bug

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5416: Fix an impala-shell command recursion bug
..


Patch Set 4:

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

The build failed on building stage. The error message is 
"./bin/jenkins/build-all-flag-combinations.sh: line 59: syntax error near 
unexpected token `fi". Pretty strange since the script is not touched and it 
builds on my machine.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I453af2d4694d47e184031cb07ecd2af259ba20f3
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 7:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS7, Line 39: i32_max
> Constants should be upper case, i.e. I32_MAX.
Done


PS7, Line 49: MakeOk
> Maybe MakeOkTest or MakeTestOkFn? It would make it clearer that this return
Done


PS7, Line 65: KEY
> MAKE_KEYDEF seems clearer to me.
Done


Line 81: auto lb = test_case.second.first;
> No. Will fix.
Done


PS7, Line 90: uint64_t
> Use static_cast()
Done


Line 103: for (auto& value : common_value) {
> Will fix.
Done


PS7, Line 182: Entry
> ENTRY
Done


PS7, Line 184: Entries
> ENTRIES
Done


Line 198:   auto enum_and_int = fusion::make_tuple(
> Does std::make_tuple not work here?
It didn't though boost claims it should.


Line 198:   auto enum_and_int = fusion::make_tuple(
> Could we avoid the use of tuples here if we changed TestEnumCaseSet to just
Done.


Line 306: #undef KEY
> It seems ok to leave this macro defined for the rest of the file.
Removed.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

Tianyi Wang has uploaded a new patch set (#8).

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..

IMPALA-5425: Add test for validating input when setting query options

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 248 insertions(+), 150 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5954: Set DO NOT UPDATE STATS in alterTable() RPCs to HMS.

2017-09-20 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5954: Set DO_NOT_UPDATE_STATS in alterTable() RPCs to 
HMS.
..


Patch Set 1: -Code-Review

(2 comments)

Sorry, got a couple more questions after going through the code again. Will +1 
again once I understand those scenarios.

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

Line 800:   
partition.putToParameters(MetastoreShim.statsGeneratedViaStatsTaskParam());
Are partition level stats also affected, just wanted to make sure that we don't 
need to set it here as well.


PS1, Line 822: Pair statsTaskParam = 
MetastoreShim.statsGeneratedViaStatsTaskParam();
 : msTbl.putToParameters(statsTaskParam.first, 
statsTaskParam.second);
This looks redundant and is overwritten anyway right?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I05c63315579d66efeaa4c65105372ae68820ab2f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5955: Use totalSize tblproperty instead of rawDataSize.

2017-09-20 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5955: Use totalSize tblproperty instead of rawDataSize.
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7c2c4e1e99b297c849f9f0d18b2bef34ad811c6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-HasComments: No

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

Matthew Jacobs has posted comments on this change.

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


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG
Commit Message:

PS9, Line 29: For request_pool, this means that setting the default
: request_pool via impalad command line is now a bad idea
> could you check with MJ about whether people do that? I get the feeling tha
I don't have data to back this up, but I'd guess that some folks are probably 
doing it even though it isn't the right thing to do (they should use the 
"default" placement rule to map specifically into the default pool.)

The case that might break is when a user has:
a) placement rules set up like "1) specified, 2) anything else, e.g. 'default' 
or 'user'"
b) they set --default_query_options=request_pool=foo
c) then rely on manually setting the session query option request_pool="" to 
get the mapping defined by the 2nd placement rule.

That said, I think this is a bad practice so I wouldn't be opposed to fixing 
this as long as we clearly release note it, and perhaps issue a warning if 
starting up the impala server with default_query_options=request_pool=foo


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

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

[Impala-ASF-CR] IMPALA-5949: fix test exchange small delay failure

2017-09-20 Thread Thomas Tauber-Marshall (Code Review)

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-5949: fix test_exchange_small_delay failure
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia75c42be2de600344de7af5a917d7843880ea6de
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5954: Set DO NOT UPDATE STATS in alterTable() RPCs to HMS.

2017-09-20 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5954: Set DO_NOT_UPDATE_STATS in alterTable() RPCs to 
HMS.
..


Patch Set 1: Code-Review+1

Should we add more alter scenarios to "alter-table.test"  to make sure 
alterTable() doesn't update stats?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I05c63315579d66efeaa4c65105372ae68820ab2f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-HasComments: No

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

Dan Hecht has posted comments on this change.

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

Patch Set 9:

(9 comments)

If you haven't already, please file a doc jira to update the docs based on this
new behavior.

http://gerrit.cloudera.org:8080/#/c/8070/9//COMMIT_MSG
Commit Message:

PS9, Line 29: For request_pool, this means that setting the default
: request_pool via impalad command line is now a bad idea
could you check with MJ about whether people do that? I get the feeling that
they might.

Line 31: doesn't seem worse than before.
what do you mean by "before"? what was the before behavior you are comparing
with?

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

PS9, Line 346: server
let's be super explicit and say ImpalaServer's

PS9, Line 352: default_query_options
update

http://gerrit.cloudera.org:8080/#/c/8070/9/be/src/service/query-options.cc
File be/src/service/query-options.cc:

PS9, Line 61:
we had never set the dst isset bits? how did that not cause trouble before?

http://gerrit.cloudera.org:8080/#/c/8070/9/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

PS9, Line 75: to override the defaults, which are stored in the
: // SessionState
maybe that could use some rewording (depending on what "defaults" is referring
to)

PS9, Line 79: SET
: // ="".
unset will also affect options set by OpenSession() RPC, right? Let's clarify
that.

And unset doesn't affect options set in (1) and (2), right? (it causes revert
back to those "defaults").

http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_admission_controller.py
File tests/custom_cluster/test_admission_controller.py:

Line 263: # abort on error, because it's back to being the default.
nit: line breaking seems early

http://gerrit.cloudera.org:8080/#/c/8070/9/tests/custom_cluster/test_set_and_unset.py
File tests/custom_cluster/test_set_and_unset.py:

PS9, Line 52: # "set" returns the session options; you have to run a real
query to
: # see what would actually take effect.
i'm not following that comment given you had just used SET to see the query
option setting. (The test case below makes sense to me though.)

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

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

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

2017-09-20 Thread Michael Brown (Code Review)

Michael Brown has posted comments on this change.

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 1:

(1 comment)

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

PS1, Line 22: CDH-59396
> CDH-xxx94 replaced with IMPALA-5960.
> if the test data (generator) is public as well, then we should replace this 
> ticket with a new IMPALA ticket, right?

Yes, please use IMPALA Jiras to track upstream work.

If additional test data is needed other than what Apache Impala provides via 
"buildall.sh -testdata", then you need to handle that in this Jira; otherwise 
the new TPC DS test cases won't work on jenkins.impala.io.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-HasComments: Yes

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

2017-09-20 Thread Thomas Tauber-Marshall (Code Review)

Thomas Tauber-Marshall has posted comments on this change.

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


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8029/3/be/src/exec/CMakeLists.txt
File be/src/exec/CMakeLists.txt:

Line 40:   filter-context-ir.cc
> Missing this file?
Done


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

Line 225:   Constant* expr_type_arg = codegen->ConstantToGVPtr(
> This does look like it should work to me. I think the types should line up 
Yeah, I have no idea why it doesn't work. The types definitely line up or it 
wouldn't pass verification, and as far as I can tell the type is being passed 
correctly, its just somehow corrupting/overwriting the actual value that's 
getting hashed.

I've been playing around with it, and the only thing I can figure out that 
makes it work is to change the cross-complied function to assign the ColumnType 
to a local variable rather than just handling a ref to it, but of course that 
somewhat defeats the point here.

Is there someone who knows more about llvm that can help me with this? I can 
post the generated IR somewhere for someone to look at. I've already spent 
quite a lot of time trying to get this to work, and I'm not sure what else to 
do since I'm finding llvm to be very poorly documented.


http://gerrit.cloudera.org:8080/#/c/8029/3/be/src/exprs/scalar-expr.cc
File be/src/exprs/scalar-expr.cc:

Line 368:   // Set the pointer to NULL in case it evaluates to NULL.
> It seems like the main way this differs from other places is returning a nu
I don't know what you mean by this. If we're going to replace 
ScalarExprEval::GetValue() with ReplaceCallSites, then we need another function 
to replace it with.


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

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

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

2017-09-20 Thread Thomas Tauber-Marshall (Code Review)

Hello Tim Armstrong,

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

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

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

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

IMPALA-3360: Codegen inserting into runtime filters

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

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

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

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


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

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

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

2017-09-20 Thread Thomas Tauber-Marshall (Code Review)

Hello Tim Armstrong,

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

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

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

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

IMPALA-3360: Codegen inserting into runtime filters

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

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

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

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


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

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

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

2017-09-20 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..


Patch Set 4:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 7:

The really nice thing about the patch is that all the tests are all 
table-driven so adding query options should just require adding entries to 
tables describing the valid values of each option (this also helps enforce that 
behaviour of query options is consistent).I think we should preserve that 
aspect regardless.

>From what I can see the use of templates is directly in service of supporting 
>table-driven tests without copying and pasting the same code for every type 
>(which would be particularly painful for the enums). I think there are some 
>opportunities to remove indirection at the cost of repetition. E.g. if we 
>think people will be confused by functions returning functions we could avoid 
>that, but it makes sense to me and avoids a lot of repetition in the test 
>cases themselves.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4670: Introduces RpcMgr class

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4670: Introduces RpcMgr class
..


Patch Set 6:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/exec/kudu-util.h
File be/src/exec/kudu-util.h:

Line 87: /// Impala Status object.
maybe note that the returned error is always of type GENERAL, i.e. we don't try 
to translate error codes.


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

Line 30: using std::move;
why is that needed?


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

Line 76: ///
what is the thread safety story of this class? it looks like initializing it is 
not thread safe, but then Shutdown() is thread safe? i.e. Shutdown() can be 
called while RPCs to/from services are in flight?  let's briefly document 
whatever the thread-safety or lack of is.


PS6, Line 89: processed
that's talking about being processed by either acceptor or service, right? As 
opposed to being processed by 'reactor'.  Could be clarified.


PS6, Line 89: FIFO fixed-size queue
is that queue per service? and is there a queue for connection requests as 
well? or is it one big global queue?


PS6, Line 104: address
does that one have to be an IP addr or is hostname okay?


PS6, Line 115: being
begin


Line 133:   /// All acceptor and reactor threads within the messenger are also 
shut down.
what happens to stuff in flight, or should we not care for some reason?


PS6, Line 156: scoped_refptr
I guess using that is kinda required by krpc? What does it do? ServicePool 
destroys itself based on a ref count or something?


PS6, Line 169: shared_ptr
I guess using shared_ptr here is required by the MessangerBuilder::Build(). 
Let's note that in the comment.


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/runtime/exec-env.h
File be/src/runtime/exec-env.h:

PS6, Line 135: krpc_port
does that still need to be exposed if we have krpc_address()? And looking ahead 
to your other patch, it looks like this is used to construct the krpc_address, 
yet it comes from the krpc address, so I'm confused.


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

Line 73
maybe this comment is still useful in the new code (minus the KRPC bits) in a 
slightly different form:

Store our IP address, so that each ...


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/util/network-util.cc
File be/src/util/network-util.cc:

Line 120:   Sockaddr sock;
okay to ignore, but i think keeping the kudu:: namespace here makes it easier 
to understand why we have this and where to look. (and since it's not 
wide-spread, it's not so bad to type it out here).


PS6, Line 218: Sockaddr
same


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8adb10ae375d7bf945394c38a520f12d29cf7b46
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 198:   auto enum_and_int = fusion::make_tuple(
> Does std::make_tuple not work here?
Could we avoid the use of tuples here if we changed TestEnumCaseSet to just run 
a single test at a time? E.g.

  TestEnumCaseSet(MAKE_KEYDEF(prefetchmode, TPrefetchMode, (NONE, HT_BUCKET));
  TestEnumCaseSet(...);


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5954: Set DO NOT UPDATE STATS in alterTable() RPCs to HMS.

Alex Behm has uploaded a new change for review.

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

Change subject: IMPALA-5954: Set DO_NOT_UPDATE_STATS in alterTable() RPCs to 
HMS.
..

IMPALA-5954: Set DO_NOT_UPDATE_STATS in alterTable() RPCs to HMS.

This improves an existing workaround for HIVE-12730 to avoid
having the HMS wipe stats as a side-effect of an alterTable()
RPC. The new workaround uses DO_NOT_UPDATE_STATS which is
the recommended way of telling the HMS not to recompute/reset
statistics on its own.

Testing:
- core/hdfs run passed

Change-Id: I05c63315579d66efeaa4c65105372ae68820ab2f
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
1 file changed, 7 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I05c63315579d66efeaa4c65105372ae68820ab2f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm

[Impala-ASF-CR] IMPALA-5949: fix test exchange small delay failure

Tim Armstrong has uploaded a new change for review.

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

Change subject: IMPALA-5949: fix test_exchange_small_delay failure
..

IMPALA-5949: fix test_exchange_small_delay failure

Avoid running the problematic query with short delays. This combination
doesn't add coverage - the short delay was meant to test behaviour when
multiple batches were sent, but there are deliberately no batches sent
with this query.

Testing:
Ran a build against Isilon, which succeeded. Ran the test in a loop
locally overnight.

Change-Id: Ia75c42be2de600344de7af5a917d7843880ea6de
---
A 
testdata/workloads/functional-query/queries/QueryTest/exchange-delays-zero-rows.test
M testdata/workloads/functional-query/queries/QueryTest/exchange-delays.test
M tests/custom_cluster/test_exchange_delays.py
3 files changed, 14 insertions(+), 10 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-5955: Use totalSize tblproperty instead of rawDataSize.

Alex Behm has uploaded a new change for review.

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

Change subject: IMPALA-5955: Use totalSize tblproperty instead of rawDataSize.
..

IMPALA-5955: Use totalSize tblproperty instead of rawDataSize.

Today, Impala populates the 'rawDataSize' property
during COMPUTE STATS for the purpose of extrapolating
row counts based on file sizes.

Intended meaning/use of tblproperties:
- rawDataSize' is the estimated in-memory size of a table
  (without encoding and compression)
- 'totalSize' represents the on-disk size

Using the fields correctly is important for compatibility
with other users of the HMS such as Hive and SparkSQL.
For example, SparkSQL relies on the 'totalSize' for
join ordering.

Testing:
- core/hdfs run passed

Change-Id: If7c2c4e1e99b297c849f9f0d18b2bef34ad811c6
---
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/test/java/org/apache/impala/planner/StatsExtrapolationTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test
4 files changed, 23 insertions(+), 23 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If7c2c4e1e99b297c849f9f0d18b2bef34ad811c6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 7:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS7, Line 39: i32_max
Constants should be upper case, i.e. I32_MAX.


PS7, Line 49: MakeOk
Maybe MakeOkTest or MakeTestOkFn? It would make it clearer that this returns a 
function.


PS7, Line 65: KEY
MAKE_KEYDEF seems clearer to me.


PS7, Line 90: uint64_t
Use static_cast()


PS7, Line 182: Entry
ENTRY


PS7, Line 184: Entries
ENTRIES


Line 198:   auto enum_and_int = fusion::make_tuple(
Does std::make_tuple not work here?


Line 306: #undef KEY
It seems ok to leave this macro defined for the rest of the file.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5920: addendum - add missing RAT check

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5920: addendum - add missing RAT check
..


Patch Set 2:

Thanks for fixing this.

@Alex The test job had a known spurious failure cleaning up the workspace, so I 
didn't notice the RAT failure. Also I hadn't seen RAT fail before, so didn't 
know to even look for it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44654004bef74b741cfdf4fb07c274a77320b818
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

Matthew Jacobs has posted comments on this change.

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


Patch Set 7:

> The rat-check job actually failed because of a file introduced here
 > - it broke my GVO. https://jenkins.impala.io/job/rat-check/1929/

Sorry about that - I haven't seen the RAT check fail before. Thanks for 
updating the rat exclude list.

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

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

[Impala-ASF-CR] IMPALA-5920: addendum - add missing RAT check

2017-09-20 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-5920: addendum - add missing RAT check
..


IMPALA-5920: addendum - add missing RAT check

Change-Id: I44654004bef74b741cfdf4fb07c274a77320b818
Reviewed-on: http://gerrit.cloudera.org:8080/8108
Reviewed-by: Bharath Vissapragada 
Tested-by: Impala Public Jenkins
---
M bin/rat_exclude_files.txt
1 file changed, 1 insertion(+), 0 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Bharath Vissapragada: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I44654004bef74b741cfdf4fb07c274a77320b818
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5920: addendum - add missing RAT check

2017-09-20 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5920: addendum - add missing RAT check
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44654004bef74b741cfdf4fb07c274a77320b818
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

2017-09-20 Thread Impala Public Jenkins (Code Review)

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

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


IMPALA-5895: clean up runtime profile lifecycle

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

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

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

Consolidate 'time_series_counter_map_lock_' and 'counter_map_lock_'
to reduce complexity of the locking scheme. I didn't see any benefit
to sharding the locks in this way - there are only two time series
counters per fragment instance, which a small fraction of the
total number of counters.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Reviewed-on: http://gerrit.cloudera.org:8080/8069
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 458 insertions(+), 428 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong

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

2017-09-20 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

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


Patch Set 12: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5920: addendum - add missing RAT check

2017-09-20 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5920: addendum - add missing RAT check
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44654004bef74b741cfdf4fb07c274a77320b818
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5920: addendum - add missing RAT check

2017-09-20 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-5920: addendum - add missing RAT check
..


Patch Set 1: Code-Review+2

Looks simple enough to me. Hence +2'ing.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44654004bef74b741cfdf4fb07c274a77320b818
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3516: Avoid writing to /tmp in testing

2017-09-20 Thread Bharath Vissapragada (Code Review)

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-3516: Avoid writing to /tmp in testing
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8047/3/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java:

Line 763: FileWriter fw = new FileWriter(outDir_ + testFile + ".test");
> There's a bug in this patch. A "/" is missing here between outDir_ and test
I think its better to switch outDir_ to a "Path" type to avoid these kind of 
flaky stuff and use Path.toFile() wherever required.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e503eb7d333c1b89dc8aea87cf30504838c44f9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5920: addendum - add missing RAT check