date:20171213

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

Change subject: IMPALA-6290: limit ScannerContext to 1 buffer at a time.
..


Patch Set 3:

(14 comments)

Thank you for working on this! I left some comments around clarifying things 
and on GetBytesInternal().

http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/hdfs-scanner.h
File be/src/exec/hdfs-scanner.h:

http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/hdfs-scanner.h@97
PS3, Line 97: /// This class also encapsulates row batch management.  
Subclasses should call CommitRows()
nit: long line (and the new Gerrit UI breaks it now, making it look bad).


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h
File be/src/exec/scanner-context.h:

http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@143
PS3, Line 143: /// (the scan range could be longer than the file).
Can we extend this comment with what the implication are? Is the stream still 
valid afterwards? How is this different from scan_range_eosr_?


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@146
PS3, Line 146: /// If true, the stream has reached the end of the file.
Can we extend this comment with what the implication are? Is the stream still 
valid afterwards?


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@161
PS3, Line 161: bool ReadBoolean(bool* boolean, Status* status);
Should we add WARN_UNUSED_RESULT while you're here?


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@193
PS3, Line 193: /// Release resources from previous reads in this stream. If 
'done' is true all
This only releases buffers that are completely read, right? In that case, 
wouldn't adding back "completed" be more clear?


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@224
PS3, Line 224: initial
"initial" implies that there are more, no?


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@254
PS3, Line 254: // We always want output_buffer_bytes_left_ to be non-NULL, 
so we can avoid a NULL check
nit: long line


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@298
PS3, Line 298: 2
Huh?


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@320
PS3, Line 320: Always
 :   /// returns the current I/O buffer to the I/O manager.
This only seems true when the buffer has been read/skipped completely.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc
File be/src/exec/scanner-context.cc:

http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc@101
PS3, Line 101: buffer_range->ReturnBuffer(move(io_buffer_));
Can we reset io_buffer_pos_ here, too? It looks to me below like we're using 
(io_buffer_bytes_left_ == 0, io_buffer_pos_ != nullptr) as an indicator that 
we're not at the end of the file. Would it make mistakes less likely if we 
added that state explicitly in some variable?


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc@182
PS3, Line 182:   if (eosr()) return Status::OK();
Can you explain here (or in the header at either this function or 
scan_range_eosr_) why we don't use scan_range_eosr_ here? It's not clear to me.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc@226
PS3, Line 226: Status ScannerContext::Stream::GetBytesInternal(int64_t 
requested_len,
This method still looks very confusing to me. Can you think of ways to make the 
handling of the various cases more obvious? For example by handling case 1 
(read from io_buffer) explicitly? Let's chat in person if you think it'll help.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc@228
PS3, Line 228:   DCHECK_GT(requested_len, boundary_buffer_bytes_left_);
Can you also add a DCHECK to document and assert requested_len > 
io_buffer_bytes_left_?


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc@260
PS3, Line 260: num_bytes
This is the number of bytes we still need to copy over from the io_buffer, 
right? Can you think of a better name? num_bytes_left_to_copy?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I74c5960a75f7d88b0e1de4199af731fb13e592f0
Gerrit-Change-Number: 8814
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 03:05:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6070: Parallelize another bit of data load.

2017-12-13 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-6070: Parallelize another bit of data load.
..


Patch Set 2: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e93ee5a77ec9271b980b88bef7ad512ecbe0407
Gerrit-Change-Number: 8822
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 14 Dec 2017 02:28:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6070: Parallelize another bit of data load.

2017-12-13 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8822 )

Change subject: IMPALA-6070: Parallelize another bit of data load.
..

IMPALA-6070: Parallelize another bit of data load.

The two Kudu loads and Hive UDFs can all run in parallel. This
should shave about 4 minutes off of the data load. (Current
timings are 3.5, 4, and 0.6 minutes, see below.)

I've run dataload with this change many times.

   Loading Kudu functional (logging to 
/home/ubuntu/Impala/logs/data_loading/load-kudu.log)...
 Loading workload 'functional-query' using exploration strategy 'core' in 
table formats 'kudu/none/none' OK (Took: 3 min 29 sec)
   Loading Kudu TPCH (logging to 
/home/ubuntu/Impala/logs/data_loading/load-kudu-tpch.log)...
 Loading workload 'tpch' using exploration strategy 'core' in table formats 
'kudu/none/none' OK (Took: 4 min 0 sec)
   Loading Hive UDFs (logging to 
/home/ubuntu/Impala/logs/data_loading/build-and-copy-hive-udfs.log)...
 Loading Hive UDFs OK (Took: 0 min 41 sec)

Change-Id: I7e93ee5a77ec9271b980b88bef7ad512ecbe0407
Reviewed-on: http://gerrit.cloudera.org:8080/8822
Reviewed-by: Dimitris Tsirogiannis 
Tested-by: Impala Public Jenkins
---
M testdata/bin/create-load-data.sh
1 file changed, 4 insertions(+), 3 deletions(-)

Approvals:
  Dimitris Tsirogiannis: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7e93ee5a77ec9271b980b88bef7ad512ecbe0407
Gerrit-Change-Number: 8822
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger

[Impala-ASF-CR] IMPALA-6284: Mark the intermediate decimal avg struct as packed

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8836


Change subject: IMPALA-6284: Mark the intermediate decimal avg struct as packed
..

IMPALA-6284: Mark the intermediate decimal avg struct as packed

We saw some failures on the exhaustive release build because the
compiler assumed that the pointer to the intermediate struct that is
used for computing decimal average was aligned.

To fix the problem, we mark the struct with a "packed" attribute so
that the compiler does not expect it to be aligned.

Testing:
- Ran the failing test locally on an release build and it passed.

Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
---
M be/src/exprs/aggregate-functions-ir.cc
1 file changed, 5 insertions(+), 1 deletion(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Gerrit-Change-Number: 8836
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky

[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata

2017-12-13 Thread Tianyi Wang (Code Review)

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

Change subject: IMPALA-5990: End-to-end compression of metadata
..

IMPALA-5990: End-to-end compression of metadata

Currently the catalog data is compressed in the statestore, but
uncompressed when passed between FE and BE. It results in a ~2GB limit
on the metadata. IMPALA-3499 introduced a workaround in the impalad but
there isn't one in the catalogd. This patch aims to increase the size
limit for statestore updates, reduce the copying of the metadata and
reduce the memory footprint. With this patch, the catalog objects are
passed and (de)compressed between FE and BE one at a time. The new
limits are:
- A single catalog object cannot be larger than ~2GB on openjdk 8. It
cannot be larger than ~1GB on openjdk 7.
- A statestore catalog update cannot be larger than ~4GB, compressed.
The behavior of the catalog op executer is not changed. The data is not
compressed and the size limit is still 2GB.

Testing: Ran existing tests. Manually tested with a 1.95GB catalog object
and a 3.90 GB uncompressed statestore update.

Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/rpc/thrift-util.h
M be/src/service/fe-support.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-server.cc
M be/src/util/jni-util.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
A fe/src/main/java/org/apache/impala/util/TByteBuffer.java
22 files changed, 576 insertions(+), 553 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Tianyi Wang

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..


Patch Set 7: Code-Review+1

lgtm, thanks for the changes!

alex, please take a look.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 14 Dec 2017 01:54:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

Hello John Russell, Alex Behm, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..

IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

Testing:
Add unit tests to ParserTest#TestPlanHints
Add plan check tests to PlannerTest#testInsert, PlannerTest#testKuduUpsert
Add tests to ToSqlTest#planHintsTest

Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
7 files changed, 242 insertions(+), 84 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8676/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8676/5//COMMIT_MSG@12
PS5, Line 12: rebuild
> omit "query rebuild"
Done


http://gerrit.cloudera.org:8080/#/c/8676/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:

http://gerrit.cloudera.org:8080/#/c/8676/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@56
PS5, Line 56: AtStatement
> argh... I was inconsistent with my suggestion and see it here now. I have S
I see. I prefer Start/End. It's more intuitive. I think the query 
statement(e.g. INSERT ... SELECT ...) consists of two statements: INSERT 
statement + SELECT statement. We can specify a hint either at the begin of or 
at the end of INSERT statement. AtStatement/AtQuery are unclear to me.


http://gerrit.cloudera.org:8080/#/c/8676/5/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

http://gerrit.cloudera.org:8080/#/c/8676/5/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@310
PS5, Line 310: actualHints, String... expectedHints) {
 : List actualHints = Lists.newArrayList();
 : for (PlanHint hint: actualHints) actualHints.add
> there's some shadowing here (actualHints is a parameter and local variable)
I recognized the mistake and pushed a fix on the patch set#6. In the last 
change, the parameter name was actualPlanHints to avoid the name conflict, but 
let me accept your suggestion.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 14 Dec 2017 01:50:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6300: Fix decimal modulo overflow

Taras Bobrovytsky has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8833


Change subject: IMPALA-6300: Fix decimal modulo overflow
..

IMPALA-6300: Fix decimal modulo overflow

In order to compute the modulo of two decimals, we need to bring the
underlying datatype to the same scale first. It turns out we could
overflow when scaling up one of the values.

In this patch we fix the problem by using a larger data type when we
detect that the scaled up value will not fit into the original data
type.

Testing:
- Added some expr tests that reproduce the issue.

Change-Id: I27c7f25f68353c19c315e1639311ec06b2dea686
---
M be/src/exprs/expr-test.cc
M be/src/runtime/decimal-value.inline.h
M be/src/util/bit-util.h
3 files changed, 135 insertions(+), 49 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I27c7f25f68353c19c315e1639311ec06b2dea686
Gerrit-Change-Number: 8833
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky

[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata

2017-12-13 Thread Tianyi Wang (Code Review)

Tianyi Wang has restored this change. ( http://gerrit.cloudera.org:8080/8825 )

Change subject: IMPALA-5990: End-to-end compression of metadata
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: restore
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Tianyi Wang

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..


Patch Set 6:

(3 comments)

several more follow-up comments and we should be good. thx!

http://gerrit.cloudera.org:8080/#/c/8676/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8676/5//COMMIT_MSG@12
PS5, Line 12: rebuild
omit "query rebuild"


http://gerrit.cloudera.org:8080/#/c/8676/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:

http://gerrit.cloudera.org:8080/#/c/8676/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@56
PS5, Line 56: AtStatement
argh... I was inconsistent with my suggestion and see it here now. I have 
Start/End vs. AtStatement/AtQuery. I have a slight preference for the latter 
since its more precise. I'll let you decide that one. But it should be 
consistent.


http://gerrit.cloudera.org:8080/#/c/8676/5/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

http://gerrit.cloudera.org:8080/#/c/8676/5/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@310
PS5, Line 310: actualPlanHints, String... expectedHints) {
 : List actualHints = Lists.newArrayList();
 : for (PlanHint hint: actualPlanHints) actualHints
there's some shadowing here (actualHints is a parameter and local variable)-- 
does it do what you intend? perhaps to make it clearer, call the local one 
"stringHints".



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 14 Dec 2017 01:33:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

Hello John Russell, Alex Behm, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..

IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

Testing:
Add unit tests to ParserTest#TestPlanHints
Add plan check tests to PlannerTest#testInsert, PlannerTest#testKuduUpsert
Add query rebuild tests to ToSqlTest#planHintsTest

Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
7 files changed, 241 insertions(+), 83 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-5315: Cast to timestamp fails for YYYY-M-D format

Hello Lars Volker, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-5315: Cast to timestamp fails for -M-D format
..

IMPALA-5315: Cast to timestamp fails for -M-D format

This change allows casting of a string in 'lazy' date
format to timestamp. The supported lazy date formats are:
  -[M]M-[d]d
  -[M]M-[d]d [H]H:[m]m:[s]s[.S]
  -[M]M-[d]dT[H]H:[m]m:[s]s[.S]

We will also take a variety of separators (including "-",
"/", "T", "Z", ":")

The parse logic will:
1) Try to use the default template timestamp formats first since it is
more performant.
2) Try to reuse the previously generated lazy
timestamp format (if exists) to maintain comparable SCAN performance if
all values in column have the same timestamp format.
3) Discover the new template format if both prior paths fail.

In the worst case, where we have to generate a new timestamp format for
each value in a column (columns with variable timestamp format), we will
incur a SCAN performance penalty (approximately 1/2
TotalReadThroughput).

Finally, this change also moves the following two functions to be
adjacent to each other for readability:

Parse(const char* str, int len, boost::gregorian::date* d,
  boost::posix_time::time_duration* t);

Parse(const char* str, int len, const DateTimeFormatContext& dt_ctx,
  boost::gregorian::date* d, boost::posix_time::time_duration* t);
Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
3 files changed, 152 insertions(+), 29 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
Gerrit-Change-Number: 7009
Gerrit-PatchSet: 10
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran

[Impala-ASF-CR] IMPALA-4993: extend dictionary filtering to collections

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

Change subject: IMPALA-4993: extend dictionary filtering to collections
..


Patch Set 6: Code-Review+1

Carry +1 from Lars


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd
Gerrit-Change-Number: 8775
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 14 Dec 2017 01:08:22 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4993: extend dictionary filtering to collections

Hello Lars Volker,

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

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

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

Change subject: IMPALA-4993: extend dictionary filtering to collections
..

IMPALA-4993: extend dictionary filtering to collections

Currently, top-level scalar columns in parquet files can
be used at runtime to prune row-groups by evaluating certain
conjuncts over the column's dictionary (if available).

This change extends such pruning to scalar values that are
stored in collection type columns. Currently, dictionary
pruning works by finding eligible conjuncts for top-level
slots. Since only top-level slots are supported, the slots
are implicitly part of the scan node's tuple descriptor.
With this change, we track eligible conjuncts by slot as well
as the tuple that contains the slot (either top-level or
nested collection). Since collection conjuncts are already
managed by a map that associates tuple descriptors to a list
of their conjuncts, this extension follows the existing
representation.

The frontend builds the mapping of  to
conjuncts that are dictionary filterable. The backend is
adjusted to use the same representation. In addition, collection
readers are decomposed into scalar filterable columns and
other, non-dictionary filterable readers. When filtering
a row group using a conjunct associated to a (possibly)
nested collection type, an additional tuple buffer is
allocated per tuple descriptor.

Testing:
- e2e test extended to illustrate row-groups that are pruned
  by nested collection dictionary filters.

Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/runtime/collection-value-builder.h
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test
13 files changed, 652 insertions(+), 201 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd
Gerrit-Change-Number: 8775
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-4993: extend dictionary filtering to collections

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

Change subject: IMPALA-4993: extend dictionary filtering to collections
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8775/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8775/5/be/src/exec/hdfs-parquet-scanner.cc@799
PS5, Line 799:   back_inserter(dict_filterable_readers_));
> nit: We usually omit the std:: in cc files. Also these (and other places be
done. found some other uses so made this file consistent.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd
Gerrit-Change-Number: 8775
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 14 Dec 2017 01:06:24 +
Gerrit-HasComments: Yes

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

Philip Zeyliger 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
> The exception is "Incorrect padding". I only see that when I run a specific
This smells like a bug to me. Are we returning something invalid?



--
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:47:26 +
Gerrit-HasComments: Yes

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

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

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.

Philip Zeyliger 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)

