date:20170919

Alex Behm has posted comments on this change.

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


Patch Set 1:

How did the original IMPALA-5920 change pass GVO?

-- 
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: Matthew Jacobs 
Gerrit-HasComments: No

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

2017-09-19 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:

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

-- 
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-5895: clean up runtime profile lifecycle

Tim Armstrong has posted comments on this change.

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


Patch Set 12: Code-Review+2

rebase onto the rat check fix to unblock merging

-- 
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

Tim Armstrong has uploaded a new change for review.

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

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

IMPALA-5920: addendum - add missing RAT check

Change-Id: I44654004bef74b741cfdf4fb07c274a77320b818
---
M bin/rat_exclude_files.txt
1 file changed, 1 insertion(+), 0 deletions(-)


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

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

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

Tim Armstrong 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/

-- 
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-5416: Fix an impala-shell command recursion bug

2017-09-19 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 4: Verified-1

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

-- 
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-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");
There's a bug in this patch. A "/" is missing here between outDir_ and 
testFile. Try running the planner tests and see where the files land.


-- 
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: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

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

2017-09-19 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 11: Verified-1

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

-- 
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: 11
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-5908: Allow SET to unset modified query options.

Philip Zeyliger has posted comments on this change.

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


Patch Set 9:

This passes tests, so is ready to be reviewed.

-- 
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: No

[Impala-ASF-CR] IMPALA-5932: Improve transitive closure computation performance in FE

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

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5932: Improve transitive closure computation performance 
in FE
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5932: Improve transitive closure computation performance in FE

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

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

Change subject: IMPALA-5932: Improve transitive closure computation performance 
in FE
..


IMPALA-5932: Improve transitive closure computation performance in FE

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

Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
Reviewed-on: http://gerrit.cloudera.org:8080/8098
Reviewed-by: Alex Behm 
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
1 file changed, 7 insertions(+), 16 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
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: Tianyi Wang

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

Lars Volker has posted comments on this change.

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


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8049/3/bin/clean.sh
File bin/clean.sh:

Line 33: pushd ${IMPALA_HOME}/ext-data-source
> Some of these are still missing quotes
Thank you for catching these. I had only fixed the {}.


Line 35: ${IMPALA_HOME}/bin/mvn-quiet.sh clean
> Here
Done


Line 40: pushd ${IMPALA_FE_DIR}
> Here too.
Done


-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

Hello Tim Armstrong,

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

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

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

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

IMPALA-5927: Fix enable_distcc for zsh

enable_distcc didn't work on zsh anymore since it relies on automatic
variable splitting, which only works in bash.

This change moves clean-up actions for cmake files into an own bash
script.

This change also unifies variable quoting in clean.sh.

Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
---
A bin/clean-cmake.sh
M bin/clean.sh
M bin/distcc/distcc_env.sh
3 files changed, 50 insertions(+), 23 deletions(-)


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

Gerrit-MessageType: newpatchset
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

