[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression

[Alex Behm (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger,

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

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

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

Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
..

IMPALA-5990: Part 1: JNI-based LZ4 de/compression

Adds LZ4 de/compression in FeSupport geared towards
eventually compressing catalog objects end-to-end
in their journey from the catalogd over the statestored
to coordinator impalads.

A follow-on change will use the new LZ4 functions to
do the actual compression.

Testing:
- Added a new unit test

Change-Id: I237802944875b07080db0159ff8ec548150fd95e
---
M be/src/service/fe-support.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M fe/src/main/java/org/apache/impala/service/FeSupport.java
A fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
5 files changed, 401 insertions(+), 10 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 5
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression

[Alex Behm (Code Review)]

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

Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
..


Patch Set 2:

(2 comments)

Just realized I forgot these comments, sorry.

http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@454
PS2, Line 454: 0
> I'm not fully familiar with how it works internally, but per my reading of
Why would it need to free the copied bytes? The copied bytes are on the Java 
heap and will be GC'ed as usual (just like the original array once the GC is 
unlocked again)


http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java
File fe/src/main/java/org/apache/impala/service/FeSupport.java:

http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java@306
PS2, Line 306: ByteBuffer.wrap(dst).putInt(0, payload.length);
 : ByteBuffer.wrap(dst).putInt(4, compressedSize);
> Ok, sorry for not being clear. My point is, why have two versions, Lz4Compr
We may not need the ByteBuffer version. Removed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Sat, 30 Sep 2017 04:23:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression

[Alex Behm (Code Review)]

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

Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
..


Patch Set 3:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@452
PS3, Line 452: the
> remove
Done


http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@458
PS3, Line 458: Note that it does not matter if 'src' was copied
> Why don't we treat it as an error? This could mean an OOM on the JVM?
We only read the source, so we don't care if we get the bytes directly or a 
copy. We write into 'dst' and writing into a copy is not useful because then 
the caller has no data in the real 'dst'.

Added comment and code to handle the is_copied case instead of returning an 
error.


http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@471
PS3, Line 471: "
> May be better to add that the JVM could've run out of memory (supportabilit
Done


http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@472
PS3, Line 472: env->ReleasePrimitiveArrayCritical(src, src_data, 0);
I think there was a subtle bug here with not calling 
ReleasePrimitiveArrayCritical() on the dst if it was copied.

I restored the scoped array critical class.


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/main/java/org/apache/impala/service/FeSupport.java
File fe/src/main/java/org/apache/impala/service/FeSupport.java:

http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/main/java/org/apache/impala/service/FeSupport.java@306
PS3, Line 306: if (compressedSize < 0) {
> Looks like this check should be <=.
Done


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
File fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java:

http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java@66
PS3, Line 66: } catch (Throwable e) {
> I think fail() throws an "Error" (AssertionError) which this Throwable can
Reworked. Still want to catch Throwable here to check for OOM. See test in L162.


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java@72
PS3, Line 72: e.printStackTrace();
> unreachable?
Done


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java@78
PS3, Line 78:   private byte[] seqPayload(int numBytes) {
> Add one liner comments on these helpers.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 3
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Sat, 30 Sep 2017 04:03:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression

[Alex Behm (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger,

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

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

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

Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
..

IMPALA-5990: Part 1: JNI-based LZ4 de/compression

Adds LZ4 de/compression in FeSupport geared towards
eventually compressing catalog objects end-to-end
in their journey from the catalogd over the statestored
to coordinator impalads.

A follow-on change will use the new LZ4 functions to
do the actual compression.

Testing:
- Added a new unit test

Change-Id: I237802944875b07080db0159ff8ec548150fd95e
---
M be/src/service/fe-support.cc
M be/src/util/jni-util.cc
M be/src/util/jni-util.h
M fe/src/main/java/org/apache/impala/service/FeSupport.java
A fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
5 files changed, 426 insertions(+), 10 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 4
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-2636: HS2 GetTables() returns TABLE TYPE as TABLE for VIEW

[Alex Behm (Code Review)]

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

Change subject: IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for 
VIEW
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7353/3/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

http://gerrit.cloudera.org:8080/#/c/7353/3/fe/src/main/java/org/apache/impala/service/MetadataOp.java@496
PS3, Line 496: upperCaseTableTypes.add(tableType);
I think the toUpperCase() here should stay.


http://gerrit.cloudera.org:8080/#/c/7353/3/fe/src/main/java/org/apache/impala/service/MetadataOp.java@497
PS3, Line 497: if (tableType.equalsIgnoreCase(TABLE_TYPE_TABLE)) 
hasValidTableType = true;
You can use equals() if you leave the toUpperCase() above.


http://gerrit.cloudera.org:8080/#/c/7353/3/fe/src/test/java/org/apache/impala/service/FrontendTest.java
File fe/src/test/java/org/apache/impala/service/FrontendTest.java:

http://gerrit.cloudera.org:8080/#/c/7353/3/fe/src/test/java/org/apache/impala/service/FrontendTest.java@181
PS3, Line 181: // It should return two tables: one in "default"db and one 
in "testdb1".
space after "default"


http://gerrit.cloudera.org:8080/#/c/7353/3/fe/src/test/java/org/apache/impala/service/FrontendTest.java@209
PS3, Line 209: assertEquals("alltypes_hive_view", 
resp.rows.get(0).colVals.get(2).string_val.toLowerCase());
Does this test succeed when run on its own? For these views to be returned they 
have to be loaded, but that's not necessarily the case when running this test 
on its own.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I90616388e6181cf342b3de389af940214ed46428
Gerrit-Change-Number: 7353
Gerrit-PatchSet: 3
Gerrit-Owner: sandeep akinapelli 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: sandeep akinapelli 
Gerrit-Comment-Date: Sat, 30 Sep 2017 03:13:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5448: fix invalid number of splits reported in Parquet scan node

[Quanlong Huang (Code Review)]

Quanlong Huang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8147 )

Change subject: IMPALA-5448: fix invalid number of splits reported in Parquet 
scan node
..


Patch Set 2:

Hi Tim Armstrong and Mostafa Mokhtar,

I've uploaded a new patch after review. Could you reexamine it?
 https://gerrit.cloudera.org/#/c/8147/

Thanks


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaacc2d775032f5707061e704f12e0a63cde695d1
Gerrit-Change-Number: 8147
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 30 Sep 2017 03:13:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4951: Fix database visibility for user with only column privilege

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4951: Fix database visibility for user with only column 
privilege
..


Patch Set 4: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id77904876729c0223fd6ace2d5e7199bd700a33a
Gerrit-Change-Number: 8168
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Sat, 30 Sep 2017 02:28:00 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 12: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 12
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 30 Sep 2017 02:08:32 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7805 )

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 10:

(12 comments)

> Patch Set 10: Code-Review+1
>
> (13 comments)
>
> I read through the code, but I still found it difficult to understand. 
> Partially it seems to be a more concise reimplementation of the server-side 
> code. Ideally we'd rework our server-side code itself, so that it's more 
> obviously correct and easier to test.
>
> Hopefully others will find it easy enough to understand and modify this test. 
> To simplify it a bit, you could replace some of the pairs with structs, thus 
> reducing the use of "first" and "second".
>
> That being said, I think that the complexity is ok for a test, and I 
> appreciate the effort you put into this!
>
> I also pointed out a few spelling mistakes.

Thanks for the review.
Most of the pairs is removed in the latest patch. I hope it's easier to read 
now.

http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@34
PS10, Line 34: a
> nit: an
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@73
PS10, Line 73: lb
> Can you name those lower_bound and upper_bound for readability?
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@75
PS10, Line 75: T
> const T&?
In the next line there is an assignment `if (boundary == -1) boundary = 0;` so 
it cannot be a reference here.


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@75
PS10, Line 75:
> nit: remove space
Done.


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@76
PS10, Line 76: These
> I don't understand which options this comment refers to. Can you rephrase i
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@105
PS10, Line 105: accepts
> nit: typo
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@106
PS10, Line 106: threat
> nit: typo
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@106
PS10, Line 106: has
> have
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@146
PS10, Line 146:
> nit: space
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@148
PS10, Line 148: auto
> specify the type
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@172
PS10, Line 172:   // CASE(make_pair("prefetch_mode", _mode),
> This comment looks stale
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@226
PS10, Line 226: have
> has
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 30 Sep 2017 01:57:29 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Tianyi Wang (Code Review)]

Hello Lars Volker, Tim Armstrong, Alex Behm, Dan Hecht,

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

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

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

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..

IMPALA-5425: Add test for validating input when setting query options

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 268 insertions(+), 154 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2636: HS2 GetTables() returns TABLE TYPE as TABLE for VIEW

[sandeep akinapelli (Code Review)]

Hello Matthew Jacobs, Alex Behm, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for 
VIEW
..

IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for VIEW

Added code to read the table type from metastore table but defaults to
"TABLE" if the metastore table is not loaded.
After the change, GetTabletypes also returns "VIEW" apart from "TABLE"
Changed unit and jdbc testcases for GetTableTypes.
Added new Frontend test for reading views.

Change-Id: I90616388e6181cf342b3de389af940214ed46428
---
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M fe/src/test/java/org/apache/impala/service/FrontendTest.java
M fe/src/test/java/org/apache/impala/service/JdbcTest.java
3 files changed, 136 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I90616388e6181cf342b3de389af940214ed46428
Gerrit-Change-Number: 7353
Gerrit-PatchSet: 3
Gerrit-Owner: sandeep akinapelli 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: sandeep akinapelli 

[Impala-ASF-CR] IMPALA-2636: HS2 GetTables() returns TABLE TYPE as TABLE for VIEW

[sandeep akinapelli (Code Review)]

sandeep akinapelli has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7353 )

Change subject: IMPALA-2636: HS2 GetTables() returns TABLE_TYPE as TABLE for 
VIEW
..


Patch Set 3:

(12 comments)

review comments

http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java
File fe/src/main/java/org/apache/impala/service/MetadataOp.java:

http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@63
PS2, Line 63:   // Result set schema for each of the metadata operations.
> TABLE_TYPE_INDEX
removed altogether


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@77
PS2, Line 77:
> Remove "INDEX", see comment below
Done


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@221
PS2, Line 221:
> nit: tale -> table
Done


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@494
PS2, Line 494:   upperCaseTableTypes = Lists.newArrayList();
> upperCaseTableTypes
Done


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@495
PS2, Line 495:   for (String tableType : tableTypes) {
> Let's add tests to cover passing in TABLE, VIEW or both.
added tests


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@514
PS2, Line 514: String tableType = dbsMetadata.tableTypes.get(i).get(j);
> remove blank line (the following check still belongs to tableType)
Done


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@515
PS2, Line 515: if (upperCaseTableTypes != null && 
!upperCaseTableTypes.contains(tableType)) continue;
> No need tor tableType.toUpperCase() - we know if must already be upper case
True. removed the unneccesary toupper


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/main/java/org/apache/impala/service/MetadataOp.java@635
PS2, Line 635:* Fills the GET_TYPEINFO_RESULTS with "TABLE", "VIEW".
> I'm thinking we should remove "INDEX" here so as not to confuse clients and
Done


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/FrontendTest.java
File fe/src/test/java/org/apache/impala/service/FrontendTest.java:

http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/FrontendTest.java@244
PS2, Line 244:
> nit: Issue
Done


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/FrontendTest.java@308
PS2, Line 308: // same as ODBC SQLColumns, which has 18 columns. But the 
HS2 implementation has
> Remove, I don't think we should advertise this as a valid table type - Impa
Done


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/JdbcTest.java
File fe/src/test/java/org/apache/impala/service/JdbcTest.java:

http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/JdbcTest.java@193
PS2, Line 193: ResultSet rs = con_.getMetaData().getTabl
> stale comment?
Yep. removed.


http://gerrit.cloudera.org:8080/#/c/7353/2/fe/src/test/java/org/apache/impala/service/JdbcTest.java@200
PS2, Line 200:   }
> remove
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I90616388e6181cf342b3de389af940214ed46428
Gerrit-Change-Number: 7353
Gerrit-PatchSet: 3
Gerrit-Owner: sandeep akinapelli 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: sandeep akinapelli 
Gerrit-Comment-Date: Sat, 30 Sep 2017 01:53:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5994: Lower case struct-field names

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-5994: Lower case struct-field names
..

IMPALA-5994: Lower case struct-field names

Impala tries to always store column names in lower case. As part of a
cleanup of issues related to upper case Kudu column names, a check was
added in Analyzer to enforce this.

The check fails when doing star expansion on a struct to select all
fields in the case where a table was created in Hive with upper case
letters in a struct field name. This happens because Hive does not
covert struct field names to all lower case in HMS.

The solution is to force StructField names to lower case.

Testing:
- Added a test in test_nested_types.py
- Fixed FE test that expected struct field to be output in upper case.

Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Reviewed-on: http://gerrit.cloudera.org:8080/8169
Reviewed-by: Thomas Tauber-Marshall 
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/catalog/StructField.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M tests/query_test/test_nested_types.py
3 files changed, 19 insertions(+), 2 deletions(-)

Approvals:
  Thomas Tauber-Marshall: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5994: Lower case struct-field names

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-5994: Lower case struct-field names
..


Patch Set 4: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Sat, 30 Sep 2017 01:13:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

[Matthew Mulder (Code Review)]

Matthew Mulder has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8102 )

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 12:

The stress test is at tests/stress/concurrent_select.py. The list of queries to 
run is created in load_tpc_queries(). The queries are initially executed in 
populate_all_queries(). Any query error in populate_all_queries() seems to be 
fatal. I see two general choices:
1. Do something in load_tpc_queries() to blacklist the unsupported queries.
2. Do something in main() before calling populate_all_queries() to remove the 
unsupported queries.
There are a few options for what "do something" would be.
I wouldn't recommend simply skipping queries with errors in 
populate_all_queries() because that could conceal a regression.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-Change-Number: 8102
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Sat, 30 Sep 2017 00:59:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5951: Remove flaky test catalogd timeout

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-5951: Remove flaky test_catalogd_timeout
..


Patch Set 2: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29fd67d0acc0ee15943c416f2179ad716d2cac05
Gerrit-Change-Number: 8154
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Sat, 30 Sep 2017 00:50:42 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5951: Remove flaky test catalogd timeout

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-5951: Remove flaky test_catalogd_timeout
..

IMPALA-5951: Remove flaky test_catalogd_timeout

test_catalogd_timeout sets a Kudu operation timeout of 1ms and then
performs various Kudu operations which it expects to fail due to a
timeout.

Since the test was written, things have sped up - for example, Impala
used to create a new Kudu client for each operation, but that was
changed in IMPALA-5167, such that the operations now occasionally
complete quickly enough that they don't timeout.

There's not really any way to rewrite this test to ensure that it
won't be flaky, so the patch removes it.

Change-Id: I29fd67d0acc0ee15943c416f2179ad716d2cac05
Reviewed-on: http://gerrit.cloudera.org:8080/8154
Reviewed-by: Dimitris Tsirogiannis 
Tested-by: Impala Public Jenkins
---
D 
testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-catalogd.test
M tests/custom_cluster/test_kudu.py
2 files changed, 0 insertions(+), 33 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I29fd67d0acc0ee15943c416f2179ad716d2cac05
Gerrit-Change-Number: 8154
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 10:

I'd like to give Dan and Alex the chance to have a look, too.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 30 Sep 2017 00:20:31 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

[Matthew Mulder (Code Review)]

Matthew Mulder has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8102 )

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8102/12/testdata/workloads/tpcds/queries/tpcds-q36.test
File testdata/workloads/tpcds/queries/tpcds-q36.test:

http://gerrit.cloudera.org:8080/#/c/8102/12/testdata/workloads/tpcds/queries/tpcds-q36.test@8
PS12, Line 8: grouping
> Impala doesn't seem to accept this syntax.
I should have mentioned that this turns out to be a fatal error for the stress 
test.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-Change-Number: 8102
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Sat, 30 Sep 2017 00:16:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

[Matthew Mulder (Code Review)]

Matthew Mulder has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8102 )

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8102/12/testdata/workloads/tpcds/queries/tpcds-q36.test
File testdata/workloads/tpcds/queries/tpcds-q36.test:

http://gerrit.cloudera.org:8080/#/c/8102/12/testdata/workloads/tpcds/queries/tpcds-q36.test@8
PS12, Line 8: grouping
Impala doesn't seem to accept this syntax.
impala.error.HiveServer2Error: AnalysisException: 
tpcds_300_decimal_parquet.grouping() unknown



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-Change-Number: 8102
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Sat, 30 Sep 2017 00:15:06 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7061 )

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 8:

> No problem, I just got back from a month long wedding/honeymoon
 > myself.
 >
 > I'll try to address your comments in the next few days.
 >
 > > (3 comments)
 > >
 > > Sorry for the slow response and thanks for your patience, John.
 > > Henry is away on holiday, but let's try to get this in in the
 > > meantime. The patch looks good to me. I just have a couple small
 > > comments. And rebasing the patch before posting the next patch
 > set
 > > would be a good idea.

Sounds good. And congratulations!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-Change-Number: 7061
Gerrit-PatchSet: 8
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 23:52:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] PREVIEW: IMPALA-5307: Part 4: copy out uncompressed text and seq

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8172 )

Change subject: PREVIEW: IMPALA-5307: Part 4: copy out uncompressed text and seq
..

PREVIEW: IMPALA-5307: Part 4: copy out uncompressed text and seq

This is the final patch for IMPALA-5307.

Text and Seq scanners are converted to use the same approach
as Avro.

contains_tuple_data is now false so a bunch of dead code in
ScannerContext can be removed. We also no longer attach I/O
buffers to row batches so that logic can be removed.

Testing:
Ran core ASAN tests.

Perf:
TODO - need to run numbers

Change-Id: I304fd002b61bfedf41c8b1405cd7eb7b492bb941
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-avro-scanner.h
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scanner-ir.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M be/src/runtime/row-batch.cc
M be/src/runtime/row-batch.h
20 files changed, 118 insertions(+), 192 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I304fd002b61bfedf41c8b1405cd7eb7b492bb941
Gerrit-Change-Number: 8172
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] PREVIEW: IMPALA-5307: Part 4: copy out uncompressed text and seq

[Tim Armstrong (Code Review)]

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

Change subject: PREVIEW: IMPALA-5307: Part 4: copy out uncompressed text and seq
..


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/exec/exec-node.h@109
PS1, Line 109:   /// Caller must not be holding any io buffers. This will cause 
deadlock.
> Remove - not relevant even before this patch
Done


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

http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/exec/hdfs-scanner.h@99
PS1, Line 99: /// resources (IO buffers and mem pools) to the current row 
batch, and passing row batches
> Needs update
Done


http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:

http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h@218
PS1, Line 218:   /// make it consistent between buffers and I/O buffers.
> Needs update
Done


http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h@254
PS1, Line 254:   /// pool and io buffers.
> needs update
Done


http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h@275
PS1, Line 275:   /// Acquires state from the 'src' row batch into this row 
batch. This includes all IO
> Needs update
Done


http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h@397
PS1, Line 397:   /// Sum of all auxiliary bytes. This includes IoBuffers and 
memory from
> Old reference to IoBuffers. Can probably rename to total_buffer_bytes_ or s
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I304fd002b61bfedf41c8b1405cd7eb7b492bb941
Gerrit-Change-Number: 8172
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 23:51:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

[John Sherman (Code Review)]

John Sherman has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7061 )

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 8:

No problem, I just got back from a month long wedding/honeymoon myself.

I'll try to address your comments in the next few days.

 > (3 comments)
 >
 > Sorry for the slow response and thanks for your patience, John.
 > Henry is away on holiday, but let's try to get this in in the
 > meantime. The patch looks good to me. I just have a couple small
 > comments. And rebasing the patch before posting the next patch set
 > would be a good idea.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-Change-Number: 7061
Gerrit-PatchSet: 8
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 23:48:59 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5394: Change ThriftServer() to always use TAcceptQueueServer

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7061 )

Change subject: IMPALA-5394: Change ThriftServer() to always use 
TAcceptQueueServer
..


Patch Set 8:

(3 comments)

Sorry for the slow response and thanks for your patience, John. Henry is away 
on holiday, but let's try to get this in in the meantime. The patch looks good 
to me. I just have a couple small comments. And rebasing the patch before 
posting the next patch set would be a good idea.