One last question.

Yes, go ahead and do the rebase with the next patch set.

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
> I've seen this fail like below:
What's the exception? What's the profile that it failed to read? Is there a bug?

(Basically, if we're seeing an invalid profile via this code path, perhaps a 
tool could see an invalid profile, and we shouldn't be emitting invalid code 
paths.)



--
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:23:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

Hello John Russell, Andre Araujo, Zoltan Borok-Nagy, Tim Armstrong,

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

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

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

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..

IMPALA-4664: Unexpected string conversion in Shell

Impala shell can accidentally convert certain
literal strings to lowercase. Impala shell splits
each command into tokens and then converts the
first token to lowercase to figure out how it
should execute the command. The splitting is done
by spaces only. Thus, if the user types a TAB
after the SELECT, the first token after the split
becomes the SELECT plus whatever comes after it.

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
---
M LICENSE.txt
M shell/impala_shell.py
A tests/shell/shell_case_sensitive.cmds
A tests/shell/shell_case_sensitive2.cmds
M tests/shell/test_shell_interactive.py
5 files changed, 112 insertions(+), 49 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 11
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

Kim Jin Chul has abandoned this change. ( http://gerrit.cloudera.org:8080/8639 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Abandoned

The problem in IMPALA-4664 should be also resolved by the change: 
https://gerrit.cloudera.org/#/c/8762
--
To view, visit http://gerrit.cloudera.org:8080/8639
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
Gerrit-Change-Number: 8639
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden

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

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:

BTW, gerrit says there's going to be a merge conflict. Should I resolve that 
first and then push the patch 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:10:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2640: Make a given command case-sensitive

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

Change subject: IMPALA-2640: Make a given command case-sensitive
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8762/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8762/6//COMMIT_MSG@7
PS6, Line 7: IMPALA-2640: Make a given command case-sensitive
> I think the lower-case column description is intended.
Sure. The both of IMPALA-2640 and IMPALA-4664 should be resolved by this 
change. Now we don't need the change: https://gerrit.cloudera.org/#/c/8639/. 
Let me abandon it.


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@555
PS9, Line 555:
> We still need to add a line in LICENSE.txt like:
Done


http://gerrit.cloudera.org:8080/#/c/8762/10/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8762/10/shell/impala_shell.py@569
PS10, Line 569: do_' + cmd_.lower())
> nit: maybe you could write "This code is based on the code from the standar
Thanks. It's more clear!



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 00:11:37 +
Gerrit-HasComments: Yes

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

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: 
-- 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-5315: Cast to timestamp fails for YYYY-M-D format

Hello Lars Volker, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-5315: Cast to timestamp fails for -M-D format
..

IMPALA-5315: Cast to timestamp fails for -M-D format

This change allows casting of a string in 'lazy' date
format to timestamp. The supported lazy date formats are:
  -[M]M-[d]d
  -[M]M-[d]d [H]H:[m]m:[s]s[.S]
  -[M]M-[d]dT[H]H:[m]m:[s]s[.S]

We will also take a variety of separators (including "-",
"/", "T", "Z", ":")

The parse logic will:
1) Try to use the default template timestamp formats first since it is
more performant.
2) Try to reuse the previously generated lazy
timestamp format (if exists) to maintain comparable SCAN performance if
all values in column have the same timestamp format.
3) Discover the new template format if both prior paths fail.

In the worst case, where we have to generate a new timestamp format for
each value in a column (columns with variable timestamp format), we will
incur a SCAN performance penalty (approximately 1/2
TotalReadThroughput).

Finally, this change also moves the following two functions to be
adjacent to each other for readability:

Parse(const char* str, int len, boost::gregorian::date* d,
  boost::posix_time::time_duration* t);

Parse(const char* str, int len, const DateTimeFormatContext& dt_ctx,
  boost::gregorian::date* d, boost::posix_time::time_duration* t);

Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
3 files changed, 199 insertions(+), 71 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
Gerrit-Change-Number: 7009
Gerrit-PatchSet: 9
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran

[Impala-ASF-CR] IMPALA-5315: Cast to timestamp fails for YYYY-M-D format

Vincent Tran has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7009 )

Change subject: IMPALA-5315: Cast to timestamp fails for -M-D format
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7009/5/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:

http://gerrit.cloudera.org:8080/#/c/7009/5/be/src/runtime/timestamp-parse-util.cc@294
PS5, Line 294: *t = 
boost::posix_time::time_duration(boost::posix_time::not_a_date_time);
> This should use a stringstream, otherwise each append may result in a copy
Refactored this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
Gerrit-Change-Number: 7009
Gerrit-PatchSet: 8
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran 
Gerrit-Comment-Date: Thu, 14 Dec 2017 00:00:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4993: extend dictionary filtering to collections

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

Change subject: IMPALA-4993: extend dictionary filtering to collections
..


Patch Set 5: Code-Review+1

(2 comments)

LGTM.

http://gerrit.cloudera.org:8080/#/c/8775/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8775/5/be/src/exec/hdfs-parquet-scanner.cc@799
PS5, Line 799:   back_inserter(dict_filterable_readers_));
nit: We usually omit the std:: in cc files. Also these (and other places below) 
should probably follow our indentation standard.


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

http://gerrit.cloudera.org:8080/#/c/8775/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1099
PS4, Line 1099: output.append(String.format("%sparquet statistics 
predicates: %s\n",
> sure, and found one more place.
Thanks



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd
Gerrit-Change-Number: 8775
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 13 Dec 2017 23:59:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5315: Cast to timestamp fails for YYYY-M-D format

Hello Lars Volker, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-5315: Cast to timestamp fails for -M-D format
..

IMPALA-5315: Cast to timestamp fails for -M-D format

This change allows casting of a string in 'lazy' date
format to timestamp. The supported lazy date formats are:
  -[M]M-[d]d
  -[M]M-[d]d [H]H:[m]m:[s]s[.S]
  -[M]M-[d]dT[H]H:[m]m:[s]s[.S]

We will also take a variety of separators (including "-",
"/", "T", "Z", ":")

The parse logic will:
1) Try to use the default template timestamp formats first since it is
more performant.
2) Try to reuse the previously generated lazy
timestamp format (if exists) to maintain comparable SCAN performance if
all values in column have the same timestamp format.
3) Discover the new template format if both prior paths fail.

In the worst case, where we have to generate a new timestamp format for
each value in a column (collumns withvariable timestamp format), we
will incur a SCAN performance decrease (approximately 1/2
TotalReadThroughput).