[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 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8049/3/bin/clean.sh
File bin/clean.sh:

Line 33: pushd ${IMPALA_HOME}/ext-data-source
Some of these are still missing quotes


Line 35: ${IMPALA_HOME}/bin/mvn-quiet.sh clean
Here


Line 40: pushd ${IMPALA_FE_DIR}
Here too.


-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

Tim Armstrong has posted comments on this change.

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


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7974/9/.clang-tidy
File .clang-tidy:

Line 27: -clang-analyzer-cplusplus.NewDeleteLeaks,\
> I'm concerned that some of these went in with only self-review. Some of the
I didn't remove any coverage and the suppressed warnings were mostly special 
cases or variants of ones that are already suppressed (e.g. new/delete is the 
same as the malloc one that's suppressed).

I looked at the output of the warnings and they didn't turn up anything helpful 
so IMO not worth putting time into.


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

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

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

Lars Volker has posted comments on this change.

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


Patch Set 2:

(2 comments)

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

Line 12: This change makes zsh temporarily emulate sh to make enable_distcc work
> Update
Done


http://gerrit.cloudera.org:8080/#/c/8049/2/bin/clean.sh
File bin/clean.sh:

Line 77: $IMPALA_HOME/bin/clean-cmake.sh
> nit: maybe quote "$IMPALA_HOME" for consistency
I cleaned up the file. Do you want to have another look?


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

Hello Tim Armstrong,

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

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

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

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

IMPALA-5927: Fix enable_distcc for zsh

enable_distcc didn't work on zsh anymore since it relies on automatic
variable splitting, which only works in bash.

This change moves clean-up actions for cmake files into an own bash
script.

This change also unifies variable quoting in clean.sh.

Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
---
A bin/clean-cmake.sh
M bin/clean.sh
M bin/distcc/distcc_env.sh
3 files changed, 49 insertions(+), 22 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong

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

2017-09-19 Thread Jim Apple (Code Review)

Jim Apple has posted comments on this change.

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


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7974/9/.clang-tidy
File .clang-tidy:

Line 27: -clang-analyzer-cplusplus.NewDeleteLeaks,\
I'm concerned that some of these went in with only self-review. Some of them, 
by their name, look very useful to me. What is your preferred forum to discuss 
those new warning suppressions?


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

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

[Impala-ASF-CR] IMPALA-5174: Suppress kudu flags that aren't relevant to Impala

2017-09-19 Thread Joe McDonnell (Code Review)

Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5174: Suppress kudu flags that aren't relevant to Impala
..


Patch Set 1: Code-Review+1

I think this makes sense. At some point, should we add a test that tracks when 
parameters are added or removed from the UI? It seems like something that could 
happen inadvertently. It would add some overhead to adding/removing params.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I499c903cde92595c4a02803ecbf98ac1d41517b4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-HasComments: No

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

2017-09-19 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:

(1 comment)

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

PS1, Line 22: CDH-59396
> I logged a CDH ticket here because the issue is with the test data.  But if
CDH-xxx94 replaced with IMPALA-5960.
CDH-xxx96 replaced with IMPALA-5961.


-- 
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

2017-09-19 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 4:

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

-- 
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-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 4: 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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

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

Tim Armstrong has posted comments on this change.

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


Patch Set 11: Code-Review+2

-- 
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

2017-09-19 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 11:

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

-- 
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: 11
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-5895: clean up runtime profile lifecycle

Dan Hecht has posted comments on this change.

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


Patch Set 10: Code-Review+2

-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

Alex Behm has posted comments on this change.

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


Patch Set 2:

(12 comments)

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

Line 62:   private static final String TABLE_TYPE_VIEW = "VIEW";
> Hive does return VIRTUAL_VIEW if "hive.server2.table.type.mapping" is set t
Agree. Only two types of tables for us: TABLE or VIEW.

Let's avoid doing something like "hive.server2.table.type.mapping" which sounds 
like unnecessary complexity to me.


Line 298: if (table.getMetaStoreTable() != null) {
> I ran this locally and sometime I get null, not sure why!
Looks like you have a solution, but let's spend the time to understand the 
problem. I can help.


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

Line 468
Let's keep this hasValidTableType type flag. Calling getDbsMetadata() can be 
expensive and we don't want to do it unnecessarily.


Line 63:   private static final String TABLE_TYPE_INDEX_TABLE = "INDEX";
TABLE_TYPE_INDEX


Line 77:   // GetTableTypes returns values: "TABLE", "VIEW", "INDEX"
Remove "INDEX", see comment below


Line 494: List upperCaseTypes = null;
upperCaseTableTypes


Line 495: if (tableTypes != null && !tableTypes.isEmpty()) {
Let's add tests to cover passing in TABLE, VIEW or both.


Line 514: 
remove blank line (the following check still belongs to tableType)


Line 515: if (upperCaseTypes != null && 
!upperCaseTypes.contains(tableType.toUpperCase())) continue;
No need tor tableType.toUpperCase() - we know if must already be upper case 
(otherwise we must have a bug somewhere)


Line 635:* Fills the GET_TYPEINFO_RESULTS with "TABLE", "VIEW", "INDEX".
I'm thinking we should remove "INDEX" here so as not to confuse clients and 
users. Impala cannot load index tables so we should not advertise them as a 
valid table type (you will always get an empty result set if you ask for index 
tables).


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

Line 308: assertEquals("INDEX", 
resp.rows.get(2).getColVals().get(0).string_val);
Remove, I don't think we should advertise this as a valid table type - Impala 
cannot load or use Hive indexes.


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

Line 200: assertEquals(rs.getString(1).toLowerCase(), "index");
remove


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

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

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

Sailesh Mukil has posted comments on this change.

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


Patch Set 10: Code-Review+1

I went over the fix to AddTimeSeriesCounter, and it looks correct. This patch 
LGTM.

-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

Dan Hecht has posted comments on this change.

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


Patch Set 2:

(2 comments)

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

Line 2078:   }
i think we still need to reset these pointers since they hold a shared_ptr to 
'this'. Otherwise, we have a cycle and 'this' will never be destructed.

For that reason, I'm not really sold that moving the ownership of the 
ThriftServer's into this class is an improvement (due to this ownership cycle), 
but as long as you add the comment in the header file about that, we can go 
forward.


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

Line 999:   /// Init() were <= 0.
add to comment:
Note that these hold a shared pointer to 'this', and so need to be reset() 
explicitly.


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

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

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

2017-09-19 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:

(1 comment)

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

PS1, Line 22: CDH-59396
> This looks like internal Cloudera information. Please excise all internal C
I logged a CDH ticket here because the issue is with the test data.  But if the 
test data (generator) is public as well, then we should replace this ticket 
with a new IMPALA ticket, right?


-- 
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-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 6:

(1 comment)

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

Line 171: fusion::for_each(case_set, [, accept_integer](auto& kase) 
{
> I thought that every CASE would be a parameter value, stored as a struct, t
I don't understand. Again for the example on that gtest page:

class FooTest : public ::testing::TestWithParam 

What should "???" be?


-- 
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

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

Lars Volker has posted comments on this change.

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


Patch Set 7:

(2 comments)

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

PS6, Line 137:   };
 :   TestByteCaseSet(options, case_set_i64);
 :   TestByteCaseSet(options, case_set_i32);
 : }
 : 
 : // Test an enum testcase. May or may not accept integer
 : template
 : void TestEnumCase(TQueryOptions& options,
> Do you mean replacing the make_pair in L142 or replacing the make_tuple in 
See my reply below.


Line 171:   });
> I'm not sure how these are related. I think value-parameterized-tests is us
I thought that every CASE would be a parameter value, stored as a struct, 
templatized if necessary.


-- 
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-HasComments: Yes

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

2017-09-19 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:

(10 comments)

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

PS1, Line 22: CDH-59396
This looks like internal Cloudera information. Please excise all internal
Cloudera information from the commit message and diff, as we have non-Cloudera
community members not privy to that information.

PS1, Line 25: --- tpcds-q39.test / MULTIPLE RESULT SET not recognized by test
framework / MARKED XFAIL.
: --- tpcds-q47.test / RESULT MISMATCH in LSD of DECIMAL values /
ADDED ROUND(2) TO 8th COLUMN OF WITH TABLE, TAKE ACTUAL RESULT AS EXPECTED.
: --- tpcds-q49.test / RESULT MISMATCH in LSD of DECIMAL values /
MARKED XFAIL, IMPALA-5945
: --- tpcds-q57.test / RESULT MISMATCH, excess scale in DECIMAL
values / FIXED, ADDED TRUNCATE(2) AROUND 6th COLUMN.
These are good comments, but they are not line-wrapped. They could also stand
to have some vertical white space. Could you please make this section
cosmetically cleaner?

PS1, Line 32: --- tpcds-q63.test / RESULT MISMATCH, excess scale in DECIMAL
values / ADDED CAST(DECIMAL(7, 2)) TO 3rd COLUMN
Comments like this are useful. It would be good for you to note this in
tpcds-q63.test directly in a comment. Please do the same for all annotations.

http://gerrit.cloudera.org:8080/#/c/8102/1/testdata/workloads/tpcds/queries/tpcds-q12.test
File testdata/workloads/tpcds/queries/tpcds-q12.test:

PS1, Line 4: ,i_item_desc
: ,i_category
: ,i_class
Gerrit highlights trailing white space in files. Click on this file in the
WebUI to see what I mean.

It's best not to have trailing white space since it makes diffing harder to
read over time. Can you please remove the trailing white space from all files?
I'm sure there are Vim, sed, or perl tricks that you know that could do it
quickly.

PS1, Line 12: web_sales
: ,item
: ,date_dim
I believe these characters (visible in Gerrit WebUI when you look at the file
in its diff viewer) indicate the presence of tab characters in the file. I
expect it's better to replace these with spaces; I'm not aware that we use tabs
anywhere in the code base. For all files, can you replace with spaces and make
sure the alignment is clean?

http://gerrit.cloudera.org:8080/#/c/8102/1/testdata/workloads/tpcds/queries/tpcds-q23.test
File testdata/workloads/tpcds/queries/tpcds-q23.test:

PS1, Line 51: limit 100;
:
: with frequent_ss_items as
The stress test gets its bank of queries to run from files such as this, but
it's not clear to me how the stress test handles multiple queries and results
gathering. This matters both when collecting runtime info and when running the
stress test proper. Can you please investigate how the stress test handles
having two queries here? You can talk to Matt Mulder or me about this.

http://gerrit.cloudera.org:8080/#/c/8102/1/testdata/workloads/tpcds/queries/tpcds-q24.test
File testdata/workloads/tpcds/queries/tpcds-q24.test:

PS1, Line 48: having sum(netpaid) > (select 0.05*avg(netpaid)
: from ssales)
: ;
:
: with ssales as
Highlighting another place where there are 2 queries and the stress test's
behavior isn't clear.

PS1, Line 104: --
Is this a results delimiter? If yes, why doesn't q23 have a results delimiter?

http://gerrit.cloudera.org:8080/#/c/8102/1/testdata/workloads/tpcds/queries/tpcds-q39.test
File testdata/workloads/tpcds/queries/tpcds-q39.test:

PS1, Line 27: ;
: with inv as
Another multi-query file, like previously.

http://gerrit.cloudera.org:8080/#/c/8102/1/tests/query_test/test_tpcds_queries.py
File tests/query_test/test_tpcds_queries.py:

PS1, Line 87: @pytest.mark.xfail(reason="IMPALA-5226")
: def test_tpcds_q10(self, vector):
: self.run_test_case(self.get_workload() + '-q10', vector)
While this works for functional end-to-end tests, we need a way to tell the
stress test to skip this (and other) queries so as not to include it when
collecting runtime info or as part of a stress run. You can talk to Matt Mulder
or me about this.

--
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

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

Tim Armstrong has posted comments on this change.

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


Patch Set 10:

Fix wasn't totally trivial so would be good to have you check my work

-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

Tim Armstrong has posted comments on this change.

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


Patch Set 9:

(2 comments)

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

PS9, Line 368:   
PeriodicCounterUpdater::StopRateCounter(total_throughput_counter());
 :   
PeriodicCounterUpdater::StopTimeSeriesCounter(bytes_read_timeseries_counter_);
> Spurious. These should be stopped in ScanNode::Close() ?
Good point, I missed those.


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

PS9, Line 244: PeriodicCounterUpdater::StopTimeSeriesCounter(
 :   recvr_->bytes_received_time_series_counter_);
> Seems like an outlier.
Good point. I released that we didn't actually call StopPeriodicCounters() on 
the profile_ here, which made me realise there was a bug in 
AddTimeSeriesCounter() where the flag wasn't set.


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

Hello Sailesh Mukil, Dan Hecht,

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

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

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

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
---
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(-)


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

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

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

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

David Knupp has posted comments on this change.

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


Patch Set 4:

This was submitted by hand. I accidentally had clicked "dry_run" in 
jenkins.impala.io.

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

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

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

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

David Knupp has posted comments on this change.

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


Patch Set 3: Verified+1

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

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

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

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

David Knupp has submitted this change and it was merged.

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


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

The Hive Metastore schema script includes other SQL
scripts using \i, which expects absolute paths. Since
we currently invoke it from outside the schema script
directory, it is unable to find those included scripts.

The fix is to switch to the Hive Metastore script
directory when invoking the schema script.

Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
Reviewed-on: http://gerrit.cloudera.org:8080/8081
Reviewed-by: Tim Armstrong 
Tested-by: David Knupp 
---
M bin/create-test-configuration.sh
1 file changed, 5 insertions(+), 2 deletions(-)

Approvals:
  David Knupp: Verified
  Tim Armstrong: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong

[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:

(5 comments)

Thanks for the detailed review. I'd like to discuss in person if possible.

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

PS6, Line 137:   };
 :   TestByteCaseSet(options, case_set_i64);
 :   TestByteCaseSet(options, case_set_i32);
 : }
 : 
 : // Test an enum testcase. May or may not accept integer
 : template
 : void TestEnumCase(TQueryOptions& options,
> Can you use a templatiezed class to store the test cases instead of a tuple
Do you mean replacing the make_pair in L142 or replacing the make_tuple in L148?


Line 171:   });
> Could you use https://github.com/google/googletest/blob/master/googletest/d
I'm not sure how these are related. I think value-parameterized-tests is used 
when you want to run a test multiple times, with a variable being different 
values in each run. Here it is about iterating through test cases (tuples for 
now) with different types.