http://gerrit.cloudera.org:8080/#/c/7061/8/be/src/rpc/thrift-server-test.cc
File be/src/rpc/thrift-server-test.cc:

http://gerrit.cloudera.org:8080/#/c/7061/8/be/src/rpc/thrift-server-test.cc@403
PS8, Line 403: TEST(ConcurrencyTest, MaxConcurrentConnections) {
Could you write a brief comment about what this test is trying to achieve?


http://gerrit.cloudera.org:8080/#/c/7061/8/be/src/rpc/thrift-server-test.cc@439
PS8, Line 439:   EXPECT_TRUE(did_reach_max);
> Not sure if this will be effective. Could end up being racy? If so probably
It looks like it could be racy, but very rarely. We have no control over how 
the OS would schedule these threads. We can revisit this test later if it shows 
up to be a problem, but for now, I'm okay with it.


http://gerrit.cloudera.org:8080/#/c/7061/3/be/src/transport/TSaslServerTransport.cpp
File be/src/transport/TSaslServerTransport.cpp:

http://gerrit.cloudera.org:8080/#/c/7061/3/be/src/transport/TSaslServerTransport.cpp@164
PS3, Line 164:   new TSaslServerTransport(serverDefinitionMap_, trans));
It would be good to add the comment here acknowledging the "logical" race, i.e. 
we know a logical race exists but it won't ever happen because getTransport() 
won't be called concurrently from different threads with the same 'trans' 
object.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-Change-Number: 7061
Gerrit-PatchSet: 8
Gerrit-Owner: John Sherman 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: John Sherman 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 23:45:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Re-apply: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

[Dan Hecht (Code Review)]

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

Change subject: Re-apply: IMPALA-5589: change "set" in impala-shell to show 
empty string for unset query options
..


Patch Set 3:

But I'll wait to submit until change 8070 goes in, in case that one fails.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29f5d8ab874cb1338077f16019a9537766cac0c4
Gerrit-Change-Number: 8096
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 29 Sep 2017 23:25:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] Re-apply: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

[Dan Hecht (Code Review)]

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

Change subject: Re-apply: IMPALA-5589: change "set" in impala-shell to show 
empty string for unset query options
..


Patch Set 3: Code-Review+2

Cool, thanks for clarifying.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29f5d8ab874cb1338077f16019a9537766cac0c4
Gerrit-Change-Number: 8096
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 29 Sep 2017 23:24:32 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4939, IMPALA-4940: Decimal V2 multiplication

[Dan Hecht (Code Review)]

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

Change subject: IMPALA-4939, IMPALA-4940: Decimal V2 multiplication
..


Patch Set 6:

(7 comments)

mostly just some questions.

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

http://gerrit.cloudera.org:8080/#/c/7438/6//COMMIT_MSG@67
PS6, Line 67:   after:  126.17s
is it always the case that this regression is in cases that would have 
(incorrectly) returned null before this change?


http://gerrit.cloudera.org:8080/#/c/7438/6/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

http://gerrit.cloudera.org:8080/#/c/7438/6/be/src/runtime/decimal-value.inline.h@a253
PS6, Line 253:
great to see that go away


http://gerrit.cloudera.org:8080/#/c/7438/6/be/src/runtime/decimal-value.inline.h@245
PS6, Line 245: overflow
instead of saying "we will overflow", I think "we indicate overflow" or "return 
overflow" or something like that. I was trying to figure out where the overflow 
occurs until I continued reading.


http://gerrit.cloudera.org:8080/#/c/7438/6/be/src/runtime/decimal-value.inline.h@244
PS6, Line 244: If delta scale is 0, we need to do a more precise check if 
converting to 256 bits
 :   // is necessary
I don't understand that comment, given that line 240-241 says the first check 
was conservative.


http://gerrit.cloudera.org:8080/#/c/7438/6/be/src/runtime/decimal-value.inline.h@273
PS6, Line 273:   // with a delta_scale 39 does not fit into int128.
so normally the (38,38) * (38,38) case would be handled in the needs_int256 
case, is that right?  is this case just when the values are so small that they 
turn into 0?


http://gerrit.cloudera.org:8080/#/c/7438/6/be/src/runtime/decimal-value.inline.h@284
PS6, Line 284:
nit: let's eliminate this blank line (and maybe others. this function already 
barely fits on one screen which is important for readability.


http://gerrit.cloudera.org:8080/#/c/7438/6/be/src/util/bit-util.h
File be/src/util/bit-util.h:

http://gerrit.cloudera.org:8080/#/c/7438/6/be/src/util/bit-util.h@292
PS6, Line 292: __int128 shifted = v >> 64;
 : if (shifted > 0) {
given that 'v' is unsigned, won't the shift be unsigned, meaning 'shifted' will 
always get zeros in the top 64, meaning shifted is always >= 0?  oh, so is this 
really just checking "shifted != 0"?  If so, that seems clearer, especially 
since the else-clause only makes sense for 0, not for negative.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I37ad6232d7953bd75c18dc86e665b2b501a1ebe1
Gerrit-Change-Number: 7438
Gerrit-PatchSet: 6
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Fri, 29 Sep 2017 23:23:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Re-apply: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

[Philip Zeyliger (Code Review)]

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

Change subject: Re-apply: IMPALA-5589: change "set" in impala-shell to show 
empty string for unset query options
..


Patch Set 3:

> Patch Set 3:
>
> > Patch Set 3:
> >
> > Is this the same patch as before (I.e. can we just carry forward that +2), 
> > or did something in this diff change since the +2?
>
> This is a computer-generated revert-of-a-revert with a slight change to the 
> commit message.
>
> I double checked with git patch-id; they're the same diff.
>
> $git log --oneline --grep IMPALA-5589 | grep -v Revert
> 598b72c Re-apply: IMPALA-5589: change "set" in impala-shell to show empty 
> string for unset query options
> 387bde0 IMPALA-5589: change "set" in impala-shell to show empty string for 
> unset query options
>
> $git show 387bde0 | git-patch-id --stable; git show 598b72c | git-patch-id 
> --stable
> 47b02290d8b30ffa520ea19dac8353075c5a5bc7 
> 387bde0639ffd8ef580ccbf727152954e62bacbe
> 47b02290d8b30ffa520ea19dac8353075c5a5bc7 
> 598b72cf43a326dd3c8a9a35994f8007db665b93

And, yes, I think we should carry forward the +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29f5d8ab874cb1338077f16019a9537766cac0c4
Gerrit-Change-Number: 8096
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:57:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] Re-apply: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

[Philip Zeyliger (Code Review)]

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

Change subject: Re-apply: IMPALA-5589: change "set" in impala-shell to show 
empty string for unset query options
..


Patch Set 3:

> Patch Set 3:
>
> Is this the same patch as before (I.e. can we just carry forward that +2), or 
> did something in this diff change since the +2?

This is a computer-generated revert-of-a-revert with a slight change to the 
commit message.

I double checked with git patch-id; they're the same diff.

$git log --oneline --grep IMPALA-5589 | grep -v Revert
598b72c Re-apply: IMPALA-5589: change "set" in impala-shell to show empty 
string for unset query options
387bde0 IMPALA-5589: change "set" in impala-shell to show empty string for 
unset query options

$git show 387bde0 | git-patch-id --stable; git show 598b72c | git-patch-id 
--stable
47b02290d8b30ffa520ea19dac8353075c5a5bc7 
387bde0639ffd8ef580ccbf727152954e62bacbe
47b02290d8b30ffa520ea19dac8353075c5a5bc7 
598b72cf43a326dd3c8a9a35994f8007db665b93


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29f5d8ab874cb1338077f16019a9537766cac0c4
Gerrit-Change-Number: 8096
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:56:48 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5383: [DOCS] Document unpartitioned Kudu tables

[John Russell (Code Review)]

John Russell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8180 )

Change subject: IMPALA-5383: [DOCS] Document unpartitioned Kudu tables
..


Patch Set 3:

Thomas, can I tag you in for a few Kudu-related reviews for 5.13, in place of 
MJ?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2b466e1e482d62de84253c0cb406668fd5ad5eb
Gerrit-Change-Number: 8180
Gerrit-PatchSet: 3
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:43:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

[Tim Wood (Code Review)]

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

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 12:

Uploaded patch to fix section separator.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-Change-Number: 8102
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:39:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

[Tim Wood (Code Review)]

Hello Matthew Mulder, Michael Brown, David Knupp, Alex Behm, Mostafa Mokhtar, 
Michael Ho,

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

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

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

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..

IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

Main source for TPCDS query and result definitions: 
https://github.com/gregrahn/tpcds-kit.
TPC-DS v2.5.0 qualification queries from G. Rahn, Cloudera, Inc.
Data set constructed in mini-cluster using $IMPALA_HOME/buildall.sh 
-testdata
This commit continues previous work on IMPALA-5376 in the ASF Impala repo
and the Cloudera Gerrit service.

This commit splits multi-query tests in the TPC-DS suite definition into one
query and result set per test file, as the test framework requires.  Names for
such files have -1, -2... inner suffixes.

The complete TPC-DS test suite runs with passes, skips and xfails,
but no failures, as reflected by runs of
$IMPALA_HOME/tests/run-tests.py query_test/test_tpcds_queries.py ...
Expected result sets come from the TPC-DS kit.  Some TPC-DS test cases
in this commit have been modified in sematically-neutral ways so as to pass
on Impala; others are marked to skip or xfail due to bugs.  Tests that flap
are marked to skip, with a bug ID, since they don't reliably pass or xfail.
The tests/query_test/test_tpcds_queries.py driver file is authoritative for the
active/skip/xfail status for each case and a brief reason.  The following list
describes the current status as:
--- test-name
deviance from TPC-DS spec
changes made

