[Impala-ASF-CR] IMPALA-6220: Revert "IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is slow"

[Xianda Ke (Code Review)]

Xianda Ke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8597 )

Change subject: IMPALA-6220: Revert "IMPALA-6128: Spill-to-disk 
Encryption(AES-CFB + SHA256) is slow"
..


Patch Set 1: Code-Review+1

> Assignee added: Sailesh Mukil 

it is ok to revert since it blocks compiling now.
I'll investigate it to find out a solution for CTR mode.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id31d5fcfec5c6d777d4acee5c1be2d4fc4605efb
Gerrit-Change-Number: 8597
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Xianda Ke 
Gerrit-Comment-Date: Mon, 20 Nov 2017 09:23:27 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is slow

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8510 )

Change subject: IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is slow
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8510/7/be/src/util/openssl-util.cc
File be/src/util/openssl-util.cc:

http://gerrit.cloudera.org:8080/#/c/8510/7/be/src/util/openssl-util.cc@147
PS7, Line 147:   return (SSLeay() >= OPENSSL_VERSION_1_0_1);
When we re-do this patch we should also add a test option to force use of CFB 
mode and make sure that the unit test tests both encryption modes.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib97939f2334838263364b53ef3413871638bf53e
Gerrit-Change-Number: 8510
Gerrit-PatchSet: 7
Gerrit-Owner: Xianda Ke 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Mike Yoder 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xianda Ke 
Gerrit-Comment-Date: Mon, 20 Nov 2017 17:57:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Gabor Kaszab (Code Review)]

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 14:

FYI, both the core and the exhaustive tests passed for this patch.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 14
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Mon, 20 Nov 2017 18:23:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6092: avoid drop/create function interactions in e2e tests

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8593 )

Change subject: IMPALA-6092: avoid drop/create function interactions in e2e 
tests
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8593/1//COMMIT_MSG@10
PS1, Line 10: IMPALA-6215 explains a race between the lib_cache
> Absolutely we should fix 6215, but we should also avoid unspecified test in
IMO, this is not an "inadvertent interaction". I'm pretty sure many users use 
the UDFs this way (have a single source fat jar and reference it across 
multiple functions/queries/DDLs). My feeling is that the way this e-e test case 
is written, it provides test coverage for that use case scenario and I feel we 
shouldn't change that.

I'm not too strong about this, but may be better to check with Dimitris/Alex 
too.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica3538788b1d2ab5e361261e2ade62780b838e65
Gerrit-Change-Number: 8593
Gerrit-PatchSet: 1
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Mon, 20 Nov 2017 18:36:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo

[Csaba Ringhofer (Code Review)]

Hello Lars Volker, Philip Zeyliger, Dan Hecht,

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

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

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

Change subject: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo
..

IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo

Running shell commands from impalad can be problematic, because using popen() 
leads
to forking which causes a spike in virtual memory. To avoid this, "ls" is 
replaced
with POSIX API calls.

FileDescriptorMap fd_desc_ was only used to get the number of file descriptors, 
so
it was unneccesery work to initialize it. It is removed, and only the number of 
file
descriptors is computed.

The automatic test for this function is only a sanity check,  because there is 
no
way to know the "expected value" in advance, and the number of file desciptors 
can
change anytime.

Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0
---
M be/src/util/proc-info-test.cc
M be/src/util/process-state-info.cc
M be/src/util/process-state-info.h
3 files changed, 35 insertions(+), 44 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0
Gerrit-Change-Number: 8546
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-6220: Revert "IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is slow"

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8597 )

Change subject: IMPALA-6220: Revert "IMPALA-6128: Spill-to-disk 
Encryption(AES-CFB + SHA256) is slow"
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id31d5fcfec5c6d777d4acee5c1be2d4fc4605efb
Gerrit-Change-Number: 8597
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xianda Ke 
Gerrit-Comment-Date: Mon, 20 Nov 2017 17:00:50 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6220: Revert "IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is slow"

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8597 )

Change subject: IMPALA-6220: Revert "IMPALA-6128: Spill-to-disk 
Encryption(AES-CFB + SHA256) is slow"
..


Patch Set 1:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id31d5fcfec5c6d777d4acee5c1be2d4fc4605efb
Gerrit-Change-Number: 8597
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xianda Ke 
Gerrit-Comment-Date: Mon, 20 Nov 2017 18:25:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

[Zoltan Borok-Nagy (Code Review)]

Hello Michael Ho, Thomas Tauber-Marshall, Laszlo Gaal, Gabor Kaszab, Attila 
Jeges, Tim Armstrong, Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..

IMPALA-2248: Make idle_session_timeout a query option

This commit makes idle_session_timeout a query option.
idle_session_timeout currently can be set as a command line
option, which will be the default timeout for sessions.
HS2 sessions can override it with a smaller value by setting
it in the configuration overlay of HS2 OpenSession().

However, we can't override idle_session_timeout for JDBC/ODBC
connections, because we cannot put this in the connection string.

This commit is a workaround for this problem, it allows JDBC/ODBC
connections to set the session timeout as a query option
with the SET statement.

I created an automated test case in JdbcTest.java based on
test_hs2.py::test_concurrent_session_mixed_idle_timeout. I also
extended the test_session_expiration and test_set_and_unset
test suites.

Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
---
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/ImpalaService.thrift
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
A fe/src/test/java/org/apache/impala/util/Metrics.java
M tests/custom_cluster/test_session_expiration.py
M tests/custom_cluster/test_set_and_unset.py
13 files changed, 340 insertions(+), 34 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-1144: Fix exception when cancelling query in Impala-shell with CTRL-C

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8549 )

Change subject: IMPALA-1144: Fix exception when cancelling query in 
Impala-shell with CTRL-C
..


Patch Set 5:

(10 comments)

Overall approach seems good to me, but had quite a few comments about comments 
and naming.

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

http://gerrit.cloudera.org:8080/#/c/8549/2//COMMIT_MSG@25
PS2, Line 25: Change-Id: I6cefaf1dae78baae238289816a7cb9d210fb38e2
> Thanks for the nice example, Phil!
It seems ok that it doesn't reproduce it every time - it's inherently racy. We 
could probably have a deterministic unit test by mocking out enough things, but 
I think I prefer the end-to-end shell test.

