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

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger, Dan Hecht, Impala Public Jenkins,

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

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

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

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

IMPALA-6225: Part 1: 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

Test cases that inspect timestamps of Impala query profiles from the
debug web page will be introduced in a follow-on commit.

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, 14 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8611/8
--
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: 8
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

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


Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..

IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of
characters.

This patch generalizes ltrim()/rtrim() functions to accept a second
argument that specifies the set of characters to be removed from the
leading/trailing end of the target string:

ltrim(string text[, characters text])
rtrim(string text[, characters text])

A common string trimming method has been added to StringFunctions,
which is called from the general ltrim/rtrim/btrim functions. The
functions also share prepare and close operations.

New StringFunctions tests have been added to ExprTest for the new
forms of ltrim() and rtrim(). New tests to cover handling of special
characters have also been added.

Note that our string handling functions only work with the ASCII
character set. Handling of other character sets lies outside the
scope of this patch.

The existing ltrim()/rtrim()/trim() functions that take only one
argument have been updated to use the more general methods.

Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 166 insertions(+), 77 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

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

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger,

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

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

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

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

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

This commit follows 16d8dd58.

This patch adds a test case that inspects the thrift profile of a
completed query, and verifies that the "Start Time" and
"End Time" of the query have nanosecond precision. We chose to
work with the thrift profile directly, rather than parse the debug
web page, as it is the thrift profile which is consumed by
management API clients of Impala.

Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
---
M tests/common/impala_service.py
M tests/query_test/test_observability.py
2 files changed, 77 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784 )

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


Patch Set 6:

> (1 comment)
 >
 > Thanks for the comments. Uploading #7.

Sorry ... that's #6


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 15 Dec 2017 23:13:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..


Patch Set 5:

Thanks for the comments. Please have a look at patch set #6


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 14 Dec 2017 21:31:20 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Alex Behm,

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

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

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..

IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of
characters.

This patch generalizes ltrim()/rtrim() functions to accept a second
argument that specifies the set of characters to be removed from the
leading/trailing end of the target string:

ltrim(string text[, characters text])
rtrim(string text[, characters text])

A common string trimming method has been added to StringFunctions,
which is called from the general ltrim/rtrim/btrim functions. The
functions also share prepare and close operations.

New StringFunctions tests have been added to ExprTest for the new
forms of ltrim() and rtrim(). New tests to cover handling of special
characters have also been added.

Note that our string handling functions only work with the ASCII
character set. Handling of other character sets lies outside the
scope of this patch.

The existing ltrim()/rtrim()/trim() functions that take only one
argument have been updated to use the more general methods.

Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 166 insertions(+), 81 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..


Patch Set 4:

(6 comments)

Thanks for the comments. Uploading patch set #5.

http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@406
PS4, Line 406: StringFunctions::Ltrim
> nit: Can you please group these 3 functions close to *TrimString variant so
Done


http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@411
PS4, Line 411: static_cast(" ")
> This seems unnecessary now. Can we just pass StringVal::null() in this case
Can do this, but please see response to comment on line 475. Retaining this for 
now, albeit in a simpler form.


http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@418
PS4, Line 418: new bitset<256>()
> context->Allocate<>() ?
Allocate<>() returns an uninitialized buffer. Can't use this. Else I'l have to 
explicitly call bitset.reset() on the returned unique_chars. Seems to me it's a 
lot simpler to just use the existing new/delete combination and let the bitset 
constructor do the initialization.


http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@439
PS4, Line 439: delete unique_chars;
> context->Free();
Please see response to previous comment.


http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@443
PS4, Line 443: DoTrimStringImpl
> Seems that this function can be merged with DoTrimString().
Done


http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@475
PS4, Line 475: chars_to_trim.is_null
> Shouldn't we skip if chars_to_trim.is_null ? We cannot make assumption abou
If we're going to skip on is_null, then I think we have to stick with passing 
StringVal(" ") from the no-args trim functions. Otherwise it's going to be hard 
to make a distinction on whether we want to trim spaces, or the chars_to_trim 
is null, so there's no work to be done.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 15 Dec 2017 20:35:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Alex Behm,

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

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

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..

IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of
characters.

This patch generalizes ltrim()/rtrim() functions to accept a second
argument that specifies the set of characters to be removed from the
leading/trailing end of the target string:

ltrim(string text[, characters text])
rtrim(string text[, characters text])

A common string trimming method has been added to StringFunctions,
which is called from the general ltrim/rtrim/btrim functions. The
functions also share prepare and close operations.

New StringFunctions tests have been added to ExprTest for the new
forms of ltrim() and rtrim(). New tests to cover handling of special
characters have also been added.

Note that our string handling functions only work with the ASCII
character set. Handling of other character sets lies outside the
scope of this patch.

The existing ltrim()/rtrim()/trim() functions that take only one
argument have been updated to use the more general methods.

Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 186 insertions(+), 79 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784 )

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


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8784/4/tests/common/impala_service.py
File tests/common/impala_service.py:

http://gerrit.cloudera.org:8080/#/c/8784/4/tests/common/impala_service.py@81
PS4, Line 81: return None
> Ok, that makes sense.
The HTTP response code is 200!



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 15 Dec 2017 22:31:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784 )

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


Patch Set 4:

(3 comments)

Thanks for the comments. Uploading patch set #5.

http://gerrit.cloudera.org:8080/#/c/8784/4/tests/common/impala_service.py
File tests/common/impala_service.py:

http://gerrit.cloudera.org:8080/#/c/8784/4/tests/common/impala_service.py@70
PS4, Line 70:   LOG.info("Thrift profile for query %s not yet available: 
%s", query_id, str(e))
> This is really esoteric, but you can get python to print the exception by p
Thanks for the information :)


http://gerrit.cloudera.org:8080/#/c/8784/4/tests/common/impala_service.py@81
PS4, Line 81: return None
> I don't think this should return None. It's not expected that we can't dese
I've seen this fail like below:

$ ./run-tests.py -k test_query_profile_thrift_timestamps 
query_test/test_observability.py

$ cat $IMPALA_HOME/logs/ee_tests/results/TEST-impala-verify-metrics.xml

-- fetching results from: tests.common.impala_connection.OperationHandle 
object at 0x7fad2b6ac390
-- closing connection to: localhost:21000
MainThread: Exception while deserializing query profile of 
745d01f212903da:243a23c6: Incorrect padding


Sometimes I've seen it raise exception 3 times before finally succeeding in 
retrieving a valid thrift profile. This being an "API" I'd like to return None 
here, and let the caller deal with that.


http://gerrit.cloudera.org:8080/#/c/8784/4/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/8784/4/tests/query_test/test_observability.py@155
PS4, Line 155: dbg_str = 'Debug thrift profile for query %s' + 
str(query_id) + ' not available in '
> I don't think that %s is interpolated.
Thanks for catching that! Fixed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 14 Dec 2017 00:08:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784 )

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


Patch Set 5:

> (1 comment)
 >
 > Uploading patch set #6 post rebase.

Umm..that should've said path set #5.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 14 Dec 2017 00:40:42 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784 )

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


Patch Set 4:

(1 comment)

Uploading patch set #6 post rebase.

http://gerrit.cloudera.org:8080/#/c/8784/4/tests/common/impala_service.py
File tests/common/impala_service.py:

http://gerrit.cloudera.org:8080/#/c/8784/4/tests/common/impala_service.py@81
PS4, Line 81: return None
> What's the exception? What's the profile that it failed to read? Is there a
The exception is "Incorrect padding". I only see that when I run a specific 
test case with the -k option. If I run all the tests in 
query_test/test_observability.py, the exception is not thrown. Instead, I get 
the runtime profile that does not yet have the "End Time" value set.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 14 Dec 2017 00:39:23 +
Gerrit-HasComments: Yes

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

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger,

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

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

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

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

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

This commit follows 16d8dd58.

This patch adds a test case that inspects the thrift profile of a
completed query, and verifies that the "Start Time" and
"End Time" of the query have nanosecond precision. We chose to
work with the thrift profile directly, rather than parse the debug
web page, as it is the thrift profile which is consumed by
management API clients of Impala.

Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
---
M tests/common/impala_service.py
M tests/query_test/test_observability.py
2 files changed, 72 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

Hello Bharath Vissapragada, Michael Ho, Dimitris Tsirogiannis, Alex Behm,

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

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

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

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..

IMPALA-6114: Require type equality of NumericLiteral::localEquals().

This patch fixes a regression introduced as part of IMPALA-1788, where
an expression like 'CAST(0 AS DECIMAL(14))' is rewritten as a
NumericLiteral expression of type DECIMAL(14,0). The query had
another NumericLiteral of type TINYINT. While analyzing the DISTINCT
aggregation clause of the SELECT query, AggregateInfo::create() removes
duplicate expressions from groupingExprs.

NumericLiteral::localEquals() is used to check for equality. Now
since the method does not consider expression types, a TINYINT literal
is considered to be duplicate of a DECIMAL literal. This results in a
query like the following to fail:

SELECT DISTINCT CAST(0 AS DECIMAL(14), 0 FROM functional.alltypes

We propose to fix the issue by accounting for types as well
when comparing analyzed numeric literals. A test case has been added
to AnalyzeExprsTest.

Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
---
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 13 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..


Patch Set 3:

(1 comment)

Thanks for the comments. Please see patch set #4.

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

http://gerrit.cloudera.org:8080/#/c/8448/2/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@141
PS2, Line 141: if ((isAnalyzed() && tmp.isAnalyzed()) && 
(!getType().equals(tmp.getType( return false;
> 2. NullLiterals may be cast to any type. Casts are applied in-place and wil
NullLiteral::localEquals() added.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 12 Dec 2017 21:17:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

Hello Bharath Vissapragada, Michael Ho, Dimitris Tsirogiannis, Alex Behm,

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

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

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

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..

IMPALA-6114: Require type equality of NumericLiteral::localEquals().

This patch fixes a regression introduced as part of IMPALA-1788, where
an expression like 'CAST(0 AS DECIMAL(14))' is rewritten as a
NumericLiteral expression of type DECIMAL(14,0). The query had
another NumericLiteral of type TINYINT. While analyzing the DISTINCT
aggregation clause of the SELECT query, AggregateInfo::create() removes
duplicate expressions from groupingExprs.

NumericLiteral::localEquals() is used to check for equality. Now
since the method does not consider expression types, a TINYINT literal
is considered to be duplicate of a DECIMAL literal. This results in a
query like the following to fail:

SELECT DISTINCT CAST(0 AS DECIMAL(14), 0 FROM functional.alltypes

We propose to fix the issue by accounting for types as well
when comparing analyzed numeric literals. A similar check has been
added to NullLiteral as well.

A test case has been added to AnalyzeExprsTest.

Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
---
M fe/src/main/java/org/apache/impala/analysis/NullLiteral.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 22 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Alex Behm,

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

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

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..

IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of
characters.

This patch generalizes ltrim()/rtrim() functions to accept a second
argument that specifies the set of characters to be removed from the
leading/trailing end of the target string:

ltrim(string text[, characters text])
rtrim(string text[, characters text])

A common string trimming method has been added to StringFunctions,
which is called from the general ltrim/rtrim/btrim functions. The
functions also share prepare and close operations.

New StringFunctions tests have been added to ExprTest for the new
forms of ltrim() and rtrim(). New tests to cover handling of special
characters have also been added.

Note that our string handling functions only work with the ASCII
character set. Handling of other character sets lies outside the
scope of this patch.

The existing ltrim()/rtrim()/trim() functions that take only one
argument have been updated to use the more general methods.

Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 173 insertions(+), 82 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784 )

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


Patch Set 6:

Hi PhilZ, are you ok with the latest patch?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 19 Dec 2017 01:01:37 +
Gerrit-HasComments: No

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

[Zoram Thanga (Code Review)]

Zoram Thanga 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:

(2 comments)

> (1 comment)
 >
 > Please add a test as Phil suggested. If we decide to add 64-bit
 > timestamps to the profile, please file a JIRA for it.

Alas, I haven't yet found a quick way to add the test. How does one get a query 
id from a query handle/result (returned by execute_query_async/execute_query)? 
If I know the query id, that can be passed to read_profile_page from where we 
can extract the fields of interest.

http://gerrit.cloudera.org:8080/#/c/8611/2/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8611/2/be/src/service/client-request-state.cc@110
PS2, Line 110:   summary_profile_->AddInfoString("Start Time", 
ToStringFromUnixMicros(start_time_us(),
 :   TimePrecision::Nanosecond));
> I'm not sure that TODO is the right thing because then the units are even l
That's a good point.


http://gerrit.cloudera.org:8080/#/c/8611/2/be/src/service/client-request-state.cc@110
PS2, Line 110:   summary_profile_->AddInfoString("Start Time", 
ToStringFromUnixMicros(start_time_us(),
 :   TimePrecision::Nanosecond));
> Please add a comment as to why we use nano-second here:
Added comments explaining why nanosecond precision is used.



--
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: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 21 Nov 2017 08:39:03 +
Gerrit-HasComments: Yes

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

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger, Dan Hecht,

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

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

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

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, 14 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8611/3
--
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: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

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

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger, Dan Hecht,

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

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

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

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

Also added TestObservability::test_query_profile_timestamps test case.

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
M tests/query_test/test_observability.py
4 files changed, 52 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8611/4
--
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: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: 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/8611 )

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


Patch Set 3:

> > (2 comments)
 > >
 > > > (1 comment)
 > > >
 > > > Please add a test as Phil suggested. If we decide to add 64-bit
 > > > timestamps to the profile, please file a JIRA for it.
 > >
 > > Alas, I haven't yet found a quick way to add the test. How does
 > one
 > > get a query id from a query handle/result (returned by
 > > execute_query_async/execute_query)? If I know the query id, that
 > > can be passed to read_profile_page from where we can extract the
 > > fields of interest.
 >
 > ./experiments/test_process_failures.py and ./webserver/test_web_pages.py
 > looks like they have examples for beeswax and operation_id_to_query_id()
 > looks like for HS2.

Thanks Dan! Have added a new test case that checks nanosecond-precision start 
and end date-time stamps.


--
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: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 21 Nov 2017 20:18:22 +
Gerrit-HasComments: No

[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/8611 )

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


Patch Set 4:

(5 comments)

Uploading a new patch set. Please let me know if this looks ok.

http://gerrit.cloudera.org:8080/#/c/8611/4/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/8611/4/tests/query_test/test_observability.py@116
PS4, Line 116: nanosecond precision."""
> I think this needs a bit more explanation as to why nanoseconds precision i
Added more explanation.


http://gerrit.cloudera.org:8080/#/c/8611/4/tests/query_test/test_observability.py@128
PS4, Line 128: while (time.time() - start_time < 10):
> My suspicion is that it would be safer to have a larger timeout.
Setting the timeout to 120 seconds. But we should be breaking the loop much 
much sooner in almost all cases.


http://gerrit.cloudera.org:8080/#/c/8611/4/tests/query_test/test_observability.py@129
PS4, Line 129: query_profile
> should that be query_profile_encoded?  seems like that's the one that would
query_profile_encoded gives the raw thrift object.


http://gerrit.cloudera.org:8080/#/c/8611/4/tests/query_test/test_observability.py@130
PS4, Line 130: if len(profile_page) < 100:
> so i guess the internval/timeout for read_debug_webpage() isn't sufficient
Good suggestion. I've changed the loop to lookout for start and end times, and 
break as soon as end time shows up.


http://gerrit.cloudera.org:8080/#/c/8611/4/tests/query_test/test_observability.py@131
PS4, Line 131: print 'Profile page does not seem ready (len=%d)' % 
(len(profile_page))
> May consider throttling this log output to only print it if the debug webpa
Done



--
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: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 21 Nov 2017 22:47:59 +
Gerrit-HasComments: Yes

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

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger, Dan Hecht,

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

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

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

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

Also added TestObservability::test_query_profile_timestamps test case.

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
M tests/query_test/test_observability.py
4 files changed, 57 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8611/5
--
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: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: 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/8611 )

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


Patch Set 6:

(4 comments)

Uploading patch #7.

http://gerrit.cloudera.org:8080/#/c/8611/6/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/8611/6/tests/query_test/test_observability.py@133
PS6, Line 133: time.time() - start_time < 120
> May be a counter here is sufficient as we have sleep below.
Now using a retry counter instead.


http://gerrit.cloudera.org:8080/#/c/8611/6/tests/query_test/test_observability.py@149
PS6, Line 149: if len(start_time_sub_sec_str) == 0 or len(end_time_sub_sec_str) 
== 0:
> The test will fail at this point anyway so may as well do what Phil suggest
Done


http://gerrit.cloudera.org:8080/#/c/8611/6/tests/query_test/test_observability.py@151
PS6, Line 151:   print 'Test case will fail'
> I think the following will fail more obviously:
Done


http://gerrit.cloudera.org:8080/#/c/8611/6/tests/query_test/test_observability.py@152
PS6, Line 152: assert len(start_time_sub_sec_str) == 9
> I don't think this line is ever reached, because in the successful case, li
Removed these lines.



--
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: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 22 Nov 2017 00:33:01 +
Gerrit-HasComments: Yes

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

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger, Dan Hecht,

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

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

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

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

Also added TestObservability::test_query_profile_timestamps test case.

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
M tests/query_test/test_observability.py
4 files changed, 56 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/8611/7
--
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: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784 )

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


Patch Set 2:

(9 comments)

Thanks for the comments. Please have a look at patch set #3

http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@116
PS2, Line 116:   def __test_query_profile_thrift_timestamps(self):
> I wouldn't have a function named __x as well as a function named x. It's ju
Removed __, and the other function below.


http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@126
PS2, Line 126: service = ImpalaCluster().get_any_impalad().service
> Why is this "any_impalad"? It really has to be the one that you executed th
Is there a way to get this from the handle or something? Would love to use it. 
Changed to get_first_impalad() for now.


http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@132
PS2, Line 132:   tree = service.get_thrift_profile(query_id)
> Where is get_thrift_profile() defined?
This is defined in common/impala_service.py, as part of this patch. I figured 
it could be useful more generally.


http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@134
PS2, Line 134:   start_time = tree.nodes[1].info_strings["Start Time"]
> Add a comment about why it's nodes[1] here.
Done


http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@136
PS2, Line 136:   start_time_sub_sec_str = start_time.split('.')[-1]
> Add a comment here about what start_time looks like, so it's clear what the
Done


http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@142
PS2, Line 142:   assert len(end_time_sub_sec_str) == 9
> Add ", end_time" here, so that we can see the failed string if it ever fail
Done


http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@147
PS2, Line 147: # This could happen due to heavy system load. The test is 
then inconclusive.
> This is a controlled environment--our tests. Let's be assertive and do the
Changed to assert statement


http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@152
PS2, Line 152: print dbg_str
> Prefer logging to printing. We do both, but "import logging; logging.info(.
Done


http://gerrit.cloudera.org:8080/#/c/8784/2/tests/query_test/test_observability.py@155
PS2, Line 155:   def test_query_profile_thrift_timestamps(self):
 : if not self.__test_query_profile_thrift_timestamps():
 :   dbg_str = 'Debug thrift profile not available in ' + 
str(MAX_RETRIES) + ' seconds?'
 :   dbg_str += ' Retrying the test'
 :   assert True, self.__test_query_profile_thrift_timestamps()
> Do we really need to run it twice? Why isn't it passing? I'll bet my hat on
Removed this one. Instead asserting that the actual test above is successful.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 08 Dec 2017 07:03:36 +
Gerrit-HasComments: Yes

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

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger,

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

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

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

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

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

This commit follows 16d8dd58.

This patch adds a test case that inspects the thrift profile of a
completed query, and verifies that the "Start Time" and
"End Time" of the query have nanosecond precision. We chose to
work with the thrift profile directly, rather than parse the debug
web page, as it is the thrift profile which is consumed by
management API clients of Impala.

Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
---
M tests/common/impala_service.py
M tests/query_test/test_observability.py
2 files changed, 63 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

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

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger,

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

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

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

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

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

This commit follows 16d8dd58.

This patch adds a test case that inspects the thrift profile of a
completed query, and verifies that the "Start Time" and
"End Time" of the query have nanosecond precision. We chose to
work with the thrift profile directly, rather than parse the debug
web page, as it is the thrift profile which is consumed by
management API clients of Impala.

Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
---
M tests/common/impala_service.py
M tests/query_test/test_observability.py
2 files changed, 66 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784 )

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


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8784/1/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/8784/1/tests/query_test/test_observability.py@119
PS1, Line 119: __
> Why __ prefix ?
I meant this as a way to convey that this function is 'private', and is not a 
test case that is exposed. Suggestions for alternatives welcome!


http://gerrit.cloudera.org:8080/#/c/8784/1/tests/query_test/test_observability.py@136
PS1, Line 136: while (retries < 60):
> for i in xrange(MAX_RETRIES):
Done


http://gerrit.cloudera.org:8080/#/c/8784/1/tests/query_test/test_observability.py@139
PS1, Line 139: ["Start Time"
> Is this key always in info_strings[] ? Is there a chance this will result i
I think the keys are always there, unless they are removed from 
ClientRequestState::ClientRequestState() and ClientRequestState::Done(). We'd 
be able to catch the regression then. Since clients expect to see these, I 
think it's good to assume so in the test case as well.


http://gerrit.cloudera.org:8080/#/c/8784/1/tests/query_test/test_observability.py@140
PS1, Line 140: ["End Time"]
> Same question for "End Time".
See the previous reply.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 07 Dec 2017 20:07:46 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784


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

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

This commit follows 16d8dd58.

This patch adds a test case that inspects the thrift profile of a
completed query, and verifies that the "Start Time" and
"End Time" of the query have nanosecond precision. We chose to
work with the thrift profile directly, rather than parse the debug
web page, as it is the thrift profile which is consumed by
management API clients of Impala.

Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
---
M tests/common/impala_service.py
M tests/query_test/test_observability.py
2 files changed, 70 insertions(+), 1 deletion(-)



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

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

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784 )

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


Patch Set 1:

The earlier attempt parsed the debug webpage; this one gets the thrift profile 
and extracts the time stamps from the profile tree. Thanks PhilZ for sharing 
how to do that.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 06 Dec 2017 23:30:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8448/2/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java@141
PS2, Line 141: if ((isAnalyzed() && tmp.isAnalyzed()) && 
(!getType().equals(tmp.getType( return false;
> 1. Use equals() for the type
1. Done.
2. Not clear about how to add this for NullLiteral, because its type is always 
NULL.

Test case has been added.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 08 Dec 2017 20:35:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..

IMPALA-6114: Require type equality of NumericLiteral::localEquals().

This patch fixes a regression introduced as part of IMPALA-1788, where
an expression like 'CAST(0 AS DECIMAL(14))' is rewritten as a
NumericLiteral expression of type DECIMAL(14,0). The query had
another NumericLiteral of type TINYINT. While analyzing the DISTINCT
aggregation clause of the SELECT query, AggregateInfo::create() removes
duplicate expressions from groupingExprs.

NumericLiteral::localEquals() is used to check for equality. Now
since the method does not consider expression types, a TINYINT literal
is considered to be duplicate of a DECIMAL literal. This results in a
query like the following to fail:

SELECT DISTINCT CAST(0 AS DECIMAL(14), 0 FROM functional.alltypes

We propose to fix the issue by accounting for types as well
when comparing analyzed numeric literals. A test case has been added
to AnalyzeExprsTest.

Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
---
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 13 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::equals().

[Zoram Thanga (Code Review)]

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


Change subject: IMPALA-6114: Require type equality of NumericLiteral::equals().
..

IMPALA-6114: Require type equality of NumericLiteral::equals().

This patch fixes a regression introduced as part of IMPALA-1788, where
an expression like 'CAST(0 AS DECIMAL(14))' is rewritten as a
NumericLiteral expression of type DECIMAL(14,0). The query had
another NumericLiteral of type TINYINT. While analyzing the DISTINCT
aggregation clause of the SELECT query, AggregateInfo::create() removes
duplicate expressions from groupingExprs.

NumericLiteral::equals() is used to check for equality. Now
since the method does not consider expression types, a TINYINT literal
is considered to be duplicate of a DECIMAL literal. This results in a
query like the following to fail:

SELECT DISTINCT CAST(0 AS DECIMAL(14) as foo, 0 AS bar where ...

where foo and bar are columns of type DECIMAL and INT respectively.

We propose to fix the issue by accounting for types as well for
analyzed numeric literals.

Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
TODO: Add test case.
---
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
1 file changed, 7 insertions(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
..


Patch Set 1:

Ping?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701
Gerrit-Change-Number: 10364
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 14 May 2018 23:27:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6997: Avoid redundant dumping in SetMemLimitExceeded()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

http://gerrit.cloudera.org:8080/#/c/10364/1/be/src/runtime/runtime-state.cc@210
PS1, Line 210:   // (e.g. once per row) before the fragment aborts. See 
IMPALA-6997.
I wonder if we should explicitly check that the existing error is 
TErrorCode::MEM_LIMIT_EXCEEDED? I mean, could we come here after hitting some 
other error condition?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I92b87f370a68a2f695ebbc2520a98dd143730701
Gerrit-Change-Number: 10364
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 11 May 2018 16:23:29 +
Gerrit-HasComments: Yes

[native-toolchain-CR] Bump libunwind version to 1.3-rc1

[Zoram Thanga (Code Review)]

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

Change subject: Bump libunwind version to 1.3-rc1
..


Patch Set 2: Code-Review+1

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/10576/2//COMMIT_MSG@14
PS2, Line 14: The patch disables a test for remote unwindind. I wasn't able to 
figure
Typo: unwindind -> unwinding?



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

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieb277ba55fb28145367d90dcbd868bf6b3c1710a
Gerrit-Change-Number: 10576
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 01 Jun 2018 18:15:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [DRAFT] IMPALA-6189: Add thread watchdogs for HDFS IO calls

[Zoram Thanga (Code Review)]

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

Change subject: [DRAFT] IMPALA-6189: Add thread watchdogs for HDFS IO calls
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/10696/3//COMMIT_MSG@20
PS3, Line 20: --thread_watchdog_threshold_ms=0 (default = 5000ms)
I wonder if the default threshold is too low, in the sense that we may get a 
lot of 'false positives' on 'hangs'. The Linux kernel hung task detector has a 
default timeout of 120 seconds by comparison.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I28e918761c120043d332b034450208eaf34e3e2b
Gerrit-Change-Number: 10696
Gerrit-PatchSet: 3
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Balazs Jeszenszky 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 21 Jun 2018 07:00:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Zoram Thanga has abandoned this change. ( http://gerrit.cloudera.org:8080/10842 
)

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Abandoned

Raising a new review.
--
To view, visit http://gerrit.cloudera.org:8080/10842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I45f5b51363afbe46653b131fad9bf68e3e3ce71f
Gerrit-Change-Number: 10842
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 1:

(3 comments)

Abandoning this patch, and raising a new one instead.

http://gerrit.cloudera.org:8080/#/c/10842/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java:

http://gerrit.cloudera.org:8080/#/c/10842/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@2576
PS1, Line 2576:   AuthzOk(ctx, "create function default.to_lower(string) 
returns string " +
> This doesn't actually create a function in the catalog. You need to do the
Thanks for the suggestion. I did not intent for the function to be reused 
across test cases, and hence adding to the catalog is probably not required. 
I've eliminated this step altogether in the new patch.


http://gerrit.cloudera.org:8080/#/c/10842/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@2578
PS1, Line 2578: ToUpper
> nit: it's probably better to call the function to_upper since the actual fu
Oops. Thanks for catching that. I meant to use ToLower and c/p'd the wrong line.


http://gerrit.cloudera.org:8080/#/c/10842/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@2596
PS1, Line 2596: AuthzError(ctx1, "select default.to_lower('ABCDEF')",
> Due to the way authorization works to prefer AuthorizationException over An
Please see above response. I've botched the rebase on top of 
https://gerrit.cloudera.org/#/c/10841/, so I am going to abandon this change 
and raise a new one instead.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45f5b51363afbe46653b131fad9bf68e3e3ce71f
Gerrit-Change-Number: 10842
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 02 Jul 2018 20:50:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

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


Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..

IMPALA-6086: Use of permanent function should require SELECT privilege
on DB

To use a permanent UDF should require at leat SELECT privilege on the
database. Functions that have constant arguments get constant-folded
into string literals, losing their privilege requests in the process.

This patch saves the privilege requests found during the first phase
of query analysis, where all the objects and the privileges required
to access them are identified. The requests are added back to the
new analyzer created for re-analysis post expression rewrite.

New FE test cases have been added to AuthorizationStmtTest.

Manual tests were also done to identify the bug, as well as to test
the fix.

Ran private parameterized Jenkins job, exhaustive exploration and
covering all tests.

Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 30 insertions(+), 0 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-6954: Fix problems with CTAS into Kudu with an expr rewrite

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6954: Fix problems with CTAS into Kudu with an expr 
rewrite
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I731743bd84cc695119e99342e1b155096147f0ed
Gerrit-Change-Number: 10251
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 30 Apr 2018 23:10:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..

IMPALA-6086: Use of permanent function should require SELECT privilege
on DB

To use a permanent UDF should require at leat SELECT privilege on the
database. Functions that have constant arguments get constant-folded
into string literals, losing their privilege requests in the process.

This patch saves the privilege requests found during the first phase
of query analysis, where all the objects and the privileges required
to access them are identified. The requests are added back to the
new analyzer created for re-analysis post expression rewrite.

New FE test cases have been added to AuthorizationStmtTest.

Manual tests were also done to identify the bug, as well as to test
the fix.

Ran private parameterized Jenkins job, exhaustive exploration and
covering all tests.

Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 56 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 2:

(6 comments)

Thanks. Please see PS#3.

http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:

http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2084
PS2, Line 2084:
> nit: add two extra spaces for continued indentation
Done


http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2087
PS2, Line 2087:   "select functional.to_lower('ABCDEF')")
> nit: move L2088 to L2087
Done


http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2091
PS2, Line 2091: .ok(onDatabase("functional", TPrivilegeLevel.ALL))
> nit: move .ok(onDatabase("functional", TPrivilegeLevel.ALL)) to be at the b
Done


http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2092
PS2, Line 2092: .ok(onDatabase("functional", TPrivilegeLevel.INSERT))
  : .ok(onDatabase("functional", 
TPrivilegeLevel.REFRESH))
> INSERT, REFRESH, and SELECT can be simplified to .ok(onDatabase("functional
Done. Want me to wrap ALL under viewMetadataPrivileges() as well?


http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2096
PS2, Line 2096: TPrivilegeLevel.SELECT, TPrivilegeLevel.ALL,
  : TPrivilegeLevel.INSERT, 
TPrivilegeLevel.REFRESH
> can be simplified to allExcept(viewMetadataPrivileges())
Done


http://gerrit.cloudera.org:8080/#/c/10850/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2167
PS2, Line 2167: uriPath, symbolName, null, null,
> nit: move L2168 to L2167
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 03 Jul 2018 20:40:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 1:

(1 comment)

Thanks for the review. Please have a look at PS#2

http://gerrit.cloudera.org:8080/#/c/10850/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:

http://gerrit.cloudera.org:8080/#/c/10850/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2071
PS1, Line 2071: authorize("create function to_lower(string) returns string 
location " +
> As mentioned in the previous review, this code doesn't create a function in
Thanks for the education! I've made the suggested changes.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 03 Jul 2018 20:02:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6307: CTAS statement fails with duplicate column exception.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dimitris Tsirogiannis, Alex Behm,

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

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

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

Change subject: IMPALA-6307: CTAS statement fails with duplicate column 
exception.
..

IMPALA-6307: CTAS statement fails with duplicate column exception.

A CTAS statement with a 'partition by' clause causes the statement
to fail with a duplicate column name exception. This is happening
because on expression rewrite, the partition defs state is not reset.

IMPALA-5796 added TableDef::reset(). This patch expands the method by
adding calls to reset ColumnDefs and PartitionColumnDefs.

Testing:
  * Regression test added to AnalyzeDDLTest.
  * Exhaustive Jenkins build and test.

Change-Id: Iee053abecd4384e15eec8db10cb06f5ace159da2
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
5 files changed, 21 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee053abecd4384e15eec8db10cb06f5ace159da2
Gerrit-Change-Number: 8930
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6307: CTAS statement fails with duplicate column exception.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6307: CTAS statement fails with duplicate column 
exception.
..


Patch Set 2:

(3 comments)

Thanks! Uploading patch set #3.

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

http://gerrit.cloudera.org:8080/#/c/8930/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@408
PS2, Line 408: // of the table that will be created.
> including the  partition columns, if any.
Done


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

http://gerrit.cloudera.org:8080/#/c/8930/2/fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java@56
PS2, Line 56: partitionColDefs_.clear(); kuduPartitionParams_.clear();
> formatting nit: one statement per line
Fixed.


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

http://gerrit.cloudera.org:8080/#/c/8930/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1510
PS2, Line 1510: IMPALA-6307: A CTAS query fails with error: AnalysisException:
  : // Duplicate column name: 
> I think it's best to outline what this query is testing, i.e. CTAS with a s
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee053abecd4384e15eec8db10cb06f5ace159da2
Gerrit-Change-Number: 8930
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 09 Jan 2018 22:04:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6307: CTAS statement fails with duplicate column exception.

[Zoram Thanga (Code Review)]

Hello Dimitris Tsirogiannis, Alex Behm,

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

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

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

Change subject: IMPALA-6307: CTAS statement fails with duplicate column 
exception.
..

IMPALA-6307: CTAS statement fails with duplicate column exception.

A CTAS statement with a 'partition by' clause causes the statement
to fail with a duplicate column name exception. This is happening
because on expression rewrite, the partition defs state is not reset.

IMPALA-5796 added TableDef::reset(). This patch expands the method by
adding calls to reset ColumnDefs and PartitionColumnDefs.

Testing:
  * Regression test added to AnalyzeDDLTest.
  * Exhaustive Jenkins build and test.

Change-Id: Iee053abecd4384e15eec8db10cb06f5ace159da2
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
5 files changed, 19 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee053abecd4384e15eec8db10cb06f5ace159da2
Gerrit-Change-Number: 8930
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6307: CTAS statement fails with duplicate column exception.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6307: CTAS statement fails with duplicate column 
exception.
..


Patch Set 1:

(3 comments)

Thanks for the comments. Please have a look at patch set #2.

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

http://gerrit.cloudera.org:8080/#/c/8930/1/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java@223
PS1, Line 223: Preconditions.checkState(colDefs.size() + 
partitionColDefs.size() == types.size());
 : for (int i = 0; i < colDefs.size(); ++i) 
colDefs.get(i).setType(types.get(i));
 : for (int i = 0; i < partitionColDefs.size(); ++i) {
 :   partitionColDefs.get(i).setType(types.get(i + 
colDefs.size()));
 : }
> I believe it would be nice to add a comment in AnalysisContext.java L405 to
Done


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

http://gerrit.cloudera.org:8080/#/c/8930/1/fe/src/main/java/org/apache/impala/analysis/TableDef.java@160
PS1, Line 160: dataLayout_.getPartitionColumnDefs().clear();
> I would add a reset() function in TableDataLayout() and put that there.
Done


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

http://gerrit.cloudera.org:8080/#/c/8930/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1510
PS1, Line 1510: AnalyzesOk("create table functional.ctas_tbl partitioned by 
(year) as " +
  : "with tmp as (select a.timestamp_col, a.year from 
functional.alltypes a " +
  : "left join functional.alltypes b " +
  : "on b.timestamp_col between a.timestamp_col and 
a.timestamp_col) " +
  : "select a.timestamp_col, a.year from tmp a");
> Add a comment. It will not be clear to everyone what this thing is testing.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee053abecd4384e15eec8db10cb06f5ace159da2
Gerrit-Change-Number: 8930
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 08 Jan 2018 19:52:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..


Patch Set 6:

(4 comments)

Changed implementation to use templates. Also ran perf measurements to check 
for regressions. Please see patch set #7

http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@475
PS4, Line 475: tionContext* context,
> Sure, we can keep passing StringVal(" ") for the no-arg case. It's not used
There are test cases that pass NULL for chars_to_trim, so that's taken care.


http://gerrit.cloudera.org:8080/#/c/8349/6/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8349/6/be/src/exprs/string-functions-ir.cc@451
PS6, Line 451: direction == LEADING || direction == BOTH
> To avoid a logic or, we can consider doing:
Changed this to a templatized implementation to avoid branching overhead.


http://gerrit.cloudera.org:8080/#/c/8349/6/be/src/exprs/string-functions-ir.cc@464
PS6, Line 464:   //return DoTrimStringImpl(str, unique_chars, direction);
> delete
Done


http://gerrit.cloudera.org:8080/#/c/8349/6/be/src/exprs/string-functions-ir.cc@466
PS6, Line 466:
 : StringVal StringFunctions::Trim(FunctionContext* context, const 
StringVal& str) {
 :   return DoTrimString(context, str, StringVal(" "), BOTH);
 : }
 :
 : StringVal StringFunctions::Ltrim(FunctionContext* context, const 
StringVal& str) {
 :   return DoTrimString(context, str, StringVal(" "), LEADING);
 : }
 :
 : StringVal StringFunctions::Rtrim(FunctionContext* context, const 
StringVal& str) {
 :   return DoTrimString(context, str, StringVal(" "), TRAILING);
 : }
> Can you please do a quick benchmark to make sure we didn't regress the perf
Have measured the performance of the new code and old trim/ltrim/rtrim on a 
tpch_parquet table that has 1.5 billion rows. The test query basically scans 
all the string columns of the table (8 of them) and calls trim/ltrim/rtrim on 
each element, where each operation is something like select 
count(trim(l_shipinstruct)), etc. The idea is to check for any unacceptable 
perf regression due to how the code has been refactored. Time spent in these 
function calls aggregated by count()s are the same in the old and new code, 
within margin of error.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 08 Jan 2018 22:06:12 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Fix typo in test observability.

[Zoram Thanga (Code Review)]

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

Change subject: Fix typo in test_observability.
..


Patch Set 1: Code-Review+1

Looks good. Sorry for the typo!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ida3f355c6c6be5ed52e4d445f8f80665cdc8e2b8
Gerrit-Change-Number: 9003
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 11 Jan 2018 06:11:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..


Patch Set 2:

(3 comments)

Working on the test case.

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

http://gerrit.cloudera.org:8080/#/c/9038/2//COMMIT_MSG@17
PS2, Line 17: Testing: The problem is easily reproduced by lowering the value of
: STATESTORE_MAX_SUBSCRIBERS to 3, and then launching a mini cluster
: with 3 impalads. With the fix, we can see the following entry in 
the
: statestore log:
> Is it possible to add a test for this?
Doing it. The approach I am taking requires changing STATESTORE_MAX_SUBSCRIBERS 
to FLAGS_statestore_max_subscribers, so that we can start statestored with a 
low max value. I think this is good to do anyway, as that allows us to test 
other boundary behavior.


http://gerrit.cloudera.org:8080/#/c/9038/2/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

http://gerrit.cloudera.org:8080/#/c/9038/2/be/src/statestore/statestore.cc@353
PS2, Line 353: // Assumes that the caller has taken subscribers_lock_
> I would add this comment in the header comment in the .h file.
Done


http://gerrit.cloudera.org:8080/#/c/9038/2/be/src/statestore/statestore.cc@353
PS2, Line 353: // Assumes that the caller has taken subscribers_lock_
> Its kind of weird IMO that OfferUpdate() expects the caller to take subscri
Thought of that, ie, moving the call to OfferUpdate from DoSubscriberUpdate to 
another scope that does not have subscribers_lock_. Then I saw that 
UnregisterSubscriber() also expects the caller to take subscribers_lock_. I 
thought it made sense to follow the same pattern here.

I think there is scope for cleaning up the way locking in the statestore code. 
For instance, RegisterSubscriber() takes subscribers_lock_, instead of its 
caller. I am just not sure if it makes sense to do that as part of this jira.

Another thing: BlockingQueue::BlockingPut() does not release the mutex it 
acquires in the shut down path. I know we're shutting down anyway, but if ever 
we implement clean shut down that does some work, a deadlock might happen here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 17 Jan 2018 19:22:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..


Patch Set 9:

(2 comments)

Thanks. Please see patch set 10.

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

http://gerrit.cloudera.org:8080/#/c/8349/9/be/src/exprs/string-functions-ir.cc@412
PS9, Line 412: context->GetNumArgs() < 2
> nit: it seems slightly simpler to say context->GetNumArgs() == 1 given the
Done


http://gerrit.cloudera.org:8080/#/c/8349/9/be/src/exprs/string-functions-ir.cc@434
PS9, Line 434: IS_CONSTANT
> As discussed, this may not be entirely correct to call this "IS_CONSTANT" a
Renamed it as IS_IMPLICIT_WHITESPACE. Hopefully this one sticks.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 9
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 18 Jan 2018 19:31:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Alex Behm,

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

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

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..

IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of
characters.

This patch generalizes ltrim()/rtrim() functions to accept a second
argument that specifies the set of characters to be removed from the
leading/trailing end of the target string:

ltrim(string text[, characters text])
rtrim(string text[, characters text])

A common string trimming method has been added to StringFunctions,
which is called from the general ltrim/rtrim/btrim functions. The
functions also share prepare and close operations.

New StringFunctions tests have been added to ExprTest for the new
forms of ltrim() and rtrim(). New tests to cover handling of special
characters have also been added.

Note that our string handling functions only work with the ASCII
character set. Handling of other character sets lies outside the
scope of this patch.

The existing ltrim()/rtrim()/trim() functions that take only one
argument have been updated to use the more general methods.

Testing: Queries like the following were run on a 1.5-billion row
tpch_parquet.lineitem table, with the old and new implementations
to ensure there is no performance regression:

  1. select count(trim(l_shipinstruct)), count(trim(l_returnflag)), ...
  2. select count(*) from t where trim(l_shipinstruct) = '' and ...

Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 178 insertions(+), 88 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 10
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6399: Fix timeout logic in test query profile thrift timestamps

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6399: Fix timeout logic in 
test_query_profile_thrift_timestamps
..


Patch Set 1:

(1 comment)

Thanks for fixing this.

http://gerrit.cloudera.org:8080/#/c/9079/1/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/9079/1/tests/query_test/test_observability.py@164
PS1, Line 164: elapsed = time.time() - (end - MAX_WAIT)
Can you add start = time.time() at L146, and use that to calculate the elapsed 
time instead? It would be more obvious that way.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f0f3db75c52b8b2081dd15ed7e7d2a1d6b22389
Gerrit-Change-Number: 9079
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 19 Jan 2018 19:50:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6399: Fix timeout logic in test query profile thrift timestamps

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6399: Fix timeout logic in 
test_query_profile_thrift_timestamps
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f0f3db75c52b8b2081dd15ed7e7d2a1d6b22389
Gerrit-Change-Number: 9079
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 19 Jan 2018 20:24:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

Hello Bharath Vissapragada, Michael Ho, Sailesh Mukil,

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

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

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

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..

IMPALA-2642: Fix a potential deadlock in statestore

The statestored can deadlock if the number of subscribers has
reached STATESTORE_MAX_SUBSCRIBERS, because the DoSubscriberUpdate()
method calls OfferUpdate(), while holding subscribers_lock_, which
also tries to take the same lock in this situation.

Fix the issue by moving out the call to acquire subscribers_lock_ from
OfferUpdate(), and depend on the callers to take it. We also make
the maximum number of statestore subscribers a start-up time tuneable,
to allow us to test the limit more easily.

Testing: The problem is easily reproduced by lowering the value of
STATESTORE_MAX_SUBSCRIBERS to 3, and then launching a mini cluster
with 3 impalads. Without the fix, the statestored becomes completely
deadlocked.

A new EE test has been added to exercise this scenario. The test
verifies that statestored correctly rejects new subscription
requests when the limit it reached.

Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
A tests/custom_cluster/test_custom_statestore.py
3 files changed, 111 insertions(+), 15 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..


Patch Set 9:

(3 comments)

Thanks. Please see patch set 9.

http://gerrit.cloudera.org:8080/#/c/8349/8/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8349/8/be/src/exprs/string-functions-ir.cc@410
PS8, Line 410:   // There can be either 1 or 2 arguments.
> Please DCHECK(context->GetNumArgs() == 1 or context->GetNumArgs() == 2);
Done


http://gerrit.cloudera.org:8080/#/c/8349/8/be/src/exprs/string-functions-ir.cc@436
PS8, Line 436: const StringVal& str, const StringVal& chars_to_trim) {
> Seems that this line can be moved. If str.len == 0, the while loop will tak
Done


http://gerrit.cloudera.org:8080/#/c/8349/8/be/src/exprs/string-functions.h
File be/src/exprs/string-functions.h:

http://gerrit.cloudera.org:8080/#/c/8349/8/be/src/exprs/string-functions.h@153
PS8, Line 153: , is true when the set of characters
 :   /// to trim is constant.
> is true when the set of characters to trim is constant.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 9
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Sat, 13 Jan 2018 00:19:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Alex Behm,

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

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

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..

IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of
characters.

This patch generalizes ltrim()/rtrim() functions to accept a second
argument that specifies the set of characters to be removed from the
leading/trailing end of the target string:

ltrim(string text[, characters text])
rtrim(string text[, characters text])

A common string trimming method has been added to StringFunctions,
which is called from the general ltrim/rtrim/btrim functions. The
functions also share prepare and close operations.

New StringFunctions tests have been added to ExprTest for the new
forms of ltrim() and rtrim(). New tests to cover handling of special
characters have also been added.

Note that our string handling functions only work with the ASCII
character set. Handling of other character sets lies outside the
scope of this patch.

The existing ltrim()/rtrim()/trim() functions that take only one
argument have been updated to use the more general methods.

Testing: Queries like the following were run on a 1.5-billion row
tpch_parquet.lineitem table, with the old and new implementations
to ensure there is no performance regression:

  1. select count(trim(l_shipinstruct)), count(trim(l_returnflag)), ...
  2. select count(*) from t where trim(l_shipinstruct) = '' and ...

Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 177 insertions(+), 88 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 9
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6307: CTAS statement fails with duplicate column exception.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6307: CTAS statement fails with duplicate column 
exception.
..


Patch Set 3:

(1 comment)

Thanks for the comment. Have added another test case, this one for Kudu.

http://gerrit.cloudera.org:8080/#/c/8930/3/fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java
File fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java:

http://gerrit.cloudera.org:8080/#/c/8930/3/fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java@58
PS3, Line 58: kuduPartitionParams_.clear();
> This is new. Do we have any existing tests with a CTAS on Kudu where the se
Added a new CTAS test for Kudu, which is the same repro test case tailored for 
Kudu.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee053abecd4384e15eec8db10cb06f5ace159da2
Gerrit-Change-Number: 8930
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 11 Jan 2018 22:44:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6307: CTAS statement fails with duplicate column exception.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Dimitris Tsirogiannis, Alex Behm,

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

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

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

Change subject: IMPALA-6307: CTAS statement fails with duplicate column 
exception.
..

IMPALA-6307: CTAS statement fails with duplicate column exception.

A CTAS statement with a 'partition by' clause causes the statement
to fail with a duplicate column name exception. This is happening
because on expression rewrite, the partition defs state is not reset.

IMPALA-5796 added TableDef::reset(). This patch expands the method by
adding calls to reset ColumnDefs and PartitionColumnDefs.

Testing:
  * Regression test added to AnalyzeDDLTest.
  * Exhaustive Jenkins build and test.

Change-Id: Iee053abecd4384e15eec8db10cb06f5ace159da2
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/TableDataLayout.java
M fe/src/main/java/org/apache/impala/analysis/TableDef.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
5 files changed, 27 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee053abecd4384e15eec8db10cb06f5ace159da2
Gerrit-Change-Number: 8930
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6384: RequestPoolService should honor custom group mapping config

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6384: RequestPoolService should honor custom group 
mapping config
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb93870c0cc37e2432a643a274931f1d3d13fb96
Gerrit-Change-Number: 9000
Gerrit-PatchSet: 2
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 11 Jan 2018 18:59:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

Hello Michael Ho, Alex Behm,

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

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

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..

IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of
characters.

This patch generalizes ltrim()/rtrim() functions to accept a second
argument that specifies the set of characters to be removed from the
leading/trailing end of the target string:

ltrim(string text[, characters text])
rtrim(string text[, characters text])

A common string trimming method has been added to StringFunctions,
which is called from the general ltrim/rtrim/btrim functions. The
functions also share prepare and close operations.

New StringFunctions tests have been added to ExprTest for the new
forms of ltrim() and rtrim(). New tests to cover handling of special
characters have also been added.

Note that our string handling functions only work with the ASCII
character set. Handling of other character sets lies outside the
scope of this patch.

The existing ltrim()/rtrim()/trim() functions that take only one
argument have been updated to use the more general methods.

Testing: Queries like the following were run on a 1.5-billion row
tpch_parquet.lineitem table, with the old and new implementations
to ensure there is no performance regression:

  1. select count(trim(l_shipinstruct)), count(trim(l_returnflag)), ...
  2. select count(*) from t where trim(l_shipinstruct) = '' and ...

Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
---
M be/src/exprs/expr-test.cc
M be/src/exprs/string-functions-ir.cc
M be/src/exprs/string-functions.h
M common/function-registry/impala_functions.py
4 files changed, 175 insertions(+), 88 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 8
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..


Patch Set 7:

(4 comments)

Thanks for the comments. Please see patch set #8

http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions-ir.cc@441
PS7, Line 441: IS_CONSTANT
> We can possibly get rid of this after IMPALA-6380 is fixed.
Agree. Will revisit this aspect of the code.


http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions-ir.cc@443
PS7, Line 443: chars_to_trim.is_null
> As discussed offline, we cannot make any assumption about the length of Str
As discussed, this seems like a bad DCHECK anyway - because '' and NULL are 
legit values of chars_to_trim, even when the arguments come from column 
elements. Removing the DCHECK, and instead check for zero-length or NULL 
chars_to_trim, as these do not modify the target string.

Have added code to check for NULL chars_to_trim in TrimPrepare().


http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions.h
File be/src/exprs/string-functions.h:

http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions.h@87
PS7, Line 87:   static void TrimPrepare(FunctionContext*, 
FunctionContext::FunctionStateScope);
> A quick comment on what this Prepare() function do would be useful.
Done


http://gerrit.cloudera.org:8080/#/c/8349/7/be/src/exprs/string-functions.h@149
PS7, Line 149: template 
> Please add a comment on what these template parameters stand for.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 12 Jan 2018 00:14:12 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

Hello Bharath Vissapragada, Michael Ho, Sailesh Mukil,

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

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

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

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..

IMPALA-2642: Fix a potential deadlock in statestore

The statestored can deadlock if the number of subscribers has
reached STATESTORE_MAX_SUBSCRIBERS, because the DoSubscriberUpdate()
method calls OfferUpdate(), while holding subscribers_lock_, which
also tries to take the same lock in this situation.

Fix the issue by moving out the call to acquire subscribers_lock_ from
OfferUpdate(), and depend on the callers to take it. We also make
the maximum number of statestore subscribers a start-up time tuneable,
to allow us to test the limit more easily.

Testing: The problem is easily reproduced by lowering the value of
STATESTORE_MAX_SUBSCRIBERS to 3, and then launching a mini cluster
with 3 impalads. Without the fix, the statestored becomes completely
deadlocked.

A new EE test has been added to exercise this scenario. The test
verifies that statestored correctly rejects new subscription
requests when the limit it reached.

Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
---
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
A tests/custom_cluster/test_custom_statestore.py
3 files changed, 107 insertions(+), 15 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..


Patch Set 5:

Thanks Sailesh. I do not have the permission to carry your +2, so could you 
please fo that again?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 30 Jan 2018 18:08:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9038/3/tests/custom_cluster/test_custom_statestore.py
File tests/custom_cluster/test_custom_statestore.py:

http://gerrit.cloudera.org:8080/#/c/9038/3/tests/custom_cluster/test_custom_statestore.py@34
PS3, Line 34: from thrift.transport import TSocket, TTransport
:
> one line
Done


http://gerrit.cloudera.org:8080/#/c/9038/3/tests/custom_cluster/test_custom_statestore.py@39
PS3, Line 39:
: LOG = logging.getLogger('custom_statestore_test')
> Needed?
Removed.


http://gerrit.cloudera.org:8080/#/c/9038/3/tests/custom_cluster/test_custom_statestore.py@42
PS3, Line 42:
> Needed?
Removed


http://gerrit.cloudera.org:8080/#/c/9038/3/tests/custom_cluster/test_custom_statestore.py@52
PS3, Line 52:   _, port = handle.getsockname()
:
:   @classmethod
:   def get_workload(self):
> Does this mean that every "statestore subscriber" uses the same port? Looks
That's correct. All the subscribers we create below will connect from the same 
port number, but with different subscriber IDs. Is that ok with you? It seems 
simpler and puts less load on the system than creating a new port every time.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 29 Jan 2018 20:39:54 +
Gerrit-HasComments: Yes

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

[Zoram Thanga (Code Review)]

Hello Michael Ho, Philip Zeyliger,

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

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

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

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

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

This commit follows 16d8dd58.

This patch adds a test case that inspects the thrift profile of a
completed query, and verifies that the "Start Time" and
"End Time" of the query have nanosecond precision. We chose to
work with the thrift profile directly, rather than parse the debug
web page, as it is the thrift profile which is consumed by
management API clients of Impala.

Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
---
M tests/common/impala_service.py
M tests/query_test/test_observability.py
2 files changed, 78 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6225: Part 2: 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/8784 )

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


Patch Set 6:

(1 comment)

Thanks for the comments. Please see #7

http://gerrit.cloudera.org:8080/#/c/8784/6/tests/query_test/test_observability.py
File tests/query_test/test_observability.py:

http://gerrit.cloudera.org:8080/#/c/8784/6/tests/query_test/test_observability.py@156
PS6, Line 156: tree.validate()
> Just for my own knowledge: where is validate() defined ?
This is defined in shell/gen-py/RuntimeProfile/ttypes.py

If validate() fails it raises a TProtocolException. The validation is very 
minimal, actually. Will move it to get_thrift_profile().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3421a34cc029ebca551730084c7cbd402d5c109
Gerrit-Change-Number: 8784
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 21 Dec 2017 00:08:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7444: Improve logging of opening/closing/expiring sessions.

[Zoram Thanga (Code Review)]

Hello Michael Ho, anujphadke, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7444: Improve logging of opening/closing/expiring 
sessions.
..

IMPALA-7444: Improve logging of opening/closing/expiring sessions.

Recent troubleshooting efforts have shown we can improve
logging of client session opening and expiry processing to
enhance serviceability.

This patch adds minor, but useful debug log improvements.

Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
---
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
2 files changed, 47 insertions(+), 32 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
Gerrit-Change-Number: 11234
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-7444: Improve logging of opening/closing/expiring sessions.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-7444: Improve logging of opening/closing/expiring 
sessions.
..


Patch Set 1:

(2 comments)

> Just being paranoid here but I hope this doesn't make the log too
 > verbose.

It's definitely more verbose than before, but I think it's worth it. 
Ordinarily, sessions are opened and stay around for a long time compared to 
individual queries. We would ordinarily see a lot of other messages in the log 
between open and close sessions.

http://gerrit.cloudera.org:8080/#/c/11234/1/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/11234/1/be/src/service/impala-hs2-server.cc@367
PS1, Line 367: Done opening
> Opened
Done


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

http://gerrit.cloudera.org:8080/#/c/11234/1/be/src/service/impala-server.cc@1236
PS1, Line 1236: Done closing
> Closed
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
Gerrit-Change-Number: 11234
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 16 Aug 2018 05:24:17 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7444: Improve logging of opening/closing/expiring sessions.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-7444: Improve logging of opening/closing/expiring 
sessions.
..


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/11234/2/be/src/service/impala-server.cc@1962
PS2, Line 1962: expired_cnt++;
> ++expired_cnt;
Done


http://gerrit.cloudera.org:8080/#/c/11234/2/be/src/service/impala-server.cc@1975
PS2, Line 1975: LOG_IF(INFO, expired_cnt > 0) << "Expired sessions. Count: 
" << expired_cnt
  :   << ". Time: " << UnixMillis() 
- now << " milliseconds.";
> What's the purpose of this line ? Won't we be able to tell that from the "E
The idea is to gauge how long session_state_map_lock_ is held in the process of 
expiring sessions. Probably less of an issue today after stack symbolization is 
removed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
Gerrit-Change-Number: 11234
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 16 Aug 2018 23:00:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7444: Improve logging of opening/closing/expiring sessions.

[Zoram Thanga (Code Review)]

Hello Michael Ho, anujphadke, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7444: Improve logging of opening/closing/expiring 
sessions.
..

IMPALA-7444: Improve logging of opening/closing/expiring sessions.

Recent troubleshooting efforts have shown we can improve
logging of client session opening and expiry processing to
enhance serviceability.

This patch adds minor, but useful debug log improvements.

Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
---
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
2 files changed, 47 insertions(+), 32 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
Gerrit-Change-Number: 11234
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-7444: Improve logging of opening/closing/expiring sessions.

[Zoram Thanga (Code Review)]

Hello Michael Ho, anujphadke, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7444: Improve logging of opening/closing/expiring 
sessions.
..

IMPALA-7444: Improve logging of opening/closing/expiring sessions.

Recent troubleshooting efforts have shown we can improve
logging of client session opening and expiry processing to
enhance serviceability.

This patch adds minor, but useful debug log improvements.

Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
---
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
2 files changed, 46 insertions(+), 32 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iecf2d3ce707cc36c21da8a2459c2f68cf0b56a4a
Gerrit-Change-Number: 11234
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Reviewer: anujphadke 

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 5: Code-Review+1

LGTM


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 5
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 17 Jul 2018 17:15:41 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7014: Disable stacktrace symbolisation by default

[Zoram Thanga (Code Review)]

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


Change subject: IMPALA-7014: Disable stacktrace symbolisation by default
..

IMPALA-7014: Disable stacktrace symbolisation by default

Stacktrace symbolization has been shown to be 2500x slower
compared to just printing the un-symbolized one.

This has burned us a few times now, so let's disable it by
default.

Change-Id: If3af209890ccc242beb742145c63eb6836d4bfbb
---
M be/src/common/init.cc
1 file changed, 2 insertions(+), 0 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-7508: Add Impala Python GDB module
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11352/2/lib/python/impala_py_lib/gdb/impala-gdb.py
File lib/python/impala_py_lib/gdb/impala-gdb.py:

http://gerrit.cloudera.org:8080/#/c/11352/2/lib/python/impala_py_lib/gdb/impala-gdb.py@30
PS2, Line 30: def get_fragment_instances():
> flake8: E302 expected 2 blank lines, found 1
Fixed.


http://gerrit.cloudera.org:8080/#/c/11352/2/lib/python/impala_py_lib/gdb/impala-gdb.py@44
PS2, Line 44: s
> flake8: F841 local variable 'symbol' is assigned to but never used
Fixed



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
Gerrit-Change-Number: 11352
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 30 Aug 2018 22:26:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Philip Zeyliger, David Knupp, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7508: Add Impala Python GDB module
..

IMPALA-7508: Add Impala Python GDB module

This patch adds a new Impala Python GDB module, and implements
a couple of covenience commands to make core dump analysis
easier.

The initial commands let us find queries and fragment instances
currently executing in an impalad at the time the daemon crashed:

(gdb) source impala-gdb.py
(gdb) find-query-ids
f74c863dff66a34d:1d983cc3
364525e12495932b:73f5dd02
bc4a3eec25481981:edda04b8

(gdb) find-fragment-instances
Fragment Instance IdThread IDs

364525e12495932b:73f5dd0200a2   [69]
364525e12495932b:73f5dd020171   [196, 136]
bc4a3eec25481981:edda04b801a8   [252, 237, 206]
f74c863dff66a34d:1d983cc3009b   [200, 14, 13, 12, 6, 5, 3, 2]
f74c863dff66a34d:1d983cc3013a   [4]

The commands have only been tested with Impala 2.12, and are not
expected to work with older versions since it uses ThreadDebugInfo
stuff from IMPALA-6416.

It is hoped that people contribute more commands to the module.

Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
---
A lib/python/impala_py_lib/gdb/README.md
A lib/python/impala_py_lib/gdb/impala-gdb.py
2 files changed, 142 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
Gerrit-Change-Number: 11352
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-7508: Add Impala Python GDB module
..


Patch Set 1:

(10 comments)

Thanks for the comments. Uploading PS#2.

http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py
File infra/gdb/impala-gdb.py:

http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@1
PS1, Line 1: #
> I feel like this file should more properly be added to:
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@1
PS1, Line 1: #
> That would be fine by me.
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@22
PS1, Line 22: import gdb
> Please add a README.md in this directory or include usage here.
Added a README.md.


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@28
PS1, Line 28: class ImpalaGDB:
> I think we usually use new-style classes.
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@29
PS1, Line 29: # Walk the threadlist to find fragment instance ids. Assumes 
IMPALA-6416, so
> Use triple " for docstring.
I meant this as a comment, not a doc string. Leaving is as is.


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@34
PS1, Line 34: def getFragmentInstances(self):
> Python's convention is to use snake_case instead of camelCase.
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@40
PS1, Line 40: while f is not None
> nit: can be simplified to while f:
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@62
PS1, Line 62: class FindFragmentInstances(gdb.Command, ImpalaGDB):
: "Find all query fragment instance to thread Id mappings in 
this impalad."
:
: def __init__(self):
: super(FindFragmentInstances, self).__init__(
> multiple-inheritance and inheritance in general seem unnecessary here.
Thanks! I wasn't happy with the inheritance either. Made 
get_fragment_instances() a free function.


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@80
PS1, Line 80: "Find IDs of all queries this impalad is currently executing."
> Use triple " for docstring.
Done


http://gerrit.cloudera.org:8080/#/c/11352/1/infra/gdb/impala-gdb.py@95
PS1, Line 95: qid_hi + ':' + qid_low
> nit: usually it's preferable to use string format instead of +
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
Gerrit-Change-Number: 11352
Gerrit-PatchSet: 1
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 30 Aug 2018 22:11:07 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Philip Zeyliger, David Knupp, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7508: Add Impala Python GDB module
..

IMPALA-7508: Add Impala Python GDB module

This patch adds a new Impala Python GDB module, and implements
a couple of covenience commands to make core dump analysis
easier.

The initial commands let us find queries and fragment instances
currently executing in an impalad at the time the daemon crashed:

(gdb) source impala-gdb.py
(gdb) find-query-ids
f74c863dff66a34d:1d983cc3
364525e12495932b:73f5dd02
bc4a3eec25481981:edda04b8

(gdb) find-fragment-instances
Fragment Instance IdThread IDs

364525e12495932b:73f5dd0200a2   [69]
364525e12495932b:73f5dd020171   [196, 136]
bc4a3eec25481981:edda04b801a8   [252, 237, 206]
f74c863dff66a34d:1d983cc3009b   [200, 14, 13, 12, 6, 5, 3, 2]
f74c863dff66a34d:1d983cc3013a   [4]

The commands have only been tested with Impala 2.12, and are not
expected to work with older versions since it uses ThreadDebugInfo
stuff from IMPALA-6416.

It is hoped that people contribute more commands to the module.

Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
---
A lib/python/impala_py_lib/gdb/README.md
A lib/python/impala_py_lib/gdb/impala-gdb.py
2 files changed, 140 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
Gerrit-Change-Number: 11352
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-1760: Implement shutdown command

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-1760: Implement shutdown command
..


Patch Set 18:

(3 comments)

Thanks for doing this.

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

http://gerrit.cloudera.org:8080/#/c/10744/18/be/src/service/impala-server.h@108
PS18, Line 108: /// 2. The startup grace period starts, during which:
Can this be based instead on a successful startup indicator/flag that gets set 
when the daemon is considered to be fully started up?


http://gerrit.cloudera.org:8080/#/c/10744/18/be/src/service/impala-server.h@119
PS18, Line 119: ///executing). If it is quiesced then it cleanly shut downs 
by exiting the process.
nit: ...cleanly shuts down...


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

http://gerrit.cloudera.org:8080/#/c/10744/18/be/src/service/impala-server.cc@2408
PS18, Line 2408:   if (set_grace) {
nit: online?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d5606ccfec84db4482c1e7f0f198103aad141a0
Gerrit-Change-Number: 10744
Gerrit-PatchSet: 18
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 10 Sep 2018 18:20:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7508: Add Impala Python GDB module

[Zoram Thanga (Code Review)]

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


Change subject: IMPALA-7508: Add Impala Python GDB module
..

IMPALA-7508: Add Impala Python GDB module

This patch adds a new Impala Python GDB module, and implements
a couple of covenience commands to make core dump analysis
easier.

The initial commands let us find queries and fragment instances
currently executing in an impalad at the time the daemon crashes:

(gdb) source impala-gdb.py
(gdb) find-query-ids
f74c863dff66a34d:1d983cc3
364525e12495932b:73f5dd02
bc4a3eec25481981:edda04b8

(gdb) find-fragment-instances
Fragment Instance IdThread IDs

364525e12495932b:73f5dd0200a2   [69]
364525e12495932b:73f5dd020171   [196, 136]
bc4a3eec25481981:edda04b801a8   [252, 237, 206]
f74c863dff66a34d:1d983cc3009b   [200, 14, 13, 12, 6, 5, 3, 2]
f74c863dff66a34d:1d983cc3013a   [4]

The commands have only been tested with Impala 2.12, and are not
expected to work with older versions since it uses ThreadDebugInfo
stuff from IMPALA-6416.

It is hoped that people contribute more commands to the module.

Change-Id: I24e7026f2265954ed592d6f62110cf8cb2c2202a
---
A infra/gdb/impala-gdb.py
1 file changed, 101 insertions(+), 0 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-7542: fix find-fragment-instances to find all "root threads"

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-7542: fix find-fragment-instances to find all "root 
threads"
..


Patch Set 2: Code-Review+1

Thanks for fixing the bug! Change LGTM.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I35ae1a6b384b002b343689469f02ceabd84af1b6
Gerrit-Change-Number: 11396
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 07 Sep 2018 20:35:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc@64
PS2, Line 64:LOG(INFO) << "SIGTERM encountered invoking default handler 
system going down" << endl;
I would change the wording to "Caught SIGTERM. Daemon going down."


http://gerrit.cloudera.org:8080/#/c/10847/2/be/src/service/impalad-main.cc@108
PS2, Line 108: old_action
> Do we currently set a handler for SIGTERM elsewhere? If not, we should remo
Please add a comment explaining why SIGTERM is specifically handled (i.e., we 
want to log a message when Impalad/statestored/catalogd is being shut down via 
the signal for whatever reason).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 2
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 06 Jul 2018 23:56:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7288: Fix Codegen Crash in FinalizeModule()

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-7288: Fix Codegen Crash in FinalizeModule()
..


Patch Set 1:

> I am not sure what kind of tests I can add to this, since this
 > looks more like an abuse of API than an actual functionality that i
 > can test. any ideas welcomed

How about the test case that repro'd this? I mean the one Balasz posted?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f0b527909a9fb3090996bb7510e4d58350c21b0
Gerrit-Change-Number: 10933
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 12 Jul 2018 22:50:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..

IMPALA-6086: Use of permanent function should require SELECT privilege
on DB

To use a permanent UDF should require at least SELECT privilege on the
database. Functions that have constant arguments get constant-folded
into string literals, losing their privilege requests in the process.

This patch saves the privilege requests found during the first phase
of query analysis, where all the objects and the privileges required
to access them are identified. The requests are added back to the
new analyzer created for re-analysis post expression rewrite.

Testing:
New FE test cases have been added to AuthorizationStmtTest.

Manual tests were also done to identify the bug, as well as to test
the fix.

Ran exhaustive and covering tests.

Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 50 insertions(+), 4 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 4:

(8 comments)

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

http://gerrit.cloudera.org:8080/#/c/10850/3//COMMIT_MSG@10
PS3, Line 10: leas
> nit: least
Done


http://gerrit.cloudera.org:8080/#/c/10850/3//COMMIT_MSG@18
PS3, Line 18:
> add a "Testing: " section header.
Done


http://gerrit.cloudera.org:8080/#/c/10850/3//COMMIT_MSG@24
PS3, Line 24:
: Ran exhaustive and
> nit: replace with "Ran exhaustive and covering tests."
Done


http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@481
PS3, Line 481: LOG.trace("rewrittenStmt: " + analysisResult_.stmt_.toSql());
> ok. looks like this change is an improvement over the current behavior. pls
Filed https://issues.apache.org/jira/browse/IMPALA-7285


http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:

http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2070
PS3, Line 2070:
> nit: remove
Done


http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2074
PS3, Line 2074: fo
> nit: remove
Done


http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2085
PS3, Line 2085: .ok(onServer(TPrivilegeLevel
> move above L2082?
Changed the text so that the context is clearer.


http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2153
PS3, Line 2153: ivate static String alterError(String object) {
  : return "User '%s' does not have privileges to execute 
'ALTER' on: " + object;
  :   }
  :
> replace with a call to the more generic function on L2160
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 12 Jul 2018 23:37:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

http://gerrit.cloudera.org:8080/#/c/10850/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@481
PS3, Line 481: LOG.trace("rewrittenStmt: " + analysisResult_.stmt_.toSql());
> supposing the folded fn reveals something interesting, e.g., getSSN("some u
I think that's the right approach in general: Parse the statement, then check 
access, then perform analysis. I think that would be well outside the scope of 
this JIRA, though. Please also note that this code was existing.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 3
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 11 Jul 2018 22:59:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6271: Impala daemon should log a message when it's being 
shut down
..


Patch Set 4:

Can you also add test case to look for the SIGTERM induced 'shutdown log' in 
the log file? Since that's the whole point of adding the handler, I think it 
would be good to ensure we don't lose the log message.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29
Gerrit-Change-Number: 10847
Gerrit-PatchSet: 4
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Wed, 11 Jul 2018 23:03:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

Hello Fredy Wijaya, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..

IMPALA-6086: Use of permanent function should require SELECT privilege
on DB

To use a permanent UDF should require at least SELECT privilege on the
database. Functions that have constant arguments get constant-folded
into string literals, losing their privilege requests in the process.

This patch saves the privilege requests found during the first phase
of query analysis, where all the objects and the privileges required
to access them are identified. The requests are added back to the
new analyzer created for re-analysis post expression rewrite.

Testing:
New FE test cases have been added to AuthorizationStmtTest.

Manual tests were also done to identify the bug, as well as to test
the fix.

Ran exhaustive and covering tests.

Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
2 files changed, 50 insertions(+), 4 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6086: Use of permanent function should require SELECT privilege on DB

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6086: Use of permanent function should require SELECT 
privilege on DB
..


Patch Set 5: Code-Review+1

(1 comment)

Fixed the indentation. Thanks for the review.

http://gerrit.cloudera.org:8080/#/c/10850/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java:

http://gerrit.cloudera.org:8080/#/c/10850/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2183
PS4, Line 2183:   Type retType, String uriPath, String symbolName) {
> nit: incorrect indentation
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee70f15e4c04f7daaed9cac2400ec626e1fb0e57
Gerrit-Change-Number: 10850
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 13 Jul 2018 16:15:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2624: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

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


Change subject: IMPALA-2624: Fix a potential deadlock in statestore
..

IMPALA-2624: Fix a potential deadlock in statestore

The statestored can deadlock if the number of subscribers has
reached STATESTORE_MAX_SUBSCRIBERS, because the DoSubscriberUpdate()
method calls OfferUpdate(), while holding subscribers_lock_, which
also tries to take the same lock in this situation.

Fix the issue by moving out the call to acquire subscribers_lock_ from
OfferUpdate(), and depend on the callers to take it.

Testing: The problem is easily reproduced by lowering the value of
STATESTORE_MAX_SUBSCRIBERS to 3, and then launching a mini cluster
with 3 impalads. With the fix, we can see the following entry in the
statestore log:

I0116 11:56:19.050271  6884 status.cc:125] Maximum subscriber limit
reached: 3

Without the fix, the statestored becomes completely deadlocked.

Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
---
M be/src/statestore/statestore.cc
1 file changed, 6 insertions(+), 6 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

Hello Bharath Vissapragada, Michael Ho,

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

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

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

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..

IMPALA-2642: Fix a potential deadlock in statestore

The statestored can deadlock if the number of subscribers has
reached STATESTORE_MAX_SUBSCRIBERS, because the DoSubscriberUpdate()
method calls OfferUpdate(), while holding subscribers_lock_, which
also tries to take the same lock in this situation.

Fix the issue by moving out the call to acquire subscribers_lock_ from
OfferUpdate(), and depend on the callers to take it.

Testing: The problem is easily reproduced by lowering the value of
STATESTORE_MAX_SUBSCRIBERS to 3, and then launching a mini cluster
with 3 impalads. With the fix, we can see the following entry in the
statestore log:

I0116 11:56:19.050271  6884 status.cc:125] Maximum subscriber limit
reached: 3

Without the fix, the statestored becomes completely deadlocked.

Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
---
M be/src/statestore/statestore.cc
1 file changed, 6 insertions(+), 6 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-2642: Fix a potential deadlock in statestore

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-2642: Fix a potential deadlock in statestore
..


Patch Set 2:

(1 comment)

Fixed the Jira Id.

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

http://gerrit.cloudera.org:8080/#/c/9038/1//COMMIT_MSG@7
PS1, Line 7: IMPALA-2642
> Can you update the correct jira.
Oops. Fixed it. Thanks.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d49dede221ce1f50ec299643b5532c61f93f0c6
Gerrit-Change-Number: 9038
Gerrit-PatchSet: 2
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Tue, 16 Jan 2018 20:13:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Zoram Thanga (Code Review)]

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

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..


Patch Set 11:

Let's wait for https://gerrit.cloudera.org/#/c/9079/ to be merged before 
rebasing, so that we're not again held up by a flaky test failing.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 11
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Mon, 22 Jan 2018 21:36:07 +
Gerrit-HasComments: No