--- tpcds-q22a.test
RESULT MISMATCH in LSD of AVG() values
Fixed AVG()s
--- tpcds-q30.test
UNRECOGNIZED CHARACTER
MARKED XFAIL, IMPALA-5961.
--- tpcds-q31.test
RESULT MISMATCH in LSD of DECIMAL values
ADDED TRUNCATE(2)s AROUND LAST 4 COLUMNS. MARKED SKIP, IMPALA-5956
--- tpcds-q35a.test
RESULT MISMATCH
MARKED XFAIL, IMPALA-5950.
--- tpcds-q36a.test
RESULT MISMATCH
MARKED XFAIL, IMPALA-4741
--- tpcds-q39.test
MULTIPLE RESULT SET not recognized by test framework
MARKED XFAIL.
--- tpcds-q47.test
RESULT MISMATCH in LSD of DECIMAL values
ADDED TRUNCATE(2) TO 8th COLUMN OF WITH TABLE, TAKE ACTUAL RESULT AS EXPECTED.
--- tpcds-q49.test
RESULT MISMATCH in LSD of DECIMAL values
MARKED XFAIL, IMPALA-5945
--- tpcds-q57.test
RESULT MISMATCH, excess scale in DECIMAL values
FIXED, ADDED TRUNCATE(2) AROUND 6th COLUMN.
--- tpcds-q58.test
RESULT MISMATCH in DECIMAL values
MARKED XFAIL. IMPALA-5946
--- tpcds-q59.test
RESULT MISMATCH, excess scale in DECIMAL values
FIXED, ADDED TRUNCATE(2) AROUND 4th-10th COLUMNS.
--- tpcds-q61.test
RESULT MISMATCH in DECIMAL value
FIXED. CAST RESULT QUOTIENT TO DECIMAL(15, 4), TAKE ACTUAL RESULT AS EXPECTED
--- tpcds-q63.test
RESULT MISMATCH, excess scale in DECIMAL values
ADDED TRUNCATE(2) TO 3rd COLUMN
--- tpcds-q64.test
RESULT MISMATCH
ADDED ORDER BY COLUMNS.
--- tpcds-q66.test
RESULT MISMATCH
MARKED XFAIL, IMPALA-4741
--- tpcds-q77a.test
RESULT MISMATCH
FIXED. TAKE ACTUAL RESULT AS EXPECTED
--- tpcds-q78.test
RESULT MISMATCH
FIXED. TAKE ACTUAL RESULT AS EXPECTED
--- tpcds-q83.test
RESULT MISMATCH
MARKED XFAIL. IMPALA-5945.
--- tpcds-q85.test
MISSING TABLE "reason"
MARKED XFAIL, IMPALA-5960
--- tpcds-q86a.test
RESULT MISMATCH
FIXED. TAKE ACTUAL RESULT AS EXPECTED
--- tpcds-q89.test
RESULT MISMATCH, DECIMAL values flap
MARKED XFAIL. ADDED ROUND(2) TO 8th COLUMN, TAKE ACTUAL RESULTS AS EXPECTED, 
IMPALA-5956.
--- tpcds-q90.test
RESULT MISMATCH
MARKED XFAIL, IMPALA-5945.
--- tpcds-q93.test
MISSING TABLE "reason"
MARKED XFAIL, IMPALA-5960
--- tpcds-q98.test
RESULT MISMATCH
FIXED, ADDED ROUND() TO LAST COLUMN

IMPALA-5986: Allow SET option names to contain digits when resetting them 
between queries.

Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
---
A testdata/workloads/tpcds/queries/tpcds-q10.test
A testdata/workloads/tpcds/queries/tpcds-q10a.test
A testdata/workloads/tpcds/queries/tpcds-q11.test
A testdata/workloads/tpcds/queries/tpcds-q12.test
A testdata/workloads/tpcds/queries/tpcds-q13.test
A testdata/workloads/tpcds/queries/tpcds-q14-1.test
A testdata/workloads/tpcds/queries/tpcds-q14-2.test
A testdata/workloads/tpcds/queries/tpcds-q14a-1.test
A testdata/workloads/tpcds/queries/tpcds-q14a-2.test
A testdata/workloads/tpcds/queries/tpcds-q15.test
A testdata/workloads/tpcds/queries/tpcds-q16.test
A testdata/workloads/tpcds/queries/tpcds-q17.test
A testdata/workloads/tpcds/queries/tpcds-q18.test
A testdata/workloads/tpcds/queries/tpcds-q18a.test
A testdata/workloads/tpcds/queries/tpcds-q20.test
A testdata/workloads/tpcds/queries/tpcds-q21.test
A testdata/workloads/tpcds/queries/tpcds-q22.test
A testdata/workloads/tpcds/queries/tpcds-q22a.test
M testdata/workloads/tpcds/queries/tpcds-q23-1.test
M testdata/workloads/tpcds/queries/tpcds-q23-2.test
A testdata/workloads/tpcds/queries/tpcds-q24-1.test

[Impala-ASF-CR] IMPALA-4252: Move runtime filters to ScanNode

[Thomas Tauber-Marshall (Code Review)]

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

Change subject: IMPALA-4252: Move runtime filters to ScanNode
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.h
File be/src/exec/scan-node.h:

http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.h@195
PS1, Line 195: Scanner
> Comment needs an update - this is no longer an argument.
Done


http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.h@198
PS1, Line 198: rs to arrive, check
> Both HdfsScanNodeBase and KuduScanNodeBase have a "RuntimeState* runtime_st
Done


http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.cc
File be/src/exec/scan-node.cc:

http://gerrit.cloudera.org:8080/#/c/8148/1/be/src/exec/scan-node.cc@79
PS1, Line 79: // TODO: Move this to Prepare()
> Does this comment still make sense? I wish whoever added it had mentioned w
Not sure, it was left by Michael (I'll ping him about it) in the review 
"Disentangle Expr and ExprContext"

You asked about it during that review too, and his response was "I am thinking 
of moving it when PlanNode is introduced." but I'm not sure what that means.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I17bdc869046dc2cd837d02f333441fa6324ff086
Gerrit-Change-Number: 8148
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:34:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4252: Move runtime filters to ScanNode

[Thomas Tauber-Marshall (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-4252: Move runtime filters to ScanNode
..

IMPALA-4252: Move runtime filters to ScanNode

As a preliminary step towards adding runtime filters for Kudu scans,
this patch moves runtime filter related code from HdfsScanNodeBase to
ScanNode so that it's available to KuduScanNodeBase.

The change was mechanical with no logic changes, except for moving the
calculation of the max wait time into WaitForRuntimeFilters().

Testing:
- Ran existing runtime filter tests.

Change-Id: I17bdc869046dc2cd837d02f333441fa6324ff086
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
6 files changed, 131 insertions(+), 114 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17bdc869046dc2cd837d02f333441fa6324ff086
Gerrit-Change-Number: 8148
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4951: Fix database visibility for user with only column privilege

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4951: Fix database visibility for user with only column 
privilege
..


Patch Set 4:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id77904876729c0223fd6ace2d5e7199bd700a33a
Gerrit-Change-Number: 8168
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:24:08 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4951: Fix database visibility for user with only column privilege

[Bikramjeet Vig (Code Review)]

Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8168 )

Change subject: IMPALA-4951: Fix database visibility for user with only column 
privilege
..


Patch Set 4: Code-Review+2

rebased, carried +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id77904876729c0223fd6ace2d5e7199bd700a33a
Gerrit-Change-Number: 8168
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:23:58 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 5: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:20:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5990: Part 1: JNI-based LZ4 de/compression

[Bharath Vissapragada (Code Review)]

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

Change subject: IMPALA-5990: Part 1: JNI-based LZ4 de/compression
..


Patch Set 3:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

http://gerrit.cloudera.org:8080/#/c/8150/2/be/src/service/fe-support.cc@454
PS2, Line 454:
> My understanding is that the copied bytes would also be freed via ReleasePr
I'm not fully familiar with how it works internally, but per my reading of the 
JVM code, all this method does is to unlock the GC thread[1]. It looks to me 
like it doesn't free the copied bytes (I could be wrong).

(If you think this case is possible, I think we should have code to perform 
cleanup of copies. Please ignore if you think that is not possible.)

[1] 
http://hg.openjdk.java.net/jdk8/jdk8/hotspot/file/87ee5ee27509/src/share/vm/prims/jni.cpp#l4275


http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@452
PS3, Line 452: the
remove


http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@458
PS3, Line 458: Note that it does not matter if 'src' was copied
Why don't we treat it as an error? This could mean an OOM on the JVM?


http://gerrit.cloudera.org:8080/#/c/8150/3/be/src/service/fe-support.cc@471
PS3, Line 471: "
May be better to add that the JVM could've run out of memory (supportability).


http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java
File fe/src/main/java/org/apache/impala/service/FeSupport.java:

http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java@289
PS2, Line 289:  - Storing the uncompressed data size allows decompression to 
pre-allocate its output
 :* - Decompression requires the exact compressed data size, so 
storing it allows us to
 :*   avoid compacting the returned byte[]
 :*/
> I don't think that's right because NativeLz4CompressBound() returns 0 if th
Oh, I thought this was an overflow check. Just realized LZ4 has an inbuilt 
limit of 2113929216 bytes(~2GB)


http://gerrit.cloudera.org:8080/#/c/8150/2/fe/src/main/java/org/apache/impala/service/FeSupport.java@306
PS2, Line 306: if (compressedSize < 0) {
 :   throw new InternalException(
> Not sure what you mean. A Java byte[] cannot be shrunk without copying it i
Ok, sorry for not being clear. My point is, why have two versions, 
Lz4CompressToByteBuffer() and Lz4Compress(). Is that a requirement for the 
follow on patch?


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/main/java/org/apache/impala/service/FeSupport.java
File fe/src/main/java/org/apache/impala/service/FeSupport.java:

http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/main/java/org/apache/impala/service/FeSupport.java@306
PS3, Line 306: if (compressedSize < 0) {
Looks like this check should be <=.

Per the documentation of LZ4_compress(),

return : the number of bytes written in buffer dest
 or 0 if the compression fails


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java
File fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java:

http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java@66
PS3, Line 66: } catch (Throwable e) {
I think fail() throws an "Error" (AssertionError) which this Throwable can 
catch. May be make it Exception?


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java@72
PS3, Line 72: e.printStackTrace();
unreachable?


http://gerrit.cloudera.org:8080/#/c/8150/3/fe/src/test/java/org/apache/impala/service/Lz4CompressionTest.java@78
PS3, Line 78:   private byte[] seqPayload(int numBytes) {
Add one liner comments on these helpers.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I237802944875b07080db0159ff8ec548150fd95e
Gerrit-Change-Number: 8150
Gerrit-PatchSet: 3
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:16:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Re-apply: IMPALA-5589: change "set" in impala-shell to show empty string for unset query options

[Dan Hecht (Code Review)]

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

Change subject: Re-apply: IMPALA-5589: change "set" in impala-shell to show 
empty string for unset query options
..


Patch Set 3:

Is this the same patch as before (I.e. can we just carry forward that +2), or 
did something in this diff change since the +2?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29f5d8ab874cb1338077f16019a9537766cac0c4
Gerrit-Change-Number: 8096
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:08:17 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 12:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 12
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:06:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Dan Hecht (Code Review)]

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 12: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 12
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:06:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4939, IMPALA-4940: Decimal V2 multiplication

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4939, IMPALA-4940: Decimal V2 multiplication
..


Patch Set 6: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I37ad6232d7953bd75c18dc86e665b2b501a1ebe1
Gerrit-Change-Number: 7438
Gerrit-PatchSet: 6
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:04:49 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5994: Lower case struct-field names

[Thomas Tauber-Marshall (Code Review)]

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

Change subject: IMPALA-5994: Lower case struct-field names
..


Patch Set 4: Code-Review+2

Carrying forward +2.

Gvo failed due to trivial test issue.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 29 Sep 2017 21:14:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5994: Lower case struct-field names

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-5994: Lower case struct-field names
..


Patch Set 4:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 29 Sep 2017 21:13:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5994: Lower case struct-field names

[Thomas Tauber-Marshall (Code Review)]

Hello Lars Volker, Alex Behm, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-5994: Lower case struct-field names
..

IMPALA-5994: Lower case struct-field names

Impala tries to always store column names in lower case. As part of a
cleanup of issues related to upper case Kudu column names, a check was
added in Analyzer to enforce this.

The check fails when doing star expansion on a struct to select all
fields in the case where a table was created in Hive with upper case
letters in a struct field name. This happens because Hive does not
covert struct field names to all lower case in HMS.

The solution is to force StructField names to lower case.

Testing:
- Added a test in test_nested_types.py
- Fixed FE test that expected struct field to be output in upper case.

Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
---
M fe/src/main/java/org/apache/impala/catalog/StructField.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M tests/query_test/test_nested_types.py
3 files changed, 19 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..


Patch Set 5: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 21:08:50 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5994: Lower case struct-field names

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-5994: Lower case struct-field names
..


Patch Set 3: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 29 Sep 2017 20:53:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5951: Remove flaky test catalogd timeout

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-5951: Remove flaky test_catalogd_timeout
..


Patch Set 2:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29fd67d0acc0ee15943c416f2179ad716d2cac05
Gerrit-Change-Number: 8154
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 29 Sep 2017 20:46:21 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4951: Fix database visibility for user with only column privilege

[Dimitris Tsirogiannis (Code Review)]

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

Change subject: IMPALA-4951: Fix database visibility for user with only column 
privilege
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id77904876729c0223fd6ace2d5e7199bd700a33a
Gerrit-Change-Number: 8168
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Comment-Date: Fri, 29 Sep 2017 20:40:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5951: Remove flaky test catalogd timeout

[Dimitris Tsirogiannis (Code Review)]

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

Change subject: IMPALA-5951: Remove flaky test_catalogd_timeout
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29fd67d0acc0ee15943c416f2179ad716d2cac05
Gerrit-Change-Number: 8154
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 29 Sep 2017 20:40:41 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Philip Zeyliger (Code Review)]

Hello Bharath Vissapragada, Matthew Jacobs, Tim Armstrong, Dan Hecht, Impala 
Public Jenkins,

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

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

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

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET =""' will now unset an option within the session,
reverting it to its default state.

This change became necessary when "SET" started returning an empty
string for unset options which don't have a default. The test
infrastructure (impala_test_suite.py) resets options to what it thinks
is its defaults, and, when this broke, some ASAN builds started to fail,
presumably due to a timing issue with how we re-use connections between
tests.

Previously, SessionState copied over the default options from the server
when the session was created and then mutated that. To support unsetting
options at the session layer, this change keeps a pointer to the default
server settings, keeps separately the mutations, and overlays the
options each time they're requested. Similarly, for configuration
overlays that happen per-query, the overlay is now done explicitly,
because empty per-query overlay values (key=..., value="") now have no effect.

Because "set key=''" is ambiguous between "set to the empty string" and
"unset", it's now impossible to set to the empty string, at the session
layer, an option that is configured at a previous layer. In practice,
this is just debug_action and request_pool. debug_action is essentially
an internal tool. For request_pool, this means that setting the default
request_pool via impalad command line is now a bad idea, as it can't
be cleared at a per-session level. For request_pool, the correct
course of action for users is to use placement rules, and to have a
default placement rule.

Testing:
* Added a simple test that triggered this side-effect without this code.
  Specifically, "impala-python infra/python/env/bin/py.test 
tests/metadata/test_set.py -s"
  with the modified set.test triggers.
* Amended tests/custom_cluster/test_admission_controller.py; it was
  useful for testing these code paths.
* Added cases to query-options-test to check behavior for both
  defaulted and non-defaulted values.
* Added a custom cluster test that checks that overlays are
  working against
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
---
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
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-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
M tests/custom_cluster/test_admission_controller.py
A tests/custom_cluster/test_set_and_unset.py
M tests/hs2/hs2_test_suite.py
14 files changed, 266 insertions(+), 26 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-Change-Number: 8070
Gerrit-PatchSet: 11
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5940: Avoid log spew by using Status::Expected.

[Philip Zeyliger (Code Review)]

Hello Tim Armstrong, Alex Behm, Mostafa Mokhtar, Dan Hecht,

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

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

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

Change subject: IMPALA-5940: Avoid log spew by using Status::Expected.
..

IMPALA-5940: Avoid log spew by using Status::Expected.

In IMPALA-5926, we fixed a case where closing the session triggered a
stack trace in the logs which impacted performance for short-running
queries. Looking at log files from several active clusters, I identified a few
other cases where we could clean up log spew with the same (trivial)
approach.

The following messages will no longer log:

  "Failed to codegen MaterializeTuple() due to unsupported type: $0"
  "Not implemented for this format."
  "ScalarFnCall Codegen not supported for CHAR"
  "Could not codegen CodegenMaterializeExprs: "
  "Could not codegen TupleRowComparator::Compare(): $0"

These codegen related messages were happening at every execution node, and were
therefore very common.

In addition, the following messages, which were very frequent in the logs, now
will log, but without the back trace:

  "... Client ... timed-out during recv call."
  "Query Id ... not found."

To do this, I changed them from logging implicitly via Status(...)
to logging explicitly.

The snippet I used to identify these was:

  find . -type f -name '*IMPALAD*.gz' | xargs gzcat  | awk '/^I/ { if(x) { 
print x; } x = "" } /status.cc/ { x=" "; } { if(x) { x=x  $0 } }'  | sed -e 
's/0x[0-9a-fx]* //g' | sed -e 's/[0-9a-f]\{16\}:[0-9a-f]*/QUERYID/g' |  tr -s 
'\t' ' ' | tr '[0-9]' 'N' | sort | uniq -c  | sort -n | tee output.txt

I also analyzed some logs using SQL, against a pre-processed logs table:

  with v as (
select regexp_replace(
regexp_replace(
  translate(substr(message, 42), "\n\t", "  "),
  "[a-zA-Z0-9.-]*[.][a-zA-Z0-9-]*:[0-9]*",
  ""),
"@.*$", "@@@...") as m
from logs_table where `class`="status.cc")
  select m, count(*) from v group by 1 order by 2 desc limit 100

Testing:
* Automated tests.
* Manual testing for one of the new back-trace-suppressed paths:

  $ impala-python shell/gen-py/ImpalaService/ImpalaService-remote -h 
localhost:21000 GetRuntimeProfile 'beeswaxd.ttypes.QueryHandle()'
  Traceback (most recent call last):
File "shell/gen-py/ImpalaService/ImpalaService-remote", line 106, in 

  pp.pprint(client.GetRuntimeProfile(eval(args[0]),))
File "/home/philip/src/impala/shell/gen-py/ImpalaService/ImpalaService.py", 
line 161, in GetRuntimeProfile
  return self.recv_GetRuntimeProfile()
File "/home/philip/src/impala/shell/gen-py/ImpalaService/ImpalaService.py", 
line 184, in recv_GetRuntimeProfile
  raise result.error
  beeswaxd.ttypes.BeeswaxException: 
BeeswaxException(handle=QueryHandle(log_context='', id=''), log_context='', 
SQLState='HY000', _message='GetRuntimeProfile error: Query id 0:0 not 
found.\n', errorCode=0)

  $ grep 'Query id' logs/cluster/impalad.INFO | tail -n 1
  I0926 20:29:09.44  6787 impala-server.cc:642] Query id 0:0 not found.

Change-Id: I38088482377a1c3e794a9c8178ef83f29957a330
---
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/client-cache.h
M be/src/runtime/tuple.cc
M be/src/service/impala-server.cc
M be/src/util/tuple-row-compare.cc
7 files changed, 26 insertions(+), 19 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I38088482377a1c3e794a9c8178ef83f29957a330
Gerrit-Change-Number: 8100
Gerrit-PatchSet: 4
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5940: Avoid log spew by using Status::Expected.

[Philip Zeyliger (Code Review)]

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

Change subject: IMPALA-5940: Avoid log spew by using Status::Expected.
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/exec/hdfs-avro-scanner.cc
File be/src/exec/hdfs-avro-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/exec/hdfs-avro-scanner.cc@972
PS3, Line 972:   return Status::Expected(Substitute(
> Yes that's correct.
I double-checked a variant here. It shows up there. (It's not from this Avro 
code path, but they're similar. I wasn't able to quickly find an avro test 
table with the right types...)

[philip-dev.gce.cloudera.com:21000] > use functional_rc;
Query: use functional_rc
[philip-dev.gce.cloudera.com:21000] > create table t2 as select * from alltypes;
Query: create table t2 as select * from alltypes
...
HDFS_SCAN_NODE (id=0):(Total: 35.999ms, non-child: 35.999ms, % 
non-child: 100.00%)
  Hdfs split stats (:<# splits>/): 
-1:8/140.12 KB
  ExecOption: RC_FILE Codegen Disabled: Not implemented for this format.


http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/runtime/client-cache.h
File be/src/runtime/client-cache.h:

http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/runtime/client-cache.h@243
PS3, Line 243: string err = strings::Substitute("Client $0 timed-out 
during recv call.", TNetworkAddressToString(address_));
> long line
done.


http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/runtime/client-cache.h@281
PS3, Line 281: return 
Status::Expected(ErrorMsg(TErrorCode::RPC_RECV_TIMEOUT, err));
> same set of comment/question
done.


http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/runtime/tuple.cc
File be/src/runtime/tuple.cc:

http://gerrit.cloudera.org:8080/#/c/8100/3/be/src/runtime/tuple.cc@309
PS3, Line 309:   stringstream ss;
 :   ss << "Could not codegen CodegenMaterializeExprs: " << 
status.GetDetail();
> okay to ignore, but could clean that up with Substitute()
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I38088482377a1c3e794a9c8178ef83f29957a330
Gerrit-Change-Number: 8100
Gerrit-PatchSet: 3
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 19:51:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

[Matthew Mulder (Code Review)]

Matthew Mulder has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8102 )

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8102/10/testdata/workloads/tpcds/queries/tpcds-q48.test
File testdata/workloads/tpcds/queries/tpcds-q48.test:

http://gerrit.cloudera.org:8080/#/c/8102/10/testdata/workloads/tpcds/queries/tpcds-q48.test@1
PS10, Line 1: h
stress test fails on this file, probably because this 'h' should be '='.
Exception: Expected exactly 1 query to be in file 
tests/stress/../../testdata/workloads/tpcds/queries/tpcds-q48.test but got 0



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-Change-Number: 8102
Gerrit-PatchSet: 11
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Fri, 29 Sep 2017 19:48:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Add tpcds-unmodified performance workload.

[Matthew Mulder (Code Review)]

Matthew Mulder has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/5813


Change subject: Add tpcds-unmodified performance workload.
..

Add tpcds-unmodified performance workload.

Add Mostafa's tpcds-unmodified performance workload for use in the
performance test runs. This workload includes 97 queries.

Mostafa writes:
Before Impala 2.5 we didn't have Runtimefilters, which made TPC-DS
queries run very slow, so we ran queries that have explicit partition
filters to workaround the limitation.
Queries under "tpcds" has those explicit filters, post Impala 2.5 I
added the un-modified version of the workload to have more coverage.

Q24 and Q67 aren't added because the unmodified version of the query is
not supported by Impala and the rewrite version takes a very long time
to complete.

This workload has already been running nightly for many months
out of Mostafa's private branch. This checkin simply does a little
cleanup to prepare it for commit to the public repo.

How It Was Tested
-
impala-python -u bin/run-workload.py --client_type=beeswax
--workloads=tpcds-unmodified:_1000 --query_iterations 1
--workload_iterations 1
--exec_options='sync_ddl:1;num_scanner_threads:12;MAX_NUM_RUNTIME_FILTERS:50'
--impalads=:21000 --num_clients=1
--table_formats=parquet/none --query_names='.*'
--results_json_file=/tmp/benchmark_results.json

Change-Id: Iea8f3b4e20a30553f1a5a321b2053ff9ca967e92
---
A testdata/workloads/tpcds-unmodified/queries/tpcds-q1.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q10.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q11.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q12.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q13.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q14.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q15.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q16.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q17.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q18.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q19.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q2.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q20.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q21.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q22.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q23.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q25.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q26.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q27.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q28.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q29.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q3.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q30.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q31.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q32.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q33.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q34.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q35.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q36.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q37.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q38.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q39.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q4.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q40.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q41.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q42.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q43.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q44.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q45.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q46.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q47.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q48.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q49.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q5.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q50.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q51.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q52.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q53.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q54.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q55.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q56.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q57.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q58.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q59.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q6.test
A testdata/workloads/tpcds-unmodified/queries/tpcds-q60.test
A 

[Impala-ASF-CR] IMPALA-5383: [DOCS] Document unpartitioned Kudu tables

[John Russell (Code Review)]

John Russell has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/8180 )

Change subject: IMPALA-5383: [DOCS] Document unpartitioned Kudu tables
..

IMPALA-5383: [DOCS] Document unpartitioned Kudu tables

Change-Id: Ia2b466e1e482d62de84253c0cb406668fd5ad5eb
---
M docs/topics/impala_create_table.xml
1 file changed, 22 insertions(+), 6 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia2b466e1e482d62de84253c0cb406668fd5ad5eb
Gerrit-Change-Number: 8180
Gerrit-PatchSet: 3
Gerrit-Owner: John Russell 

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

[Tim Wood (Code Review)]

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

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 11:

> Uploaded patch set 11: Patch Set 10 was rebased.

Rebased onto master locally and pushed.  No conflicts found.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-Change-Number: 8102
Gerrit-PatchSet: 11
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Fri, 29 Sep 2017 19:35:49 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5951: Remove flaky test catalogd timeout

[Thomas Tauber-Marshall (Code Review)]

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

Change subject: IMPALA-5951: Remove flaky test_catalogd_timeout
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8154/1/testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-catalogd.test
File 
testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-catalogd.test:

http://gerrit.cloudera.org:8080/#/c/8154/1/testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-catalogd.test@24
PS1, Line 24:
> Isn't this test also susceptible to the same timing issue? I don't see much
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I29fd67d0acc0ee15943c416f2179ad716d2cac05
Gerrit-Change-Number: 8154
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:55:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5951: Remove flaky test catalogd timeout

[Thomas Tauber-Marshall (Code Review)]

Hello Dimitris Tsirogiannis,

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

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

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

Change subject: IMPALA-5951: Remove flaky test_catalogd_timeout
..

IMPALA-5951: Remove flaky test_catalogd_timeout

test_catalogd_timeout sets a Kudu operation timeout of 1ms and then
performs various Kudu operations which it expects to fail due to a
timeout.

Since the test was written, things have sped up - for example, Impala
used to create a new Kudu client for each operation, but that was
changed in IMPALA-5167, such that the operations now occasionally
complete quickly enough that they don't timeout.

There's not really any way to rewrite this test to ensure that it
won't be flaky, so the patch removes it.

Change-Id: I29fd67d0acc0ee15943c416f2179ad716d2cac05
---
D 
testdata/workloads/functional-query/queries/QueryTest/kudu-timeouts-catalogd.test
M tests/custom_cluster/test_kudu.py
2 files changed, 0 insertions(+), 33 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29fd67d0acc0ee15943c416f2179ad716d2cac05
Gerrit-Change-Number: 8154
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 

[Impala-ASF-CR] IMPALA-3316: [DOCS] Add known issue for timezone conversion slowdown

[John Russell (Code Review)]

John Russell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8165 )

Change subject: IMPALA-3316: [DOCS] Add known issue for timezone conversion 
slowdown
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8165/1/docs/topics/impala_known_issues.xml
File docs/topics/impala_known_issues.xml:

http://gerrit.cloudera.org:8080/#/c/8165/1/docs/topics/impala_known_issues.xml@316
PS1, Line 316:   of slowdown depends on factors such as the number of 
cores and number of threads involved in the query.
> State explicitly that slowdowns of up to 30x have been observed for scan-he
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9933ced07e339d589f7f74173cfebe938084e65c
Gerrit-Change-Number: 8165
Gerrit-PatchSet: 2
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:40:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3316: [DOCS] Add known issue for timezone conversion slowdown

[John Russell (Code Review)]

Hello Attila Jeges, Alex Behm, Mostafa Mokhtar,

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

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

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

Change subject: IMPALA-3316: [DOCS] Add known issue for timezone conversion 
slowdown
..

IMPALA-3316: [DOCS] Add known issue for timezone conversion slowdown

Change-Id: I9933ced07e339d589f7f74173cfebe938084e65c
---
M docs/topics/impala_known_issues.xml
1 file changed, 26 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9933ced07e339d589f7f74173cfebe938084e65c
Gerrit-Change-Number: 8165
Gerrit-PatchSet: 2
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Mostafa Mokhtar 

[Impala-ASF-CR] IMPALA-992: [DOCS] Document impala-shell 'rerun' command

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-992: [DOCS] Document impala-shell 'rerun' command
..


Patch Set 3: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b353af3b3d8386c243f884b37442b5283a96a8
Gerrit-Change-Number: 8044
Gerrit-PatchSet: 3
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:38:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-992: [DOCS] Document impala-shell 'rerun' command

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-992: [DOCS] Document impala-shell 'rerun' command
..

IMPALA-992: [DOCS] Document impala-shell 'rerun' command

Change-Id: I78b353af3b3d8386c243f884b37442b5283a96a8
Reviewed-on: http://gerrit.cloudera.org:8080/8044
Reviewed-by: John Russell 
Tested-by: Impala Public Jenkins
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_shell_commands.xml
M docs/topics/impala_shell_running_commands.xml
3 files changed, 90 insertions(+), 0 deletions(-)

Approvals:
  John Russell: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I78b353af3b3d8386c243f884b37442b5283a96a8
Gerrit-Change-Number: 8044
Gerrit-PatchSet: 4
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-992: [DOCS] Document impala-shell 'rerun' command

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-992: [DOCS] Document impala-shell 'rerun' command
..


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-docs-submit/156/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b353af3b3d8386c243f884b37442b5283a96a8
Gerrit-Change-Number: 8044
Gerrit-PatchSet: 3
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:31:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-992: [DOCS] Document impala-shell 'rerun' command

[John Russell (Code Review)]

John Russell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8044 )

Change subject: IMPALA-992: [DOCS] Document impala-shell 'rerun' command
..


Patch Set 3: Code-Review+2

One tiny invisible change to make the new subtopic easier to link to. I'll 
carry over Alex's +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I78b353af3b3d8386c243f884b37442b5283a96a8
Gerrit-Change-Number: 8044
Gerrit-PatchSet: 3
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:30:59 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-992: [DOCS] Document impala-shell 'rerun' command

[John Russell (Code Review)]

Hello Tianyi Wang, Sailesh Mukil, Alex Behm,

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

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

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

Change subject: IMPALA-992: [DOCS] Document impala-shell 'rerun' command
..

IMPALA-992: [DOCS] Document impala-shell 'rerun' command

Change-Id: I78b353af3b3d8386c243f884b37442b5283a96a8
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_shell_commands.xml
M docs/topics/impala_shell_running_commands.xml
3 files changed, 90 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78b353af3b3d8386c243f884b37442b5283a96a8
Gerrit-Change-Number: 8044
Gerrit-PatchSet: 3
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 5:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:14:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 5: Code-Review+2

Hit a flaky test. Will investigate and open a JIRA.

Rebase, carry +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:13:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4856: Port data stream service to KRPC

[Michael Ho (Code Review)]

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

Change subject: IMPALA-4856: Port data stream service to KRPC
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8023/2/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:

http://gerrit.cloudera.org:8080/#/c/8023/2/be/src/runtime/krpc-data-stream-sender.cc@103
PS2, Line 103: address_(MakeNetworkAddress(destination.hostname, 
destination.port)),
address_(destination)


http://gerrit.cloudera.org:8080/#/c/8023/2/be/src/runtime/krpc-data-stream-sender.cc@141
PS2, Line 141:  TNetworkAddress address_;
const TNetworkAddress address_;



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0b8c1e50678da66ab1547d16530f88b323ed8c1
Gerrit-Change-Number: 8023
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 17:49:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4951: Fix database visibility for user with only column privilege

[Bikramjeet Vig (Code Review)]

Hello Dimitris Tsirogiannis,

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

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

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

Change subject: IMPALA-4951: Fix database visibility for user with only column 
privilege
..

IMPALA-4951: Fix database visibility for user with only column privilege

Currently a database is not visible to a user that only has column
level privileges for tables in that database. This patch will make
the database visible, which is the expected behavior in this case.

Testing: added a test case to verify the same.

Change-Id: Id77904876729c0223fd6ace2d5e7199bd700a33a
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
M testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test
2 files changed, 73 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id77904876729c0223fd6ace2d5e7199bd700a33a
Gerrit-Change-Number: 8168
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dimitris Tsirogiannis 

[Impala-ASF-CR] IMPALA-4951: Fix database visibility for user with only column privilege

[Bikramjeet Vig (Code Review)]

Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8168 )

Change subject: IMPALA-4951: Fix database visibility for user with only column 
privilege
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8168/2/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test
File testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test:

http://gerrit.cloudera.org:8080/#/c/8168/2/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test@744
PS2, Line 744: revoke role grant_revoke_test_ROOT from group root
> Is this needed for your test?
Done


http://gerrit.cloudera.org:8080/#/c/8168/2/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test@750
PS2, Line 750:  QUERY
 : show current roles
 :  RESULTS: VERIFY_IS_SUBSET
 : 'grant_revoke_test_ALL_SERVER'
 :  TYPES
 : STRING
 : 
> No need for that. Let's remove to make the test a bit smaller.
Done


http://gerrit.cloudera.org:8080/#/c/8168/2/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test@808
PS2, Line 808: # IMPALA-4951: make sure database is visible to a user having 
only column level access
 : # to a table in the database
> I think it may be best to move this comment at the beginning of your change
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id77904876729c0223fd6ace2d5e7199bd700a33a
Gerrit-Change-Number: 8168
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Comment-Date: Fri, 29 Sep 2017 17:20:49 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..


Patch Set 5:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 17:05:10 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Csaba Ringhofer (Code Review)]