It would be good to also test on release and ASAN builds to make sure that it 
doesn't immediately fail when the daemon is slow or fast.


http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py
File shell/impala_client.py:

http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@58
PS5, Line 58: QueryCancelledException
Can we call this QueryCancelledByShellException or 
ExpectedQueryCancellationException or something like that to distinguish from 
the case when the query was cancelled unexpectedly (e.g. by an admin from the 
impala debug ui).


http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@60
PS5, Line 60:   self.value = value
The value might not be necessary if we're always going to pass in the same 
string. The existing exceptions do this but seems like an odd pattern. We could 
just print that string instead of stringifying the exception.


http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@83
PS5, Line 83: self.is_query_cancelled = False
Can you add a brief comment explaining how this works. I.e. that it's set to 
true by the cancellation signal handler and suppresses any errors after 
cancellation.


http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_client.py@435
PS5, Line 435: except BeeswaxService.QueryNotFoundException:
It's weird that we don't need the logic for QueryNotFoundException. Does Impala 
even throw this? I'm ok with leaving this in since it's part of the beeswax 
protocol but we should also check for cancellation for consistency.


http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8549/5/shell/impala_shell.py@1026
PS5, Line 1026:   print_to_stderr(str(e))
It might be to just not print anything - we don't always throw this exception 
when the query is cancelled so the message only gets printed sometimes.


http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@333
PS5, Line 333: 'select * from v, v v2, v v3, v v4, v v5, v v6, v v7, v 
v8, v v9;"'
Maybe add a few more tables to the cross join to make sure that it runs for a 
very long time. We don't want a flaky test if it sometimes finishes quicker.


http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@342
PS5, Line 342:'tpch.customer c3 order by c1.c_name"'
You could also do something like:
  order by c1.c_name, sleep(1)

That would make it pause 1ms per row and slow it down further.


http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@346
PS5, Line 346: query_txt
Isn't this the command-line arguments?


http://gerrit.cloudera.org:8080/#/c/8549/5/tests/shell/test_shell_commandline.py@349
PS5, Line 349: sleep(3)
Can you add a comment to the function explaining that it waits 3 seconds before 
cancelling and expects the query to be cancelled.

Is there any reason why 3 seconds is the right amount of time? Currently it 
just seems like a magic number. Would be good to reduce it if possible, since 
it will make the tests very slow (other shell tests have this problem already).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6cefaf1dae78baae238289816a7cb9d210fb38e2
Gerrit-Change-Number: 8549
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-6220: Revert "IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is slow"

[Michael Ho (Code Review)]

Michael Ho has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8597 )

Change subject: IMPALA-6220: Revert "IMPALA-6128: Spill-to-disk 
Encryption(AES-CFB + SHA256) is slow"
..

IMPALA-6220: Revert "IMPALA-6128: Spill-to-disk Encryption(AES-CFB + SHA256) is 
slow"

Compilation failed on some platforms as ‘EVP_aes_256_ctr’ not declared in 
openssl-util.cc.
Reverting the change to unbreak the builds for now.

This reverts commit fb4c3b01240d8f65fc2c45bf27b668ae9b1fa5d2.

Change-Id: Id31d5fcfec5c6d777d4acee5c1be2d4fc4605efb
Reviewed-on: http://gerrit.cloudera.org:8080/8597
Reviewed-by: Xianda Ke 
Reviewed-by: Tim Armstrong 
Tested-by: Michael Ho 
---
M be/src/runtime/tmp-file-mgr.cc
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
3 files changed, 11 insertions(+), 22 deletions(-)

Approvals:
  Xianda Ke: Looks good to me, but someone else must approve
  Tim Armstrong: Looks good to me, approved
  Michael Ho: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id31d5fcfec5c6d777d4acee5c1be2d4fc4605efb
Gerrit-Change-Number: 8597
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xianda Ke 

[Impala-ASF-CR] IMPALA-4591: Bound Kudu client error mem usage

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8464 )

Change subject: IMPALA-4591: Bound Kudu client error mem usage
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8464/2/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

http://gerrit.cloudera.org:8080/#/c/8464/2/be/src/exec/kudu-table-sink.cc@350
PS2, Line 350:   mem_tracker_->Release(client_tracked_bytes_);
> When you say "cleared" do you mean zeroed or freed? Also, if CountPendingEr
Good points. Looking through the Kudu client code, I think null-ing the 
shared_ptr is the right idea.


http://gerrit.cloudera.org:8080/#/c/8464/2/tests/custom_cluster/test_kudu.py
File tests/custom_cluster/test_kudu.py:

http://gerrit.cloudera.org:8080/#/c/8464/2/tests/custom_cluster/test_kudu.py@78
PS2, Line 78:   cursor.execute(
> But what about when the limit is set to 1024? Is there a way to determine t
Good point. I'll add another query to this test that doesn't overflow the limit.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I186ddb3f3b5865e08f17dba57cf6640591d06b14
Gerrit-Change-Number: 8464
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 20 Nov 2017 23:00:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4591: Bound Kudu client error mem usage

[Thomas Tauber-Marshall (Code Review)]

Hello Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-4591: Bound Kudu client error mem usage
..

IMPALA-4591: Bound Kudu client error mem usage

Previously, Kudu client errors could grow in size unbounded,
potentially causing the process to be killed. This patch sets a
bound on the mem that can be used for these error messages, with
the size determined by the flag 'kudu_error_buffer_size'.

If the errors for a Kudu client exceed this size, the query will fail,
as some errors will be dropped and we won't be able to tell if all of
the errors can be safely ignored.

Testing:
- Added a custom cluster test that verifies that a query that exceeds
  the limit fails.

Change-Id: I186ddb3f3b5865e08f17dba57cf6640591d06b14
---
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-table-sink.h
M tests/custom_cluster/test_kudu.py
3 files changed, 45 insertions(+), 26 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I186ddb3f3b5865e08f17dba57cf6640591d06b14
Gerrit-Change-Number: 8464
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5019: Decimal V2 addition

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8309 )

