date:20180104

[Impala-ASF-CR] IMPALA-6128: Add support for AES-CTR encryption when spilling to disk CFB mode is a stream cipher and is secure when used with a different nonce/IV for every message. However it can be

2018-01-04 Thread Xianda Ke (Code Review)

Hello Sailesh Mukil, Tim Armstrong, Bikramjeet Vig,

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

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

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

Change subject: IMPALA-6128: Add support for AES-CTR encryption when spilling 
to disk CFB mode is a stream cipher and is secure when used with a different 
nonce/IV for every message. However it can be a performance bottleneck. CTR 
mode is also stream cipher and is secure
..

IMPALA-6128: Add support for AES-CTR encryption when spilling to disk
CFB mode is a stream cipher and is secure when used with a different nonce/IV
for every message. However it can be a performance bottleneck.
CTR mode is also stream cipher and is secure, 4~6x faster than CFB mode in
OpenSSL. AES-CTR+SHA256 is about 40~70% faster than AES-CFB+SHA256.

CTR mode is used if OpenSSL version>=1.0.1 at runtime, otherwise
fall back to using CFB mode.

Testing:
run runtime tmp-file-mgr-test, openssl-util-test, buffer-pool-test and
buffered-tuple-stream-test
The ut case openssl-util-test.EncryptInPlace tests encryption in both modes.

Change-Id: I9debc240615dd8cdbf00ec8730cff62ffef52aff
---
M be/src/runtime/tmp-file-mgr.cc
M be/src/util/openssl-util-test.cc
M be/src/util/openssl-util.cc
M be/src/util/openssl-util.h
4 files changed, 94 insertions(+), 44 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9debc240615dd8cdbf00ec8730cff62ffef52aff
Gerrit-Change-Number: 8861
Gerrit-PatchSet: 4
Gerrit-Owner: Xianda Ke 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Xianda Ke

[Impala-ASF-CR] IMPALA-6364: Bypass file handle cache for ineligible files

2018-01-04 Thread Mostafa Mokhtar (Code Review)

Mostafa Mokhtar has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8945 )

Change subject: IMPALA-6364: Bypass file handle cache for ineligible files
..


Patch Set 2: Code-Review+1

Validated the the regression is addressed with this change.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ab52b0884a909a4faeb6692f32d45878ea2838f
Gerrit-Change-Number: 8945
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 05 Jan 2018 06:29:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3651: Adds murmur hash() built-in function

2018-01-04 Thread Kim Jin Chul (Code Review)

Hello Attila Jeges, Tim Armstrong,

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

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

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

Change subject: IMPALA-3651: Adds murmur_hash() built-in function
..

IMPALA-3651: Adds murmur_hash() built-in function

murmur_hash relys on HashUtil::MurmurHash2_64 which MurmurHash2 64-bit
version.

Testing:
Add unit tests for primitive types: ExprTest.MurmurHashFunction
Add E2E tests into exprs.test

Change-Id: I14d56ffb8fab256f3f66a2669271fd4b3c50cc29
---
M be/src/exprs/expr-test.cc
M be/src/exprs/utility-functions-ir.cc
M be/src/exprs/utility-functions.h
M be/src/util/hash-util.h
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
6 files changed, 177 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14d56ffb8fab256f3f66a2669271fd4b3c50cc29
Gerrit-Change-Number: 8893
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-3651: Adds murmur hash() built-in function

2018-01-04 Thread Kim Jin Chul (Code Review)

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

Change subject: IMPALA-3651: Adds murmur_hash() built-in function
..


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8893/2//COMMIT_MSG@13
PS2, Line 13: Add unit tests for primitive types
> Can you add a test query to exprs.test, just to make sure it works end-to-e
Done


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

http://gerrit.cloudera.org:8080/#/c/8893/2/be/src/exprs/expr-test.cc@4564
PS2, Line 4564:   // Test murmur_hash
> nit: comment is not really needed. Maybe it would actually be best to make
Done. Add a new function: ExprTest.MurmurHashFunction


http://gerrit.cloudera.org:8080/#/c/8893/2/be/src/exprs/expr-test.cc@4571
PS2, Line 4571: expected = HashUtil::MurmurHash2_64(s.data(), s.size(), 
HashUtil::MURMUR_DEFAULT_SEED);
> In the cases where we're hashing a constant, can you add a test that compar
Done. Good point. Thanks.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14d56ffb8fab256f3f66a2669271fd4b3c50cc29
Gerrit-Change-Number: 8893
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 05 Jan 2018 05:45:39 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3651: Adds murmur hash() built-in function

2018-01-04 Thread Kim Jin Chul (Code Review)

Hello Attila Jeges, Tim Armstrong,

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

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

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

Change subject: IMPALA-3651: Adds murmur_hash() built-in function
..

IMPALA-3651: Adds murmur_hash() built-in function

murmur_hash relys on HashUtil::MurmurHash2_64 which MurmurHash2 64-bit
version.

Testing:
Add unit tests for primitive types: ExprTest.MurmurHashFunction
Add E2E tests into exprs.test

Change-Id: I14d56ffb8fab256f3f66a2669271fd4b3c50cc29
---
M be/src/exprs/expr-test.cc
M be/src/exprs/utility-functions-ir.cc
M be/src/exprs/utility-functions.h
M be/src/util/hash-util.h
M common/function-registry/impala_functions.py
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
6 files changed, 179 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14d56ffb8fab256f3f66a2669271fd4b3c50cc29
Gerrit-Change-Number: 8893
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-6370: fix partitioned parquet tables with nested types

2018-01-04 Thread Alex Behm (Code Review)

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

Change subject: IMPALA-6370: fix partitioned parquet tables with nested types
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8947/2/tests/query_test/test_nested_types.py
File tests/query_test/test_nested_types.py:

http://gerrit.cloudera.org:8080/#/c/8947/2/tests/query_test/test_nested_types.py@126
PS2, Line 126:   self.run_stmt_in_hive("""
Hive+refresh is pretty slow, could we instead use a custom LOCATION pointing to 
the same location of complextypestbl? You can use self._get_table_location



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic808b824ce3b31af0539036d8ca23d17b18deab4
Gerrit-Change-Number: 8947
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 05 Jan 2018 02:47:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5052: Read and write signed integer logical types in Parquet

2018-01-04 Thread anujphadke (Code Review)

anujphadke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8548 )

Change subject: IMPALA-5052: Read and write signed integer logical types in 
Parquet
..


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8548/1/testdata/data/signed_integer_logical_types.parquet
File testdata/data/signed_integer_logical_types.parquet:

PS1:
> Can you add a description of this file to the readme - i.e. what it has in
Done


http://gerrit.cloudera.org:8080/#/c/8548/1/tests/query_test/test_insert_parquet.py
File tests/query_test/test_insert_parquet.py:

http://gerrit.cloudera.org:8080/#/c/8548/1/tests/query_test/test_insert_parquet.py@295
PS1, Line 295: column
> column
Done


http://gerrit.cloudera.org:8080/#/c/8548/1/tests/query_test/test_insert_parquet.py@303
PS1, Line 303: stored as parquet""".format(src_tbl, hdfs_path)
> Why is there a space after {1}?
Removed


http://gerrit.cloudera.org:8080/#/c/8548/1/tests/query_test/test_insert_parquet.py@321
PS1, Line 321: result = self.execute_query_expect_success(self.client, 
insert_stmt)
> remove semicolon
Done


http://gerrit.cloudera.org:8080/#/c/8548/1/tests/query_test/test_insert_parquet.py@325
PS1, Line 325: ame values in
> should these (above and below as well) be execute_query_expect_success?
Done


http://gerrit.cloudera.org:8080/#/c/8548/1/tests/query_test/test_insert_parquet.py@331
PS1, Line 331: dst_tbl = "{0}.{1}".format(unique_database, 
"read_write_logical_type_dst")
> +1. I think the test would be a little easier to understand too if we asser
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I47a8371858c9597c6a440808cf6f933532468927
Gerrit-Change-Number: 8548
Gerrit-PatchSet: 2
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 05 Jan 2018 02:45:33 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5052: Read and write signed integer logical types in Parquet

2018-01-04 Thread anujphadke (Code Review)

Hello Tianyi Wang, Tim Armstrong,

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

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

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

Change subject: IMPALA-5052: Read and write signed integer logical types in 
Parquet
..

IMPALA-5052: Read and write signed integer logical types in Parquet

This patch maps a signed integer logical type in parquet to a supported
Impala column type. This change introduces the following mapping -

  INT_8  -> TINYINT
  INT_16 -> SMALLINT
  INT_32 -> INT
  INT_64 -> BIGINT

Also, added a parquet file with the following schema for testing -

  schema {
optional int32 id;
optional int32 tinyint_col (INT_8);
optional int32 smallint_col (INT_16);
optional int32 int_col;
optional int64 bigint_col;
  }

Change-Id: I47a8371858c9597c6a440808cf6f933532468927
---
M be/src/exec/hdfs-parquet-table-writer.cc
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java
M testdata/data/README
A testdata/data/signed_integer_logical_types.parquet
M tests/query_test/test_insert_parquet.py
5 files changed, 94 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I47a8371858c9597c6a440808cf6f933532468927
Gerrit-Change-Number: 8548
Gerrit-PatchSet: 2
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-6370: fix partitioned parquet tables with nested types

2018-01-04 Thread Thomas Tauber-Marshall (Code Review)

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

Change subject: IMPALA-6370: fix partitioned parquet tables with nested types
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic808b824ce3b31af0539036d8ca23d17b18deab4
Gerrit-Change-Number: 8947
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 05 Jan 2018 02:23:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug mode

2018-01-04 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug 
mode
..


Patch Set 5: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I93e2b1efb325100d01d398e68e789d87b877167e
Gerrit-Change-Number: 8923
Gerrit-PatchSet: 5
Gerrit-Owner: Manaswini Maharana 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Manaswini Maharana 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 05 Jan 2018 02:18:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug mode

2018-01-04 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug 
mode
..

IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug mode

Currently, when debug mode is enabled, any query using codegen can result
in an Impala daemon crash as it hits a DCHECK.

This patch ensures the DCHECK is hit only when specific condition is met to
avoid the crash. That condition here is to DCHECK only when 'emit_perf_map_'
evaluates to True ensuring 'perf_map_lock_' is not empty when asserted.

Change-Id: I93e2b1efb325100d01d398e68e789d87b877167e
Reviewed-on: http://gerrit.cloudera.org:8080/8923
Reviewed-by: Tim Armstrong 
Reviewed-by: Thomas Tauber-Marshall 
Tested-by: Impala Public Jenkins
---
M be/src/codegen/codegen-symbol-emitter.cc
1 file changed, 6 insertions(+), 4 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I93e2b1efb325100d01d398e68e789d87b877167e
Gerrit-Change-Number: 8923
Gerrit-PatchSet: 6
Gerrit-Owner: Manaswini Maharana 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Manaswini Maharana 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong

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

2018-01-04 Thread Kim Jin Chul (Code Review)

Hello John Russell, Dimitris Tsirogiannis, Alex Behm, Vuk Ercegovac,

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

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

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

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

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

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

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


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

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

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

2018-01-04 Thread Kim Jin Chul (Code Review)

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

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


Patch Set 8:

(4 comments)

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

http://gerrit.cloudera.org:8080/#/c/8676/8/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@303
PS8, Line 303: String pattern
> Can you add a brief comment or example of what this pattern should look lik
Done


http://gerrit.cloudera.org:8080/#/c/8676/8/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@319
PS8, Line 319: TestInsertStmtHints
> nit: "This function"
Done


http://gerrit.cloudera.org:8080/#/c/8676/8/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@330
PS8, Line 330: ParserErrorOnInsertStmtHints
> nit: "It covers..." No need to repeat the function name in the comment.
Done


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

http://gerrit.cloudera.org:8080/#/c/8676/8/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@505
PS8, Line 505: private String InjectInsertHint(String pattern, String hint,
 :   InsertStmt.HintLocation loc) {
 : final String oracleHint = (loc == 
InsertStmt.HintLocation.Start) ? hint : "";
 : final String defaultHint  = (loc == 
InsertStmt.HintLocation.End) ? hint : "";
 : return String.format(pattern, oracleHint, defaultHint);
 :   }
> Isn't the same function as in parserTest.java? Plz avoid code duplication.
It is moved to the super class(i.e. FrontendTestBase).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 05 Jan 2018 01:32:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6364: Bypass file handle cache for ineligible files

2018-01-04 Thread Joe McDonnell (Code Review)

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8945 )

Change subject: IMPALA-6364: Bypass file handle cache for ineligible files
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8945/1/be/src/runtime/io/disk-io-mgr.cc@123
PS1, Line 123:  16
> I'm open to thoughts on what a good default would be. The contention we hav
Added a TODO to look into this. Since the file handle cache is off by default, 
this should not delay this change.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ab52b0884a909a4faeb6692f32d45878ea2838f
Gerrit-Change-Number: 8945
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 05 Jan 2018 01:26:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6364: Bypass file handle cache for ineligible files

2018-01-04 Thread Joe McDonnell (Code Review)

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-6364: Bypass file handle cache for ineligible files
..

IMPALA-6364: Bypass file handle cache for ineligible files

Currently, all HdfsFileHandles are owned and constructed
by the file handle cache. When the file handle cache
is disabled or the file handle is not eligible for
caching, the HdfsFileHandle is stored exclusively in
ScanRange::exclusive_hdfs_fh_, but the HdfsFileHandle still
comes from the file handle cache. It is created via a call to
DiskIoMgr::GetCachedHdfsFileHandle() with 'require_new_handle'
set to true and destroyed via
DiskIoMgr::ReleaseCachedHdfsFileHandle() with 'destroy_handle'
set to true.