Finally, this change also moves the following two functions to be
adjacent to each other for readability:

Parse(const char* str, int len, boost::gregorian::date* d,
  boost::posix_time::time_duration* t);

Parse(const char* str, int len, const DateTimeFormatContext& dt_ctx,
  boost::gregorian::date* d, boost::posix_time::time_duration* t);

Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
3 files changed, 199 insertions(+), 71 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
Gerrit-Change-Number: 7009
Gerrit-PatchSet: 8
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran

[Impala-ASF-CR] IMPALA-5315: Cast to timestamp fails for YYYY-M-D format

Hello Lars Volker, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-5315: Cast to timestamp fails for -M-D format
..

IMPALA-5315: Cast to timestamp fails for -M-D format

This change allows casting of a string in 'lazy' date
format to timestamp. The supported lazy date formats are:
  -[M]M-[d]d
  -[M]M-[d]d [H]H:[m]m:[s]s[.S]
  -[M]M-[d]dT[H]H:[m]m:[s]s[.S]

We will also take a variety of separators (including "-",
"/", "T", "Z", ":")

The parse logic will:
1) Try to use the default template timestamp formats
first since it is more performant.
2) Try to reuse the previously generated lazy
timestamp format (if exists) to maintain comparable
SCAN performance if all values in column have the
same timestamp format.
3) Discover the new template format if both prior paths
fail.

In the worst case, where we have to generate a new
timestamp format for each value in a column (collumns with
variable timestamp format), we will incur a SCAN performance
decrease (apprimately 1/2 TotalReadThroughput).

Finally, this change also moves the following
two functions to be adjacent to each other:

Parse(const char* str, int len, boost::gregorian::date* d,
  boost::posix_time::time_duration* t);

Parse(const char* str, int len, const DateTimeFormatContext& dt_ctx,
  boost::gregorian::date* d, boost::posix_time::time_duration* t);

Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
3 files changed, 199 insertions(+), 71 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
Gerrit-Change-Number: 7009
Gerrit-PatchSet: 7
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran

[Impala-ASF-CR] IMPALA-5931: Generates scan ranges in planner for s3/adls (wip)

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

Change subject: IMPALA-5931: Generates scan ranges in planner for s3/adls (wip)
..


Patch Set 2:

getting back to this one... re-worked the expansion to happen at the backend 
coordinator instead of in the frontend planner.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I326065adbb2f7e632814113aae85cb51ca4779a5
Gerrit-Change-Number: 8523
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 13 Dec 2017 23:43:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5931: Generates scan ranges in planner for s3/adls (wip)

Vuk Ercegovac has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8523 )

Change subject: IMPALA-5931: Generates scan ranges in planner for s3/adls (wip)
..

IMPALA-5931: Generates scan ranges in planner for s3/adls (wip)

Currently, for filesystems that do not include physical
block information (e.g., block replica locations, caching),
synthetic blocks are generated and stored in the catalog
when metadata is loaded. Example file systems for which this is done
includes S3, ADLS, and local fs.

This change avoids generating these blocks when metadata is loaded.
Instead, scan ranges are directly generated from such files by the
backend coordinator. Previously, all scan ranges were produced by
the planner in HDFSScanNode in the frontend. Now, those files without
block information are sent to the coordinator represented by a split
specification that determines how the coordinator will create scan ranges
to send to executors.

This change reduces the space needed in the catalog and reduces the
scan range datastructures that are passed from the frontend to the
backend when planning and coordinating a query.
In addition a bug is avoided where non-splittable files were being
split anyways to support the query parameter that places a limit on
scan ranges.

The "wip" status is currently to add/run more tests.

Testing:
- local filesystem tests exercise this code path
- manually tried larger local filesystem tables (tpch) with multiple
  partitions and observed the same scan ranges.
  - TODO: s3 and adls testing

Change-Id: I326065adbb2f7e632814113aae85cb51ca4779a5
---
M be/src/scheduling/scheduler.cc
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
5 files changed, 282 insertions(+), 165 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I326065adbb2f7e632814113aae85cb51ca4779a5
Gerrit-Change-Number: 8523
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..


Patch Set 4:

(19 comments)

Thanks a lot for the kind review. I can feel the improvement of code quality!

http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/cup/sql-parser.cup@717
PS4, Line 717: Please beware of that the both of Oracle and default hint styles 
are supported when
 : // extending INSERT/UPSERT syntax.
> Replace with: "Note: when extending INSERT/UPSERT syntax, hinting is suppor
Thanks, it's more clear.


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/cup/sql-parser.cup@807
PS4, Line 807: Please beware of that the both of Oracle and default hint styles 
are supported when
 : // extending INSERT/UPSERT syntax.
> make this consistent with the proposal on L717.
Done


http://gerrit.cloudera.org:8080/#/c/8676/4/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/8676/4/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java@86
PS4, Line 86: InsertStmt.HintLocation.DefaultLoc
> more intuitive to pass in null here (what does DefaultLoc for no hints mean
You're right. null is looks more intuitive.


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:

http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@56
PS4, Line 56:   // A flag to determine the location of a hint
> This is a good place to describe the alternatives, including examples. Also
The values of the enumeration are more readable and intuitive. Thanks.


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@210
PS4, Line 210: hintLoc_ = hintLoc;
> can a caller pass in null? how about we pick the default (current placement
Yes, I agree on your idea.


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@908
PS4, Line 908: hintLoc_ == HintLocation.OracleLoc && !planHints_.isEmpty()
> I would flip these tests around (test for whether we have hints, then test
Done


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@303
PS4, Line 303: BuildQueryStmt
> more specific: rename to InjectInsertHint
Done


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@310
PS4, Line 310: hints
> rename to actualHints
Done


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@310
PS4, Line 310: TestHints
> rename to VerifyHints
Done


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@311
PS4, Line 311: actualHints
> rename to actualStringHints
Done


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@318
PS4, Line 318: Parses stmt and checks that the insert hints stmt are the 
expected hints.
> How about:
Thanks for the suggestion. It's more clear!


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@320
PS4, Line 320: TestInsertHints
> rename this to: TestInsertStmtHints... our InsertStmt covers both inserts a
Done


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@327
PS4, Line 327: /**
 :* Parses stmt and checks that the upsert hints stmt are the 
expected hints.
 :*/
 :   private void TestUpsertHints(String pattern, String hint, 
String... expectedHints) {
 : TestInsertHints(pattern, hint, expectedHints);
 :   }
> remove-- not needed.
Done


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:

http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@505
PS4, Line 505: BuildQueryStmt
> for consistency with the proposed change in the parse test: InjectInsertHin
Done


http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/workloads/functional-planner/queries/PlannerTest/insert.test
File testdata/workloads/functional-planner/queries/PlannerTest/insert.test:

http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/wor

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

Hello John Russell, Alex Behm, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..

IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

Testing:
Add unit tests to ParserTest#TestPlanHints
Add plan check tests to PlannerTest#testInsert, PlannerTest#testKuduUpsert
Add query rebuild tests to ToSqlTest#planHintsTest

Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
7 files changed, 241 insertions(+), 83 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-2640: Make a given command case-sensitive

2017-12-13 Thread Tim Armstrong (Code Review)

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

Change subject: IMPALA-2640: Make a given command case-sensitive
..


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8762/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8762/6//COMMIT_MSG@7
PS6, Line 7: IMPALA-2640: Make a given command case-sensitive
> In the fix of IMPALA-4664(https://gerrit.cloudera.org/#/c/8639/4/shell/impa
I think the lower-case column description is intended.

The tests you added in 
https://gerrit.cloudera.org/#/c/8639/4/tests/shell/test_shell_interactive.py 
all appear to be fixed by this patch set, so I think we should add those tests 
to this patch.


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@555
PS9, Line 555:
> I added some comment for the coping. The PSF license is already included in
We still need to add a line in LICENSE.txt like:

  Parts of shell/impala_shell.py: Python Software License V2



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 23:09:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6301: Fix test failures when username or group name contains dots

2017-12-13 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-6301: Fix test failures when username or group name 
contains dots
..


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8ae15bb6a929dc48d3ad2176c8b3fafff87f32b
Gerrit-Change-Number: 8807
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Wed, 13 Dec 2017 23:06:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6301: Fix test failures when username or group name contains dots

2017-12-13 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8807 )

Change subject: IMPALA-6301: Fix test failures when username or group name 
contains dots
..

IMPALA-6301: Fix test failures when username or group name contains dots

Some tests use the local user's group name to construct SQLs, which may
lead to syntax errors when group name contains dots. We need to quote
the group names in SQL to avoid this error. Besides, a test in
test_admission_controller uses '\w+' to match the local user name. This
expression cannot match usernames with dots, which causes test failure
as well. Instead, we should use '\S+'.

Change-Id: Ib8ae15bb6a929dc48d3ad2176c8b3fafff87f32b
Reviewed-on: http://gerrit.cloudera.org:8080/8807
Reviewed-by: Thomas Tauber-Marshall 
Reviewed-by: Michael Ho 
Tested-by: Impala Public Jenkins
---
M testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test
M testdata/workloads/functional-query/queries/QueryTest/grant_revoke_kudu.test
M tests/authorization/test_grant_revoke.py
M tests/custom_cluster/test_admission_controller.py
4 files changed, 31 insertions(+), 31 deletions(-)

Approvals:
  Thomas Tauber-Marshall: Looks good to me, but someone else must approve
  Michael Ho: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib8ae15bb6a929dc48d3ad2176c8b3fafff87f32b
Gerrit-Change-Number: 8807
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Thomas Tauber-Marshall

[Impala-ASF-CR] IMPALA-6070: Parallelize another bit of data load.

2017-12-13 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-6070: Parallelize another bit of data load.
..


Patch Set 2:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e93ee5a77ec9271b980b88bef7ad512ecbe0407
Gerrit-Change-Number: 8822
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 13 Dec 2017 23:05:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6070: Parallelize another bit of data load.

2017-12-13 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8822 )

Change subject: IMPALA-6070: Parallelize another bit of data load.
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e93ee5a77ec9271b980b88bef7ad512ecbe0407
Gerrit-Change-Number: 8822
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 13 Dec 2017 23:04:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6070: Parallelize another bit of data load.

2017-12-13 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8822 )

Change subject: IMPALA-6070: Parallelize another bit of data load.
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e93ee5a77ec9271b980b88bef7ad512ecbe0407
Gerrit-Change-Number: 8822
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 13 Dec 2017 23:04:00 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6222: Add details to error msg on failure to get min reservation

2017-12-13 Thread Tim Armstrong (Code Review)

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

Change subject: IMPALA-6222: Add details to error msg on failure to get min 
reservation
..


Patch Set 3: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8781/3/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:

http://gerrit.cloudera.org:8080/#/c/8781/3/be/src/runtime/mem-tracker.cc@324
PS3, Line 324:   std::priority_queue, 
vector>,
nit: add a using up the top of the file or in common/names.h instead of using 
the std:: prefixes.


http://gerrit.cloudera.org:8080/#/c/8781/3/be/src/runtime/mem-tracker.cc@340
PS3, Line 340: int limit) {
nit: weird wrapping.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74
Gerrit-Change-Number: 8781
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 13 Dec 2017 22:51:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6270: remove redundant version properties

2017-12-13 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-6270: remove redundant version properties
..


Patch Set 1: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6812e11bb41716450ef29bb523773479e9f76eec
Gerrit-Change-Number: 8827
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 13 Dec 2017 22:48:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6270: remove redundant version properties

2017-12-13 Thread Impala Public Jenkins (Code Review)

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8827 )