Change subject: IMPALA-5019: Decimal V2 addition
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8309/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8309/4/be/src/exprs/expr-test.cc@a2402
PS4, Line 2402: 
> Still not following. The old V2 expected value was 999.6 not 999.5.
Regarding the first point:
the type of "power(10, 3) - 0.45" in decimal v2 before this patch is 
decimal(38, 17).
the type of "power(10, 3) - 0.45" in decimal v2 after this patch is decimal(38, 
16).

This causes the result of the conversion to double to be slightly different.

Before this patch (in decimal v2) after converting to double it looks like 
this: 999.5500156861889
After this patch, it looks like this: 999.5499727558438

This is why the result was 999.5 and not 999.6 after the patch.

This is all due to IMPALA-6183, which was fixed (and is reflected in the latest 
patch).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I401049c56d910eb1546a178c909c923b01239336
Gerrit-Change-Number: 8309
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Mon, 20 Nov 2017 21:34:17 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6092: avoid drop/create function interactions in e2e tests

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8593 )

Change subject: IMPALA-6092: avoid drop/create function interactions in e2e 
tests
..


Patch Set 2: Code-Review+1

(1 comment)

+1 assuming the new test works on all FSes.

http://gerrit.cloudera.org:8080/#/c/8593/1/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:

http://gerrit.cloudera.org:8080/#/c/8593/1/tests/query_test/test_udfs.py@422
PS1, Line 422: check_call(["hadoop", "fs", "-put", "-f", src_udf_path, 
tgt_udf_path])
> Couple of points. This always copies from a local source and it renames to
What I meant was that "hadoop fs -put/copyFromLocal" from what I understand is 
used to put/copy stuff across file systems and I just wanted to be sure if it 
works even in local FS mode too (we had many flaky tests in local fs mode in 
the past).

To test locally,

export TARGET_FILESYSTEM="local"
export WAREHOUSE_LOCATION_PREFIX=/foo
source bin/impala-config.sh
./run-tests.py

I think you need to have the test-snapshot loaded in the local fs too, so it 
might not run that smoothly, but you can repro the steps manually, outside of 
an e2e.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica3538788b1d2ab5e361261e2ade62780b838e65
Gerrit-Change-Number: 8593
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Mon, 20 Nov 2017 22:22:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](asf-site) Make wording around adopters of Impala more consistent.

[Lars Volker (Code Review)]

Lars Volker has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8609


Change subject: Make wording around adopters of Impala more consistent.
..

Make wording around adopters of Impala more consistent.

Change-Id: I728f86fbf2db3d201c53c8ea6182727006e9c7fc
---
M index.html
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: newchange
Gerrit-Change-Id: I728f86fbf2db3d201c53c8ea6182727006e9c7fc
Gerrit-Change-Number: 8609
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 

[Impala-ASF-CR] IMPALA-6225: Query profile date-time strings should have ns precision.

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8610


Change subject: IMPALA-6225: Query profile date-time strings should have ns 
precision.
..

IMPALA-6225: Query profile date-time strings should have ns precision.

IPALA-5599 changed the precision of query start and end time date-time
string representations to microseconds. This ended up breaking
compatibility with some API clients.

This patch restores the precision to nanosecond, even though the
timestamps themselves have only microsecond precision. Effectively,
what we end up doing is to zero-pad the fractional second part to
nine decimal places.

Change-Id: I95bab8de19d437956f42e13b754ab4dbb52284ca
---
M be/src/service/client-request-state.cc
1 file changed, 4 insertions(+), 2 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I95bab8de19d437956f42e13b754ab4dbb52284ca
Gerrit-Change-Number: 8610
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Zach Amsden,

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

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

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

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..

IMPALA-4927: Impala should handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 31 insertions(+), 24 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8414 )

Change subject: IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation
..


Patch Set 9:

(19 comments)

http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/disk-io-mgr.h
File be/src/runtime/io/disk-io-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/disk-io-mgr.h@a222
PS11, Line 222:
  :
  :
> did you mean to just delete that?
Done


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/disk-io-mgr.h@457
PS11, Line 457: XCEED
> queued?
Done


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/disk-io-mgr.cc@333
PS11, Line 333: CANCELLED
> seems like the right thing to do but are you sure we weren't depending on p
Before this patch the only place when RequestContext::Cancel() was called with 
a non-CANCELLED status was with MEM_LIMIT_EXCEEDED. This patch removes that 
call and the need for any asynchronous error propagation via RequestContext, so 
can't introduce any new bugs in this area.

Error propagation bugs are still possible in client code if they cancel the 
context before propagating the original error, but I believe the HDFS scan node 
code is written to avoid this.


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/disk-io-mgr.cc@581
PS11, Line 581:   DCHECK(!buffer->is_cached()) << "HDFS cache reads don't go 
through this code path.";
> maybe put that dcheck upstream too (like ReadRange() or even earlier) to he
There is already a DCHECK in ReadRange() that catches this, but its 
non-obvious. I added a string to it to make it clearer what it's checking.


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/disk-io-mgr.cc@665
PS11, Line 665: DCHECK
> nit: _EQ
The strong typed enums prevent that unfortunately. I did change this to print 
out the integer value on failure.


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/request-context.h
File be/src/runtime/io/request-context.h:

http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/request-context.h@448
PS11, Line 448: again.
> disk threads?
Done


http://gerrit.cloudera.org:8080/#/c/8414/9/be/src/runtime/io/request-context.h
File be/src/runtime/io/request-context.h:

http://gerrit.cloudera.org:8080/#/c/8414/9/be/src/runtime/io/request-context.h@27
PS9, Line 27: For most I/O manager clients it is an
: /// opaque pointer, but some clients may need to include this 
heade
> is that still true?
Done


http://gerrit.cloudera.org:8080/#/c/8414/9/be/src/runtime/io/request-context.h@93
PS9, Line 93:   void Cancel();
> how did you decide to make this a first class thing in the RequestContext,
Yeah, I'd probably call it DiskIoMgrClient or something like that if I was 
writing this from scratch.

The ones I moved were the trivial methods that were only called during 
RequestContext set-up and tear-down. The other DiskIoMgr methods are called 
from more places and are more about the I/O operations than setting up the 
context.