Recent testing has revealed that the lock on the file handle
cache is a bottleneck for workloads with many small remote
files. There is no benefit to storing these exclusive file
handles in the file handle cache, as they do not participate
in the caching.

This change introduces DiskIoMgr::GetExclusiveHdfsFileHandle()
and DiskIoMgr::ReleaseExclusiveHdfsFileHandle(). These are
equivalent to the Get/ReleaseCachedHdfsFileHandle() calls, except
they bypass the file handle cache and create/destroy the
file handle directly. ScanRange::Open()/Close(), which
populates and frees ScanRange::exclusive_hdfs_fh_, now uses
these new calls rather than accessing the file handle cache.
This avoids the locking entirely, solving the bottleneck.

To draw a distinction between the two codepaths, HdfsFileHandle
is now an abstract class with two subclasses:
 - CachedHdfsFileHandles cover all handles that live in file handle
   cache. Get/ReleaseCachedHdfsFileHandle() use this subclass.
 - ExclusiveHdfsFileHandles cover all cases where a file handle
   does not come from the cache. The new
   Get/ReleaseExclusiveHdfsFileHandle() use this subclass.

Separately, testing revealed that increasing the number of
partitions for the file handle cache also fixes the contention
problem. This changes the file handle cache to make the number
of partitions configurable via startup parameter
num_file_handle_cache_partitions. This allows mitigation of
future bottlenecks without a patch.

Change-Id: I4ab52b0884a909a4faeb6692f32d45878ea2838f
---
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
M be/src/runtime/io/handle-cache.h
M be/src/runtime/io/handle-cache.inline.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
6 files changed, 138 insertions(+), 87 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4ab52b0884a909a4faeb6692f32d45878ea2838f
Gerrit-Change-Number: 8945
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-6364: Bypass file handle cache for ineligible files

2018-01-04 Thread Joe McDonnell (Code Review)

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8945 )

Change subject: IMPALA-6364: Bypass file handle cache for ineligible files
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8945/1//COMMIT_MSG@43
PS1, Line 43: Separately, testing revealed that increasing the number of
> Have you run any tests on systems where we'd exercise the exclusive path, e
I haven't run on s3. The exclusive path is also used when file handle cache is 
off, so all of our tests currently run through it. I have an exhaustive build 
running.

We are also planning on verifying the performance on a cluster that does remote 
reads. I think that can also include verifying remote read performance even 
with the file handle cache on.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ab52b0884a909a4faeb6692f32d45878ea2838f
Gerrit-Change-Number: 8945
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 05 Jan 2018 01:21:42 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6370: fix partitioned parquet tables with nested types

2018-01-04 Thread Tim Armstrong (Code Review)

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


Change subject: IMPALA-6370: fix partitioned parquet tables with nested types
..

IMPALA-6370: fix partitioned parquet tables with nested types

When materialising a nested collection, has_template_tuple() should use
the template tuple for the collection, not the top-level tuple.

Testing:
Added tests based on nested-types-basic.test that operate on a simple
partitioned table.

Change-Id: Ic808b824ce3b31af0539036d8ca23d17b18deab4
---
M be/src/exec/hdfs-scanner.h
A 
testdata/workloads/functional-query/queries/QueryTest/nested-types-basic-partitioned.test
M tests/query_test/test_nested_types.py
3 files changed, 390 insertions(+), 3 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-3060: draft: Supports IS [NOT] NULL feature for complex type

2018-01-04 Thread Kim Jin Chul (Code Review)

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

Change subject: IMPALA-3060: draft: Supports IS [NOT] NULL feature for complex 
type
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I986e1bae82d5ca7a82fc1bab2e412c4733928dce
Gerrit-Change-Number: 8710
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul

[Impala-ASF-CR] IMPALA-5341: [draft] introduce row-size match

2018-01-04 Thread Kim Jin Chul (Code Review)

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


Change subject: IMPALA-5341: [draft] introduce row-size match
..

IMPALA-5341: [draft] introduce row-size match

Change-Id: I9dcd15e4fb4cba4a00c6e7435dcd2400b65c0759
---
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M fe/src/test/java/org/apache/impala/testutil/TestUtils.java
2 files changed, 66 insertions(+), 17 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-3060: draft: Supports IS [NOT] NULL feature for complex type

2018-01-04 Thread Kim Jin Chul (Code Review)

Kim Jin Chul has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8710 )

Change subject: IMPALA-3060: draft: Supports IS [NOT] NULL feature for complex 
type
..

IMPALA-3060: draft: Supports IS [NOT] NULL feature for complex type

Change-Id: I986e1bae82d5ca7a82fc1bab2e412c4733928dce
---
M be/src/runtime/descriptors.cc
M be/src/runtime/types.h
M fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
5 files changed, 17 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I986e1bae82d5ca7a82fc1bab2e412c4733928dce
Gerrit-Change-Number: 8710
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul

[Impala-ASF-CR] IMPALA-5341: [draft] introduce row-size match

2018-01-04 Thread Kim Jin Chul (Code Review)

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

Change subject: IMPALA-5341: [draft] introduce row-size match
..


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I9dcd15e4fb4cba4a00c6e7435dcd2400b65c0759
Gerrit-Change-Number: 8542
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul

[Impala-ASF-CR] IMPALA-3282: Adds regexp escape built-in function

2018-01-04 Thread Tianyi Wang (Code Review)

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

Change subject: IMPALA-3282: Adds regexp_escape built-in function
..


Patch Set 2:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/8900/2//COMMIT_MSG@10
PS2, Line 10: ".*\\+?^[](){}$!=:-#\n\r\t\v "
Where does this list come from? Impala uses RE2 syntax, which does not escape 
'#', '!', etc. https://github.com/google/re2/wiki/Syntax


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

http://gerrit.cloudera.org:8080/#/c/8900/2/be/src/exprs/expr-test.cc@4159
PS2, Line 4159:   TestStringValue("regexp_escape('Helloworld')", 
"Helloworld");
It seems that the parameter to the regexp_escape function is escaped once so 
that it is interpreted as only 1 slash, '\\', during the execution. I haven't 
found the particular code doing so but we'd better add a comment here.


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

http://gerrit.cloudera.org:8080/#/c/8900/2/be/src/exprs/string-functions-ir.cc@624
PS2, Line 624:   const string input = AnyValUtil::ToString(str);
We can directly iterate with the pointer here. e.g. for (char* c = str.ptr; c 
<= str.ptr + str.len; ++c). It saves us a copying and a malloc.


http://gerrit.cloudera.org:8080/#/c/8900/2/be/src/exprs/string-functions-ir.cc@627
PS2, Line 627: const bool need_escape = special_character_set.find(c) != 
special_character_set.end();
I think using std::find on the string literal might be faster than on a 
set here.


http://gerrit.cloudera.org:8080/#/c/8900/2/be/src/exprs/string-functions-ir.cc@638
PS2, Line 638:   default: ss << "\\" << c; break;
Use '\\'.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84c3e0ded26f6eb20794c38b75be9b25cd111e4b
Gerrit-Change-Number: 8900
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 05 Jan 2018 00:13:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6364: Bypass file handle cache for ineligible files