Change subject: IMPALA-6270: remove redundant version properties
..

IMPALA-6270: remove redundant version properties

Removes properties that are already defined in the impala-parent pom.

I ran the tests.

Change-Id: I6812e11bb41716450ef29bb523773479e9f76eec
Reviewed-on: http://gerrit.cloudera.org:8080/8827
Reviewed-by: Zach Amsden 
Tested-by: Impala Public Jenkins
---
M tests/test-hive-udfs/pom.xml
1 file changed, 0 insertions(+), 2 deletions(-)

Approvals:
  Zach Amsden: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6812e11bb41716450ef29bb523773479e9f76eec
Gerrit-Change-Number: 8827
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zach Amsden

[Impala-ASF-CR] [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web UI.

2017-12-13 Thread Bharath Vissapragada (Code Review)

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

Change subject: [PREVIEW] IMPALA-4886: Expose table metrics in the catalog web 
UI.
..


Patch Set 3:

(20 comments)

The patch mostly lgtm. I have some minor suggestions before I can +1.

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

http://gerrit.cloudera.org:8080/#/c/8529/3//COMMIT_MSG@7
PS3, Line 7: [PREVIEW]
Remove?


http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.cc@359
PS2, Line 359:   GetCatalogUsage(document);
> In principal, that's a good idea. I plan on doing it in the future for more
Makes sense. If you don't mind, can you please raise jiras for these to-dos so 
that we don't lose track of them. Also, they sound like great "ramp-up" tasks, 
that others can pick up.


http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog-server.cc@492
PS2, Line 492:   // TODO: Enable json view of table metrics
> Good idea. Left a TODO.
I think thats a one-liner looking at other places in the code.

document->AddMember(Webserver::ENABLE_RAW_JSON_KEY, true, 
document->GetAllocator());


http://gerrit.cloudera.org:8080/#/c/8529/3/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

http://gerrit.cloudera.org:8080/#/c/8529/3/be/src/catalog/catalog-server.cc@359
PS3, Line 359:   GetCatalogUsage(document);
Check the return value and return early if it threw a failure?

if (!GetCatalogUsage().ok()) return;


http://gerrit.cloudera.org:8080/#/c/8529/3/be/src/catalog/catalog-server.cc@516
PS3, Line 516: object_name
name?


http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:

http://gerrit.cloudera.org:8080/#/c/8529/2/be/src/catalog/catalog.h@96
PS2, Line 96:   Status GetCatalogUsage(TGetCatalogUsageResponse* response);
> I renamed the other one to GetCatalogUsage. I thought of keeping it a bit m
Agree.


http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java
File fe/src/main/java/org/apache/impala/catalog/Catalog.java:

http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@166
PS3, Line 166: if (tbl != null) 
CatalogUsageMonitor.INSTANCE.removeTable(tbl);
I think this is only executed on the Catalog service side and it looks to me 
like the CatalogUsageMonitor is populated on the coordinators too. May be we 
should just restrict it to the catalogd?


http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java
File fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java:

http://gerrit.cloudera.org:8080/#/c/8529/2/fe/src/main/java/org/apache/impala/catalog/CatalogUsageMonitor.java@47
PS2, Line 47: true
> I wanted to avoid the scenario where 25 tables that were accessed in the pa
Oh, I see your point. It can be argued both ways.


http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
File fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java:

http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java@934
PS3, Line 934: 
thriftHdfsPart.setHas_incremental_stats(hasIncrementalStats());
IMO, we should also add the incremental stats size estimate by computing the 
obj sizes of the params map (if incr stats are present of course).


http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@196
PS3, Line 196:
..any of the partitions in.. ?


http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1720
PS3, Line 1720: metadataSizeEstimate
may be memUsageEstimate to be inline with CatalogUsage..terminology?


http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@1743
PS3, Line 1743:  stats.numBlocks += tHdfsPartition.getNum_blocks();
  :   stats.numFiles +=
  :   tHdfsPartition.isSetFile_desc() ? 
tHdfsPartition.getFile_desc().size() : 0;
  :   stats.totalFileBytes += 
tHdfsPartition.getTotal_file_size_bytes();
Shouldn't these be populated irrespective of "includeFileDesc" since they 
account for memory usage?


http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:

http://gerrit.cloudera.org:8080/#/c/8529/3/fe/src/main/java/org/apache/impala/catalog/Table.java@151
PS3, Line 151:   public Metrics getMetri

[Impala-ASF-CR] IMPALA-6070: Parallelize another bit of data load.

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

Change subject: IMPALA-6070: Parallelize another bit of data load.
..


Patch Set 1:

https://jenkins.impala.io/view/Utility/job/pre-review-test/87/ passed just fine.

The logs from 
https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/798/consoleFull look 
fine to me.

00:53:42   Creating many block table OK (Took: 0 min 23 sec)
00:53:42 Started Loading Kudu functional in background; pid 19994.
00:53:42 Loading Kudu functional (logging to 
/home/ubuntu/Impala/logs/data_loading/load-kudu.log)...
00:53:42 Started Loading Kudu TPCH in background; pid 19995.
00:53:42 Loading Kudu TPCH (logging to 
/home/ubuntu/Impala/logs/data_loading/load-kudu-tpch.log)...
00:53:42 Started Loading Hive UDFs in background; pid 19996.
00:53:42 Loading Hive UDFs (logging to 
/home/ubuntu/Impala/logs/data_loading/build-and-copy-hive-udfs.log)...
00:54:24   Loading Hive UDFs OK (Took: 0 min 42 sec)
00:56:56   Loading workload 'functional-query' using exploration strategy 
'core' in table formats 'kudu/none/none' OK (Took: 3 min 14 sec)
00:57:38   Loading workload 'tpch' using exploration strategy 'core' in table 
formats 'kudu/none/none' OK (Took: 3 min 56 sec)
00:57:38 Running custom post-load steps (logging to 
/home/ubuntu/Impala/logs/data_loading/custom-post-load-steps.log)...


Comparing to some arbitrary other run of this job 
(https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/796/consoleFull; see 
below) this seems to save 4 minutes. The overall run time change here is still 
within the run-to-run noise, but I think this will move the baseline down 3-4 
minutes.

19:26:09 Loading Kudu functional (logging to 
/home/ubuntu/Impala/logs/data_loading/load-kudu.log)...
19:29:38   Loading workload 'functional-query' using exploration strategy 
'core' in table formats 'kudu/none/none' OK (Took: 3 min 29 sec)
19:29:38 Loading Kudu TPCH (logging to 
/home/ubuntu/Impala/logs/data_loading/load-kudu-tpch.log)...
19:33:38   Loading workload 'tpch' using exploration strategy 'core' in table 
formats 'kudu/none/none' OK (Took: 4 min 0 sec)
19:33:38 Loading Hive UDFs (logging to 
/home/ubuntu/Impala/logs/data_loading/build-and-copy-hive-udfs.log)...
19:34:19   Loading Hive UDFs OK (Took: 0 min 41 sec)
19:34:19 Running custom post-load steps (logging to 
/home/ubuntu/Impala/logs/data_loading/custom-post-load-steps.log)...


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e93ee5a77ec9271b980b88bef7ad512ecbe0407
Gerrit-Change-Number: 8822
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 13 Dec 2017 22:31:32 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6070: Parallelize another bit of data load.

Philip Zeyliger has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8822


Change subject: IMPALA-6070: Parallelize another bit of data load.
..

IMPALA-6070: Parallelize another bit of data load.

The two Kudu loads and Hive UDFs can all run in parallel. This
should shave about 4 minutes off of the data load. (Current
timings are 3.5, 4, and 0.6 minutes, see below.)

I've run dataload with this change many times.

   Loading Kudu functional (logging to 
/home/ubuntu/Impala/logs/data_loading/load-kudu.log)...
 Loading workload 'functional-query' using exploration strategy 'core' in 
table formats 'kudu/none/none' OK (Took: 3 min 29 sec)
   Loading Kudu TPCH (logging to 
/home/ubuntu/Impala/logs/data_loading/load-kudu-tpch.log)...
 Loading workload 'tpch' using exploration strategy 'core' in table formats 
'kudu/none/none' OK (Took: 4 min 0 sec)
   Loading Hive UDFs (logging to 
/home/ubuntu/Impala/logs/data_loading/build-and-copy-hive-udfs.log)...
 Loading Hive UDFs OK (Took: 0 min 41 sec)

Change-Id: I7e93ee5a77ec9271b980b88bef7ad512ecbe0407
---
M testdata/bin/create-load-data.sh
1 file changed, 4 insertions(+), 3 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7e93ee5a77ec9271b980b88bef7ad512ecbe0407
Gerrit-Change-Number: 8822
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Philip Zeyliger

[Impala-ASF-CR] IMPALA-5191: Standardize column alias behavior

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

Change subject: IMPALA-5191: Standardize column alias behavior
..


Patch Set 5:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8801/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@817
PS5, Line 817:  boolean preserveRootType, boolean substituteChildren
> I find it a bit hard to read code calling methods with multiple boolean arg
Agreed. I like trySubstituteRecursive() and trySubstituteRootOnly()



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 22:26:36 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5191: Standardize column alias behavior

2017-12-13 Thread Tim Armstrong (Code Review)

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

Change subject: IMPALA-5191: Standardize column alias behavior
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8801/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8801/5//COMMIT_MSG@14
PS5, Line 14: SELECT int_col / 2 AS x
Do we have existing end-to-end tests for queries of these forms? Would be good 
to confirm that the frontend produces plans that are executable and correct.


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

http://gerrit.cloudera.org:8080/#/c/8801/5/fe/src/main/java/org/apache/impala/analysis/Expr.java@817
PS5, Line 817:  boolean preserveRootType, boolean substituteChildren
I find it a bit hard to read code calling methods with multiple boolean 
arguments since it's easy to get the flags mixed up at the callsite.

I'm ok with this as-is, but we could consider adding public methods instead of 
adding flags. E.g. trySubsitute() vs trySubsituteRoot() or 
trySubstituteRecursive() vs trySubstituteRoot().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 21:50:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4993: extend dictionary filtering to collections

Hello Lars Volker,

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

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

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

Change subject: IMPALA-4993: extend dictionary filtering to collections
..

IMPALA-4993: extend dictionary filtering to collections

Currently, top-level scalar columns in parquet files can
be used at runtime to prune row-groups by evaluating certain
conjuncts over the column's dictionary (if available).

This change extends such pruning to scalar values that are
stored in collection type columns. Currently, dictionary
pruning works by finding eligible conjuncts for top-level
slots. Since only top-level slots are supported, the slots
are implicitly part of the scan node's tuple descriptor.
With this change, we track eligible conjuncts by slot as well
as the tuple that contains the slot (either top-level or
nested collection). Since collection conjuncts are already
managed by a map that associates tuple descriptors to a list
of their conjuncts, this extension follows the existing
representation.

The frontend builds the mapping of  to
conjuncts that are dictionary filterable. The backend is
adjusted to use the same representation. In addition, collection
readers are decomposed into scalar filterable columns and
other, non-dictionary filterable readers. When filtering
a row group using a conjunct associated to a (possibly)
nested collection type, an additional tuple buffer is
allocated per tuple descriptor.

Testing:
- e2e test extended to illustrate row-groups that are pruned
  by nested collection dictionary filters.

Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/runtime/collection-value-builder.h
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test
13 files changed, 650 insertions(+), 198 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd
Gerrit-Change-Number: 8775
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-4993: extend dictionary filtering to collections

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

Change subject: IMPALA-4993: extend dictionary filtering to collections
..


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8775/4/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:

http://gerrit.cloudera.org:8080/#/c/8775/4/be/src/exec/hdfs-scan-node-base.h@90
PS4, Line 90:   /// sequence-based file
> This looks like a rebase artifact. I often look at diffs between patchsets
ok, I think I ran into a conflict so wanted to merge it.


http://gerrit.cloudera.org:8080/#/c/8775/4/be/src/exec/hdfs-scanner.cc
File be/src/exec/hdfs-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8775/4/be/src/exec/hdfs-scanner.cc@90
PS4, Line 90: std::
> We usually drop the std:: prefix in cc files.
Done


http://gerrit.cloudera.org:8080/#/c/8775/4/be/src/exec/hdfs-scanner.cc@96
PS4, Line 96: std::
> same here.
Done


http://gerrit.cloudera.org:8080/#/c/8775/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/8775/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@656
PS1, Line 656: // Check to see if the conjunct evaluates to true when the 
slot is NULL
> nit: conjuncts still lacks the _ :)
whoops, done.


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