It isn't really principled now that I think about it.


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/request-context.cc
File be/src/runtime/io/request-context.cc:

http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/request-context.cc@42
PS11, Line 42: << static_cast(range->external_buffer_tag_);
> oh, I guess DCHECK_EQ doesn't work with enum or something?
Yeah, not with the strongly-typed "enum class" enums. It works with weakly 
typed enums because they get implicitly converted to integers.


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/request-context.cc@75
PS11, Line 75: reader
> what about the writing case? is that relevant?
Yep, this needed updating.


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/request-context.cc@76
PS11, Line 76: GetNext
> GetNext()
Done


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/request-context.cc@79
PS11, Line 79: it
> does this referring to the context or something else?
Done


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/request-context.cc@90
PS11, Line 90: cancelled Status
> does that mean the CANCELLED status or the status that lead to cancellation
The RequestContext can't be cancelled with an arbitrary status. If there was an 
earlier error for a ScanRange, the cancelled status doesn't overwrite the prior 
status.


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/request-context.cc@91
PS11, Line 91:  the cancelled state.
> what does that mean?
reworded.


http://gerrit.cloudera.org:8080/#/c/8414/11/be/src/runtime/io/request-context.cc@95
PS11, Line 95: decrements the number of disk queues the context
 : // is on.
> which code is this talking about? Oh, I guess that's 

[Impala-ASF-CR] IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation

[Tim Armstrong (Code Review)]

Hello Tianyi Wang, Dan Hecht,

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

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

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

Change subject: IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation
..

IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation

In preparation for switching the I/O mgr to the buffer pool, this
removes and cleans up a lot of code so that the switchover patch starts
from a cleaner slate.

* Remove the free buffer cache (which will be replaced by buffer pool's
  own caching).
* Make memory limit exceeded error checking synchronous (in anticipation
  of having to propagate buffer pool errors synchronously).
* Simplify error propagation - remove the (ineffectual) code that
  enqueued BufferDescriptors containing error statuses.
* Document locking scheme better in a few places, make it part of the
  function signature when it seemed reasonable.
* Move ReturnBuffer() to ScanRange, because it is intrinsically
  connected with the lifecycle of a scan range.
* Separate external ReturnBuffer() and internal CleanUpBuffer()
  interfaces - previously callers of ReturnBuffer() were fudging
  the num_buffers_in_reader accounting to make the external interface work.
* Eliminate redundant state in ScanRange: 'eosr_returned_' and
  'is_cancelled_'.
* Clarify the logic around calling Close() for the last
  BufferDescriptor.
  -> There appeared to be an implicit assumption that buffers would be
 freed in the order they were returned from the scan range, so that
 the "eos" buffer was returned last. Instead just count the number
 of outstanding buffers to detect the last one.
  -> Touching the is_cancelled_ field without holding a lock was hard to
 reason about - violated locking rules and it was unclear that it
 was race-free.
* Remove DiskIoMgr::Read() to simplify interface. It is trivial to
  inline at the callsites.

This will probably regress performance somewhat because of the cache
removal, so my plan is to merge it around the same time as switching
the I/O mgr to allocate from the buffer pool. I'm keeping the patches
separate to make reviewing easier.

Testing:
* Ran exhaustive tests
* Ran the disk-io-mgr-stress-test overnight

Change-Id: If5cb42437d11c13bc4a55c3ab426b66777332bd1
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/scanner-context.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/test-env.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
19 files changed, 575 insertions(+), 904 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If5cb42437d11c13bc4a55c3ab426b66777332bd1
Gerrit-Change-Number: 8414
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5019: Decimal V2 addition

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8309 )

Change subject: IMPALA-5019: Decimal V2 addition
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8309/4/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/8309/4/be/src/exprs/expr-test.cc@a2402
PS4, Line 2402:
> Regarding the first point:
I'm not sure what the intent of the original test case is and why it was 
written this way.  Zach added this test case as part of the rounding for cast 
to DECIMAL, so it seems we may be losing some coverage he intended to get.  
Could you check with Zach to see what he thinks and if we should rewrite this 
(and other) test cases?

If we leave this test case here, I think a comment explaining why 999.5 is the 
"correct" answer would be helpful, given that CAST(999.55 as DECIMAL(4.1)) 
should normally result in 999.6



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I401049c56d910eb1546a178c909c923b01239336
Gerrit-Change-Number: 8309
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Mon, 20 Nov 2017 22:01:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4927: Impala should be able to handle invalid input from Sentry

[Pranay Singh (Code Review)]

Hello Bharath Vissapragada,

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

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

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

Change subject: IMPALA-4927: Impala should be able to handle invalid input from 
Sentry
..

IMPALA-4927: Impala should be able to handle invalid input from Sentry

Impala requests a list of roles from Sentry and then asks for privileges
for each role. If Sentry returns a non existent role in the first step,
then there will be a Java exception in Impala in the second step and
the communication with Sentry is aborted.

The issue is fixed by handling the exception if an invalid role is
found and continue with getting permissions for the rest of the roles.

Testing:
---
Since invalid role could not be created through impala-shell/Hue
interface the code was instrumented to have a invalid Role " " and
see how the condition is handled.

Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
---
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
1 file changed, 31 insertions(+), 24 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-6225: Query profile date-time strings should have ns precision.

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8610 )

Change subject: IMPALA-6225: Query profile date-time strings should have ns 
precision.
..


Patch Set 1:

Thanks!

I think tests/query_test/test_observability.py may be the cheapest place to add 
a test for this. I think it'd be useful to add a quick test, to help us think 
twice about changing this in the future.

Code itself looks straight-forward to me.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95bab8de19d437956f42e13b754ab4dbb52284ca
Gerrit-Change-Number: 8610
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Mon, 20 Nov 2017 23:57:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6092: avoid drop/create function interactions in e2e tests

[Vuk Ercegovac (Code Review)]

Vuk Ercegovac has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8593 )

Change subject: IMPALA-6092: avoid drop/create function interactions in e2e 
tests
..


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8593/1//COMMIT_MSG@13
PS1, Line 13: test_udf_invalid_symbol drops a function from that jar, which 
causes the use
> nit: line overflow in multiple places.
Done