2018-01-04 Thread Joe McDonnell (Code Review)

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8945 )

Change subject: IMPALA-6364: Bypass file handle cache for ineligible files
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8945/1/be/src/runtime/io/disk-io-mgr.cc@123
PS1, Line 123:  16
I'm open to thoughts on what a good default would be. The contention we have 
been seeing is mainly from the codepath where the file handle cache is off 
(remote files) and that will no longer care about this, but it might make sense 
to bump this just in case.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ab52b0884a909a4faeb6692f32d45878ea2838f
Gerrit-Change-Number: 8945
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 05 Jan 2018 00:02:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6364: Bypass file handle cache for ineligible files

2018-01-04 Thread Joe McDonnell (Code Review)

Joe McDonnell has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8945


Change subject: IMPALA-6364: Bypass file handle cache for ineligible files
..

IMPALA-6364: Bypass file handle cache for ineligible files

Currently, all HdfsFileHandles are owned and constructed
by the file handle cache. When the file handle cache
is disabled or the file handle is not eligible for
caching, the HdfsFileHandle is stored exclusively in
ScanRange::exclusive_hdfs_fh_, but the HdfsFileHandle still
comes from the file handle cache. It is created via a call to
DiskIoMgr::GetCachedHdfsFileHandle() with 'require_new_handle'
set to true and destroyed via
DiskIoMgr::ReleaseCachedHdfsFileHandle() with 'destroy_handle'
set to true.

Recent testing has revealed that the lock on the file handle
cache is a bottleneck for workloads with many small remote
files. There is no benefit to storing these exclusive file
handles in the file handle cache, as they do not participate
in the caching.

This change introduces DiskIoMgr::GetExclusiveHdfsFileHandle()
and DiskIoMgr::ReleaseExclusiveHdfsFileHandle(). These are
equivalent to the Get/ReleaseCachedHdfsFileHandle() calls, except
they bypass the file handle cache and create/destroy the
file handle directly. ScanRange::Open()/Close(), which
populates and frees ScanRange::exclusive_hdfs_fh_, now uses
these new calls rather than accessing the file handle cache.
This avoids the locking entirely, solving the bottleneck.

To draw a distinction between the two codepaths, HdfsFileHandle
is now an abstract class with two subclasses:
 - CachedHdfsFileHandles cover all handles that live in file handle
   cache. Get/ReleaseCachedHdfsFileHandle() use this subclass.
 - ExclusiveHdfsFileHandles cover all cases where a file handle
   does not come from the cache. The new
   Get/ReleaseExclusiveHdfsFileHandle() use this subclass.

Separately, testing revealed that increasing the number of
partitions for the file handle cache also fixes the contention
problem. This changes the file handle cache to make the number
of partitions configurable via startup parameter
num_file_handle_cache_partitions. This allows mitigation of
future bottlenecks without a patch.

Change-Id: I4ab52b0884a909a4faeb6692f32d45878ea2838f
---
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
M be/src/runtime/io/handle-cache.h
M be/src/runtime/io/handle-cache.inline.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
6 files changed, 137 insertions(+), 87 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4ab52b0884a909a4faeb6692f32d45878ea2838f
Gerrit-Change-Number: 8945
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell

[Impala-ASF-CR](asf-site) Fixed broken link to sha for 2.10 on downloads page.

2018-01-04 Thread Thomas Tauber-Marshall (Code Review)

Thomas Tauber-Marshall has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8944 )

Change subject: Fixed broken link to sha for 2.10 on downloads page.
..

Fixed broken link to sha for 2.10 on downloads page.

Change-Id: I32055599bc83b28cc964e10014a257ecb48c8a44
Reviewed-on: http://gerrit.cloudera.org:8080/8944
Reviewed-by: Michael Brown 
Tested-by: Thomas Tauber-Marshall 
---
M downloads.html
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  Michael Brown: Looks good to me, approved
  Thomas Tauber-Marshall: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: merged
Gerrit-Change-Id: I32055599bc83b28cc964e10014a257ecb48c8a44
Gerrit-Change-Number: 8944
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Thomas Tauber-Marshall

[Impala-ASF-CR](asf-site) Fixed broken link to sha for 2.10 on downloads page.

2018-01-04 Thread Thomas Tauber-Marshall (Code Review)

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

Change subject: Fixed broken link to sha for 2.10 on downloads page.
..


Patch Set 2: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: comment
Gerrit-Change-Id: I32055599bc83b28cc964e10014a257ecb48c8a44
Gerrit-Change-Number: 8944
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Thu, 04 Jan 2018 23:34:56 +
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Update download and signature links for 2.11.0 release.

2018-01-04 Thread Michael Brown (Code Review)

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

Change subject: Update download and signature links for 2.11.0 release.
..


Patch Set 2:

> Looks right to me, maybe your browser has an old version cached?

Working now. Not sure what that was about.

 > I did however break one of the links: https://gerrit.cloudera.org/#/c/8944/

+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: comment
Gerrit-Change-Id: I82ead909394f810d980da903412a50228af86a00
Gerrit-Change-Number: 8926
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Thu, 04 Jan 2018 23:32:50 +
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Fixed broken link to sha for 2.10 on downloads page.

2018-01-04 Thread Michael Brown (Code Review)

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

Change subject: Fixed broken link to sha for 2.10 on downloads page.
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: comment
Gerrit-Change-Id: I32055599bc83b28cc964e10014a257ecb48c8a44
Gerrit-Change-Number: 8944
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Michael Brown 
Gerrit-Comment-Date: Thu, 04 Jan 2018 23:32:13 +
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Fixed broken link to sha for 2.10 on downloads page.

2018-01-04 Thread Thomas Tauber-Marshall (Code Review)

Thomas Tauber-Marshall has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8944 )

Change subject: Fixed broken link to sha for 2.10 on downloads page.
..

Fixed broken link to sha for 2.10 on downloads page.

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32055599bc83b28cc964e10014a257ecb48c8a44
Gerrit-Change-Number: 8944
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall

[Impala-ASF-CR](asf-site) Update download and signature links for 2.11.0 release.

2018-01-04 Thread Thomas Tauber-Marshall (Code Review)

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

Change subject: Update download and signature links for 2.11.0 release.
..


Patch Set 2:

> Is something bused with site refresh? Looks like this is in the
 > asf-site branch, but http://impala.apache.org/downloads.html
 > doesn't show it.

Looks right to me, maybe your browser has an old version cached?

I did however break one of the links: https://gerrit.cloudera.org/#/c/8944/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: comment
Gerrit-Change-Id: I82ead909394f810d980da903412a50228af86a00
Gerrit-Change-Number: 8926
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Thu, 04 Jan 2018 22:39:15 +
Gerrit-HasComments: No

[Impala-ASF-CR](asf-site) Fixed broken link to sha for 2.10 on downloads page.

2018-01-04 Thread Thomas Tauber-Marshall (Code Review)

Thomas Tauber-Marshall has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8944


Change subject: Fixed broken link to sha for 2.10 on downloads page.
..

Fixed broken link to sha for 2.10 on downloads page.

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



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: newchange
Gerrit-Change-Id: I32055599bc83b28cc964e10014a257ecb48c8a44
Gerrit-Change-Number: 8944
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall

[Impala-ASF-CR] IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug mode

2018-01-04 Thread Thomas Tauber-Marshall (Code Review)

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

Change subject: IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug 
mode
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I93e2b1efb325100d01d398e68e789d87b877167e
Gerrit-Change-Number: 8923
Gerrit-PatchSet: 5
Gerrit-Owner: Manaswini Maharana 
Gerrit-Reviewer: Manaswini Maharana 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 04 Jan 2018 22:30:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug mode

2018-01-04 Thread Impala Public Jenkins (Code Review)

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

Change subject: IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug 
mode
..


Patch Set 5:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I93e2b1efb325100d01d398e68e789d87b877167e
Gerrit-Change-Number: 8923
Gerrit-PatchSet: 5
Gerrit-Owner: Manaswini Maharana 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Manaswini Maharana 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 04 Jan 2018 22:30:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4835: Part 2: Allocate scan range buffers upfront

2018-01-04 Thread Tim Armstrong (Code Review)

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

Change subject: IMPALA-4835: Part 2: Allocate scan range buffers upfront
..

IMPALA-4835: Part 2: Allocate scan range buffers upfront

This change is a step towards reserving memory for buffers from the
buffer pool and constraining per-scanner memory requirements. This
change restructures the DiskIoMgr code so that each ScanRange operates
with a fixed set of buffers that are allocated upfront and recycled as
the I/O mgr works through the ScanRange.

One major change is that ScanRanges get blocked when a buffer is not
available and get unblocked when a client returns a buffer via
ReturnBuffer(). I was able to remove the logic to maintain the
blocked_ranges_ list by instead adding a separate set for all ranges
that are active.

I plan to merge this along with the actual buffer pool switch, but
separated it out to allow review of the DiskIoMgr changes separate from
other aspects of the buffer pool switchover.

Testing:
* Ran core tests.
* TODO: run exhaustive tests.

Change-Id: Ia243bf8b62feeea602b122e0503ea4ba7d3ee70f
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scan-node-mt.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/scanner-context.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/io/disk-io-mgr-stress-test.cc
M be/src/runtime/io/disk-io-mgr-stress.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/disk-io-mgr.h
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.h
M be/src/util/runtime-profile-counters.h
19 files changed, 705 insertions(+), 401 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia243bf8b62feeea602b122e0503ea4ba7d3ee70f
Gerrit-Change-Number: 8707
Gerrit-PatchSet: 10
Gerrit-Owner: Tim Armstrong

[Impala-ASF-CR] IMPALA-6348: Redact only sensitive fields in runtime profiles

2018-01-04 Thread Sailesh Mukil (Code Review)

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

Change subject: IMPALA-6348: Redact only sensitive fields in runtime profiles
..


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8934/2//COMMIT_MSG@29
PS2, Line 29: Other fields in the runtime profile are left unredacted.
This implicitly states that we aren't allowed to log anything sensitive in any 
field other than the above 4 mentioned. Is that commented or documented 
somewhere? If not, it would be good to add that.


http://gerrit.cloudera.org:8080/#/c/8934/2/tests/custom_cluster/test_redaction.py
File tests/custom_cluster/test_redaction.py:

http://gerrit.cloudera.org:8080/#/c/8934/2/tests/custom_cluster/test_redaction.py@336
PS2, Line 336: self.assert_query_profile_contains(self.find_last_query_id(), 
user_profile_pattern)
Should we do this again? It's already done on L313?


http://gerrit.cloudera.org:8080/#/c/8934/2/tests/custom_cluster/test_redaction.py@337
PS2, Line 337: assert_query_profile_contains
Why not use assert_web_ui_redaction() here?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae3b6726009bf458a7ec73131e5d659b12ab73cf
Gerrit-Change-Number: 8934
Gerrit-PatchSet: 2
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Thu, 04 Jan 2018 21:47:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6362: avoid Reservation/MemTracker deadlock

2018-01-04 Thread Bikramjeet Vig (Code Review)

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

Change subject: IMPALA-6362: avoid Reservation/MemTracker deadlock
..


Patch Set 4: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7adbe961a925075422c685690dd3d1609779ced
Gerrit-Change-Number: 8933
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 04 Jan 2018 21:18:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1767: [DOCS] Document new Boolean operators

2018-01-04 Thread John Russell (Code Review)

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

Change subject: IMPALA-1767: [DOCS] Document new Boolean operators
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8942/1/docs/topics/impala_operators.xml
File docs/topics/impala_operators.xml:

http://gerrit.cloudera.org:8080/#/c/8942/1/docs/topics/impala_operators.xml@1345
PS1, Line 1345: b is true, b is false
> Include IS_UNKNOWN in this example so it's shown somewhere. Then consider i
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iefebf210418ec2d47b154bd37166b76720f085bb
Gerrit-Change-Number: 8942
Gerrit-PatchSet: 1
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 04 Jan 2018 19:58:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1767: [DOCS] Document new Boolean operators

2018-01-04 Thread John Russell (Code Review)

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

Change subject: IMPALA-1767: [DOCS] Document new Boolean operators
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8942/1/docs/topics/impala_operators.xml
File docs/topics/impala_operators.xml:

http://gerrit.cloudera.org:8080/#/c/8942/1/docs/topics/impala_operators.xml@1345
PS1, Line 1345: b is true, b is false
Include IS_UNKNOWN in this example so it's shown somewhere. Then consider if it 
makes sense to reuse or trim down this same example under IS NULL.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iefebf210418ec2d47b154bd37166b76720f085bb
Gerrit-Change-Number: 8942
Gerrit-PatchSet: 2
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 04 Jan 2018 19:57:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1767: [DOCS] Document new Boolean operators

2018-01-04 Thread John Russell (Code Review)

Hello Greg Rahn, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-1767: [DOCS] Document new Boolean operators
..

IMPALA-1767: [DOCS] Document new Boolean operators

Change-Id: Iefebf210418ec2d47b154bd37166b76720f085bb
---
M docs/impala_keydefs.ditamap
M docs/shared/impala_common.xml
M docs/topics/impala_conditional_functions.xml
M docs/topics/impala_operators.xml
4 files changed, 129 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iefebf210418ec2d47b154bd37166b76720f085bb
Gerrit-Change-Number: 8942
Gerrit-PatchSet: 2
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Vuk Ercegovac

[Impala-ASF-CR] IMPALA-5058: Improve the concurrency of DDL/DML operations

2018-01-04 Thread Dimitris Tsirogiannis (Code Review)

Hello Bharath Vissapragada, Tianyi Wang, Alex Behm, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-5058: Improve the concurrency of DDL/DML operations
..

IMPALA-5058: Improve the concurrency of DDL/DML operations

Problem: A long running table metadata operation (e.g. refresh) could
prevent any other metadata operation from making progress if it
coincided with the catalog topic creation operations. The problem was due
to the conservative locking scheme used when catalog topics were
created. In particular, in order to collect a consistent snapshot of
metadata changes, the global catalog lock was held for the entire
duration of that operation.

Solution: To improve the concurrency of catalog operations the following
changes are performed:
* A range of catalog versions determines the catalog changes to be
  included in a catalog update. Any catalog changes that do not fall in
  the specified range are ignored (to be processed in subsequent catalog
  topic updates).
* The catalog allows metadata operations to make progress while
  collecting catalog updates.
* To prevent starvation of catalog updates (i.e. frequently updated
  catalog objects skipping catalog updates indefinitely), we keep track
  of the number of times a catalog object has skipped an update and if
  that number exceeds a threshold it is included in the next catalog
  topic update even if its version is not in the specified topic update
  version range. Hence, the same catalog object may be sent in two
  consecutive catalog topic updates.

This commit also changes the way deletions are handled in the catalog and
disseminated to the impalad nodes through the statestore. In particular:
* Deletions in the catalog are explicitly recorded in a log with
the catalog version in which they occurred. As before, added and deleted
catalog objects are sent to the statestore.
* Topic entries associated with deleted catalog objects have non-empty
values (besided keys) that contain minimal object metadata including the
catalog version.
* Statestore is no longer using the existence or not of
topic entry values in order to identify deleted topic entries. Deleted
topic entries should be explicitly marked as such by the statestore
subscribers that produce them.
* Statestore subscribers now use the 'deleted' flag to determine if a
topic entry corresponds to a deleted item.
* Impalads use the deleted objects' catalog versions when updating the
local catalog cache from a catalog update and not the update's maximum
catalog version.

Testing:
- No new tests were added as these paths are already exercised by
existing tests.
- Run all core and exhaustive tests.

Change-Id: If12467a83acaeca6a127491d89291dedba91a35a
Reviewed-on: http://gerrit.cloudera.org:8080/7731
Reviewed-by: Dimitris Tsirogiannis 
Tested-by: Impala Public Jenkins
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler.cc
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore.cc
M be/src/statestore/statestore.h
M common/thrift/CatalogInternalService.thrift
M common/thrift/CatalogService.thrift
M common/thrift/Frontend.thrift
M common/thrift/StatestoreService.thrift
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObject.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java
A fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
A fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionQueue.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/DataSource.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/HdfsCachePool.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Role.java
M fe/src/main/java/org/apache/impala/catalog/RolePrivilege.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
A fe/src/main/java/org/apache/impala/catalog/TopicUpdateLog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M

[Impala-ASF-CR] IMPALA-5058: Improve the concurrency of DDL/DML operations

2018-01-04 Thread Dimitris Tsirogiannis (Code Review)

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

Change subject: IMPALA-5058: Improve the concurrency of DDL/DML operations
..


Patch Set 2: Code-Review+2

(14 comments)

Thanks for the reviews. Waiting on the results from the next concurrency 
testing from Mostafa and if everything looks good, I'll merge the change.

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

http://gerrit.cloudera.org:8080/#/c/8752/2/be/src/catalog/catalog-server.cc@331
PS2, Line 331: VLOG(1) << "Publishing " << (topic_deletions ? "deletion " : 
"update ")
> This could probably be verbose if we print for every catalog_object?  I agr
Yes that was intentional. The rational is the following. We already log the 
catalog objects sent in the catalog so it makes sense to me to log the received 
items at the impalads. I believe it can help debug some nasty issues (missing 
updates, etc). For now I suggest we leave it as is.


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

http://gerrit.cloudera.org:8080/#/c/8752/2/be/src/service/impala-server.h@339
PS2, Line 339:   // Wait until the minimum catalog object version in the local 
cache exceeds
> /// style
Done


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

http://gerrit.cloudera.org:8080/#/c/8752/2/be/src/service/impala-server.cc@1474
PS2, Line 1474:   VLOG_QUERY << "Done waiting for catalog version: " << 
catalog_update_version;
> nit: We should probably not log this if we detect a catalog_service_id chan
Good point. Done


http://gerrit.cloudera.org:8080/#/c/8752/2/common/thrift/CatalogService.thrift
File common/thrift/CatalogService.thrift:

http://gerrit.cloudera.org:8080/#/c/8752/2/common/thrift/CatalogService.thrift@57
PS2, Line 57: / True if this is a result of an INVALIDATE METADATA operation.
:   4: required bool is_invalidate
> Just curious, wouldn't we know this information from the ClientRequestState
Unfortunately, that's not always the case so for these cases, the simplest 
thing to do was to add that information as part of the catalog response.


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

http://gerrit.cloudera.org:8080/#/c/8752/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@89
PS2, Line 89:CatalogObjectVersionQueue.INSTANCE.removeVersion(
:   existingRole.getCatalogVersion());
> Wouldn't removeRole() -> roleCache_.remove(roleName) already do this?
You're right. It's not needed here. Removed.


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

http://gerrit.cloudera.org:8080/#/c/8752/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectVersionQueue.java@30
PS2, Line 30:  * versions. Not thread-safe.
> nit: I think it helps to define the use case of this class here. (like desc
Done


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

http://gerrit.cloudera.org:8080/#/c/8752/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@93
PS2, Line 93:  * Periodically, the CatalogServiceCatalog collects a delta of 
catalog updates (based on a
> Thanks! This is really clear to me now.
Done


http://gerrit.cloudera.org:8080/#/c/8752/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@95
PS2, Line 95:  * Each catalog topic update is defined by a range of catalog 
versions [from, to) and the
> (from, to]
Done


http://gerrit.cloudera.org:8080/#/c/8752/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@96
PS2, Line 96:  * CatalogServiceCatalog guarantees that very catalog object that 
has a version in the
> typo: every
Done


http://gerrit.cloudera.org:8080/#/c/8752/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@585
PS2, Line 585: =
> nit: maybe make >= ? just to be on the safer side.
Done


http://gerrit.cloudera.org:8080/#/c/8752/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@628
PS2, Line 628: if (LOG.isDebugEnabled()) {
 :   LOG.debug(String.format("Error calling toThrift() on 
table %s: %s",
 :   tbl.getFullName(), e.getMessage()), e);
 : }
 : return;
> I think that is a valid LOG.error(), given we are missing an update of a ve
Done

[Impala-ASF-CR] IMPALA-1767: [DOCS] Document new Boolean operators

2018-01-04 Thread John Russell (Code Review)

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

Change subject: IMPALA-1767: [DOCS] Document new Boolean operators
..


Patch Set 1:

I put IS [NOT] UNKNOWN under the existing heading for IS [NOT] NULL since those 
forms are so similar. Then a separate new heading for IS TRUE / IS FALSE.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iefebf210418ec2d47b154bd37166b76720f085bb
Gerrit-Change-Number: 8942
Gerrit-PatchSet: 1
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 04 Jan 2018 19:40:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1767: [DOCS] Document new Boolean operators

2018-01-04 Thread John Russell (Code Review)

John Russell has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8942


Change subject: IMPALA-1767: [DOCS] Document new Boolean operators
..

IMPALA-1767: [DOCS] Document new Boolean operators

Change-Id: Iefebf210418ec2d47b154bd37166b76720f085bb
---
M docs/impala_keydefs.ditamap
M docs/shared/impala_common.xml
M docs/topics/impala_conditional_functions.xml
M docs/topics/impala_operators.xml
4 files changed, 128 insertions(+), 2 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iefebf210418ec2d47b154bd37166b76720f085bb
Gerrit-Change-Number: 8942
Gerrit-PatchSet: 1
Gerrit-Owner: John Russell

[Impala-ASF-CR] IMPALA-6193: Track memory of incoming data streams

2018-01-04 Thread Lars Volker (Code Review)

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

Change subject: IMPALA-6193: Track memory of incoming data streams
..


Patch Set 3:

(10 comments)

Thanks for the comments, please see PS3.

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

http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG@9
PS2, Line 9: change
> change
Done


http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG@10
PS2, Line 10:
> nit, will help to know which mem tracker this is counted against, in case s
Done


http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG@11
PS2, Line 11: nager. There we
> another global tracker labelled "Rpc Memory"
Done


http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG@13
PS2, Line 13: register
> nit: receiver,
Done


http://gerrit.cloudera.org:8080/#/c/8914/2//COMMIT_MSG@13
PS2, Line 13: e the receiver,
: memory
> nit, just to make it more clearer what the term instance means here: its fr
Done


http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/rpc/rpc-mgr.h@102
PS2, Line 102: Status Init() WARN_UNUSED_RESULT;
> This seems like the wrong abstraction to me. I would expect the caller of R
I changed it as you suggested.


http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/rpc/rpc-mgr.cc
File be/src/rpc/rpc-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/rpc/rpc-mgr.cc@29
PS2, Line 29:
> Duplicated line. Bad rebase ?
Done


http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/exec-env.cc@325
PS2, Line 325: t to track
> "Rpc Memory" seems vague. Also, please see comments in rpc-mgr about how we
I changed it to have one pool per service. I kept separate trackers for memory 
in the inbound call queue and the early sender queue. Let me know if you prefer 
to use the same tracker for both.


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

http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/krpc-data-stream-mgr.cc@203
PS2, Line 203:   TransmitDataCtx payload(request, response, rpc_context);
 :   Status status = payload.Init();
> Not too thrilled about this extra malloc here. Can we avoid it altogether ?
I removed it so that the normal path of not queueing it does not have the 
malloc.


http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/krpc-data-stream-mgr.inline.h
File be/src/runtime/krpc-data-stream-mgr.inline.h:

http://gerrit.cloudera.org:8080/#/c/8914/2/be/src/runtime/krpc-data-stream-mgr.inline.h@25
PS2, Line 25:
:
: const TransmitDataRequestPB* TransmitDataCtx::request() const {
:   DCHECK(initialized_);
:   return request_;
: }
:
: TransmitDataResponsePB* TransmitDataCtx::response() {
:   DCHECK(initialized_);
:   return response_;
: }
:
: kudu::rpc::RpcContext* TransmitDataCtx::rpc_context() {
:   DCHECK(initialized_);
:   return rpc_context_;
: }
:
: const kudu::Slice& TransmitDataCtx::tuple_offsets() const {
:   DCHECK(initialized_);
:   return tuple_offsets_;
: }
:
: const kudu::Slice& TransmitDataCtx::tuple_data() const {
:   DCHECK(initialized_);
:   return tuple_data_;
: }
:
: int64_t TransmitDataCtx::serialized_size() const {
:   DCHECK(initialized_);
:   return serialized_size_;
: }
:
: int64_t TransmitDataCtx::deserialized_size() const {
:   DCHECK(initialized_);
:   return deserialized_size_;
: }
:
: void TransmitDataCtx::RespondStatus(const Status& status) {
:   status.ToProto(response_->mutable_status());
:   rpc_context_->RespondSuccess();
: }
:
: void EndDataStreamCtx::RespondStatus(const Status& status) {
:   status.ToProto(response_->mutable_status());
:   rpc_context_->RespondSuccess();
: }
> Most of these look like one liners which can be moved to krpc-data-stream-m
My intention was to make the code safer by adding the DCHECKs, which makes them 
2 lines. I'm still happy to move them to the header as such, let me know if you 
prefer that.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2df1204d2483313a8a18e5e3be6cec9e402614c4

[Impala-ASF-CR] IMPALA-6193: Track memory of incoming data streams

2018-01-04 Thread Lars Volker (Code Review)

Hello Michael Ho, Bikramjeet Vig,

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

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

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

Change subject: IMPALA-6193: Track memory of incoming data streams
..

IMPALA-6193: Track memory of incoming data streams

This change adds memory tracking to incoming transmit data RPCs when
using KRPC. We track memory against a global tracker called "Data Stream
Service" until it is handed over to the stream manager. There we track
it in a global tracker called "Data Stream Manager" until a receiver
registers and takes over the early sender RPCs. Inside the receiver,
memory for deferred RPCs is tracked against the fragment instance's
memtracker until we unpack the batches and add them to the row batch
queue.

The DCHECK in MemTracker::Close() covers that all memory consumed by a
tracker gets release eventually. In addition to that, this change adds a
custom cluster test that makes sure that queued memory gets tracked by
inspecting the peak consumption of the new memtrackers.

Change-Id: I2df1204d2483313a8a18e5e3be6cec9e402614c4
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/runtime/exec-env.cc
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
A be/src/runtime/krpc-data-stream-mgr.inline.h
M be/src/runtime/krpc-data-stream-recvr.cc
M be/src/runtime/krpc-data-stream-recvr.h
M be/src/runtime/mem-tracker.h
M be/src/util/memory-metrics.h
M common/protobuf/data_stream_service.proto
A tests/custom_cluster/test_krpc_mem_usage.py
A tests/verifiers/mem_usage_verifier.py
15 files changed, 432 insertions(+), 161 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2df1204d2483313a8a18e5e3be6cec9e402614c4
Gerrit-Change-Number: 8914
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho

[Impala-ASF-CR](asf-site) Update download and signature links for 2.11.0 release.

2018-01-04 Thread Michael Brown (Code Review)

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

Change subject: Update download and signature links for 2.11.0 release.
..


Patch Set 2:

Is something bused with site refresh? Looks like this is in the asf-site 
branch, but http://impala.apache.org/downloads.html doesn't show it.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-MessageType: comment
Gerrit-Change-Id: I82ead909394f810d980da903412a50228af86a00
Gerrit-Change-Number: 8926
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Thu, 04 Jan 2018 16:35:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6362: avoid Reservation/MemTracker deadlock

2018-01-04 Thread Tim Armstrong (Code Review)

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

Change subject: IMPALA-6362: avoid Reservation/MemTracker deadlock
..


Patch Set 3:

I took another look at the repro and was able to simplify it a bit further.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7adbe961a925075422c685690dd3d1609779ced
Gerrit-Change-Number: 8933
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 04 Jan 2018 16:32:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6362: avoid Reservation/MemTracker deadlock

2018-01-04 Thread Tim Armstrong (Code Review)

Hello Michael Ho, Bikramjeet Vig,

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

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

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

Change subject: IMPALA-6362: avoid Reservation/MemTracker deadlock
..

IMPALA-6362: avoid Reservation/MemTracker deadlock

Avoid the circular dependency between ReservationTracker::lock_ and
MemTracker::child_trackers_lock_ by not acquiring
ReservationTracker::lock_ in GetReservation(), where an atomic
operation is sufficient.

Testing:
Added a unit test that reproed the deadlock.

Change-Id: Id7adbe961a925075422c685690dd3d1609779ced
---
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/reservation-tracker.cc
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
5 files changed, 75 insertions(+), 15 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7adbe961a925075422c685690dd3d1609779ced
Gerrit-Change-Number: 8933
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-6362: avoid Reservation/MemTracker deadlock

2018-01-04 Thread Tim Armstrong (Code Review)

Hello Michael Ho, Bikramjeet Vig,

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

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

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

Change subject: IMPALA-6362: avoid Reservation/MemTracker deadlock
..

IMPALA-6362: avoid Reservation/MemTracker deadlock

Avoid the circular dependency between ReservationTracker::lock_ and
MemTracker::child_trackers_lock_ by not acquiring
ReservationTracker::lock_ in GetReservation(), where an atomic
operation is sufficient.

Testing:
Added a unit test that reproed the deadlock.

Change-Id: Id7adbe961a925075422c685690dd3d1609779ced
---
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/reservation-tracker.cc
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
5 files changed, 80 insertions(+), 15 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7adbe961a925075422c685690dd3d1609779ced
Gerrit-Change-Number: 8933
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-6362: avoid Reservation/MemTracker deadlock

2018-01-04 Thread Tim Armstrong (Code Review)

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

Change subject: IMPALA-6362: avoid Reservation/MemTracker deadlock
..


Patch Set 1:

(1 comment)

I was able to come up with a regression test, thanks for the suggestion, it 
seems like the right thing to do.

http://gerrit.cloudera.org:8080/#/c/8933/1/be/src/runtime/bufferpool/reservation-tracker.cc
File be/src/runtime/bufferpool/reservation-tracker.cc:

http://gerrit.cloudera.org:8080/#/c/8933/1/be/src/runtime/bufferpool/reservation-tracker.cc@209
PS1, Line 209: parent_
> I think it is possible to get to this point and have parent_ == nullptr
This isn't possible because of how the ReservationTracker is initialised - 
InitRootTracker() doesn't take a MemTracker* argument. This is confusing 
though. I restructured the branches to make the invariant more obvious.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id7adbe961a925075422c685690dd3d1609779ced
Gerrit-Change-Number: 8933
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 04 Jan 2018 16:12:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug mode

2018-01-04 Thread Manaswini Maharana (Code Review)

Manaswini Maharana has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8923 )

Change subject: IMPALA-6296: Avoid crash caused by DCHECK in Codegen in debug 
mode
..


Patch Set 5:

Jenkin tests-
pre-review-test:  https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/879/
gerrit-verify-dryrun-external:  
https://jenkins.impala.io/job/gerrit-verify-dryrun-external/52/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I93e2b1efb325100d01d398e68e789d87b877167e
Gerrit-Change-Number: 8923
Gerrit-PatchSet: 5
Gerrit-Owner: Manaswini Maharana 
Gerrit-Reviewer: Manaswini Maharana 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 04 Jan 2018 16:11:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6362: avoid Reservation/MemTracker deadlock

2018-01-04 Thread Tim Armstrong (Code Review)

Hello Michael Ho, Bikramjeet Vig,

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

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

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

Change subject: IMPALA-6362: avoid Reservation/MemTracker deadlock
..

IMPALA-6362: avoid Reservation/MemTracker deadlock

Avoid the circular dependency between ReservationTracker::lock_ and
MemTracker::child_trackers_lock_ by not acquiring
ReservationTracker::lock_ in GetReservation(), where an atomic
operation is sufficient.

Testing:
Added a unit test that reproed the deadlock.

Change-Id: Id7adbe961a925075422c685690dd3d1609779ced
---
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/reservation-tracker.cc
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/util/memory-metrics.cc
M be/src/util/memory-metrics.h
5 files changed, 80 insertions(+), 15 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id7adbe961a925075422c685690dd3d1609779ced
Gerrit-Change-Number: 8933
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong

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

2018-01-04 Thread Gabor Kaszab (Code Review)

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

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


Patch Set 8:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@67
PS5, Line 67:  // Kudu table name generated during analysis
> There are other table properties that get generated during the analysis tha
I see your point.
The intention of this map was to gradually include all the generated table 
properties. At this point this is in fact an overkill, so I would go for 
replacing this map with a single String for generatedKuduTableNameProperty or 
such.


http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@68
PS5, Line 68:
> use Guava's (Maps.newHashMap()) here and everywhere you allocate a HashMap.
Replaced the map with a String.


http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@179
PS5, Line 179:
> Not sure about the validity of this statement. I'd expect user provided val
Replaced the map with a String so this is not needed anymore.


http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@313
PS5, Line 313:   } catch (ImpalaRuntimeException e) {
 : throw new AnalysisException(String.format(
> Not following that comment. I don't see anything specific to re-analyzing s
That comment in fact was misleading. Removed it as the function comment of 
setManagedKuduTableName() should actually explain the intent.


http://gerrit.cloudera.org:8080/#/c/8820/5/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@315
PS5, Line 315: "Cannot create tab
> inline function (it only has 2 statements and it's not used anywhere else).
I kind of disagree here: The underlying function is 5 lines long, and reading 
here a single function name instead of those lines, in my opinion increases 
code readability.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieca037498abf8f5fde67b77e824b720482cdbe6f
Gerrit-Change-Number: 8820
Gerrit-PatchSet: 8
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 04 Jan 2018 15:02:25 +
Gerrit-HasComments: Yes

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

2018-01-04 Thread Gabor Kaszab (Code Review)

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

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

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

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

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

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

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

Alongside the CREATE TABLE statement also the ALTER TABLE statement
is changed not to allow the modification of Kudu.table_name of
managed Kudu tables.

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


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

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

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

2018-01-04 Thread Gabor Kaszab (Code Review)

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

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

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

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

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

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

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

Alongside the CREATE TABLE statement also the ALTER TABLE statement
is changed not to allow the modification of Kudu.table_name of
managed Kudu tables.

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


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

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

55 matches

Mail list logo