Hello Lars Volker, Tim Armstrong,

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

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

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..

IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump_path' flag

Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
---
M be/src/common/global-flags.cc
1 file changed, 4 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] PREVIEW: IMPALA-5307: Part 4: copy out uncompressed text and seq

[Tim Armstrong (Code Review)]

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

Change subject: PREVIEW: IMPALA-5307: Part 4: copy out uncompressed text and seq
..


Patch Set 1:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/exec/exec-node.h@109
PS1, Line 109:   /// Caller must not be holding any io buffers. This will cause 
deadlock.
Remove - not relevant even before this patch


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

http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/exec/hdfs-scanner.h@99
PS1, Line 99: /// resources (IO buffers and mem pools) to the current row 
batch, and passing row batches
Needs update


http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:

http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h@51
PS1, Line 51: ///   3. Auxiliary tuple memory (e.g. string data) - this can 
either be stored externally
Needs update


http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h@65
PS1, Line 65: /// is IoBuffers which are only attached to one row batch. Only 
when the row batch reaches
Needs update


http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h@218
PS1, Line 218:   /// make it consistent between buffers and I/O buffers.
Needs update


http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h@254
PS1, Line 254:   /// pool and io buffers.
needs update


http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h@275
PS1, Line 275:   /// Acquires state from the 'src' row batch into this row 
batch. This includes all IO
Needs update


http://gerrit.cloudera.org:8080/#/c/8172/1/be/src/runtime/row-batch.h@397
PS1, Line 397:   /// Sum of all auxiliary bytes. This includes IoBuffers and 
memory from
Old reference to IoBuffers. Can probably rename to total_buffer_bytes_ or 
similar.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I304fd002b61bfedf41c8b1405cd7eb7b492bb941
Gerrit-Change-Number: 8172
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:48:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:52:58 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash Impala

[Csaba Ringhofer (Code Review)]

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

Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7954/4/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

http://gerrit.cloudera.org:8080/#/c/7954/4/be/src/runtime/timestamp-value.h@173
PS4, Line 173:
> i think it would be best to avoid running the constructor given these thing
Done


http://gerrit.cloudera.org:8080/#/c/7954/4/be/src/runtime/timestamp-value.h@287
PS4, Line 287:
> nit: extraneous space
Done


http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-value.h@86
PS2, Line 86: }
> Logging in the caller is fine but I don't see that added to your diff.
I have added some logging, but not on the caller side.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff
Gerrit-Change-Number: 7954
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:40:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] PREVIEW: IMPALA-5307: Part 4: copy out uncompressed text and seq

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8172


Change subject: PREVIEW: IMPALA-5307: Part 4: copy out uncompressed text and seq
..

PREVIEW: IMPALA-5307: Part 4: copy out uncompressed text and seq

This is the final patch for IMPALA-5307.

Text and Seq scanners are converted to use the same approach
as Avro.

contains_tuple_data is now false so a bunch of dead code in
ScannerContext can be removed. We also no longer attach I/O
buffers to row batches so that logic can be removed.

Testing:
Ran core ASAN tests.

Perf:
TODO - need to run numbers

Change-Id: I304fd002b61bfedf41c8b1405cd7eb7b492bb941
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-avro-scanner.h
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-rcfile-scanner.cc
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scanner-ir.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M be/src/runtime/row-batch.cc
M be/src/runtime/row-batch.h
20 files changed, 85 insertions(+), 156 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8164/4/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:

http://gerrit.cloudera.org:8080/#/c/8164/4/be/src/common/global-flags.cc@103
PS4, Line 103: only
Remove "only", since it conflicts with the following sentence.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:32:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..


Patch Set 4:

I'll let Lars give a +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:25:10 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..


Patch Set 4: Code-Review+1

(1 comment)

Linking doesn't seem necessary to me - I think the documentation is easily 
discoverable by search (both the wiki page and the docs themselves)

http://gerrit.cloudera.org:8080/#/c/8164/4/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:

http://gerrit.cloudera.org:8080/#/c/8164/4/be/src/common/global-flags.cc@105
PS4, Line 105: "to create minidumps on demand without exiting the process 
by sending SIGUSR1. "
Thanks this is better than my suggestion.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:25:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5994: Lower case struct-field names

[Alex Behm (Code Review)]

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

Change subject: IMPALA-5994: Lower case struct-field names
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:21:57 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5994: Lower case struct-field names

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-5994: Lower case struct-field names
..


Patch Set 3:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:16:42 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5994: Lower case struct-field names

[Thomas Tauber-Marshall (Code Review)]

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

Change subject: IMPALA-5994: Lower case struct-field names
..


Patch Set 3:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8169/1//COMMIT_MSG@7
PS1, Line 7: IMPALA-5994: Lower case struct-field names
> Commit msg should describe the change/fix, not the symptom of the bug. For
Done


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

http://gerrit.cloudera.org:8080/#/c/8169/2/fe/src/main/java/org/apache/impala/catalog/StructField.java@37
PS2, Line 37: // Impala expects field names to be in lower case, but type 
strings stored in the HMS
> // Impala expects field names to be in lower case, but type strings stored
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:16:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5994: Lower case struct-field names

[Thomas Tauber-Marshall (Code Review)]

Hello Lars Volker, Alex Behm,

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

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

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

Change subject: IMPALA-5994: Lower case struct-field names
..

IMPALA-5994: Lower case struct-field names

Impala tries to always store column names in lower case. As part of a
cleanup of issues related to upper case Kudu column names, a check was
added in Analyzer to enforce this.

The check fails when doing star expansion on a struct to select all
fields in the case where a table was created in Hive with upper case
letters in a struct field name. This happens because Hive does not
covert struct field names to all lower case in HMS.

The solution is to force StructField names to lower case.

Testing:
- Added a test in test_nested_types.py

Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
---
M fe/src/main/java/org/apache/impala/catalog/StructField.java
M tests/query_test/test_nested_types.py
2 files changed, 18 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Csaba Ringhofer (Code Review)]

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..


Patch Set 4:

Would it be a good idea to refer
cwiki.apache.org/confluence/display/IMPALA/Debugging+Impala+Minidumps in the 
help string?

(By the way sorry for the large number of small patches.)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:08:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Csaba Ringhofer (Code Review)]

Hello Lars Volker, Tim Armstrong,

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

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

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..

IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump_path' flag

Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
---
M be/src/common/global-flags.cc
1 file changed, 3 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Csaba Ringhofer (Code Review)]

Hello Lars Volker, Tim Armstrong,

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

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

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..

IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump_path' flag

Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
---
M be/src/common/global-flags.cc
1 file changed, 3 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Csaba Ringhofer (Code Review)]

Hello Lars Volker, Tim Armstrong,

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

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

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..

IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump_path' flag

Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
---
M be/src/common/global-flags.cc
1 file changed, 4 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5376: Implement all TPCDS test cases or alternates for Impala.

[Michael Brown (Code Review)]

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

Change subject: IMPALA-5376: Implement all TPCDS test cases or alternates for 
Impala.
..


Patch Set 10:

> Michael commented that the stress test will pick up these new
 > queries. Do you have results from the stress test?

Matt, if you want stress test results from TIm, please help him gather them on 
a cluster of your choosing and help interpret the results.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6e284888600a7a69d1f23fcb7dac21cbb13b7d66
Gerrit-Change-Number: 8102
Gerrit-PatchSet: 10
Gerrit-Owner: Tim Wood 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Matthew Mulder 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Wood 
Gerrit-Comment-Date: Fri, 29 Sep 2017 14:19:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5448: fix invalid number of splits reported in Parquet scan node

[Quanlong Huang (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-5448: fix invalid number of splits reported in Parquet 
scan node
..

IMPALA-5448: fix invalid number of splits reported in Parquet scan node

Parquet splits with multi columns are marked as completed by using
HdfsScanNodeBase::RangeComplete(). It duplicately counts the file types
as column codec types. Thus the number of parquet splits are the real count
multiplies number of materialized columns.

Furthermore, according to the Parquet definition, it allows mixed compression
codecs on different columns. This's handled in this patch as well. A parquet 
file
using gzip and snappy compression codec will be reported as:
FileFormats: PARQUET/(GZIP,SNAPPY):1

This patch introduces a compression types set for the above cases.

Testing:
Add end-to-end tests handling parquet files with all columns compressed in
snappy, and handling parquet files with multi compression codec.

Change-Id: Iaacc2d775032f5707061e704f12e0a63cde695d1
---
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/multi_compression_parquet_data/README
A testdata/multi_compression_parquet_data/tinytable_0_gzip_snappy.parq
A testdata/multi_compression_parquet_data/tinytable_1_snappy_gzip.parq
A 
testdata/workloads/functional-query/queries/QueryTest/hdfs_parquet_scan_node_profile.test
M tests/query_test/test_scanners.py
9 files changed, 111 insertions(+), 13 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaacc2d775032f5707061e704f12e0a63cde695d1
Gerrit-Change-Number: 8147
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5448: fix invalid number of splits reported in Parquet scan node

[Quanlong Huang (Code Review)]

Quanlong Huang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8147 )

Change subject: IMPALA-5448: fix invalid number of splits reported in Parquet 
scan node
..


Patch Set 1:

(10 comments)

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

http://gerrit.cloudera.org:8080/#/c/8147/1/be/src/exec/hdfs-scan-node-base.h@497
PS1, Line 497:   /// Mapping of file formats (file type, compression types set) 
to the number of
> Can you move the comment below the class definition and above the map?
Done


http://gerrit.cloudera.org:8080/#/c/8147/1/be/src/exec/hdfs-scan-node-base.h@499
PS1, Line 499:   struct HdfsCompressionTypesSet {
> Can you make this a class and make the member variables private? I don't th
Done


http://gerrit.cloudera.org:8080/#/c/8147/1/be/src/exec/hdfs-scan-node-base.h@500
PS1, Line 500: uint32_t bit_map;
> Can you add an assertion to the constructor to make sure that bit_map is la
Done


http://gerrit.cloudera.org:8080/#/c/8147/1/be/src/exec/hdfs-scan-node-base.h@501
PS1, Line 501: THdfsCompression::type last_type;
> Is last_type needed? I think we can remove it.
Done


http://gerrit.cloudera.org:8080/#/c/8147/1/be/src/exec/hdfs-scan-node-base.h@504
PS1, Line 504: hasType
> We capitalise the first letter in C++ method names, i.e. HasType(). The goo
Done


http://gerrit.cloudera.org:8080/#/c/8147/1/be/src/exec/hdfs-scan-node-base.h@506
PS1, Line 506: }
> Please add blank lines between the method definitions.
Done


http://gerrit.cloudera.org:8080/#/c/8147/1/be/src/exec/hdfs-scan-node-base.h@507
PS1, Line 507: addType
> AddType
Done


http://gerrit.cloudera.org:8080/#/c/8147/1/be/src/exec/hdfs-scan-node-base.h@512
PS1, Line 512: bool operator< (const HdfsCompressionTypesSet& o) const {
> Can you comment that this is needed so it can be part of the std::map key.
Done


http://gerrit.cloudera.org:8080/#/c/8147/1/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

http://gerrit.cloudera.org:8080/#/c/8147/1/be/src/exec/hdfs-scan-node-base.cc@897
PS1, Line 897:   for (auto i = compressions_map.begin(); i != 
compressions_map.end(); ++i) {
> I think this would be more readable with a ranged for loop. E.g.
Cool! Done


http://gerrit.cloudera.org:8080/#/c/8147/1/testdata/multi_compression_parquet_data/README
File testdata/multi_compression_parquet_data/README:

http://gerrit.cloudera.org:8080/#/c/8147/1/testdata/multi_compression_parquet_data/README@5
PS1, Line 5: These files have two string columns 'a' and 'b'. Each columns 
using different compression types.
> Cool!
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaacc2d775032f5707061e704f12e0a63cde695d1
Gerrit-Change-Number: 8147
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 08:26:36 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4670: Introduces RpcMgr class

[Michael Ho (Code Review)]

Hello Sailesh Mukil, Dan Hecht,

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

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

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

Change subject: IMPALA-4670: Introduces RpcMgr class
..

IMPALA-4670: Introduces RpcMgr class

This patch introduces a new class, RpcMgr which is the abstraction
layer around KRPC core mechanics. It provides an interface
RegisterService() for various services to register themselves.

Kudu RPC is invoked via an auto-generated interface called proxy.
This change implements an inline wrapper for KRPC client to obtain
a proxy for a particular service exported by remote server.

Last but not least, the RpcMgr will start all registered services
if FLAGS_use_krpc is true. This patch hasn't yet added any service
except for some test services in rpc-mgr-test.

This patch is based on an abandoned patch by Henry Robinson.

Testing done: a new backend test is added to exercise the code
and demonstrate the way to interact with KRPC framework.

Change-Id: I8adb10ae375d7bf945394c38a520f12d29cf7b46
---
M CMakeLists.txt
M be/src/exec/kudu-util.h
M be/src/rpc/CMakeLists.txt
A be/src/rpc/rpc-mgr-test.cc
A be/src/rpc/rpc-mgr.cc
A be/src/rpc/rpc-mgr.h
A be/src/rpc/rpc-mgr.inline.h
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler.cc
M be/src/scheduling/scheduler.h
M be/src/service/impala-server.cc
M be/src/util/counting-barrier.h
M be/src/util/network-util.cc
M be/src/util/network-util.h
A common/protobuf/CMakeLists.txt
A common/protobuf/rpc_test.proto
18 files changed, 774 insertions(+), 39 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8adb10ae375d7bf945394c38a520f12d29cf7b46
Gerrit-Change-Number: 7901
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4670: Introduces RpcMgr class

[Michael Ho (Code Review)]

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

Change subject: IMPALA-4670: Introduces RpcMgr class
..


Patch Set 6:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/exec/kudu-util.h
File be/src/exec/kudu-util.h:

http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/exec/kudu-util.h@87
PS6, Line 87: /// Impala Status object.
> maybe note that the returned error is always of type GENERAL, i.e. we don't
Done


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr-test.cc
File be/src/rpc/rpc-mgr-test.cc:

http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr-test.cc@65
PS6, Line 65: FLAGS_hostname
> Do we want this to be the loopback address since this is only a test?
It sees fine as this is closer to what we actually do in 
ExecEnv::StartServices().


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr-test.cc@184
PS6, Line 184: rpc_mgr_.Shutdown();
> We don't need to call Shutdown here and in the other tests right? Since it
Done


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@76
PS6, Line 76: ///
> what is the thread safety story of this class? it looks like initializing i
There is none actually. The assumption is that Init() is called from the 
singleton ExecEnv when it's initialized and Shutdown() is called  when the 
singleton ExecEnv is destroyed.

Comments updated.


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@89
PS6, Line 89: FIFO fixed-size queue
> is that queue per service? and is there a queue for connection requests as
There is one queue per service. Comment updated.


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@89
PS6, Line 89: processed
> that's talking about being processed by either acceptor or service, right?
Done


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@104
PS6, Line 104: address
> does that one have to be an IP addr or is hostname okay?
Comments clarified.


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@115
PS6, Line 115: being
> begin
Done


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@133
PS6, Line 133:   /// All acceptor and reactor threads within the messenger are 
also shut down.
> what happens to stuff in flight, or should we not care for some reason?
Normally, we shouldn't shutdown as there is really no clean way to shut down 
Impala now. ServicePool internally will return errors to clients of all 
unprocessed requests.


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@156
PS6, Line 156: scoped_refptr
> I guess using that is kinda required by krpc? What does it do? ServicePool
This is similar to a shared_ptr in a way. KRPC messenger internally uses a map 
of service-name to scoped_refptr> to track all service pools.

It seems safer and more consistent to use a scoped_refptr here to avoid 
accidentally accessing freed memory. In theory, we can make sure the 
service_pools_ is cleared before we call messenger_.UnregisterAllServices() but 
it still seems a bit dicey.


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.h@169
PS6, Line 169: shared_ptr
> I guess using shared_ptr here is required by the MessangerBuilder::Build().
Yes, required by MessengerBuilder::Build().


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/rpc/rpc-mgr.cc@30
PS6, Line 30: using std::move;
> why is that needed?
Done


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/runtime/exec-env.h
File be/src/runtime/exec-env.h:

http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/runtime/exec-env.h@135
PS6, Line 135: krpc_port
> does that still need to be exposed if we have krpc_address()? And looking a
Removed. Please take a look at the changed logic in impala-server.cc


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/scheduling/scheduler.cc@a73
PS6, Line 73:
> maybe this comment is still useful in the new code (minus the KRPC bits) in
Done


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/util/network-util.cc
File be/src/util/network-util.cc:

http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/util/network-util.cc@120
PS6, Line 120:   Sockaddr sock;
> okay to ignore, but i think keeping the kudu:: namespace here makes it easi
Done


http://gerrit.cloudera.org:8080/#/c/7901/6/be/src/util/network-util.cc@218
PS6, Line 218: Sockaddr
> same
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8adb10ae375d7bf945394c38a520f12d29cf7b46
Gerrit-Change-Number: 7901