To be more specific, in the example given:
class FooTest : public ::testing::TestWithParam 

What should "const char*" be replaced with here?


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

Line 79: auto ok = MakeOk(options, test_case.first);
> I think it would be helpful to see that these are functions here. Can you r
It is a lambda with no explicit typename. It could be replaced with a functor 
class though. I think to make it more like a function we can use CamelCase. I 
checked google C++ guide and didn't find a naming convention for lambda 
expressions.


Line 81: auto lb = test_case.second.first;
> Do these need to be auto?
No. Will fix.


Line 103: for (auto& value : common_value) {
> const? Please check elsewhere, too.
Will fix.


-- 
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-HasComments: Yes

[Impala-ASF-CR] IMPALA-5932: Improve transitive closure computation performance in FE

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

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5932: Improve transitive closure computation performance 
in FE
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5932: Improve transitive closure computation performance in FE

Alex Behm has posted comments on this change.

Change subject: IMPALA-5932: Improve transitive closure computation performance 
in FE
..


Patch Set 3: Code-Review+2

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

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

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

2017-09-19 Thread Vuk Ercegovac (Code Review)

Vuk Ercegovac has posted comments on this change.

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


Patch Set 2:

(3 comments)

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

PS2, Line 221: taleTypes
nit: tale -> table


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

PS2, Line 244: issue
nit: Issue


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

PS2, Line 193: // There is only one table type: "table".
stale comment?


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

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

[Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

2017-09-19 Thread Vuk Ercegovac (Code Review)

Vuk Ercegovac has uploaded a new change for review.

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

Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, 
unknown.
..

IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

Replaces IsNullPredicate with IsNullOrBooleanPredicate in the frontend
to handle testing against null and boolean values. Similarly, the backend
expression is replaced with a corresponding expression that handles the
additional tests.

This change is a replacement candidate for gerrit.cloudera.org/8014, which
is based on front-end rewrites.

Added tests:
- Frontend: parser, analyzer, tosql
- EndToEnd query expressions

Change-Id: Ieea87e6fc8eeaf899726809d4daa4053c2bea54c
---
M be/src/codegen/impala-ir.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/expr-test.cc
A be/src/exprs/is-null-or-bool-predicate-ir.cc
A be/src/exprs/is-null-or-bool-predicate.h
D be/src/exprs/is-null-predicate-ir.cc
D be/src/exprs/is-null-predicate.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr.cc
M common/function-registry/impala_functions.py
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
A fe/src/main/java/org/apache/impala/analysis/IsNullOrBoolPredicate.java
D fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSet.java
M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
M fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java
M fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
26 files changed, 670 insertions(+), 347 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieea87e6fc8eeaf899726809d4daa4053c2bea54c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Vuk Ercegovac

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

Tim Armstrong has posted comments on this change.

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


Patch Set 3:

(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?


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 - 
i.e. ColumnType* and ColumnType*.


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 null 
pointer if IsNull() is true. Could we just wrap that up in something like a 
CodegenAnyVal.ToNullableNativePtr() method and avoid the boilerplate of 
creating a whole new function?


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

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

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

Matthew Jacobs has submitted this change and it was merged.

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


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

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

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

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

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

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

Testing:
* Ran existing tests on master.

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

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

Approvals:
  Matthew Jacobs: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
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

[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 6: Verified+1

The job actually passed, cleaning up the workspace failed

21:19:36 ERROR: Step ‘Delete workspace when build is done’ failed: Cannot 
delete workspace: remote file operation failed: /home/ubuntu at 
hudson.remoting.Channel@43c5313:ubuntu-16.04 (i-0f8cf68ee32aebe80): 
java.io.IOException: Unable to delete '/home/ubuntu'. Tried 3 times (of a 
maximum of 3) waiting 0.1 sec between attempts.


21:19:36 Finished: SUCCESS

-- 
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

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

2017-09-19 Thread Mostafa Mokhtar (Code Review)

Mostafa Mokhtar 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/testdata/workloads/tpcds/queries/tpcds-q10.test
File testdata/workloads/tpcds/queries/tpcds-q10.test:

Line 3: select  
Remove trailing spaces from all new files.


-- 
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-HasComments: Yes

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

Sailesh Mukil has posted comments on this change.

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


Patch Set 9: Code-Review+1

(2 comments)

LGTM, except these 2 comments.

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

PS9, Line 368:   
PeriodicCounterUpdater::StopRateCounter(total_throughput_counter());
 :   
PeriodicCounterUpdater::StopTimeSeriesCounter(bytes_read_timeseries_counter_);
Spurious. These should be stopped in ScanNode::Close() ?


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

PS9, Line 244: PeriodicCounterUpdater::StopTimeSeriesCounter(
 :   recvr_->bytes_received_time_series_counter_);
Seems like an outlier.


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
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 2: Code-Review+2

(2 comments)

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

Line 12: This change makes zsh temporarily emulate sh to make enable_distcc work
Update


http://gerrit.cloudera.org:8080/#/c/8049/2/bin/clean.sh
File bin/clean.sh:

Line 77: $IMPALA_HOME/bin/clean-cmake.sh
nit: maybe quote "$IMPALA_HOME" for consistency

Although this file already has three different forms of path :(


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

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

Impala Public Jenkins has posted comments on this change.

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


Patch Set 6: Verified-1

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

-- 
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

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

Tim Armstrong has posted comments on this change.

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


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8069/8//COMMIT_MSG
Commit Message:

Line 19: 
> Could you document that you removed the time_series_counter_map_lock_ and t
Done


http://gerrit.cloudera.org:8080/#/c/8069/8/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

PS8, Line 178: profile_
> How come we don't stop counters for this profile? There are other profiles 
There aren't any periodic counters registered in any data sinks so it doesn't 
make a difference. If someone adds such a counter in the future they're 
responsible for adding the StopPeriodicCounters() call. The DCHECK enforces 
this.


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

PS8, Line 950: vector* 
RuntimeProfile::AddBucketingCounters(Counter* src_counter,
> Long line
Done


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

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

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

Hello Dan Hecht,

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

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

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

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
---
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, 451 insertions(+), 416 deletions(-)


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

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

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

Sailesh Mukil has posted comments on this change.

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


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8069/8//COMMIT_MSG
Commit Message:

Line 19: 
Could you document that you removed the time_series_counter_map_lock_ and the 
reasoning?


http://gerrit.cloudera.org:8080/#/c/8069/8/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

PS8, Line 178: profile_
How come we don't stop counters for this profile? There are other profiles in 
other files where we don't stop them too.

Wouldn't that be a slight change in behavior? Since they were previously 
stopped in the destructor?


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

PS8, Line 950: vector* 
RuntimeProfile::AddBucketingCounters(Counter* src_counter,
Long line


-- 
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5932: Improve transitive closure computation performance in FE

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5932: Improve transitive closure computation performance 
in FE
..


Patch Set 2:

(1 comment)

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

Line 2703:   valueTransfer_[p[i]][p[k]] |= 
valueTransfer_[p[i]][p[j]] &&
> The following is sufficient because L2701 already establishes that valueTra
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
Gerrit-PatchSet: 2
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-5932: Improve transitive closure computation performance in FE

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

Change subject: IMPALA-5932: Improve transitive closure computation performance 
in FE
..

IMPALA-5932: Improve transitive closure computation performance in FE

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

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


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
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-5895: clean up runtime profile lifecycle

Tim Armstrong has posted comments on this change.

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


Patch Set 7:

(4 comments)

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

PS7, Line 85: DCHECK(!has_active_periodic_counters_);
> We shouldn't take locks only under debug builds since they have side effect
Also the destructor can't safely run concurrently with other method calls 
regardless.


PS7, Line 88: void RuntimeProfile::StopPeriodicCounters() {
> This doesn't stop the counters that belong to child profiles. Is it on the 
Yeah, my thinking was that whatever added the counters to the profile should 
also explicitly stop them. Updated the method comment since it wasn't very 
clear.


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

PS7, Line 407: rate_counters
> nit: rate_counters_
Done


PS7, Line 408: sampling_counters
> nit: sampling_counters_
Done


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

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

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

Hello Dan Hecht,

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

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

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

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

IMPALA-5895: clean up runtime profile lifecycle

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

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

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

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


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

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

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

Dan Hecht has posted comments on this change.

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


Patch Set 3:

(5 comments)

> (15 comments)
 > 
 > > (15 comments)
 > >
 > > Do you plan to take care of the other cases noted in the jira?
 > > Okay to do it in a follow on commit but wondering the plan.
 > >
 > Other cases, such as use of TimestampValue as a class member (see
 > ImpalaServer, for instance where it is used to track start and end
 > times of queries), need more analysis. The plan is to look at these
 > other cases in a separate commit.
 > 

Sounds good. Let's be sure to not resolve the jira until all the cases it lists 
are handled (or file a new jira to handle those).

 > > A unit test would be good for testing this kind of code.  You can
 > > take a look at the various *-test.cc files be/src/*/ for
 > examples.
 > 
 > Adding a unit test. Will refresh the patch again once I have it. In
 > the meantime, please let me know if I have addressed all of your
 > comments on the implementation.

Just a few more minor comments. I'll take another look after the unit test is 
added.

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

PS3, Line 33: time zone specified by the boolean argument 'utc'
since 'utc' doesn't tell you the other time zone, how about:

... in UTC if 'utc' is true, or the local time zone if 'utc' is false.

or similar.


PS3, Line 75: // Convert time point 't' into date-time string at precision 'p' 
in the local
: // time zone.
: static string ToString(const chrono::system_clock::time_point& t, 
TimePrecision p)
: {
:   stringstream ss;
:   ss << TimepointToString(t, false);
:   ss << FormatSubSecond(t, p);
:   return ss.str();
: }
: 
: // Convert time point 't' into date-time string at precision 'p' 
in the UTC
: // time zone.
: static string ToUtcString(const chrono::system_clock::time_point& 
t, TimePrecision p)
: {
:   stringstream ss;
:   ss << TimepointToString(t, true);
:   ss << FormatSubSecond(t, p);
:   return ss.str();
: }
up to you, but you could combine these into a single ToString() function that 
takes the bool utc, since these functions are the same otherwise.


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

PS3, Line 79: ,
maybe add: 's'
just to clarify which input.


PS3, Line 80: strin
string


PS3, Line 81: form
format


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

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

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

Dan Hecht has posted comments on this change.

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


Patch Set 7: Code-Review+2

Please let Sailesh finish his review as well.

-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

Dan Hecht has posted comments on this change.

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


Patch Set 7:

(1 comment)

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

PS7, Line 85: DCHECK(!has_active_periodic_counters_);
> #ifndef NDEBUG
We shouldn't take locks only under debug builds since they have side effects 
(w.r.t. synchronization) and so can hide bugs from debug builds.


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

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

Tim Wood has uploaded a new change for review.

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

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

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

Main source for TPCDS query and result definitions: 
https://github.com/gregrahn/tpcds-kit.
TPC-DS v2.5.0 qualification queries from G. Rahn.
Data set constructed in mini-cluster using incubator-impala/bin/buildall.sh 
-testdata

Complete TPC-DS test suite runs with passes, skips and xfails, but no failures.

Some TPC-DS test cases in this commit have been modified in sematically-neutral
ways so as to pass on Impala; others are marked to skip or x-fail due to bugs.
The tests/query_test/test_tpcds_queries.py driver file is authoritative for
the active/skip/xfail status for each case and a brief reason.  The following 
describes
the current status as test-name / deviance from TPC-DS spec / changes made:

--- tpcds-q22a.test / RESULT MISMATCH in LSD of AVG() values / Fixed AVG()s
--- tpcds-q30.test / UNRECOGNIZED CHARACTER / MARKED XFAIL, CDH-59396.
--- tpcds-q35a.test / RESULT MISMATCH / MARKED XFAIL, IMPALA-5950.
--- tpcds-q36a.test / RESULT MISMATCH / MARKED XFAIL, IMPALA-4741
--- tpcds-q39.test / MULTIPLE RESULT SET not recognized by test framework / 
MARKED XFAIL.
--- tpcds-q47.test / RESULT MISMATCH in LSD of DECIMAL values / ADDED ROUND(2) 
TO 8th COLUMN OF WITH TABLE, TAKE ACTUAL RESULT AS EXPECTED.
--- tpcds-q49.test / RESULT MISMATCH in LSD of DECIMAL values / MARKED XFAIL, 
IMPALA-5945
--- tpcds-q57.test / RESULT MISMATCH, excess scale in DECIMAL values / FIXED, 
ADDED TRUNCATE(2) AROUND 6th COLUMN.
--- tpcds-q58.test / RESULT MISMATCH in DECIMAL values / MARKED XFAIL. 
IMPALA-5946
--- tpcds-q59.test / RESULT MISMATCH, excess scale in DECIMAL values / FIXED, 
ADDED TRUNCATE(2) AROUND 4th-10th COLUMNS.
--- tpcds-q61.test / RESULT MISMATCH in DECIMAL value / FIXED. CAST RESULT 
QUOTIENT TO DECIMAL(15, 4), TAKE ACTUAL RESULT AS EXPECTED
--- tpcds-q63.test / RESULT MISMATCH, excess scale in DECIMAL values / ADDED 
CAST(DECIMAL(7, 2)) TO 3rd COLUMN
--- tpcds-q64.test / RESULT MISMATCH / ADDED ORDER BY COLUMNS.
--- tpcds-q66.test / RESULT MISMATCH / MARKED XFAIL, IMPALA-4741
--- tpcds-q77a.test / RESULT MISMATCH / FIXED. TAKE ACTUAL RESULT AS EXPECTED
--- tpcds-q78.test / RESULT MISMATCH / FIXED. TAKE ACTUAL RESULT AS EXPECTED
--- tpcds-q83.test / RESULT MISMATCH / MARKED XFAIL. IMPALA-5945.
--- tpcds-q85.test / MISSING TABLE "reason" / MARKED XFAIL, CDH-59394
--- tpcds-q86a.test / RESULT MISMATCH / FIXED. TAKE ACTUAL RESULT AS EXPECTED
--- tpcds-q89.test / RESULT MISMATCH, DECIMAL values flap / MARKED XFAIL. ADDED 
ROUND(2) TO 8th COLUMN, TAKE ACTUAL RESULTS AS EXPECTED, IMPALA-5956.
--- tpcds-q90.test / RESULT MISMATCH / MARKED XFAIL, IMPALA-5945.
--- tpcds-q93.test / MISSING TABLE "reason" / MARKED XFAIL, CDH-59394
--- tpcds-q98.test / RESULT MISMATCH / FIXED, ADDED ROUND() TO LAST COLUMN

Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
---
A testdata/workloads/tpcds/queries/tpcds-q10.test
A testdata/workloads/tpcds/queries/tpcds-q10a.test
A testdata/workloads/tpcds/queries/tpcds-q11.test
A testdata/workloads/tpcds/queries/tpcds-q12.test
A testdata/workloads/tpcds/queries/tpcds-q13.test
A testdata/workloads/tpcds/queries/tpcds-q14.test
A testdata/workloads/tpcds/queries/tpcds-q14a.test
A testdata/workloads/tpcds/queries/tpcds-q15.test
A testdata/workloads/tpcds/queries/tpcds-q16.test
A testdata/workloads/tpcds/queries/tpcds-q17.test
A testdata/workloads/tpcds/queries/tpcds-q18.test
A testdata/workloads/tpcds/queries/tpcds-q18a.test
A testdata/workloads/tpcds/queries/tpcds-q20.test
A testdata/workloads/tpcds/queries/tpcds-q21.test
A testdata/workloads/tpcds/queries/tpcds-q22.test
A testdata/workloads/tpcds/queries/tpcds-q22a.test
A testdata/workloads/tpcds/queries/tpcds-q23.test
A testdata/workloads/tpcds/queries/tpcds-q24.test
A testdata/workloads/tpcds/queries/tpcds-q25.test
A testdata/workloads/tpcds/queries/tpcds-q26.test
M testdata/workloads/tpcds/queries/tpcds-q27a.test
A testdata/workloads/tpcds/queries/tpcds-q29.test
A testdata/workloads/tpcds/queries/tpcds-q30.test
A testdata/workloads/tpcds/queries/tpcds-q31.test
A testdata/workloads/tpcds/queries/tpcds-q32.test
A testdata/workloads/tpcds/queries/tpcds-q33.test
A testdata/workloads/tpcds/queries/tpcds-q35.test
A testdata/workloads/tpcds/queries/tpcds-q35a.test
A testdata/workloads/tpcds/queries/tpcds-q36.test
A testdata/workloads/tpcds/queries/tpcds-q36a.test
A testdata/workloads/tpcds/queries/tpcds-q37.test
A testdata/workloads/tpcds/queries/tpcds-q38.test
A testdata/workloads/tpcds/queries/tpcds-q39.test
A testdata/workloads/tpcds/queries/tpcds-q40.test
A testdata/workloads/tpcds/queries/tpcds-q41.test
A testdata/workloads/tpcds/queries/tpcds-q44.test
A testdata/workloads/tpcds/queries/tpcds-q45.test
M

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

Lars Volker has posted comments on this change.

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


Patch Set 7:

(5 comments)

I'm still trying to see if there are ways to simplify the code with patterns we 
use more commonly. If you disagree, let's catch up in person, since I'm not set 
on what's the right way forward here.

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

PS6, Line 137:   };
 :   TestByteCaseSet(options, case_set_i64);
 :   TestByteCaseSet(options, case_set_i32);
 : }
 : 
 : // Test an enum testcase. May or may not accept integer
 : template
 : void TestEnumCase(TQueryOptions& options,
> I'm not sure which part I should look at. The complication here is that I n
Can you use a templatiezed class to store the test cases instead of a tuple? 
Additionally you could use a small factory method to hide the macros.


Line 171:   });
> Not that easy. Let me explain:
Could you use 
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#value-parameterized-tests
 with a templatized parameter class to achieve the same?


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

Line 79: auto ok = MakeOk(options, test_case.first);
I think it would be helpful to see that these are functions here. Can you 
remove the auto?


Line 81: auto lb = test_case.second.first;
Do these need to be auto?


Line 103: for (auto& value : common_value) {
const? Please check elsewhere, too.


-- 
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-HasComments: Yes

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

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

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

IMPALA-5927: Fix enable_distcc for zsh

enable_distcc didn't work on zsh anymore since it relies on automatic
variable splitting, which only works in bash.

This change makes zsh temporarily emulate sh to make enable_distcc work
again. I tested it on zsh and bash.

Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
---
A bin/clean-cmake.sh
M bin/clean.sh
M bin/distcc/distcc_env.sh
3 files changed, 37 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Tim Armstrong

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

Lars Volker has posted comments on this change.

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


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8049/1/bin/distcc/distcc_env.sh
File bin/distcc/distcc_env.sh:

Line 119:   if type emulate >/dev/null 2>/dev/null; then emulate -L sh; fi
> This logic is also duplicated in clean.sh. How about factoring this functio
Done


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

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

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

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

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

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

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

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

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

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

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

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

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


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 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

[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options

Lars Volker has posted comments on this change.

Change subject: IMPALA-5736: Add impala-shell argument to set default query 
options
..


Patch Set 1:

MJ, do you prefer one option with a comma separated list of key=value pairs, or 
using several options similar to --var ?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5940. Avoid log spew by using Status::Expected.

Philip Zeyliger has uploaded a new change for review.

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

Change subject: IMPALA-5940. Avoid log spew by using Status::Expected.
..

IMPALA-5940. Avoid log spew by using Status::Expected.

In IMPALA-5926, we fixed a case where closing the session triggered a
stack trace in the logs which impacted performance for short-running
queries. Looking at log files from several active clusters, I identified a few
other cases where we could clean up log spew with the same (trivial)
approach.

In practice, the expected messages here are saying that we don't support
codegen for the given file formats or data types. Because codegen
happens at every execution node, these messages are very common in the
log files.

The snippet I used to identify these was:

find . -type f -name '*IMPALAD*.gz' | xargs gzcat  | awk '/^I/ { if(x) { print 
x; } x = "" } /status.cc/ { x=" "; } { if(x) { x=x  $0 } }'  | sed -e 
's/0x[0-9a-fx]* //g' | sed -e 's/[0-9a-f]\{16\}:[0-9a-f]*/QUERYID/g' |  tr -s 
'\t' ' ' | tr '[0-9]' 'N' | sort | uniq -c  | sort -n | tee output.txt

I also analyzed some logs using SQL, against a pre-processed logs table:

  with v as (
select regexp_replace(
regexp_replace(
  translate(substr(message, 42), "\n\t", "  "),
  "[a-zA-Z0-9.-]*[.][a-zA-Z0-9-]*:[0-9]*",
  ""),
"@.*$", "@@@...") as m
from logs_table where `class`="status.cc")
  select m, count(*) from v group by 1 order by 2 desc limit 100

Testing:
* Automated tests.

Change-Id: I38088482377a1c3e794a9c8178ef83f29957a330
---
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/tuple.cc
M be/src/util/tuple-row-compare.cc
5 files changed, 7 insertions(+), 7 deletions(-)


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

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

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

Philip Zeyliger has posted comments on this change.

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


Patch Set 8:

I'm still re-running tests. The Overlay function wasn't setting __is_set when 
it should have been, so that broke some things. I'll report back when tests are 
passing.

-- 
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

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

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

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

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

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

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

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

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

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

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

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

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


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

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

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

Sailesh Mukil has posted comments on this change.

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


Patch Set 7:

(4 comments)

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

PS7, Line 85: DCHECK(!has_active_periodic_counters_);
#ifndef NDEBUG
lock_guard l(counter_map_lock_);
DCHECK(!has_active_periodic_counters_);
#endif

For correctness?


PS7, Line 88: void RuntimeProfile::StopPeriodicCounters() {
This doesn't stop the counters that belong to child profiles. Is it on the user 
to stop counters of child profiles?


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

PS7, Line 407: rate_counters
nit: rate_counters_


PS7, Line 408: sampling_counters
nit: sampling_counters_


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5932: Improve transitive closure computation performance in FE

Alex Behm has posted comments on this change.

Change subject: IMPALA-5932: Improve transitive closure computation performance 
in FE
..


Patch Set 2:

(1 comment)

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

Line 2703:   valueTransfer_[p[i]][p[k]] |= 
valueTransfer_[p[i]][p[j]] &&
The following is sufficient because L2701 already establishes that 
valueTransfer_[p[i]][p[j]] is true

valueTransfer_[p[i]][p[k]] |= valueTransfer_[p[j]][p[k]];


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
Gerrit-PatchSet: 2
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-5932: Improve transitive closure computation performance in FE

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5932: Improve transitive closure computation performance 
in FE
..


Patch Set 2:

(5 comments)

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

Line 7: IMPALA-5932: Improve transitive closure computation performance in FE
> Shrink to fit
Done


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


Line 10: closure computation for the value transfer graph, replacing the 
existing
> closure computation for the value transfer graph
Done


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

Line 2703:   valueTransfer_[p[i]][p[k]] |= 
valueTransfer_[p[i]][p[j]] &&
> Comment that this optimization works because our graphs are typically spars
Done


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

Line 2698
Removed this comment since the graph here is not a DAG:  
 select * from (select * from tb0) v0 left join (select * from tb0) v1 on 
v0.c=v1.c left join (select * from tb0) v2 on v1.c=v2.c left join (select * 
from tb0) v3 on v2.c=v3.c where v0.c=v3.c;


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idb00e3c1f904e60ae25567a52b4bf0809a84c6b3
Gerrit-PatchSet: 2
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-5932: Improve transitive closure computation performance in FE

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

Change subject: IMPALA-5932: Improve transitive closure computation performance 
in FE
..

IMPALA-5932: Improve transitive closure computation performance in FE

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

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


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

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

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

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

Impala Public Jenkins has posted comments on this change.

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


Patch Set 6:

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

-- 
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: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
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 6: Code-Review+2

I'll count the +1s from Tim and Zach, and no further comments from Phil as 
enough to commit this change.

-- 
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: 6
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: 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 5:

(2 comments)

Thanks for the detailed review, Zach. If nobody has further feedback, I'll try 
to get this in soon.

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

Line 103:   public void serviceInit(Configuration conf) throws Exception {
> Can you file a JIRA?  This is actually a well compartmentalized ramp-up or 
https://issues.apache.org/jira/browse/IMPALA-5958


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

Line 57:   
> Is it worth fixing the trailing spaces so future diffs don't give the linte
These are their trailing spaces though - I figured leaving them in would make 
diffing easier later.


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

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

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

2017-09-19 Thread Zoram Thanga (Code Review)

Zoram Thanga has uploaded a new patch set (#3).

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

IMPALA-5599: Fix for mis-use of TimestampValue

The TimestampValue class is being used for non-database purposes
in many places, such as in log messages.

This change proposes to introduce APIs to convert Unix timetamps
into the corresponding date-time strings. We provide a series of
functions for different input time units, and also give the user
control over the precision of the output date-time string. APIs
are provided to convert to UTC and local time zones. The new APIs
can be used to replace (or instead of) TimestampValue::ToString()
in those places where Unix timestamps are being converted to
strings for printing.

The current commit implements the APIs and replaces calls to
TimestampValue::ToString() in be/src/service.

Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
---
M be/src/service/impala-server.cc
M be/src/util/time.cc
M be/src/util/time.h
3 files changed, 165 insertions(+), 13 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b0ae06f6d94968c87a199625aa3332b26988142
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Zoram Thanga

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

2017-09-19 Thread Zoram Thanga (Code Review)

Zoram Thanga has posted comments on this change.

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


Patch Set 2:

(15 comments)

> (15 comments)
 > 
 > Do you plan to take care of the other cases noted in the jira? 
 > Okay to do it in a follow on commit but wondering the plan.
 >
Other cases, such as use of TimestampValue as a class member (see ImpalaServer, 
for instance where it is used to track start and end times of queries), need 
more analysis. The plan is to look at these other cases in a separate commit.
 
 > A unit test would be good for testing this kind of code.  You can
 > take a look at the various *-test.cc files be/src/*/ for examples.

Adding a unit test. Will refresh the patch again once I have it. In the 
meantime, please let me know if I have addressed all of your comments on the 
implementation.

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

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


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


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


PS2, Line 1924:  Precision::Second
> and here
Done


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

Line 32: static std::string TimepointToString(const 
chrono::system_clock::time_point& t,
> for static things not defined in header, it's helpful to include a short fu
Done


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


Line 53: 
> nit: consider removing blank lines here and line 68 to make more code fit o
Done


Line 65: // 1-second precision or unknown unit. Return empty string.
> nice to document (and prove) that invariant with a dcheck:
Done. But I had to change TimePrecision from an enum class to a regular enum, 
because DCHECK requires the argument to have an "<<" operator.


PS2, Line 89:   chrono::system_clock::time_point 
t(chrono::duration_cast(
: chrono::seconds(s)));
:   return t;
> could consider combing these like suggested at line 46-47 (but given how lo
Done


PS2, Line 95: system_clock
> the docs for chrono::system_clock::time_point claim that the epoch is unspe
It's correct that the standard does not specify the epoch for 
chrono::system_clock. The Windows and Linux implementations, however, are based 
on the Unix epoch. This is a good read 
http://www.modernescpp.com/index.php/the-three-clocks.


PS2, Line 101: chrono::microseconds
> given that the "to" type is the same as the "from" type, why is the duratio
Removed the cast by combining class instantiation and return statements.


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

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


PS2, Line 79: Unix timestamp
> I think we usually refer to this as "Unix time", and timestamp often invoke
Done


PS2, Line 81: p
> nit: in header comments to distinguish variables, we usually use single quo
Done


PS2, Line 85: Precision p=Precision::Second
> normally we should avoid default arguments, but in this case it does seem r
Done


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

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

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