[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-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-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-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-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-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-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-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] 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-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-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] 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-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-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-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: 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-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 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-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-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-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-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-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-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-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 (#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)

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

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

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

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:

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-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-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 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-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: 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)]

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

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

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