http://gerrit.cloudera.org:8080/#/c/8593/1/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:

http://gerrit.cloudera.org:8080/#/c/8593/1/tests/query_test/test_udfs.py@422
PS1, Line 422: check_call(["hadoop", "fs", "-put", "-f", src_udf_path, 
tgt_udf_path])
> Just to be sure, this works ok on local fs right? Basically we "put" from l
Couple of points. This always copies from a local source and it renames to a 
unique tgt name in local fs. Its also the way that the create/drop tests are 
also setup (they're careful not to use the same shared jar across tests).

I assume the other tests work, but just in case-- how do I run just the local 
fs version of the test to verify this?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica3538788b1d2ab5e361261e2ade62780b838e65
Gerrit-Change-Number: 8593
Gerrit-PatchSet: 1
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Mon, 20 Nov 2017 20:22:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo

[Csaba Ringhofer (Code Review)]

Csaba Ringhofer has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8546 )

Change subject: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo
..


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8546/5/be/src/util/process-state-info.h
File be/src/util/process-state-info.h:

http://gerrit.cloudera.org:8080/#/c/8546/5/be/src/util/process-state-info.h@61
PS5, Line 61: /// Below are some of the Process status information that will be 
read
> nit: long line. Please have a look at how to configure your text editor to
Done


http://gerrit.cloudera.org:8080/#/c/8546/5/be/src/util/process-state-info.cc
File be/src/util/process-state-info.cc:

http://gerrit.cloudera.org:8080/#/c/8546/5/be/src/util/process-state-info.cc@36
PS5, Line 36: using boost::algorithm::is_any_of;
> nit: sort alphabetically?
Done


http://gerrit.cloudera.org:8080/#/c/8546/5/be/src/util/process-state-info.cc@157
PS5, Line 157: // readdir() is not thread-safe according to its man page, 
but in glibc
> Can you include a reference to the source (e.g. https://www.gnu.org/softwar
Done


http://gerrit.cloudera.org:8080/#/c/8546/6/be/src/util/process-state-info.cc
File be/src/util/process-state-info.cc:

http://gerrit.cloudera.org:8080/#/c/8546/6/be/src/util/process-state-info.cc@161
PS6, Line 161:   if(dir_entry->d_name[0] != '.') ++fd_count; // . and .. do 
not count
www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html 
states:
"Portability Note: On some systems readdir may not return entries for . and ..,"
Because of this, the filename must be checked to return the exact number of 
descriptors. As every filename in fd directory is a number, it is enough to 
check the first character.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0
Gerrit-Change-Number: 8546
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Mon, 20 Nov 2017 18:51:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6217: fix DCHECK in Parquet fuzz test

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8594 )

Change subject: IMPALA-6217: fix DCHECK in Parquet fuzz test
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6ceca7de31f602b75d744dacbdf37afa75983344
Gerrit-Change-Number: 8594
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Mon, 20 Nov 2017 18:55:49 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6210: Add query id to lineage graph logging

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8589 )

Change subject: IMPALA-6210: Add query id to lineage graph logging
..

IMPALA-6210: Add query id to lineage graph logging

Some tools use lineage graph logging to collect query metrics. Currently
only query hash is present in this log. Adding query id into it makes
such accounting easier.

Testing: The equality of query id in the query profile and lineage log
is checked in test_lineage.py. A test for TUniqueIdUtil is added to the
FE tests.

Change-Id: I4adbd02df37a234dbb79f58b7c46ca11a914229f
---
M be/src/util/lineage-util.h
M common/thrift/LineageGraph.thrift
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
A fe/src/main/java/org/apache/impala/util/TUniqueIdUtil.java
A fe/src/test/java/org/apache/impala/util/TUniqueIdUtilTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/lineage.test
M tests/custom_cluster/test_lineage.py
7 files changed, 185 insertions(+), 7 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4adbd02df37a234dbb79f58b7c46ca11a914229f
Gerrit-Change-Number: 8589
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-6092: avoid drop/create function interactions in e2e tests

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8593 )

Change subject: IMPALA-6092: avoid drop/create function interactions in e2e 
tests
..


Patch Set 1:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8593/1//COMMIT_MSG@10
PS1, Line 10: IMPALA-6215 explains a race between the lib_cache
> The test that's changed was testing for an invalid symbol that was causing
Fair point, I agree that we caught the bug by fluke when the test case was 
doing something totally irrelevant. Like we discussed, I'm ok as long as we fix 
the test coverage with proper tests that rely on a single source jar and 
multiple DDLs (either in this change or with IMPALA-6215)


http://gerrit.cloudera.org:8080/#/c/8593/1//COMMIT_MSG@13
PS1, Line 13: test_udf_invalid_symbol drops a function from that jar, which 
causes the use
nit: line overflow in multiple places.


http://gerrit.cloudera.org:8080/#/c/8593/1/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:

http://gerrit.cloudera.org:8080/#/c/8593/1/tests/query_test/test_udfs.py@422
PS1, Line 422: check_call(["hadoop", "fs", "-put", "-f", src_udf_path, 
tgt_udf_path])
Just to be sure, this works ok on local fs right? Basically we "put" from local 
fs to local fs.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica3538788b1d2ab5e361261e2ade62780b838e65
Gerrit-Change-Number: 8593
Gerrit-PatchSet: 1
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Mon, 20 Nov 2017 19:40:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4591: Bound Kudu client error mem usage

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8464 )

Change subject: IMPALA-4591: Bound Kudu client error mem usage
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I186ddb3f3b5865e08f17dba57cf6640591d06b14
Gerrit-Change-Number: 8464
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 21 Nov 2017 01:07:30 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2181: Add query option levels for display

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8447 )

Change subject: IMPALA-2181: Add query option levels for display
..


Patch Set 15:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG@10
PS14, Line 10: diplayed
displayed


http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG@12
PS14, Line 12: called SET ALL shows all the options grouped by their option 
levels.
Mention that there are also server-side versions of these commands?


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

http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/impala-server.cc@1236
PS14, Line 1236:   else {
Nit:
  } else {


http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h
File be/src/service/query-options.h:

http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@45
PS15, Line 45:   QUERY_OPT_FN(disable_cached_reads, DISABLE_CACHED_READS, 
TQueryOptionLevel::REGULAR)\
Looks like disable_cached_reads is deprecated: see IMPALA-2963 . I missed that 
on an earlier pass


http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@89
PS15, Line 89:   QUERY_OPT_FN(enable_expr_rewrites, ENABLE_EXPR_REWRITES, 
TQueryOptionLevel::REGULAR)\
I think enable_expr_rewrites should be advanced. It's only disabled as a 
workaround to planner bugs.


http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@91
PS15, Line 91:   QUERY_OPT_FN(parquet_dictionary_filtering, 
PARQUET_DICTIONARY_FILTERING, TQueryOptionLevel::REGULAR)\
parquet_dictionary_filtering should probably be advanced, there's typically not 
much reason to disable it unless working around a bug.


http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@93
PS15, Line 93:   QUERY_OPT_FN(parquet_read_statistics, PARQUET_READ_STATISTICS, 
TQueryOptionLevel::REGULAR)\
Same for parquet_read_statistics.


http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.h
File be/src/service/query-options.h:

http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.h@39
PS14, Line 39:   QUERY_OPT_FN(abort_on_default_limit_exceeded, 
ABORT_ON_DEFAULT_LIMIT_EXCEEDED, TQueryOptionLevel::DEPRECATED)\
nit: can we wrap these to 90 chars?


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

http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.cc@41
PS14, Line 41: using namespace beeswax;
nit: probably best to individually import the names so it's easier to figure 
out what got imported.


http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:

http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/ImpalaService.thrift@294
PS14, Line 294: typedef map 
TQueryOptionLevels
Does this need to be a typedef here? I don't see any references in any thrift 
files. Maybe we can just use the map type directly where its needed in the 
backend


http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/beeswax.thrift
File common/thrift/beeswax.thrift:

http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/beeswax.thrift@111
PS14, Line 111:   4: optional TQueryOptionLevel level
Comment that this was added for impala-shell?


http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java
File fe/src/main/java/org/apache/impala/analysis/SetStmt.java:

http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java@55
PS14, Line 55:   if (isSetAll_) {
Nit: could be one line
 if (isSetAll_) return ...;


http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java@76
PS14, Line 76:   request.setIs_set_all(true);
nit: could be one line


http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_client.py
File shell/impala_client.py:

http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_client.py@97
PS14, Line 97:   self.query_option_levels[option.key.upper()] = option.level
Does this still work if we're talking to an older Impala server that doesn't 
return the level?

Update: looks like it doesn't:
  Traceback (most recent call last):
  File "/home/tarmstrong/Impala/incubator-impala/shell/impala_shell.py", line 15
27, in 
shell = ImpalaShell(options, query_options)
  File "/home/tarmstrong/Impala/incubator-impala/shell/impala_shell.py", line 20
5, in __init__
self.do_connect(options.impalad)
  File "/home/tarmstrong/Impala/incubator-impala/shell/impala_shell.py", line 73
1, in do_connect
self.imp_client.build_default_query_options_dict()
  File 

[Impala-ASF-CR] IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8546 )

Change subject: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo
..


Patch Set 7: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0
Gerrit-Change-Number: 8546
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 21 Nov 2017 01:30:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4506: Do not display "tip of the day" if --quiet is set

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8613


Change subject: IMPALA-4506: Do not display "tip of the day" if --quiet is set
..

IMPALA-4506: Do not display "tip of the day" if --quiet is set

Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
---
M shell/impala_shell.py
1 file changed, 10 insertions(+), 7 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-6225: Query profile date-time strings should have ns precision.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8610 )

Change subject: IMPALA-6225: Query profile date-time strings should have ns 
precision.
..


Patch Set 1:

Please see https://gerrit.cloudera.org/#/c/8611/ instead.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95bab8de19d437956f42e13b754ab4dbb52284ca
Gerrit-Change-Number: 8610
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 21 Nov 2017 00:23:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4985: use parquet stats of nested types for dynamic pruning

[Vuk Ercegovac (Code Review)]

Hello Lars Volker, Alex Behm,

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

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

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

Change subject: IMPALA-4985: use parquet stats of nested types for dynamic 
pruning
..

IMPALA-4985: use parquet stats of nested types for dynamic pruning

Currently, parquet row-groups can be pruned at run-time using
min/max stats when predicates (in, binary) are specified for
column scalar types. This patch extends pruning to nested types
for the same class of predicates. A nested value is an instance
of a nested type (struct, array, map). A nested value consists of
other nested and scalar values (as declared by its type).
Predicates that can be used for row-group pruning must be applied to
nested scalar values. In addition, the parent of the nested scalar
must also be required, that is, not empty. The latter requirement
is conservative: some filters that could be used for pruning are
not used for correctness reasons.

Testing:
- extended nested-types-parquet-stats e2e test cases.

Change-Id: I0c99e20cb080b504442cd5376ea3e046016158fe
---
M be/src/exec/hdfs-parquet-scanner.h
M fe/src/main/java/org/apache/impala/analysis/CollectionStructType.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test
M 
testdata/workloads/functional-query/queries/QueryTest/nested-types-parquet-stats.test
M tests/query_test/test_nested_types.py
7 files changed, 508 insertions(+), 35 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0c99e20cb080b504442cd5376ea3e046016158fe
Gerrit-Change-Number: 8480
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-6225: Query profile date-time strings should have ns precision.

[Michael Ho (Code Review)]