http://gerrit.cloudera.org:8080/#/c/8775/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@111
PS4, Line 111: the
> Duplicate word
Done


http://gerrit.cloudera.org:8080/#/c/8775/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1099
PS4, Line 1099: output.append(detailPrefix + "parquet statistics 
predicates: " +
> This one compiles the string differently than the others, probably I did th
sure, and found one more place.


http://gerrit.cloudera.org:8080/#/c/8775/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1116
PS4, Line 1116: List totalIdxList = Lists.newArrayList();
> This is more of a guess, but can't you pass entry.getValue() to newArrayLis
indeed. done.


http://gerrit.cloudera.org:8080/#/c/8775/1/testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test
File 
testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test:

http://gerrit.cloudera.org:8080/#/c/8775/1/testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test@275
PS1, Line 275: 1
> That makes sense. Should we add a quick comment for the next person to come
yes, good point. done.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd
Gerrit-Change-Number: 8775
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 13 Dec 2017 21:28:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] PREVIEW: IMPALA-6052: Change HDFS layout for test tables

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

Change subject: PREVIEW: IMPALA-6052: Change HDFS layout for test tables
..


Patch Set 3:

(2 comments)

The direction here seems fine to me. This is a case where I think you'll need 
to run both exhaustive and S3 tests before commit, since this is so very 
cross-cutting.

What's the bigger picture of what brought you here?

http://gerrit.cloudera.org:8080/#/c/8260/3/testdata/bin/generate-schema-statements.py
File testdata/bin/generate-schema-statements.py:

http://gerrit.cloudera.org:8080/#/c/8260/3/testdata/bin/generate-schema-statements.py@470
PS3, Line 470:   p = subprocess.Popen(['/bin/bash', '-c', cmd], 
stdout=subprocess.PIPE)
this is missing stderr=subprocess.PIPE, so stderr is always going to be None in 
the line below.


http://gerrit.cloudera.org:8080/#/c/8260/3/testdata/bin/generate-schema-statements.py@473
PS3, Line 473:   if p.returncode != 0:
 : print "eval_section command failed: {0}".format(cmd)
 : assert(False)
nit: I think this is equivalent to:

if p.returncode != 0:
  raise Exception("...")

you can also do

assert False, "message".



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3ba27ba6d3c7e445795e750281070963bbe1bb51
Gerrit-Change-Number: 8260
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 13 Dec 2017 21:27:37 +
Gerrit-HasComments: Yes

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

Philip Zeyliger 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)

Just a few more comments. Thanks!

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 
passing "exc_info=True" or using LOG.exception.

An example:

>>> import logging
>>> logging.basicConfig(level=logging.INFO)
>>> try:
...   raise Exception("hey")
... except:
...   logging.exception("x")
...   logging.info("y", exc_info=True)
...
ERROR:root:x
Traceback (most recent call last):
  File "", line 2, in 
Exception: hey
INFO:root:y
Traceback (most recent call last):
  File "", line 2, in 
Exception: hey

It doesn't really matter here, but it's a useful think to know. Feel free to 
change this without further review.


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 
deserialize this, right? So, we should fail the test.


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.



--
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: Wed, 13 Dec 2017 21:18:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4993: extend dictionary filtering to collections

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

Change subject: IMPALA-4993: extend dictionary filtering to collections
..


Patch Set 4:

(11 comments)

This looks good to me, I only added minor clean-up comments and will do a final 
pass on the next PS.

http://gerrit.cloudera.org:8080/#/c/8775/4/be/src/exec/hdfs-scan-node-base.h
File be/src/exec/hdfs-scan-node-base.h:

http://gerrit.cloudera.org:8080/#/c/8775/4/be/src/exec/hdfs-scan-node-base.h@90
PS4, Line 90:   /// sequence-based file
This looks like a rebase artifact. I often look at diffs between patchsets (and 
I think others do, too) so if possible, it helps to do a final rebase at the 
end instead of rebaseing in the middle. Sometimes rebases are inevitable, in 
which case it's fine of course.


http://gerrit.cloudera.org:8080/#/c/8775/4/be/src/exec/hdfs-scanner.cc
File be/src/exec/hdfs-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8775/4/be/src/exec/hdfs-scanner.cc@90
PS4, Line 90: std::
We usually drop the std:: prefix in cc files.


http://gerrit.cloudera.org:8080/#/c/8775/4/be/src/exec/hdfs-scanner.cc@96
PS4, Line 96: std::
same here.


http://gerrit.cloudera.org:8080/#/c/8775/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/8775/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@187
PS1, Line 187:   // tuple. Uses a linked hash map for consistent display in 
explain.
> min-max pruning is discussed in the header, but dictionary pruning is not.
Thanks.


http://gerrit.cloudera.org:8080/#/c/8775/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@624
PS1, Line 624: addNotEmptyCollections(collectionConjuncts);
> A bit unclear to me as well. However, if I understand this literally, we on
Thanks for adding the comments. This way round it makes more sense. It's still 
not obvious to me why in the prior code and before the check for slotIds.size() 
!= 1, tupleIds.size() == 1 had to be true. There seems to be some guarantee 
that conjuncts in the scanners work on a single tuple only, which surprises me.


http://gerrit.cloudera.org:8080/#/c/8775/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@656
PS1, Line 656: // Check to see if the conjunct evaluates to true when the 
slot is NULL
> Done
nit: conjuncts still lacks the _ :)


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

http://gerrit.cloudera.org:8080/#/c/8775/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@111
PS4, Line 111: the
Duplicate word


http://gerrit.cloudera.org:8080/#/c/8775/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1099
PS4, Line 1099: output.append(detailPrefix + "parquet statistics 
predicates: " +
This one compiles the string differently than the others, probably I did this 
myself. While you're here and if you don't mind, you could convert it to 
String.format().


http://gerrit.cloudera.org:8080/#/c/8775/4/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1116
PS4, Line 1116: List totalIdxList = Lists.newArrayList();
This is more of a guess, but can't you pass entry.getValue() to newArrayList?


http://gerrit.cloudera.org:8080/#/c/8775/1/testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test
File 
testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test:

http://gerrit.cloudera.org:8080/#/c/8775/1/testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test@265
PS1, Line 265: # - number of projections and their nesting depth
> I think there's some benefit to explicitly listing the areas tested/to-test
I understand your point, let's keep it here.


http://gerrit.cloudera.org:8080/#/c/8775/1/testdata/workloads/functional-query/queries/QueryTest/parquet-filtering.test@275
PS1, Line 275: 1
> there are two files in the table. one of them is dictionary encoded for int
That makes sense. Should we add a quick comment for the next person to come by 
("Only one of the files in this table is dictionary encoded")?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If3a2abcfc3d0f7d18756816659fed77ce12668dd
Gerrit-Change-Number: 8775
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 13 Dec 2017 20:39:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] PREVIEW: IMPALA-6052: Change HDFS layout for test tables

2017-12-13 Thread Joe McDonnell (Code Review)

Joe McDonnell has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/8260 )

Change subject: PREVIEW: IMPALA-6052: Change HDFS layout for test tables
..

PREVIEW: IMPALA-6052: Change HDFS layout for test tables

Currently, every table in Impala's test data has an HDFS
directory directly underneath /test-warehouse. Tables from
different databases are distinguished by their directory
name.
functional.alltypes => /test-warehouse/alltypes
functional_parquet.alltypes => /test-warehouse/alltypes_parquet
This makes the HDFS filesystem difficult to navigate.
The /test-warehouse directory has 800+ subdirectories. Listing
only the tables from a single database is difficult.

This adds a level to the directory structure for the database
and places all table directories in these databases directories.
This corresponds to the default placement for Impala-created
tables when LOCATION is not specified.
functional.alltypes => /test-warehouse/functional.db/alltypes
functional_parquet.alltypes => /test-warehouse/functional_parquet.db/alltypes
After this change, the /test-warehouse directory now has about 60
subdirectories.

This directory structure change required updating paths in
a variety of tests. With these updates, the core tests pass.

