[Impala-ASF-CR] IMPALA-4839: Remove implicit 'localhost' for KUDU MASTER HOSTS

[David Knupp (Code Review)]

David Knupp has posted comments on this change.

Change subject: IMPALA-4839: Remove implicit 'localhost' for KUDU_MASTER_HOSTS
..


Patch Set 8:

Pre-review build: 
http://jenkins.impala.io:8080/view/Utility/job/pre-review-test/18/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9df71480a165f4ce21ae3edab6ce7227fbf76f77
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Ishaan Joshi 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change.

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..


Patch Set 3:

> I'm not opposed to cleaning up the AnyVal stuff like that, but
 > given that udf.h stuff dictates UDF compatibility, it's not
 > completely trivial. It doesn't look like it would break binary
 > compatibility though. But, in case something goes wrong, how about
 > we do that as a separate change so it could be backed out without
 > affecting the decimal work? It doesn't look like the decimal stuff
 > will depend on it, right?

No, but it gets a lot cleaner to test the limits by giving the generic form an 
underlying type.  I deliberately did not change FloatVal, since the equality 
operator is currently kind of broken.  Everything else should be binary 
compatible.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Zach Amsden (Code Review)]

Zach Amsden has uploaded a new patch set (#3).

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..

IMPALA-2020: Add rounding when casting from decimal to int

Preview diff, not working yet, mostly to see if I can successfully
push diffs against my ASF Impala fork, and also to get early
feedback on the UDF change.  Update - pushing from fork didn't
work.

Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/runtime/decimal-value.h
M be/src/runtime/decimal-value.inline.h
M be/src/udf/udf.h
4 files changed, 50 insertions(+), 108 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#11).

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 483 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 

[Impala-ASF-CR] IMPALA-4810: Make DECIMAL expr-test cases table driven

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4810: Make DECIMAL expr-test cases table driven
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5933/4/be/src/testutil/impalad-query-executor.h
File be/src/testutil/impalad-query-executor.h:

Line 87:   void pushExecOption(const std::string& exec_option) {
nit: fn names are not according to our style guide


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieffb2573de46bba64d1b4e8caf15cc0238a84ea1
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4818: Ensure the same number of tests are run every time

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4818: Ensure the same number of tests are run every time
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I22cecfbe7c9a102f788d01eb80aa188579ef6d7e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4729: Implement REPLACE()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4729: Implement REPLACE()
..


Patch Set 20: Verified-1

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/253/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1780a7d8fee6d0db9dad148217fb6eb10f773329
Gerrit-PatchSet: 20
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..


Patch Set 2:

I'm not opposed to cleaning up the AnyVal stuff like that, but given that udf.h 
stuff dictates UDF compatibility, it's not completely trivial. It doesn't look 
like it would break binary compatibility though. But, in case something goes 
wrong, how about we do that as a separate change so it could be backed out 
without affecting the decimal work? It doesn't look like the decimal stuff will 
depend on it, right?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI

[Dimitris Tsirogiannis (Code Review)]

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI
..


Patch Set 2: Code-Review+1

Keep Alex's +1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I543d4d428d7240e0f710d67973867162f2fcabc8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI

[Dimitris Tsirogiannis (Code Review)]

Hello Alex Behm,

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

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

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

Change subject: IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI
..

IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI

This commit adds heap and non-heap memory usage of the embedded
JVM in the memory metrics and exposes these metrics in /memz web page of
the impalad and catalog web UI.

Change-Id: I543d4d428d7240e0f710d67973867162f2fcabc8
---
M be/src/util/default-path-handlers.cc
M fe/src/main/java/org/apache/impala/common/JniUtil.java
M www/memz.tmpl
3 files changed, 110 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I543d4d428d7240e0f710d67973867162f2fcabc8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 

[Impala-ASF-CR] IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI

[Dimitris Tsirogiannis (Code Review)]

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-4884: Add JVM heap and non-heap usage in metrics and UI
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5909/1/be/src/util/default-path-handlers.cc
File be/src/util/default-path-handlers.cc:

Line 163:   document->AddMember("jvm_pool", total, 
document->GetAllocator());
> jvm_total
Done


http://gerrit.cloudera.org:8080/#/c/5909/1/www/memz.tmpl
File www/memz.tmpl:

Line 92: JVM total pool memory usage
> It somewhat confused me, so might be better to omit.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I543d4d428d7240e0f710d67973867162f2fcabc8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2020: Add rounding when casting from decimal to int

[Zach Amsden (Code Review)]

Zach Amsden has uploaded a new patch set (#2).

Change subject: IMPALA-2020: Add rounding when casting from decimal to int
..

IMPALA-2020: Add rounding when casting from decimal to int

Preview diff, not working yet, mostly to see if I can successfully
push diffs against my ASF Impala fork, and also to get early
feedback on the UDF change.  Update - pushing from fork didn't
work.

Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/runtime/decimal-value.h
M be/src/runtime/decimal-value.inline.h
M be/src/udf/udf.h
4 files changed, 41 insertions(+), 109 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 

[Impala-ASF-CR] IMPALA-4842: BufferedBlockMgrTest.WriteError is flaky

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4842: BufferedBlockMgrTest.WriteError is flaky
..


Patch Set 1: Verified-1

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/251/

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

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

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..


Patch Set 22: Verified-1

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/250/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 22
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Impala ABM / LZCNT support

[Zach Amsden (Code Review)]

Zach Amsden has uploaded a new patch set (#3).

Change subject: Impala ABM / LZCNT support
..

Impala ABM / LZCNT support

I recently added some code that wants to do upwards power of 2
calculation.  Turns out this can be done much more quickly in
hardware.  It isn't on a perf critical code path yet but
still seems like a decent idea.

PopcountNoHw was absolutely atrocious as it contains a totally
unpredictable loop that can be computed much more efficiently,
so I fixed that.

Testing: Added a perf test to verify this is faster (it is)
and updated the bit-util-test to add better test coverage.

Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-intrinsics-benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.h
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
M be/src/util/fixed-size-hash-table.h
M be/src/util/sse-util.h
8 files changed, 287 insertions(+), 19 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#10).

Change subject: IMPALA-4822: Implement dynamic log level changes
..

IMPALA-4822: Implement dynamic log level changes

Very often we have to change the logging levels
of Impalads and Catalog server for debugging purposes.
Currently, there is no way to do that without a restart
of the daemons, which is not a viable option in production
deployments.

This patch addresses this supportability gap by exposing
the ability to set dynamic logging levels using a simple
web endpoint on Impalad and Catalog web pages.

This includes setting VLOG levels (equivalent to --v flag)
as well as setting log4j levels on the Frontend and the
Catalog JVMs.

Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
---
M be/src/catalog/catalog-server.cc
M be/src/service/impala-http-handler.cc
M be/src/statestore/statestore.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M be/src/util/logging-support.cc
M be/src/util/logging-support.h
M common/thrift/Logging.thrift
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/util/GlogAppender.java
M tests/webserver/test_web_pages.py
A www/log_level.tmpl
12 files changed, 483 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 

[Impala-ASF-CR] IMPALA-4729: Implement REPLACE()

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4729: Implement REPLACE()
..


Patch Set 20:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/253/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1780a7d8fee6d0db9dad148217fb6eb10f773329
Gerrit-PatchSet: 20
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4731: Crash when sorting on non-deterministic expr

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4731: Crash when sorting on non-deterministic expr
..


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/252/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5dcda32fc7770d42fc500ce87fc54d58e5b5dc00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4729: Implement REPLACE()

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4729: Implement REPLACE()
..


Patch Set 20:

Trivial test fix +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1780a7d8fee6d0db9dad148217fb6eb10f773329
Gerrit-PatchSet: 20
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4729: Implement REPLACE()

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4729: Implement REPLACE()
..


Patch Set 20: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1780a7d8fee6d0db9dad148217fb6eb10f773329
Gerrit-PatchSet: 20
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4895: Memory limit exceeded in test outer joins

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4895: Memory limit exceeded in test_outer_joins
..


IMPALA-4895: Memory limit exceeded in test_outer_joins

A recent change (IMPALA-3524) removed a 'CATCH' section for a
mem limit exceeded error because the other changes in the patch
reduced the memory requirements for that particular query and
the error was no longer being hit.

This seemed okay because the point of the test wasn't to trigger
the mem limit exceeded error, and I manually verified that the
situation was the test was addressing was still covered even
without the error being hit.

It turns out, though, that the test still hits the error in some
situations (local-filesystem and non-partitioned-aggs-and-joins
builds).

The fix is to make the test more permissive by adding '__NO_ERROR_'
as one of the options in the 'CATCH: ANY_OF' section, so that it
passes whether or not the mem limit is exceeded.

Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
Reviewed-on: http://gerrit.cloudera.org:8080/5941
Reviewed-by: Dan Hecht 
Tested-by: Impala Public Jenkins
---
M testdata/workloads/tpch/queries/tpch-outer-joins.test
M tests/common/impala_test_suite.py
2 files changed, 5 insertions(+), 1 deletion(-)

Approvals:
  Impala Public Jenkins: Verified
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4895: Memory limit exceeded in test outer joins

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4895: Memory limit exceeded in test_outer_joins
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

Re: [Impala-ASF-CR] IMPALA-4729: Implement REPLACE()

[Alex Behm]

Patch has some test failures, please fix.

On Tue, Feb 7, 2017 at 9:23 PM, Impala Public Jenkins (Code Review) <
ger...@cloudera.org> wrote:

> Impala Public Jenkins has posted comments on this change.
>
> Change subject: IMPALA-4729: Implement REPLACE()
> ..
>
>
> Patch Set 19: Verified-1
>
> Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/247/
>
> --
> To view, visit http://gerrit.cloudera.org:8080/5776
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
> Gerrit-MessageType: comment
> Gerrit-Change-Id: I1780a7d8fee6d0db9dad148217fb6eb10f773329
> Gerrit-PatchSet: 19
> Gerrit-Project: Impala-ASF
> Gerrit-Branch: master
> Gerrit-Owner: Zach Amsden 
> Gerrit-Reviewer: Alex Behm 
> Gerrit-Reviewer: Dan Hecht 
> Gerrit-Reviewer: Impala Public Jenkins
> Gerrit-Reviewer: Michael Ho 
> Gerrit-Reviewer: Tim Armstrong 
> Gerrit-Reviewer: Zach Amsden 
> Gerrit-HasComments: No
>
> --
> You received this message because you are subscribed to the Google Groups
> "impala-cr" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to impala-cr+unsubscr...@cloudera.com.
> For more options, visit https://groups.google.com/a/cloudera.com/d/optout.
>

[Impala-ASF-CR] IMPALA-4731: Crash when sorting on non-deterministic expr

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4731: Crash when sorting on non-deterministic expr
..


Patch Set 2:

(2 comments)

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

Line 173: ExprSubstitutionMap substOrderBy = new ExprSubstitutionMap();
It's important to update this smap according to the new materialization you are 
doing below. I'd prefer to keep the original flow where we populate this smap 
and then substituteOrderingExprs().

For example, one case that I believe will not work as expected is:

select rand() r from t order by r

The reason is that we will not substitute the rand() from the select list with 
the materialized slot produced in the order by - but we should. Similar 
arguments apply for something like:

select my_expensive_expr() e from t order by e

We will redundantly evaluate my_expensive_expr in the select list again.


Line 186: // TODO: it may be better for performance to not materialize some 
exprs that are
I think we should address this TODO while we are here.

We should materialize:
1. All known non-deterministic functions.
We can add an Expr.isDeterministic() function and implement that for 
FunctionCallExpr like we implement isConstant().

2. All UDFs. We can check whether an Expr is a UDF.

3. Exprs that are 'expensive' according to some cost threshold. If the cost is 
unknown, I'd say materialize.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5dcda32fc7770d42fc500ce87fc54d58e5b5dc00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4839: Remove implicit 'localhost' for KUDU MASTER HOSTS

[David Knupp (Code Review)]

Hello Michael Brown, Matthew Jacobs,

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

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

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

Change subject: IMPALA-4839: Remove implicit 'localhost' for KUDU_MASTER_HOSTS
..

IMPALA-4839: Remove implicit 'localhost' for KUDU_MASTER_HOSTS

The Kudu query tests were failing on a remote cluster because the Kudu
master was always set to '127.0.0.1', with no way to override it.

This patch corrects the issue with a number of changes:

- Add a pytest command line option to specify an arbitrary Kudu master

- Consolidate the place where the default Kudu master is derived. It
  had been stored both in the env and in tests/common/__init__.py,
  with different files looking to different places. For now, just look
  to the env, and remove the value from __init__.py.

- The kudu_client test fixture in conftest.py was using the connect()
  method from impala.dbapi (part of the Impyla library), without
  specifying the host param. In the absence of that, the default value
  is 'localhost', so add the host param to the connect() call.

- Define the various defaults for pytest config as constants at the top
  of conftest.py.

Change-Id: I9df71480a165f4ce21ae3edab6ce7227fbf76f77
---
M bin/impala-config.sh
M bin/start-impala-cluster.py
M testdata/bin/generate-schema-statements.py
M tests/common/__init__.py
M tests/conftest.py
M tests/custom_cluster/test_kudu.py
M tests/query_test/test_kudu.py
7 files changed, 52 insertions(+), 39 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9df71480a165f4ce21ae3edab6ce7227fbf76f77
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Ishaan Joshi 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 

[Impala-ASF-CR] IMPALA-4839: Remove implicit 'localhost' for KUDU MASTER HOSTS

[David Knupp (Code Review)]

David Knupp has posted comments on this change.

Change subject: IMPALA-4839: Remove implicit 'localhost' for KUDU_MASTER_HOSTS
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5877/7/tests/conftest.py
File tests/conftest.py:

Line 317:   try:
> With a yield fixture, py.test treats everything after the yield as teardown
That's right. Thanks for catching this. I wasn't looking at anything outside of 
the scope of this particular problem. Even though it's orthogonal, I went ahead 
and fixed it, and checked that all of the Kudu tests still run. (Also, for what 
it's worth, we aren't upgrading past 2.9.2, since the next version of pytest 
following that introduced some backwards-breaking changes.)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9df71480a165f4ce21ae3edab6ce7227fbf76f77
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: David Knupp 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Ishaan Joshi 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4842: BufferedBlockMgrTest.WriteError is flaky

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4842: BufferedBlockMgrTest.WriteError is flaky
..


Patch Set 1:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/251/

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

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

[Impala-ASF-CR] IMPALA-4842: BufferedBlockMgrTest.WriteError is flaky

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4842: BufferedBlockMgrTest.WriteError is flaky
..


Patch Set 1: Code-Review+2

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

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

[Impala-ASF-CR] IMPALA-4849: IllegalStateException from rewritten CASE expr

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4849: IllegalStateException from rewritten CASE expr
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I16ff88716b185e1d72d2bc603a42bd06c60ec18e
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..


Patch Set 22: Code-Review+2

carry +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 22
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..


Patch Set 22:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/250/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 22
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..


Patch Set 20:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5161/20/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

Line 106: ArrayList aggFnArgTypes = Lists.newArrayList();
> unused?
Done


Line 554: "in original expr %s", toSql(), 
copiedInputType.toString(),
> also use toSql() for types
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 20
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Tim Armstrong (Code Review)]

Hello Impala Public Jenkins, Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..

IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

This uses the existing infrastructure for codegening builtin UDAs and
for codegening calls to UDFs. GetUdf() is refactored to support both
UDFs and UDAs.

IR UDAs are still not allowed by the frontend. It's unclear if we want
to enable them going forward because of the difficulties in testing and
supporting IR UDFs/UDAs.

This also fixes some bugs with the Get*Type() methods of
FunctionContext. GetArgType() and related methods now always return the
logical input types of the aggregate function. Getting the tests to pass
required fixing IMPALA-4878 because they called GetIntermediateType().

Testing:
test_udfs.py tests UDAs with codegen enabled and disabled.

Added some asserts to test UDAs to check that the correct types are
passed in via the FunctionContext.

Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/agg-fn-evaluator.h
M be/src/exprs/anyval-util.cc
M be/src/exprs/anyval-util.h
M be/src/exprs/expr.cc
M be/src/exprs/expr.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
M be/src/exprs/timestamp-functions.cc
M be/src/runtime/types.cc
M be/src/runtime/types.h
M be/src/testutil/test-udas.cc
M be/src/testutil/test-udas.h
M be/src/udf/udf-internal.h
M be/src/udf/udf-ir.cc
M be/src/udf/udf.h
M common/thrift/Exprs.thrift
M fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M testdata/workloads/functional-query/queries/QueryTest/uda.test
M tests/query_test/test_udfs.py
26 files changed, 501 insertions(+), 248 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/5161/21
-- 
To view, visit http://gerrit.cloudera.org:8080/5161
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 21
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..


Patch Set 20:

(2 comments)

FE lgtm +2

http://gerrit.cloudera.org:8080/#/c/5161/20/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

Line 106: ArrayList aggFnArgTypes = Lists.newArrayList();
unused?


Line 554: "in original expr %s", toSql(), 
copiedInputType.toString(),
also use toSql() for types


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 20
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3410 [DOCS] Rework Impala security topics to be generic

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-3410 [DOCS] Rework Impala security topics to be generic
..


Patch Set 3: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie5c4431f3236b18fc282343ed98513f0e578130e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Laurel Hale 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..


Patch Set 19:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5161/19/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

Line 55:   // If this is merge aggregation function, the input argument types 
of the aggregate
> I'm fine with either approach, but:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Tim Armstrong (Code Review)]

Hello Impala Public Jenkins, Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..

IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

This uses the existing infrastructure for codegening builtin UDAs and
for codegening calls to UDFs. GetUdf() is refactored to support both
UDFs and UDAs.

IR UDAs are still not allowed by the frontend. It's unclear if we want
to enable them going forward because of the difficulties in testing and
supporting IR UDFs/UDAs.

This also fixes some bugs with the Get*Type() methods of
FunctionContext. GetArgType() and related methods now always return the
logical input types of the aggregate function. Getting the tests to pass
required fixing IMPALA-4878 because they called GetIntermediateType().

Testing:
test_udfs.py tests UDAs with codegen enabled and disabled.

Added some asserts to test UDAs to check that the correct types are
passed in via the FunctionContext.

Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/agg-fn-evaluator.h
M be/src/exprs/anyval-util.cc
M be/src/exprs/anyval-util.h
M be/src/exprs/expr.cc
M be/src/exprs/expr.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
M be/src/exprs/timestamp-functions.cc
M be/src/runtime/types.cc
M be/src/runtime/types.h
M be/src/testutil/test-udas.cc
M be/src/testutil/test-udas.h
M be/src/udf/udf-internal.h
M be/src/udf/udf-ir.cc
M be/src/udf/udf.h
M common/thrift/Exprs.thrift
M fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M testdata/workloads/functional-query/queries/QueryTest/uda.test
M tests/query_test/test_udfs.py
26 files changed, 504 insertions(+), 248 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 20
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

PS7, Line 45: TResetJavaLogLevelParams
> I would also settle for comments and / or a better method name if this is t
Henry, I agree with your original comment that passing in these flags for every 
invocation is kind of confusing. To me it seems clearer to keep these defaults 
in the BackendConfig


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

PS7, Line 45: TResetJavaLogLevelParams
> Yes, I'll do it then. Just wanted to confirm that is the ask here.
I would also settle for comments and / or a better method name if this is the 
'right' place to have the parameters.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

PS7, Line 45: TResetJavaLogLevelParams
> Seems easy enough to just keep them in the BackendConfig then
Yes, I'll do it then. Just wanted to confirm that is the ask here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5792/7/common/thrift/Logging.thrift
File common/thrift/Logging.thrift:

PS7, Line 45: TResetJavaLogLevelParams
> We already do that [1], just that we don't save them in the frontend anywhe
Seems easy enough to just keep them in the BackendConfig then


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I588418e9bcb0b66d33138baf96207a5a35bfbd63
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3410 [DOCS] Rework Impala security topics to be generic

[Ambreen Kazi (Code Review)]

Ambreen Kazi has posted comments on this change.

Change subject: IMPALA-3410 [DOCS] Rework Impala security topics to be generic
..


Patch Set 1:

(14 comments)

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

Line 10: topics. References to CDH and Cloudera Manager docs/products have been 
either
> wrap at 70 characters (the red line)
Done


http://gerrit.cloudera.org:8080/#/c/5931/1/docs/shared/impala_common.xml
File docs/shared/impala_common.xml:

PS1, Line 608: audience="Cloudera"
> This should be audience="hidden". (There shouldn't be any audience="Clouder
I found it like this. Changed to hidden now.


PS1, Line 616: appropropriate
> note to self - typo
Done


http://gerrit.cloudera.org:8080/#/c/5931/1/docs/topics/impala_authorization.xml
File docs/topics/impala_authorization.xml:

PS1, Line 319: recommends
 :   managing privileges through SQL statements, as 
described in
 :   . If you are still using policy 
files, plan to
 :   migrate to the new approach some time in the future.
> Should we genericize the wording but keep the recommendation to use policy 
Genericized the wording. The recommendation should be to use the SQL 
statements. I've removed the 'some time in the future' wording as well because 
it's been a while since the Sentry service released.


Line 850:   To enable URIs in per-DB policy files, the Java 
configuration option sentry.allow.uri.db.policyfile must be 
set to true.
> Shorter lines maker for easier reviews
Done


PS1, Line 853: 
> For example:
Done


PS1, Line 951: we strongly recommend
> I've stayed away from "we recommend" because there isn't the same commitmen
I've reworded this note to make it more of an instruction rather than a 
suggestion/recommendation.


http://gerrit.cloudera.org:8080/#/c/5931/1/docs/topics/impala_security.xml
File docs/topics/impala_security.xml:

PS1, Line 40: the Apache Sentry
:   open source project
> Optional: can be shortened to just "Apache Sentry".
Done


PS1, Line 45: focussed
> focused (American spelling)
Done


PS1, Line 116:  
> hyphen, like previous instance
Done


http://gerrit.cloudera.org:8080/#/c/5931/1/docs/topics/impala_security_install.xml
File docs/topics/impala_security_install.xml:

PS1, Line 38:  
> Trailing blank.
Done


http://gerrit.cloudera.org:8080/#/c/5931/1/docs/topics/impala_ssl.xml
File docs/topics/impala_ssl.xml:

PS1, Line 39:  
> Let's start the text on the line after the  tag.
Done


PS1, Line 44: 

[Impala-ASF-CR] IMPALA-3410 [DOCS] Rework Impala security topics to be generic

[Ambreen Kazi (Code Review)]

Ambreen Kazi has uploaded a new patch set (#3).

Change subject: IMPALA-3410 [DOCS] Rework Impala security topics to be generic
..

IMPALA-3410 [DOCS] Rework Impala security topics to be generic

This is part 1 of the changes being made to the Impala authorization
topics. References to CDH and Cloudera Manager docs/products have been
either 'hidden' or removed completely.

Examples with Sentry have been made more generic. Instances of
Cloudera-specific folders or filenames have been removed.

Change-Id: Ie5c4431f3236b18fc282343ed98513f0e578130e
---
M docs/shared/impala_common.xml
M docs/topics/impala_authorization.xml
M docs/topics/impala_security.xml
M docs/topics/impala_security_install.xml
M docs/topics/impala_ssl.xml
5 files changed, 84 insertions(+), 93 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie5c4431f3236b18fc282343ed98513f0e578130e
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Ambreen Kazi 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Laurel Hale 

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..


Patch Set 19:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5161/19/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

Line 55:   // If this is merge aggregation function, the input argument types 
of the aggregate
> Doesn't that somehow entangle the lifecycles of the two different sets of e
I'm fine with either approach, but:

This comment should explain precisely what is inside aggFnArgTypes_. Even the 
variable name is imprecise because these are not "function argument types" 
since those could be derived from the function signature.

That's why I'm saying that having the cloned parent expr is clearer - because 
we get the types from the children of an actual expr instance and not the 
function signature.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4643: [DOCS] Set up many new keydefs

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4643: [DOCS] Set up many new keydefs
..


IMPALA-4643: [DOCS] Set up many new keydefs

Make keydefs corresponding to most of the links
(especially external links) throughout the doc source.

Make keydefs for all possible IMPALA- JIRA issues, up to
IMPALA-.

Change-Id: If57c5730f80fd32ee77b31849e4e75afd53fab38
Reviewed-on: http://gerrit.cloudera.org:8080/5923
Reviewed-by: Ambreen Kazi 
Tested-by: Impala Public Jenkins
Reviewed-by: John Russell 
---
M docs/impala_keydefs.ditamap
1 file changed, 9,836 insertions(+), 193 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Ambreen Kazi: Looks good to me, but someone else must approve
  John Russell: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If57c5730f80fd32ee77b31849e4e75afd53fab38
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Laurel Hale 

[Impala-ASF-CR] IMPALA-4643: [DOCS] Set up many new keydefs

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4643: [DOCS] Set up many new keydefs
..


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-docs-submit/37/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If57c5730f80fd32ee77b31849e4e75afd53fab38
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Laurel Hale 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..


Patch Set 19:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5161/19/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

Line 55:   // If this is merge aggregation function, the input argument types 
of the aggregate
> I think the behavior and limitations are going to be clearer if we keep a p
Doesn't that somehow entangle the lifecycles of the two different sets of exprs?

As a general design pattern storing pointers from one expr tree to a separate 
expr tree that can be mutated in-place seems like a bad idea - too hard to 
reason about. I could see just storing a clone of the source expression but I'm 
not sure that that's significantly better than the current approach.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Bump Kudu version to latest Kudu master

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: Bump Kudu version to latest Kudu master
..


Patch Set 1:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/249/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib72a0b63cdffb852bd17e69faa0a36edbfda22d7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-1430,IMPALA-4878,IMPALA-4879: codegen native UDAs
..


Patch Set 19:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5161/19/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:

Line 55:   // If this is merge aggregation function, the input argument types 
of the aggregate
I think the behavior and limitations are going to be clearer if we keep a 
pointer to the 'parent' FunctionCallExpr instead of isMergAggFn_ and 
aggFnArgTypes_.

That way it's also clear that we need the actual child expr types and not just 
the argument types of the function signature (which is a subtle difference that 
is not totally clear from the comments).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1708eaa96eb76fb9bec5eeabf209f81c88eec2f
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2716: Hive/Impala incompatibility for timestamp data in Parquet

[Attila Jeges (Code Review)]

Attila Jeges has uploaded a new patch set (#2).

Change subject: IMPALA-2716: Hive/Impala incompatibility for timestamp data in 
Parquet
..

IMPALA-2716: Hive/Impala incompatibility for timestamp data in Parquet

Before this change:
Hive adjusts timestamps by subtracting the local time zone's offset
from all values when writing data to Parquet files. Hive is internally
inconsistent because it behaves differently for other file formats. As
a result of this adjustment, Impala may read "incorrect" timestamp
values from Parquet files written by Hive, and vice versa.

After this change:
Impala reads Parquet MR timestamp data and adjust values using a time
zone from a table property (parquet.mr.int96.write.zone), if set, and
will not adjust it if the property is absent. No adjustment will be
applied to data written by Impala.

New tables created by Impala will set the table property to UTC if the
global flag --prevent_parquet_mr_zone_adjustment is set to true.

Tables created using CREATE TABLE and CREATE TABLE LIKE FILE will not
set the table property unless the global flag is set to true.

Tables created using CREATE TABLE LIKE  will copy the
property of the table that is copied.

Change-Id: I3f24525ef45a2814f476bdee76655b30081079d6
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/parquet-column-readers.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.h
M be/src/runtime/timestamp-value.cc
M be/src/runtime/timestamp-value.h
M be/src/service/fe-support.cc
M be/src/service/impala-server.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/Frontend.thrift
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M tests/common/impala_test_suite.py
A tests/custom_cluster/test_parquet_timestamp_compatibility.py
M tests/metadata/test_ddl.py
23 files changed, 586 insertions(+), 27 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f24525ef45a2814f476bdee76655b30081079d6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges 

[Impala-ASF-CR] IMPALA-2716: Hive/Impala incompatibility for timestamp data in Parquet

[Attila Jeges (Code Review)]

Attila Jeges has uploaded a new patch set (#2).

Change subject: IMPALA-2716: Hive/Impala incompatibility for timestamp data in 
Parquet
..

IMPALA-2716: Hive/Impala incompatibility for timestamp data in Parquet

Before this change:
Hive adjusts timestamps by subtracting the local time zone's offset
from all values when writing data to Parquet files. Hive is internally
inconsistent because it behaves differently for other file formats. As
a result of this adjustment, Impala may read "incorrect" timestamp
values from Parquet files written by Hive, and vice versa.

After this change:
Impala reads Parquet MR timestamp data and adjust values using a time
zone from a table property (parquet.mr.int96.write.zone), if set, and
will not adjust it if the property is absent. No adjustment will be
applied to data written by Impala.

New tables created by Impala will set the table property to UTC if the
global flag --prevent_parquet_mr_zone_adjustment is set to true.

Tables created using CREATE TABLE and CREATE TABLE LIKE FILE will not
set the table property unless the global flag is set to true.

Tables created using CREATE TABLE LIKE  will copy the
property of the table that is copied.

Change-Id: I3f24525ef45a2814f476bdee76655b30081079d6
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/parquet-column-readers.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.h
M be/src/runtime/timestamp-value.cc
M be/src/runtime/timestamp-value.h
M be/src/service/fe-support.cc
M be/src/service/impala-server.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/Frontend.thrift
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M tests/common/impala_test_suite.py
A tests/custom_cluster/test_parquet_timestamp_compatibility.py
M tests/metadata/test_ddl.py
23 files changed, 586 insertions(+), 27 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f24525ef45a2814f476bdee76655b30081079d6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges 

[Impala-ASF-CR] IMPALA-4895: Memory limit exceeded in test outer joins

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4895: Memory limit exceeded in test_outer_joins
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4895: Memory limit exceeded in test outer joins

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4895: Memory limit exceeded in test_outer_joins
..


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/248/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4895: Memory limit exceeded in test outer joins

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-4895: Memory limit exceeded in test_outer_joins
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5941/1/testdata/workloads/tpch/queries/tpch-outer-joins.test
File testdata/workloads/tpch/queries/tpch-outer-joins.test:

Line 35: __NO_ERROR__
> Can we make it something that is unlikely to be a substring of a real error
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Bump Kudu version to latest Kudu master

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: Bump Kudu version to latest Kudu master
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib72a0b63cdffb852bd17e69faa0a36edbfda22d7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4895: Memory limit exceeded in test outer joins

[Thomas Tauber-Marshall (Code Review)]

Hello Dan Hecht,

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

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

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

Change subject: IMPALA-4895: Memory limit exceeded in test_outer_joins
..

IMPALA-4895: Memory limit exceeded in test_outer_joins

A recent change (IMPALA-3524) removed a 'CATCH' section for a
mem limit exceeded error because the other changes in the patch
reduced the memory requirements for that particular query and
the error was no longer being hit.

This seemed okay because the point of the test wasn't to trigger
the mem limit exceeded error, and I manually verified that the
situation was the test was addressing was still covered even
without the error being hit.

It turns out, though, that the test still hits the error in some
situations (local-filesystem and non-partitioned-aggs-and-joins
builds).

The fix is to make the test more permissive by adding '__NO_ERROR_'
as one of the options in the 'CATCH: ANY_OF' section, so that it
passes whether or not the mem limit is exceeded.

Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
---
M testdata/workloads/tpch/queries/tpch-outer-joins.test
M tests/common/impala_test_suite.py
2 files changed, 5 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4895: Memory limit exceeded in test outer joins

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4895: Memory limit exceeded in test_outer_joins
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5941/1/testdata/workloads/tpch/queries/tpch-outer-joins.test
File testdata/workloads/tpch/queries/tpch-outer-joins.test:

Line 35: none
Can we make it something that is unlikely to be a substring of a real error 
message? Just to avoid confusion. E.g. __NO_ERROR__ or something like that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4895: Memory limit exceeded in test outer joins

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4895: Memory limit exceeded in test_outer_joins
..


Patch Set 1: Code-Review+1

Seems okay to me.
Michael, are you okay with having this "none" mechanism?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4895: Memory limit exceeded in test outer joins

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded a new change for review.

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

Change subject: IMPALA-4895: Memory limit exceeded in test_outer_joins
..

IMPALA-4895: Memory limit exceeded in test_outer_joins

A recent change (IMPALA-3524) removed a 'CATCH' section for a
mem limit exceeded error because the other changes in the patch
reduced the memory requirements for that particular query and
the error was no longer being hit.

This seemed okay because the point of the test wasn't to trigger
the mem limit exceeded error, and I manually verified that the
situation was the test was addressing was still covered even
without the error being hit.

It turns out, though, that the test still hits the error in some
situations (local-filesystem and non-partitioned-aggs-and-joins
builds).

The fix is to make the test more permissive by adding 'none' as
one of the options in the 'CATCH: ANY_OF' section, so that it
passes whether or not the mem limit is exceeded.

Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
---
M testdata/workloads/tpch/queries/tpch-outer-joins.test
M tests/common/impala_test_suite.py
2 files changed, 5 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4731a3e83dd2142a1d83be963f83cd1847472295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-4842: BufferedBlockMgrTest.WriteError is flaky

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new change for review.

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

Change subject: IMPALA-4842: BufferedBlockMgrTest.WriteError is flaky
..

IMPALA-4842: BufferedBlockMgrTest.WriteError is flaky

The test should allow Unpin() to fail with a scratch allocation error to
handle the case where the first write fails and blacklists the scratch
disk around the same time that the second write starts. Usually either
the second write succeeds because it started before the first write
failed or it fails with CANCELLED because the
BufferedBlockMgr::is_cancelled_ flag is set. There is a small
window for a race after the disk is blacklisted in TmpFileMgr but
before BufferedBlockMgr::WriteComplete() is called.

Testing:
I was able to reproduce the problem locally by adding some delays
to the test. I added a variant of the WriteError test that more reliably
reproduces the bug. Ran both WriteError tests in a loop locally to try
to flush out flakiness.

Change-Id: I9878d7000b03a64ee06c2088a8c30e318fe1d2a3
---
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M common/thrift/generate_error_codes.py
3 files changed, 43 insertions(+), 13 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-4828: Alter Kudu schema outside Impala may crash on read

[Matthew Jacobs (Code Review)]

Matthew Jacobs has uploaded a new patch set (#5).

Change subject: IMPALA-4828: Alter Kudu schema outside Impala may crash on read
..

IMPALA-4828: Alter Kudu schema outside Impala may crash on read

Creating a table in Impala, changing the column schema
outside of Impala, and then reading again in Impala may
result in a crash. Impala may attempt to dereference
pointers that aren't there. This happens if a string column
is dropped and then a new, non string column is added with
the old string column's name.

The Kudu scan token contains the projection schema, and that
is validated when opening the Kudu scanner, but the issue is
that during planning, Impala assumes the types of columns
haven't changed when creating the scan tokens. This is fixed
by adding a check when creating the scan token, and failing
the query if the column types changed. Impala then relies on
the Kudu scanner to properly validate that the underlying
schema is still represented by the scan token, and that
deserialization will fail if it no longer matches. A test
case was added for this particular crash scenario, which now
fails during planning as expected. This does not attempt to
validate the Kudu client validation at deserialization time,
though that would be valuable coverage to add in the future.

Columns being removed don't produce a crash; the query fails
gracefully. A test was added for this case.

Columns being added should not affect this scenario, but a
test was added anyway.

Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a
---
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M tests/query_test/test_kudu.py
2 files changed, 116 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-2716: Hive/Impala incompatibility for timestamp data in Parquet

[Attila Jeges (Code Review)]

Attila Jeges has uploaded a new change for review.

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

Change subject: IMPALA-2716: Hive/Impala incompatibility for timestamp data in 
Parquet
..

IMPALA-2716: Hive/Impala incompatibility for timestamp data in Parquet

Before this change:
Hive adjusts timestamps by subtracting the local time zone's offset
from all values when writing data to Parquet files. Hive is internally
inconsistent because it behaves differently for other file formats. As
a result of this adjustment, Impala may read "incorrect" timestamp
values from Parquet files written by Hive, and vice versa.

After this change:
Impala reads Parquet MR timestamp data and adjust values using a time
zone from a table property (parquet.mr.int96.write.zone), if set, and
will not adjust it if the property is absent. No adjustment will be
applied to data written by Impala.

New tables created by Impala will set the table property to UTC if the
global flag --prevent_parquet_mr_zone_adjustment is set to true.

Tables created using CREATE TABLE and CREATE TABLE LIKE FILE will not
set the table property unless the global flag is set to true.

Tables created using CREATE TABLE LIKE  will copy the
property of the table that is copied.

Change-Id: I3f24525ef45a2814f476bdee76655b30081079d6
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/parquet-column-readers.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.h
M be/src/runtime/timestamp-value.cc
M be/src/runtime/timestamp-value.h
M be/src/service/fe-support.cc
M be/src/service/impala-server.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/Frontend.thrift
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M tests/common/impala_test_suite.py
A tests/custom_cluster/test_parquet_timestamp_compatibility.py
M tests/metadata/test_ddl.py
23 files changed, 588 insertions(+), 27 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3f24525ef45a2814f476bdee76655b30081079d6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges 

[Impala-ASF-CR] IMPALA-4828: Alter Kudu schema outside Impala may crash on read

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4828: Alter Kudu schema outside Impala may crash on read
..


Patch Set 2:

After discussing with Dan on the Kudu team, we can make this simpler by 
checking at plan time because the kudu scan token encodes the col metadata and 
deserializing it will fail if the projection schema is no longer valid. The 
issue for us was that we did not check the Kudu col type matched our col type 
at plan time.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6d43f5bb9811e728ad592933066d006c8fb4553a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3748: Part 1: Clean up resource estimation in planner

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3748: Part 1: Clean up resource estimation in planner
..


Patch Set 4:

(5 comments)

Some minor cleanup and tests fixes

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

Line 51:   // Default per-host memory requirement used if no valid stats are 
available.
> Need to fix comment.
Done


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

Line 85: long perInstanceInputCardinality = Math.max(1L, 
inputNode.getCardinality() / numInstances);
> Need to fix long lines
Done


Line 138: output.append(PrintUtils.printInstances(" ", 
fragment_.getNumInstances(queryOptions)));
> Need to fix long line
Done


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

Line 62:   output.append(PrintUtils.printInstances(" ", 
fragment_.getNumInstances(queryOptions)));
> need to fix long line
Done


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

Line 213: return dop * planRoot_.getNumNodes();
> Needs to handle when getNumNodes() is invalid, i.e. -1
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e358182bcf2bc5fe5c73883eb97878735b12d37
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3748: Part 1: Clean up resource estimation in planner

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-3748: Part 1: Clean up resource estimation in planner
..

IMPALA-3748: Part 1: Clean up resource estimation in planner

This is in preparation to use this code to compute the minimum
reservation.

The cleanup restructures the code slightly so that it's clearer whether
resource estimates are valid or invalid. It also removes the unused
VCore estimates.

Fixes various bugs and other issues:
* computeCosts() was not called for unpartitioned fragments, so
  the per-operator memory estimate was not visible.
* Nested loop join was not treated as a blocking join.
* The TODO comment about union was misleading
* The logic does not work for mt_dop > 1 because it conflates fragment
  instances with hosts. Fixing this requires identifying places where
  we want per-instance estimates instead of per-host.

I left one bug unfixed because it is subtle and will be easier to review
in isolation: IMPALA-4862.

There is some remaining questionable behaviour I left untouched:
* It's unclear why unpartitioned fragments are always excluded from
  total memory consumption.
* Many operators do not have estimates or have questionable heuristics.

Testing:
Re-enabled the explain_level tests, which appear to be the only
coverage for many of these estimates. Removed the complex and
brittle test cases and replaced with a couple of much simpler
end-to-end tests and a number of planner test cases for
memory estimates in both the MT and non-MT cases.

Change-Id: I1e358182bcf2bc5fe5c73883eb97878735b12d37
---
M be/src/service/query-exec-state.cc
M common/thrift/Frontend.thrift
M docs/shared/impala_common.xml
M docs/topics/impala_explain.xml
M docs/topics/impala_explain_level.xml
M docs/topics/impala_explain_plan.xml
M docs/topics/impala_hbase.xml
M docs/topics/impala_known_issues.xml
M docs/topics/impala_optimize_partition_key_scans.xml
M docs/topics/impala_perf_joins.xml
M docs/topics/impala_perf_stats.xml
M fe/src/main/java/org/apache/impala/common/PrintUtils.java
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java
M fe/src/main/java/org/apache/impala/planner/DataSink.java
M fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java
M fe/src/main/java/org/apache/impala/planner/DataStreamSink.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/EmptySetNode.java
M fe/src/main/java/org/apache/impala/planner/HBaseScanNode.java
M fe/src/main/java/org/apache/impala/planner/HBaseTableSink.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java
M fe/src/main/java/org/apache/impala/planner/JoinBuildSink.java
M fe/src/main/java/org/apache/impala/planner/JoinNode.java
M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java
M fe/src/main/java/org/apache/impala/planner/NestedLoopJoinNode.java
M fe/src/main/java/org/apache/impala/planner/PipelinedPlanNodeSet.java
M fe/src/main/java/org/apache/impala/planner/PlanFragment.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M fe/src/main/java/org/apache/impala/planner/PlanRootSink.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M fe/src/main/java/org/apache/impala/planner/UnionNode.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
A 
testdata/workloads/functional-planner/queries/PlannerTest/resource-estimates.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level0.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level1.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level2.test
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
M tests/custom_cluster/test_admission_controller.py
M tests/metadata/test_explain.py
47 files changed, 1,544 insertions(+), 1,454 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e358182bcf2bc5fe5c73883eb97878735b12d37
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: 

[Impala-ASF-CR] IMPALA-4822: Implement dynamic log level changes

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4822: Implement dynamic log level changes
..


Patch Set 7:

(29 comments)

http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

Line 37
> where has this gone? where does the definition for Webserver::UrlCallback c
It is now included in logging-support.h


PS7, Line 92:  bind(LogLevelCallBack, _1, _2))
> Everywhere else uses lambdas, please do the same here.
Moved the logic to RegisterLogLevelCallback() as per your suggestion.


http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/backend-gflag-util.h
File be/src/util/backend-gflag-util.h:

Line 29: extern int FLAGS_v_default;
> Placement seems awkward. Maybe we can move thins into logging-support.h/cc 
Done. Moved to logging-support.cc


http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.cc
File be/src/util/logging-support.cc:

PS7, Line 79: /
> only // for .cc files
Done


PS7, Line 78: static jclass log4j_logger_class_;
: /// Jni method descriptors corresponding to getLogLevel() and 
setLogLevel() operations.
: static jmethodID get_log_level_method; // 
GlogAppender.getLogLevel()
: static jmethodID set_log_level_method; // 
GlogAppender.setLogLevel()
: static jmethodID reset_log_levels_method; // 
GlogAppender.resetLogLevels()
> Do these need to be in the impala namespace? If not, move to anonymous name
Done


PS7, Line 187: rapidjson
> remove
Done


PS7, Line 197: get_
> don't need to prefix the parameters with 'get'
This was just to make it more readable. Refactored this code.


PS7, Line 207: return
> why not SetErrorMsg() ?
Done


PS7, Line 207: NULL
> prefer nullptr now
Done


PS7, Line 224: if (result == NULL) return;
> how can result be nullptr if it's a stack-allocated string? This doesn't co
Good point. Doesn't it even compile or doesn't it return 1? Anyway, NULL is 
returned by the underlying java calls GetLogLevel() or SetLogLevel() and is set 
in JniUtil::CallJniMethod()


PS7, Line 225:  result.insert(0, "Effective log level: ");
> This kind of presentation logic probably belongs in the template, not here.
I thought about this, but from what I understand, mustache templates are 
logic-less. Am I missing something? Basically this should appear only if we set 
a particular command output.


Line 227:   } else if (args.find("reset_java_log") != args.end()) {
> It might be easier to have several different callbacks:
Yes I was thinking of doing this as the big method seems to be difficult to 
maintain and understand. Made the changes.


Line 243: StringParser::StringToInt(glog_level->second.c_str(),
> use data() instead of c_str()
Removed this part of code and instead added a validator function.


Line 247:   Status s("Bad glog level input. Valid inputs are integers in 
[0-3] range.");
> Even better: in the range [0-3]
Done


Line 247:   Status s("Bad glog level input. Valid inputs are integers in 
[0-3] range.");
> in the [0-3] range
Done


PS7, Line 256: FLAGS_v
> gflags has a SetCommandLineOption() API. Consider using that here instead?
I looked into it and basically does the same thing, like parsing from a string 
and setting the flag. Made the change now, just to be on the safer side like 
avoid races etc. Also looks like gflag lets us register a validator function 
using RegisterFlagValidator() that is called everytime we reset the flag to a 
newer value. I added one so that we don't accidentally set it to a bad value. 
LKM if you'd like any changes in that.


PS7, Line 257: result = "Glog logging level reset to: " + 
std::to_string(FLAGS_v);
> use Substitute()
Done


PS7, Line 260: Value output(result.c_str(), document->GetAllocator());
 :   document->AddMember(display_member, output, 
document->GetAllocator());
> I think it would be clearer to do this inline, and return rather than carry
Done


http://gerrit.cloudera.org:8080/#/c/5792/7/be/src/util/logging-support.h
File be/src/util/logging-support.h:

PS7, Line 51: /// Helper method to set the log level of given Java class. It is 
a JNI wrapper around
: /// GlogAppender.setLogLevel().
: Status SetJavaLogLevel(const TSetJavaLogLevelParams& params, 
string* response);
: 
: /// Helper method to get the log level of given Java class. It is 
a JNI wrapper around
: /// GlogAppender.getLogLevel().
: Status GetJavaLogLevel(const TGetJavaLogLevelParams& params, 
string* response);
> Why are these exported? There are no other consumers, right?
Sorry I forgot to clean these up along with ResetJavaLogLevel. Removed.


Line 62: void LogLevelCallBack(const Webserver::ArgumentMap& args, 
rapidjson::Document* document);
> How about "RegisterLogLevelCallback(string url, Webserver*)" ?
Good idea. That cleans up the