Michael Ho has abandoned this change. ( http://gerrit.cloudera.org:8080/8610 )

Change subject: IMPALA-6225: Query profile date-time strings should have ns 
precision.
..


Abandoned

Dup with https://gerrit.cloudera.org/#/c/8611/
--
To view, visit http://gerrit.cloudera.org:8080/8610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I95bab8de19d437956f42e13b754ab4dbb52284ca
Gerrit-Change-Number: 8610
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-4132: Use -fno-omit-frame-pointer

[Gabor Kaszab (Code Review)]

Gabor Kaszab has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8612


Change subject: IMPALA-4132: Use -fno-omit-frame-pointer
..

IMPALA-4132: Use -fno-omit-frame-pointer

Using -fno-omit-frame-pointer would keep the frame pointers for
functions in the register. As a result we expect more useful stack
traces to be produced.

For testing performance benchmark was executed on TPC-H and TPC-DS
without any significant discrepancy from the baseline results.
(For the specific measures check the attachments in the Jira.)

Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
---
M be/CMakeLists.txt
1 file changed, 3 insertions(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib7d0d88ba015a847356ed0274fe91017b98cb941
Gerrit-Change-Number: 8612
Gerrit-PatchSet: 1
Gerrit-Owner: Gabor Kaszab 

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 9:

(20 comments)

http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/expr-test.cc@4653
PS9, Line 4653:   TestValue("width_bucket(22, 5, 20.1, 4)", TYPE_BIGINT, 5);
Add some tests with nulls.

For example:
(NULL, 5, 20, 3)
(12, NULL, 20, 3)
(12, 5, NULL, 3)
(12, 5, 20, NULL)


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@455
PS9, Line 455: min_range = -1 (or any negative value)
You can just write:
min_range < 0


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@460
PS9, Line 460: Incase
"In case"


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@460
PS9, Line 460:  t
extra space here


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@460
PS9, Line 460: functions
function*


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@461
PS9, Line 461: decimal8Value
I am not sure it makes sense to even mention decimal8Value here. Why would you 
even think about storing it in a decimal 8?


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@461
PS9, Line 461: stroring
spelling


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@480
PS9, Line 480:   if (min_range == max_range) {
What if min_range > max_range?

Also, add a test case.


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@481
PS9, Line 481: Lower bound cannot be equal to upper bound
Would it make sense to include the name of the function in the error message? 
Do we normally do that?


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@489
PS9, Line 489: num_buckets.val;
how about: num_buckets.val + 1

Also, add a test case where num_buckets is a maximally large integer, so adding 
one causes an overflow.


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@494
PS9, Line 494:   // FE casts all the arguments to the same type.
Maybe expand this comment a little? E.g

"expr", "min_range" and "max_range" are guaranteed to be the same scale and 
precision by the FE


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@503
PS9, Line 503:   if(overflow) {
Have we considered what happens when subtraction is successful (and overflow is 
false), but the result is larger than the largest allowed decimal (10^38 - 1). 
Add a test case like this.

For example:
min range = -100
max_range = 10^38 - 1

The result of that subtraction should not overflow, because it fits into 
int128_t


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@514
PS9, Line 514:   if(overflow) {
Can this overflow ever happen without overflowing on line 503? Add a test case 
for this overflow and error message. (You might need to add some End to end 
tests to test for the error message)


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@533
PS9, Line 533:   if(needs_int256) {
Nit: Put a space between "if" and the opening brace. (here and elsewhere)


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@539
PS9, Line 539: avoid
avoid written twice


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@549
PS9, Line 549: // bump up the denominator scale so that the scale of the 
numerator and denominator
Can you explain this a little better? It's not really clear why we have to 
scale it up.


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@551
PS9, Line 551: int128_t y = 
DecimalUtil::MultiplyByScale(range_size.value(),
Is it possible for this to overflow?


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@557
PS9, Line 557: ctx->SetError("Overflow while dividing by the range_size");
This should be moved up to around line 543.

Add a test case for this error message.


http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@571
PS9, Line 571:   if (UNLIKELY(num_buckets.val <= 0)) {
Why add this check here, instead of around line 480 where other similar check 
are located?


http://gerrit.cloudera.org:8080/#/c/6023/9/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

http://gerrit.cloudera.org:8080/#/c/6023/9/fe/src/main/java/org/apache/impala/analysis/Expr.java@459
PS9, Line 459:
Why this empty line?



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

[Impala-ASF-CR] IMPALA-6054: Parquet dictionary pages should be freed on dictionary construction

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8436 )

Change subject: IMPALA-6054: Parquet dictionary pages should be freed on 
dictionary construction
..


Patch Set 9:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d9d5f4da1028d961155dafdac0028a1c3641004
Gerrit-Change-Number: 8436
Gerrit-PatchSet: 9
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 21 Nov 2017 01:12:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/6023/9/be/src/exprs/math-functions-ir.cc@503
PS9, Line 503:   if(overflow) {
> Have we considered what happens when subtraction is successful (and overflo
Actually I'm not correct here. It does overflow in the case I provided.

Wouldn't hurt to add the test case anyways.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-Change-Number: 6023
Gerrit-PatchSet: 9
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 21 Nov 2017 01:43:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6054: Parquet dictionary pages should be freed on dictionary construction

[Csaba Ringhofer (Code Review)]

Csaba Ringhofer has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8436 )

Change subject: IMPALA-6054: Parquet dictionary pages should be freed on 
dictionary construction
..


Patch Set 8:

> Uploaded patch set 7.
Oops, there was a rebase... to see the changes that fix CHAR type handling, see 
diff between patch 6 and 8.

Tim, can you rerun the tests for me?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d9d5f4da1028d961155dafdac0028a1c3641004
Gerrit-Change-Number: 8436
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 21 Nov 2017 00:04:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6225: Query profile date-time strings should have ns precision.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8610 )

Change subject: IMPALA-6225: Query profile date-time strings should have ns 
precision.
..


Patch Set 1:

As discussed offline, it may be safe to undo the changes in time precision in 
some other locations (e.g. ImpalaHttpHandler::QueryStateToJson) to be 
consistent with what the profile is showing.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95bab8de19d437956f42e13b754ab4dbb52284ca
Gerrit-Change-Number: 8610
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 21 Nov 2017 00:00:22 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4927: Impala should handle invalid input from Sentry

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8588 )

Change subject: IMPALA-4927: Impala should handle invalid input from Sentry
..


Patch Set 4:

(3 comments)

What version of Sentry was causing this issue?  Also, what was the exact 
exception being throw?

http://gerrit.cloudera.org:8080/#/c/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:

http://gerrit.cloudera.org:8080/#/c/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@141
PS4, Line 141:   // need to be removed.
It's fine to leave this as is, the 90 column limit is not exceeded.


http://gerrit.cloudera.org:8080/#/c/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@146
PS4, Line 146:   
sentryPolicyService_.listRolePrivileges(processUser_, role.getName())) {
I think it would be better to call listRolePrivileges directly and catch 
exceptions from that instead of at the end of the function.  If the role 
doesn't exist anymore, we definitely want to remove all its privileges from the 
catalog instead of going on to exception handling.


http://gerrit.cloudera.org:8080/#/c/8588/4/fe/src/main/java/org/apache/impala/util/SentryProxy.java@168
PS4, Line 168: } catch (Exception e) {
Can you catch a more specific exception?  For example, we probably wouldn't 
want to catch a NullPointerException here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I781411018d580854d80a9cad81a1ded7ca16af8b
Gerrit-Change-Number: 8588
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 21 Nov 2017 00:57:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6225: Query profile date-time strings should have ns precision.

[Zoram Thanga (Code Review)]

Zoram Thanga has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8611 )

Change subject: IMPALA-6225: Query profile date-time strings should have ns 
precision.
..

IMPALA-6225: Query profile date-time strings should have ns precision.

IMPALA-5599 changed the precision of query start and end time date-time
string representations to microseconds. This ended up breaking
compatibility with some API clients.

This patch restores the precision to nanosecond, even though the
timestamps themselves have only microsecond precision. Effectively,
what we end up doing is to zero-pad the fractional second part to
nine decimal places.

I have manually checked from the Impala debug web page that the start
and end times of queries have nanosecond precision:

Start Time: 2017-11-20 14:59:01.954031000
End Time: 2017-11-20 15:00:02.103735000

This is basically the same as how it was before. The following is taken
from a cluster running Impala 2.11:

Start Time: 2017-11-20 14:17:52.19827
End Time: 2017-11-20 14:18:52.242868000

Change-Id: I2e124b9c7e0717b8dc2cdab46aea41d74c5f2fd0
---
M be/src/service/client-request-state.cc
M be/src/service/impala-http-handler.cc
M be/src/util/time.h
3 files changed, 10 insertions(+), 8 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2e124b9c7e0717b8dc2cdab46aea41d74c5f2fd0
Gerrit-Change-Number: 8611
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6225: Query profile date-time strings should have ns precision.

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8610 )

Change subject: IMPALA-6225: Query profile date-time strings should have ns 
precision.
..


Patch Set 1:

Looking into the test code, thanks Phil for the suggestion. Uploading the 
second patch for the product code change.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95bab8de19d437956f42e13b754ab4dbb52284ca
Gerrit-Change-Number: 8610
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 21 Nov 2017 00:19:17 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6225: Query profile date-time strings should have ns precision.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8611 )

Change subject: IMPALA-6225: Query profile date-time strings should have ns 
precision.
..


Patch Set 2:

As mentioned before, an alternate approach to go back to the previous behavior 
is to change the To*StringFromUnix* functions to use nano-second precision by 
default. That will make it consistent across the entire code base unless 
otherwise specified by the callers.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2e124b9c7e0717b8dc2cdab46aea41d74c5f2fd0
Gerrit-Change-Number: 8611
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 21 Nov 2017 00:45:57 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8546 )

Change subject: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo
..


Patch Set 7:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0
Gerrit-Change-Number: 8546
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 21 Nov 2017 01:56:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8546 )

Change subject: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo
..


Patch Set 7:

Flaky test, will restart the Jenkins job.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0
Gerrit-Change-Number: 8546
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 21 Nov 2017 01:56:31 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2250: Make multiple COUNT(DISTINCT) message state workarounds

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8614


Change subject: IMPALA-2250: Make multiple COUNT(DISTINCT) message state 
workarounds
..

IMPALA-2250: Make multiple COUNT(DISTINCT) message state workarounds

Change-Id: I5084be10946d68f3ec0760c2b7e698635df26a89
---
M fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java
1 file changed, 4 insertions(+), 0 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5084be10946d68f3ec0760c2b7e698635df26a89
Gerrit-Change-Number: 8614
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8546 )

Change subject: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo
..


Patch Set 7: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0
Gerrit-Change-Number: 8546
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 21 Nov 2017 05:28:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

[Kim Jin Chul (Code Review)]

Hello Philip Zeyliger,

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

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

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

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
..

IMPALA-4506: Do not display some intro message if --quiet is set

PS#1: Do not display "tip of day" if --quiet is set
PS#2: Do not display some intro message if -- quiet is set

Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
---
M shell/impala_shell.py
1 file changed, 20 insertions(+), 16 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py@1481
PS1, Line 1481:   intro = get_welcome_string(options.verbose)
> Others may have a different opinion, but I don't think we should show the '
Your idea has been introduced by PS#2. Let's wait for other's opinions.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 21 Nov 2017 05:52:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display "tip of the day" if --quiet is set

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8613 )

Change subject: IMPALA-4506: Do not display "tip of the day" if --quiet is set
..


Patch Set 1:

(1 comment)

Thanks for the contribution!

I think the code looks fine, but we can go even farther and remove the 
'intro'/'welcome' entirely if --quiet is set, or at least that would be my 
preference.

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py@1481
PS1, Line 1481:   intro = get_welcome_string(options.verbose)
Others may have a different opinion, but I don't think we should show the 
'intro' or 'welcome string' at all if --quiet is set.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 21 Nov 2017 04:56:17 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8414 )

Change subject: IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation
..


Patch Set 12: Code-Review+2

(1 comment)

Please see if Tianyi has any more comments too.

http://gerrit.cloudera.org:8080/#/c/8414/9/be/src/runtime/io/request-context.h
File be/src/runtime/io/request-context.h:

http://gerrit.cloudera.org:8080/#/c/8414/9/be/src/runtime/io/request-context.h@93
PS9, Line 93:   void Cancel();
> Let me take a look through the current patchset first.
Even the other methods seem to operate entirely (or almost entirely?) on the 
'reader' state, rather than the diskiomgr 'this' itself. Even the lock is the 
reader lock. So it seems they are more operations on the context.

I think the current change is okay as is, but i think we could consider moving 
even more things into here in a purely mechanical change in the future.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If5cb42437d11c13bc4a55c3ab426b66777332bd1
Gerrit-Change-Number: 8414
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 21 Nov 2017 04:27:40 +
Gerrit-HasComments: Yes