Change-Id: I3ba27ba6d3c7e445795e750281070963bbe1bb51
---
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/resources/authz-policy.ini.template
M testdata/avro_schema_resolution/create_table.sql
M testdata/bin/create-load-data.sh
M testdata/bin/create-table-many-blocks.sh
M testdata/bin/generate-schema-statements.py
M testdata/bin/load-dependent-tables.sql
M testdata/datasets/functional/functional_schema_template.sql
M testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test
M testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
M testdata/workloads/functional-planner/queries/PlannerTest/s3.test
M testdata/workloads/functional-planner/queries/PlannerTest/topn.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
M testdata/workloads/functional-query/queries/DataErrorsTest/avro-errors.test
M 
testdata/workloads/functional-query/queries/QueryTest/alter-table-set-column-stats.test
M testdata/workloads/functional-query/queries/QueryTest/alter-table.test
M testdata/workloads/functional-query/queries/QueryTest/avro-schema-changes.test
M 
testdata/workloads/functional-query/queries/QueryTest/compute-stats-incremental.test
M 
testdata/workloads/functional-query/queries/QueryTest/compute-stats-tablesample.test
M 
testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test
M 
testdata/workloads/functional-query/queries/QueryTest/multiple-filesystems.test
M 
testdata/workloads/functional-query/queries/QueryTest/parquet-continue-on-error.test
M 
testdata/workloads/functional-query/queries/QueryTest/parquet-resolution-by-name.test
M testdata/workloads/functional-query/queries/QueryTest/parquet.test
M testdata/workloads/functional-query/queries/QueryTest/show-stats.test
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_hdfs_encryption.py
M tests/metadata/test_load.py
M tests/metadata/test_refresh_partition.py
M tests/metadata/test_stale_metadata.py
M tests/query_test/test_chars.py
M tests/query_test/test_hdfs_file_mods.py
38 files changed, 759 insertions(+), 755 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3ba27ba6d3c7e445795e750281070963bbe1bb51
Gerrit-Change-Number: 8260
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

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

Change subject: IMPALA-3703: Store query context in thread-local variables
..


Patch Set 10:

(11 comments)

I added some comments to clean it up a bit, otherwise it looks good to me.

http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG@11
PS10, Line 11: in debug sessions. It needs to be allocated on the stack in
mention "on the stack of each thread"


http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG@22
PS10, Line 22: fully-fledged core
Replace with "core dump written by Impala"?


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc
File be/src/common/thread-debug-info-test.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc@20
PS10, Line 20: #include 
Still needed?


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc@30
PS10, Line 30: using boost::split;
still needed?


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h
File be/src/common/thread-debug-info.h:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@83
PS10, Line 83:   void PrintUniqueIdToMember(const TUniqueId& id, char* member) {
We could inline this now as it's only used once.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@90
PS10, Line 90:   static void InitializeThreadDebugInfo(ThreadDebugInfo* 
thread_debug_info);
I think these could benefit from a brief comment since they're defined in 
another file and have thread-local side effects.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@96
PS10, Line 96:   static constexpr int64_t THREAD_NAME_FRONT_LENGTH = 
THREAD_NAME_SIZE -
I think moving this computation to SetThreadName() would make it slightly 
easier to understand (and remove a member here).


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/blocking-join-node.cc
File be/src/exec/blocking-join-node.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/blocking-join-node.cc@199
PS10, Line 199:   ThreadDebugInfo* thread_debug_info = 
GetThreadDebugInfo();
You could also do 
GetThreadDebugInfo()->SetInstanceId(state->fragment_instance_id());


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/hdfs-scan-node.cc@353
PS10, Line 353:   ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo();
You could shorten it a bit to 
GetThreadDebugInfo()->SetInstanceId(state->fragment_instance_id());


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/fragment-instance-state.cc@232
PS10, Line 232:   ThreadDebugInfo* thread_debug_info = 
GetThreadDebugInfo();
  :   thread_debug_info->SetInstanceId(this->instance_id());
You could shorten this to 
GetThreadDebugInfo()->SetInstanceId(this->instance_id());


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/query-state.cc@377
PS10, Line 377: thread_debug_info
Inline the call to GetThreadDebugInfo() here, too?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 20:00:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6190/6246: Add instances tab and event sequence

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

Change subject: IMPALA-6190/6246: Add instances tab and event sequence
..


Patch Set 7:

(18 comments)

Thank you for the review. Please see PS7.

http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/coordinator.h@187
PS4, Line 187: instances
> instances' stats
Done


http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:

http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.cc@241
PS3, Line 241: // Release the thread token on the ro
> Please see comments in Open(). This state doesn't seem necessary anymore if
Done


http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.cc@302
PS3, Line 302: SCOPED_TIMER(ADD_CHILD_TIMER(timings_profile_, 
"ExecTreeOpenTime", OPEN_TIMER_NAME));
 : RETURN_IF_ERROR(exec_tree_->Open(runtime_state_));
 :   }
 :   return sink_->Open(runtime_state_);
 : }
 :
 : Status FragmentInstanceState::ExecInternal() {
 :   RuntimeProfile::Counter* plan_exec_timer =
 :   ADD_CHILD_TIMER(timings_profile_, "ExecTreeExecTime", 
EXEC_TIMER_NAME);
 :   SCOPED_THREAD_COUNTER_MEASUREMENT(runt
> I think it's definitely worth doing a perf run to make sure things don't re
I will kick of a perf run and will include the results in the commit message. 
Regarding your second point, I'm not sure I understand your idea. Do you think 
the compiler should inline the relevant parts from the switch?


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:

http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@63
PS4, Line 63: /// Events that are tracked in a separate timeline for each 
fragment instance.
: static const string INSTANCE_EVENT_TIMELINE_NAME =
: "Fragment Instance Lifecycle Event Timeline";
:
: /// Indicates that the call to Prepare() has been completed.
: static const string PREPARE_EVENT_NAME = "Prepare Finished";
:
: /// Indicates that the call to Open() has been completed.
> Please provide comments about these names and how they may correlate with t
Done


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@73
PS4, Line 73: he first row batch has b
> Is there any static assert to ensure that number of entries in this array m
I added one that will break if we only change one and then will require 
updating, too. I'm not particularly happy with it since it's somewhat 
ungeneric, but I couldn't find a way to get the size of the Thrift enum during 
compile time. However, it'll break if we only change one so it does what it 
should.


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@79
PS4, Line 79:
> Will "produced" be more accurate as there is usually some extra processing
Done


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@81
PS4, Line 81: bout to be sen
> "Producing row batches" seems more consistent with the rest of the state na
Done


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@283
PS4, Line 283:
> This is actually the majority of time codegen spent on. May want to extend
Done


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@302
PS4, Line 302: OPED_TIMER(ADD_CHILD_TIMER(timings_profile_
> Can this be moved out of the loop to line 297 and simplify the state machin
Done


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@306
PS4, Line 306:
> Should we not count the time to execute UpdateState() towards "plan_exec_ti
Done


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@410
PS4, Line 410: VLOG_FILE << "Reporting " << (done ? "final " : "") << 
"profile for instance "
> const StateEvent
Done


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@418
PS4, Line 418:  (per_h
> DCHECK_EQ(). Same below.
Done


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@483
PS4, Line 483:  // Allo
> nit: wrong indent
Done


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@484
PS4, Line 484: event_sequence_->MarkEvent(EXEC_INTERNAL_EVENT_NAME);
> Does it help to print "event" too ?
Done


http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@487
PS4, Line 487:
> Can there be multiple threads updating the sta

[Impala-ASF-CR] IMPALA-6190/6246: Add instances tab and event sequence

Hello Michael Ho, Joe McDonnell, Dan Hecht,

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

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

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

Change subject: IMPALA-6190/6246: Add instances tab and event sequence
..

IMPALA-6190/6246: Add instances tab and event sequence

This change adds tracking of the current state during the execution of a
fragment instance. The current state is then reported back to the
coordinator and exposed to users via a new tab in the query detail debug
webpage.

This change also adds an event timeline to fragment instances in the
query profile. The timeline measures the time since fragment start at
which particular events complete. Events are derived from the current
state of the execution of a fragment instance. For example:

- Codegen Finished: 79.312ms (79.312ms)
- Prepare Finished: 79.454ms (141.949us)
- Open Finished: 93.287ms (13.832ms)
- First Batch Received: 259.751ms (166.464ms)
- First Batch Sent: 260.219ms (467.917us)
- ExecInternal Finished: 2s733ms (2s473ms)

I added automated tests for both extensions and additionally verified
the change by manual inspection.

Change-Id: I626456b6afa9101ffd5cda10c4096d63d7f9
---
M be/src/common/atomic.h
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/query-state.cc
M be/src/service/impala-http-handler.cc
M be/src/service/impala-http-handler.h
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile.cc
M common/thrift/ImpalaInternalService.thrift
M tests/query_test/test_observability.py
M tests/webserver/test_web_pages.py
M www/query_detail_tabs.tmpl
A www/query_finstances.tmpl
17 files changed, 545 insertions(+), 37 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I626456b6afa9101ffd5cda10c4096d63d7f9
Gerrit-Change-Number: 8758
Gerrit-PatchSet: 7
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho

[Impala-ASF-CR] IMPALA-6301: Fix test failures when username or group name contains dots

2017-12-13 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-6301: Fix test failures when username or group name 
contains dots
..


Patch Set 3:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8ae15bb6a929dc48d3ad2176c8b3fafff87f32b
Gerrit-Change-Number: 8807
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Wed, 13 Dec 2017 19:30:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6301: Fix test failures when username or group name contains dots

2017-12-13 Thread Michael Ho (Code Review)

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

Change subject: IMPALA-6301: Fix test failures when username or group name 
contains dots
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8ae15bb6a929dc48d3ad2176c8b3fafff87f32b
Gerrit-Change-Number: 8807
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Wed, 13 Dec 2017 19:22:50 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5191: Standardize column alias behavior

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

Change subject: IMPALA-5191: Standardize column alias behavior
..


Patch Set 5: Code-Review+1

Looks good to me. Alex, maybe you can take a look?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 19:11:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6270: remove redundant version properties

2017-12-13 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-6270: remove redundant version properties
..


Patch Set 1:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6812e11bb41716450ef29bb523773479e9f76eec
Gerrit-Change-Number: 8827
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 13 Dec 2017 19:11:23 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6270: remove redundant version properties

2017-12-13 Thread Zach Amsden (Code Review)

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

Change subject: IMPALA-6270: remove redundant version properties
..


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8827/1/tests/test-hive-udfs/pom.xml
File tests/test-hive-udfs/pom.xml:

http://gerrit.cloudera.org:8080/#/c/8827/1/tests/test-hive-udfs/pom.xml@a40
PS1, Line 40:
I didn't even realize we had a POM here.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6812e11bb41716450ef29bb523773479e9f76eec
Gerrit-Change-Number: 8827
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 13 Dec 2017 19:10:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5990: End-to-end compression of metadata

2017-12-13 Thread Tianyi Wang (Code Review)

Tianyi Wang has abandoned this change. ( http://gerrit.cloudera.org:8080/8825 )

Change subject: IMPALA-5990: End-to-end compression of metadata
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I3a8819cad734b3a416eef6c954e55b73cc6023ae
Gerrit-Change-Number: 8825
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Tianyi Wang

[Impala-ASF-CR] IMPALA-6270: remove redundant version properties

Philip Zeyliger has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8827


Change subject: IMPALA-6270: remove redundant version properties
..

IMPALA-6270: remove redundant version properties

Removes properties that are already defined in the impala-parent pom.

I ran the tests.

Change-Id: I6812e11bb41716450ef29bb523773479e9f76eec
---
M tests/test-hive-udfs/pom.xml
1 file changed, 0 insertions(+), 2 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6812e11bb41716450ef29bb523773479e9f76eec
Gerrit-Change-Number: 8827
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Philip Zeyliger

[Impala-ASF-CR] IMPALA-6270: remove redundant version properties

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

Change subject: IMPALA-6270: remove redundant version properties
..


Patch Set 1:

https://jenkins.impala.io/view/Utility/job/pre-review-test/88/console passed 
just fine.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6812e11bb41716450ef29bb523773479e9f76eec
Gerrit-Change-Number: 8827
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 13 Dec 2017 18:18:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..


Patch Set 4:

(20 comments)

thanks for the changes! the refactor for the tests looks great. most follow-up 
comments are oriented around naming.

http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/cup/sql-parser.cup@717
PS4, Line 717: Please beware of that the both of Oracle and default hint styles 
are supported when
 : // extending INSERT/UPSERT syntax.
Replace with: "Note: when extending INSERT/UPSERT syntax, hinting is supported 
at the beginning of the statement and before the query."


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/cup/sql-parser.cup@807
PS4, Line 807: Please beware of that the both of Oracle and default hint styles 
are supported when
 : // extending INSERT/UPSERT syntax.
make this consistent with the proposal on L717.


http://gerrit.cloudera.org:8080/#/c/8676/4/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/8676/4/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java@86
PS4, Line 86: InsertStmt.HintLocation.DefaultLoc
more intuitive to pass in null here (what does DefaultLoc for no hints mean?)


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:

http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@56
PS4, Line 56:   // A flag to determine the location of a hint
This is a good place to describe the alternatives, including examples. Also, 
I'd use more descriptive names ("oracle" and "default" don't convey much). How 
about the following:

// Determines the location of optional hints. The AtStatment option
// is motivated by Oracle's hint placement at the start of the
// statement and the AtQuery option places the hint right before
// the query (if specified).
//
// Examples:
//   Start: INSERT /* my hint */  ... SELECT ...
//   End:   INSERT  /* my hint */ SELECT ...
public enum HintLocation { Start, End };


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@210
PS4, Line 210: hintLoc_ = hintLoc;
can a caller pass in null? how about we pick the default (current placement) if 
null?


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@908
PS4, Line 908: hintLoc_ == HintLocation.OracleLoc && !planHints_.isEmpty()
I would flip these tests around (test for whether we have hints, then test the 
hint location).


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@303
PS4, Line 303: BuildQueryStmt
more specific: rename to InjectInsertHint


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@310
PS4, Line 310: hints
rename to actualHints


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@310
PS4, Line 310: TestHints
rename to VerifyHints


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@311
PS4, Line 311: actualHints
rename to actualStringHints


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@318
PS4, Line 318: Parses stmt and checks that the insert hints stmt are the 
expected hints.
How about:
Injects hints into pattern and checks that the injected hints match the 
expected hints.


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@320
PS4, Line 320: TestInsertHints
rename this to: TestInsertStmtHints... our InsertStmt covers both inserts and 
upserts. Update the comment to state that its used for both inserts and updates.


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@327
PS4, Line 327: /**
 :* Parses stmt and checks that the upsert hints stmt are the 
expected hints.
 :*/
 :   private void TestUpsertHints(String pattern, String hint, 
String... expectedHints) {
 : TestInsertHints(pattern, hint, expectedHints);
 :   }
remove-- not needed.


http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@396
PS4, Line 396: T

[Impala-ASF-CR] IMPALA-5315: Cast to timestamp fails for YYYY-M-D format

Vincent Tran has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7009 )

Change subject: IMPALA-5315: Cast to timestamp fails for -M-D format
..


Patch Set 6:

The alternative here is that we only take this perf hit when non-default format 
is used and still maintain the previous default templates.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
Gerrit-Change-Number: 7009
Gerrit-PatchSet: 6
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran 
Gerrit-Comment-Date: Wed, 13 Dec 2017 16:50:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5315: Cast to timestamp fails for YYYY-M-D format

Vincent Tran has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7009 )

Change subject: IMPALA-5315: Cast to timestamp fails for -M-D format
..


Patch Set 6:

I incorporated the same heuristic into ParseFormatTokens() to eliminate the 
usage of default format strings. We observe some marginal gain of 16% in 
TotalReadThroughput:

- TotalReadThroughput: 11.69 MB/sec


@Tim, since you wrote the original ParseFormatTokens(), do you see any 
additional places where we can achieve more marginal gain?
Also, do you prefer the rewrite of the existing function or creating a new one 
such as in the current patch set?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
Gerrit-Change-Number: 7009
Gerrit-PatchSet: 6
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran 
Gerrit-Comment-Date: Wed, 13 Dec 2017 16:40:01 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5315: Cast to timestamp fails for YYYY-M-D format

Hello Lars Volker, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-5315: Cast to timestamp fails for -M-D format
..

IMPALA-5315: Cast to timestamp fails for -M-D format

This change allows casting of a string in 'lazy' date
format to timestamp. The supported lazy date formats are:
  -[M]M-[d]d
  -[M]M-[d]d [H]H:[m]m:[s]s[.S]
  -[M]M-[d]dT[H]H:[m]m:[s]s[.S]

Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
---
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-parse-util.cc
M be/src/runtime/timestamp-parse-util.h
3 files changed, 137 insertions(+), 79 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
Gerrit-Change-Number: 7009
Gerrit-PatchSet: 6
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran

[Impala-ASF-CR] IMPALA-5014: Part 1: Round when casting string to decimal

2017-12-13 Thread Zach Amsden (Code Review)

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

Change subject: IMPALA-5014: Part 1: Round when casting string to decimal
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h
File be/src/util/string-parser.h:

http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h@239
PS2, Line 239: value = DecimalUtil::ScaleDownAndRound(value, shift, 
round);
> ScaleDownAndRound implements signed rounding to do rounding away from zero,
After thinking on this some more, rounding away from zero is symmetric with 
respect to positive and negative numbers, so it doesn't matter if the number 
value is negated before or after this call.  Either way works correctly.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icd8b92727fb384e6ff2d145e4aab7ae5d27db26d
Gerrit-Change-Number: 8774
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Wed, 13 Dec 2017 16:24:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE

2017-12-13 Thread Gabor Kaszab (Code Review)

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

Change subject: IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE
..


Patch Set 4:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8820/3/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@299
PS3, Line 299: // If re-analyzing this statement then the Kudu table name 
had already been
> Agree with Tim. The analyze/reanalyze pattern is already confusing enough w
Thanks for taking a look, Tim, Alex!

I moved the flag to be CreateTableStmt specific. Do you think that is 
reasonable?

(I'd like to differentiate between the cases when the kudu.table_name is set by 
the user or when it was set automatically)


http://gerrit.cloudera.org:8080/#/c/8820/3/tests/query_test/test_kudu.py
File tests/query_test/test_kudu.py:

http://gerrit.cloudera.org:8080/#/c/8820/3/tests/query_test/test_kudu.py@a861
PS3, Line 861:
> I think we still want the test coverage from the test. It seems like the fi
Good point.
Done


http://gerrit.cloudera.org:8080/#/c/8820/3/tests/query_test/test_kudu.py@a881
PS3, Line 881:
> I this second part of the test is still valid - we want to make sure that "
Good point again :)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieca037498abf8f5fde67b77e824b720482cdbe6f
Gerrit-Change-Number: 8820
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 16:15:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5191: Standardize column alias behavior

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8801 )

Change subject: IMPALA-5191: Standardize column alias behavior
..


Patch Set 4:

(7 comments)

Thanks for the comments

http://gerrit.cloudera.org:8080/#/c/8801/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8801/4//COMMIT_MSG@13
PS4, Line 13: === Allowed ===
> Nice, thanks for the examples!
You're welcome!


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

http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@813
PS4, Line 813:  is false
> I think it's better to say what happens when it's true. (Right now there is
Done


http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@838
PS4, Line 838:   return trySubstitute(smap, analyzer, preserveRootType, 
true);
> Maybe mention why this is always true in the method comment?
Extended the method comment.


http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@850
PS4, Line 850:   result.add(e.trySubstitute(smap, analyzer, 
preserveRootTypes, true));
> Maybe add a brief comment why it's always true?
Added a comment.


http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@869
PS4, Line 869:* If substituteChildren is false, child expressions of 'this' 
will not be substituted.
> Modify comment to remove the double negative.
Done


http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java:

http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@303
PS4, Line 303: substituteExpr = expr.trySubstitute(aliasSmap_, 
analyzer, false, substituteChildren);
> long line
Done


http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

http://gerrit.cloudera.org:8080/#/c/8801/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@963
PS4, Line 963: bool_col
> maybe make this "not bool_col" so that it's an expression?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 16:10:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5191: Standardize column alias behavior

Hello Taras Bobrovytsky, Tim Armstrong, Alex Behm,

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

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

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

Change subject: IMPALA-5191: Standardize column alias behavior
..

IMPALA-5191: Standardize column alias behavior

We should not perform alias substitution in the
subexpressions of GROUP BY, HAVING, and ORDER BY
to be more standard conformant.

=== Allowed ===
SELECT int_col / 2 AS x
FROM functional.alltypes
GROUP BY x;

SELECT int_col / 2 AS x
FROM functional.alltypes
ORDER BY x;

SELECT NOT bool_col AS nb
FROM functional.alltypes
GROUP BY nb
HAVING nb;

=== Not allowed ===
SELECT int_col / 2 AS x
FROM functional.alltypes
GROUP BY x / 2;

SELECT int_col / 2 AS x
FROM functional.alltypes
ORDER BY -x;

SELECT int_col / 2 AS x
FROM functional.alltypes
GROUP BY x
HAVING x > 3;

A flag called substituteChildren is added to the
alias substitution methods. If the flag is false,
the methods won't substitute the aliases of child
expressions.

Some extra checks were added to AnalyzeExprsTest.java.

I had to update other tests to make them pass
since the new behavior is more restrictive.

Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
10 files changed, 60 insertions(+), 28 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE

2017-12-13 Thread Gabor Kaszab (Code Review)

Hello Laszlo Gaal, Zoltan Borok-Nagy, Attila Jeges, Tim Armstrong, Csaba 
Ringhofer, Alex Behm,

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

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

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

Change subject: IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE
..

IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE

This change disallows explicitly setting the Kudu table name property
for managed Kudu tables in a CREATE TABLE statement. The Kudu table
name property gets a generated value as the following:
'impala::db_name.table_name' where table_name is the one given in
the CREATE TABLE statement.
Providing the Kudu table name property when creating a managed Kudu
table results in an error without creating the table. E.g.:
CREATE TABLE t (i INT) STORED AS KUDU
  TBLPROPERTIES('kudu.table_name'='some_name');

Change-Id: Ieca037498abf8f5fde67b77e824b720482cdbe6f
---
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
M tests/query_test/test_kudu.py
5 files changed, 85 insertions(+), 45 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieca037498abf8f5fde67b77e824b720482cdbe6f
Gerrit-Change-Number: 8820
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-2640: Make a given command case-sensitive

Hello John Russell, Andre Araujo, Zoltan Borok-Nagy, Tim Armstrong,

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

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

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

Change subject: IMPALA-2640: Make a given command case-sensitive
..

IMPALA-2640: Make a given command case-sensitive

IMPALA-2180 has forced the command to be lowercase. It is better
to maintain the command given by an user.

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShell.test_var_substitution

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
---
M shell/impala_shell.py
A tests/shell/shell_case_sensitive.cmds
A tests/shell/shell_case_sensitive2.cmds
M tests/shell/test_shell_interactive.py
4 files changed, 96 insertions(+), 49 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 10
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

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

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

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


Patch Set 13:

(4 comments)

Thanks for your comments!

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

http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/impala-server.h@342
PS13, Line 342:   /// Register timeout value upon opening a new session. This 
will wake up
  :   /// session_timeout_thread_ to update its poll period.
  :   void RegisterSessionTimeout(int32_t timeout);
  :
  :   /// Unregister timeout value. This will wake up 
session_timeout_thread_
  :   /// to update its poll period.
  :   void UnregisterSessionTimeout(int32_t timeout);
> Should these functions be private ?
I like this idea.

This also means that SessionState needs a pointer to the ImpalaServer. And 
SessionState needs to be a friend of ImpalaServer to access those private 
methods.

See my new patch set how it looks.


http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/impala-server.h@437
PS13, Line 437: FLAGS_idle_session_timeout is used
> Also, it also seems to use FLAGS_idle_session_timeout as the value if reque
Yes, I rephrased this comment.


http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/impala-server.h@438
PS13, Line 438: This method also sets the query option 'idle_session_timeout'
> Is this still true ?
No, I removed this part.


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

http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/query-options.cc@582
PS13, Line 582: else if (requested_timeout == 0 && FLAGS_idle_session_timeout 
!= 0) {
  :   return Status(
  :   Substitute("Session timeout cannot be unlimited, "
  :  "maximum value: $0 seconds", 
FLAGS_idle_session_timeout));
  : } else if (requested_timeout > 
FLAGS_idle_session_timeout &&
  :FLAGS_idle_session_timeout != 0) {
  :   return Status(Substitute("Session timeout cannot be 
set longer than $0 seconds",
  :   FLAGS_idle_session_timeout));
  : }
> Just a minor suggestion:
Yeah, I was thinking about it before. This way we have less conditions, but 
more nesting.
I couldn't really decide which one is more readable, but since you are also 
suggesting this I re-structured the code.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 13
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 14:55:40 +
Gerrit-HasComments: Yes

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

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

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

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

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

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

IMPALA-2248: Make idle_session_timeout a query option

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

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

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

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

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


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

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

[Impala-ASF-CR] IMPALA-2640: Make a given command case-sensitive

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-2640: Make a given command case-sensitive
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8762/10/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8762/10/shell/impala_shell.py@569
PS10, Line 569: super(ImpalaShell, self)
nit: maybe you could write "This code is based on the code from the standard 
Python library package cmd.py: Cmd.onecmd()" or something like that.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 10
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 14:41:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2640: Make a given command case-sensitive

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

Change subject: IMPALA-2640: Make a given command case-sensitive
..


Patch Set 10:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8762/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8762/6//COMMIT_MSG@7
PS6, Line 7: IMPALA-2640: Make a given command case-sensitive
In the fix of 
IMPALA-4664(https://gerrit.cloudera.org/#/c/8639/4/shell/impala_shell.py), you 
thought a right fix is to hack code for lower-casting. I think this change for 
IMPALA-2640 can solve the lower-casting problem properly.

The result of your example looks fine to me. Would you let me know what the 
problem is in the example? I guess the lowering of column description is 
intended, right?

> sElEcT 'FoOoOo';
Query: select 'FoOoOo'
Query submitted at: 2017-12-13 22:52:08 (Coordinator: http://ubuntu:25000)
Query progress can be monitored at: 
http://ubuntu:25000/query_plan?query_id=20454e49cb053379:538981e9
+--+
| 'fo' |
+--+
| FoOoOo   |
+--+


http://gerrit.cloudera.org:8080/#/c/8762/6/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8762/6/shell/impala_shell.py@572
PS6, Line 572: # is necessary to find
> To me it looks a bit weird to pass the command string to each command.
I think this intention, passing the command, is still valid for me.
do_* methods are always not invoked here. See 
https://github.com/apache/impala/blob/master/shell/impala_shell.py#L208

Due to the above case, we should

We may get the original command from


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@305
PS9, Line 305: """Original command should be stored before running the 
method. The method is usually
> Long line
Done


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@310
PS9, Line 310:   print_to_stderr("Unexpected error: Failed to execute query 
due to command "\
> Long line.
Done


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@555
PS9, Line 555:
> Can you include attribution for where the copied code came from (e.g. see b
I added some comment for the coping. The PSF license is already included in 
LICENSE.txt.


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@571
PS9, Line 571: ne c
> _ on the end of a local variable is a bit weird to me - can we call it comm
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 10
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 14:20:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

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

Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for 
pcg-cpp
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
File be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp:

http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp@599
PS2, Line 599:  * warning: expansion of date or time macro is not reproducible
The struct is not used in our code and pcg-cpp's sample and test codes. I 
thought comment out is better than removal of the code because someone needs 
this later.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 13 Dec 2017 13:38:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

Hello Jim Apple,

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

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

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

Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for 
pcg-cpp
..

IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

In the commit 4feb4f3a, the third party library pcg-cpp was excluded
from the clang-tidy check. It could make unexpected side effect, so
fixing some warnings from clang-tidy is better than avoidance of the check.

Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
---
M be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
M bin/run_clang_tidy.sh
2 files changed, 8 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

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


Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for 
pcg-cpp
..

IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

In the commit 4feb4f3a, the third party library pcg-cpp was excluded
from the clang-tidy check. It could make unexpected side effect, so
fixing some warnings from clang-tidy is better than avoidance of the check.

Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
---
M be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
M bin/run_clang_tidy.sh
2 files changed, 4 insertions(+), 3 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 22:

@Jim, I pushed a fix for rollback of the exclusion for the third party check:
https://gerrit.cloudera.org/#/c/8829/

 > You'll notice that be/src/thirdparty/squeasel and mustache are
 > sometimes modified independent of their source, and one of the
 > warnings about reproducability can prevent support nightmares.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 22
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 13 Dec 2017 13:27:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 22:

@Jim, Michael and Attila, I appreciate your reviews!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 22
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 13 Dec 2017 13:26:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

Hello John Russell, Alex Behm, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..

IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

Testing:
Add unit tests to ParserTest#TestPlanHints
Add plan check tests to PlannerTest#testInsert, PlannerTest#testKuduUpsert
Add query rebuild tests to ToSqlTest#planHintsTest

Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
7 files changed, 375 insertions(+), 82 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..

Patch Set 2:

(4 comments)

Thanks a lot for your kind guidance and finding some missing points such as
toSql and test enhancements.

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

http://gerrit.cloudera.org:8080/#/c/8676/2//COMMIT_MSG@7
PS2, Line 7: Adopt
> The jira says "adopt", but we're really not making this style the default.
Done.

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

http://gerrit.cloudera.org:8080/#/c/8676/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@44
PS2, Line 44:
> These tests confirm that the oracle style hints are indeed added as expecte
Done. Thanks for your kind guidance.

http://gerrit.cloudera.org:8080/#/c/8676/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@410
PS2, Line 410: "shuffle"
> This difference is preventing you from having one loop and factoring lines
Thanks for the guidance. I applied refactoring these code.

http://gerrit.cloudera.org:8080/#/c/8676/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@426
PS2, Line 426: ParsesOk
> pls add a TestUpsertHints method that is similar to TestInsertHints so that
You're right. I introduced TestUpsertHints.

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: John Russell
Gerrit-Reviewer: Kim Jin Chul
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 13 Dec 2017 13:07:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

Hello John Russell, Alex Behm, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT
..

IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT

Testing:
Add unit tests to ParserTest#TestPlanHints
Add plan check tests to PlannerTest#testInsert, PlannerTest#testKuduUpsert
Add query rebuild tests to ToSqlTest#planHintsTest

Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
7 files changed, 375 insertions(+), 82 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-3703: Store query context in thread-local variables
..

IMPALA-3703: Store query context in thread-local variables

This commit introduces the ThreadDebugInfo class which can
hold information about the current thread that can be useful
in debug sessions. It needs to be allocated on the stack in
order to include it to minidumps.

Currently a ThreadDebugInfo object is created in
Thread::SuperviseThread. This object is available in all
the child stack frames through the global function
GetThreadDebugInfo().

ThreadDebugInfo has members for the thread name and instance
id. These are fixed size char buffers.

If you have a fully-fledged core file, you can locate the
ThreadDebugInfo for the current thread through the global
pointer impala::thread_debug_info.

In a core file that has been created from a minidump, we need
to select the stack frame that allocated the ThreadDebugInfo
object in order to inspect it. It is currently allocated in
Thread::SuperviseThread().

We can use printf in gdb to print the members, e.g.:
printf "%s\n" thread_debug_info.instance_id

Currently the thread name and instance id is stored.

I created some tests in thread-debug-info-test.cc

Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
---
M be/src/common/CMakeLists.txt
A be/src/common/thread-debug-info-test.cc
A be/src/common/thread-debug-info.cc
A be/src/common/thread-debug-info.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/util/thread.cc
9 files changed, 265 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8621 )

Change subject: IMPALA-3703: Store query context in thread-local variables
..


Patch Set 9:

(1 comment)

Thanks for the comment!

I also made my namings consistent, i.e. SIZE means length of the buffer (large 
enough to hold the additional '\0'), LENGTH means length of the string without 
'\0'.

http://gerrit.cloudera.org:8080/#/c/8621/8/be/src/common/thread-debug-info.h
File be/src/common/thread-debug-info.h:

http://gerrit.cloudera.org:8080/#/c/8621/8/be/src/common/thread-debug-info.h@42
PS8, Line 42:   /// Only one ThreadDebugInfo object can be alive per thread at 
a time.
> We should document whether this is copyable/movable, and if so what the sem
Currently it is not copyable nor movable, but it might change in the future. 
Some copying/cloning functionality will be required by IMPALA-6254.

Anyway, I added a comment about it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 9
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 13 Dec 2017 10:26:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables