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

[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] Added the "SET ROW FORMAT" option to the "ALTER TABLE" command.

[Kim Jin Chul (Code Review)]

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

Change subject: Added the "SET ROW FORMAT" option to the "ALTER TABLE" command.
..


Patch Set 1:

(11 comments)

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

http://gerrit.cloudera.org:8080/#/c/8928/1//COMMIT_MSG@7
PS1, Line 7: Added the "SET ROW FORMAT" option to the "ALTER TABLE" command.
Adds "IMPALA-4323: " at the start of the line.
Adds "Testing:" description.


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

http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/cup/sql-parser.cup@1003
PS1, Line 1003:   | KW_ALTER KW_TABLE table_name:table 
opt_partition_set:partition KW_SET
Add some unit tests(white & black) into ParserTest.java. See
https://github.com/cloudera/Impala/blob/cdh5-trunk/fe/src/test/java/org/apache/impala/analysis/ParserTest.java#L2317


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

http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java@35
PS1, Line 35:
nit: use space instead of tab


http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2231
PS1, Line 2231:   private boolean alterTableSetFileFormat(Table tbl,
I guess you might use the code in this function. I think there are some 
redundant code. Would you please check there is any room to refactor the both 
code? I am worry about the following case. Let's assume:
1. Function foo has a bug and a patch should be applied to the function.
2. Function boo is spawned from the function foo.
3. Unfortunately a developer isn't aware of function boo and the bug can exist 
in boo.


http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2269
PS1, Line 2269:   private boolean alterTableSetRowFormat(Table tbl, 
List partitionSet,
nit: all lines should be limited to 90 characters in width


http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2283
PS1, Line 2283: sd.getSerdeInfo().putToParameters("field.delim", 
rowFormat.getFieldDelimiter());
nit: exceeds the limit


http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2293
PS1, Line 2293: if (tbl instanceof HdfsTable) ((HdfsTable) 
tbl).addDefaultPartition(msTbl.getSd());
nit: exceeds the limit


http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2305
PS1, Line 2305: 
partition.getSerdeInfo().putToParameters("field.delim", 
rowFormat.getFieldDelimiter());
nit: exceeds the limit


http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2308
PS1, Line 2308: 
partition.getSerdeInfo().putToParameters("escape.delim", 
rowFormat.getEscapeChar());
nit: exceeds the limit


http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2311
PS1, Line 2311: 
partition.getSerdeInfo().putToParameters("line.delim", 
rowFormat.getLineDelimiter());
nit: exceeds the limit


http://gerrit.cloudera.org:8080/#/c/8928/1/tests/query_test/test_delimited_text.py
File tests/query_test/test_delimited_text.py:

http://gerrit.cloudera.org:8080/#/c/8928/1/tests/query_test/test_delimited_text.py@164
PS1, Line 164:   def test_delimited_text_partitioned(self, vector, 
unique_database):
This test is very similar to test_delimited_text_alter. Do you refactor the 
both code?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I96e347463504915a6f33932552e4d1f61e9b1154
Gerrit-Change-Number: 8928
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 03 Jan 2018 13:56:34 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

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

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

IMPALA-3282: Adds regexp_escape built-in function

Escapes the following special characters:
".*\\+?^[](){}$!=:-#\n\r\t\v "

Testing:
Adds some unit tests into ExprTest.StringRegexpFunctions

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


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/8900/2
--
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: newpatchset
Gerrit-Change-Id: I84c3e0ded26f6eb20794c38b75be9b25cd111e4b
Gerrit-Change-Number: 8900
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 

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

[Kim Jin Chul (Code Review)]

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


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

IMPALA-3282: Adds regexp_escape built-in function

Escapes the following special characters:
".*\\+?^[](){}$!=:-#\n\r\t\v "

Testing:
Adds some unit tests into ExprTest.StringRegexpFunctions

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



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/8900/1
--
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: newchange
Gerrit-Change-Id: I84c3e0ded26f6eb20794c38b75be9b25cd111e4b
Gerrit-Change-Number: 8900
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 

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

[Kim Jin Chul (Code Review)]

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

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

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
5 files changed, 120 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/8893/2
--
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: 2
Gerrit-Owner: Kim Jin Chul 

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

[Kim Jin Chul (Code Review)]

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


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

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
5 files changed, 120 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/8893/1
--
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: newchange
Gerrit-Change-Id: I14d56ffb8fab256f3f66a2669271fd4b3c50cc29
Gerrit-Change-Number: 8893
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for 
pcg-cpp
..


Patch Set 5:

Jim, thanks for your review!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 19 Dec 2017 00:03:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3942: Fix wronly escaped string literal in front-end

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-3942: Fix wronly escaped string literal in front-end
..


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8818/2/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@104
PS2, Line 104: getEscapedValue
Escaping and unescaping the value happen here. Do you have more intuitive name 
for this? (e.g. getNormalizedValue)


http://gerrit.cloudera.org:8080/#/c/8818/2/tests/query_test/test_queries.py
File tests/query_test/test_queries.py:

http://gerrit.cloudera.org:8080/#/c/8818/2/tests/query_test/test_queries.py@276
PS2, Line 276:   def test_simple(self):
Please let me know if there is any missing tests.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
Gerrit-Change-Number: 8818
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 18 Dec 2017 08:35:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3942: Fix wronly escaped string literal in front-end

[Kim Jin Chul (Code Review)]

Hello Gabor Kaszab, Jim Apple, Tim Armstrong, Alex Behm,

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

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

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

Change subject: IMPALA-3942: Fix wronly escaped string literal in front-end
..

IMPALA-3942: Fix wronly escaped string literal in front-end

String literal can be wrapped by either single or double quotes.
There are some holes in escaping the string literal. The solution
is to normalize any string which comes from user's given string or
a generated string (e.g. constant fold by the rewritter rule).

Testing:
Add some test cases to TestEscapingStringLiteral

Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
---
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M tests/query_test/test_queries.py
2 files changed, 112 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
Gerrit-Change-Number: 8818
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for 
pcg-cpp
..


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8829/2//COMMIT_MSG@11
PS2, Line 11: ck.
> nit: long line
Done


http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
File be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp:

http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp@37
PS2, Line 37: // To avoid of warning: 'PCG_USE_ZEROCHECK_ROTATE_IDIOM' is not 
defined, evaluates to 0
> Please wrap each in an ifndef block.
Done


http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp@599
PS2, Line 599:  * warning: expansion of date or time macro is not reproducible
> nit: can you reword slightly - it's not just that clang-tidy complains, it'
I agree. It is better to show a main reason why disable the code.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 18 Dec 2017 05:14:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

[Kim Jin Chul (Code Review)]

Hello Jim Apple,

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

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

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

Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for 
pcg-cpp
..

IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

In the commit 4feb4f3a, the third party library pcg-cpp was excluded
from the clang-tidy check. It could make unexpected side effect, so
fixing some warnings from clang-tidy is better than avoidance of the
check.

Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
---
M be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
M bin/run_clang_tidy.sh
2 files changed, 13 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-3193: Show table's comment on show tables
..


Patch Set 3:

@Dimitris, thanks for your review of my initial change and share the 
information.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 18 Dec 2017 00:50:41 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3193: Show table's comment on show tables

[Kim Jin Chul (Code Review)]

Hello Dimitris Tsirogiannis,

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

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

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

Change subject: IMPALA-3193: Show table's comment on show tables
..

IMPALA-3193: Show table's comment on show tables

Currently SHOW TABLES command does not display given comment
when creating a table. DESC  shows column's comment and
SHOW DATABASES also shows database's comment.

In the catalog cache, db object has a metastore db, but table
object does not have a metastore table.
When JniFrontend.getTableNamesAndComments() is invoked,
metastore tables are collected and cached on the fly.

There is a change in thrift part: a list of table comments is added to
TGetTablesResult struct as an optional element.

Testing:
Add E2E test. TestShowTables.testTableCommentVisibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/service/client-request-state.cc
M be/src/service/frontend.cc
M be/src/service/frontend.h
M be/src/service/impala-http-handler.cc
M be/src/service/impala-server.cc
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/main/java/org/apache/impala/service/MetadataOp.java
M tests/query_test/test_queries.py
14 files changed, 160 insertions(+), 38 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
Gerrit-Change-Number: 8851
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 

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

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

(2 comments)

Thanks for the finding missing parts.

http://gerrit.cloudera.org:8080/#/c/8676/7/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/7/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@a475
PS7, Line 475:
> What happened to this negative test?
Let me revive the test.


http://gerrit.cloudera.org:8080/#/c/8676/7/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@325
PS7, Line 325: }
> We should also have a negative test which shows that putting hints in both
Done. Sorry for the missing. It should have to be added.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Sat, 16 Dec 2017 12:53:19 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello John Russell, Alex Behm, Vuk Ercegovac,

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

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

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

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

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

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

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


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/8676/8
--
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: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac 

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 16:

@Zoltan, Tim, I appreciate your review!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 16
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Sat, 16 Dec 2017 07:19:52 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 15:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8762/15/bin/rat_exclude_files.txt
File bin/rat_exclude_files.txt:

http://gerrit.cloudera.org:8080/#/c/8762/15/bin/rat_exclude_files.txt@120
PS15, Line 120: tests/shell/shell_case_sensitive.cmds
  : tests/shell/shell_case_sensitive2.cmds
Fix for the failure:
+ bin/check-rat-report.py bin/rat_exclude_files.txt rat.xml
UNAPPROVED: tests/shell/shell_case_sensitive.cmds
UNAPPROVED: tests/shell/shell_case_sensitive2.cmds
(https://jenkins.impala.io/job/rat-check-ub1604/359/consoleText)


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

http://gerrit.cloudera.org:8080/#/c/8762/15/shell/impala_shell.py@1144
PS15, Line 1144: self.orig_cmd = "describe"
Fix for the failure:
E   desc test_do_methods_639a0d4c.test_do_methods
E   ^
E   Encountered: DESC
E   Expected: ALTER, COMPUTE, CREATE, DELETE, DESCRIBE, DROP, EXPLAIN, GRANT, 
INSERT, INVALIDATE, LOAD, REFRESH, REVOKE, SELECT, SET, SHOW, TRUNCATE, UPDATE, 
UPSERT, USE, VALUES, WITH
(https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/810)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 05:27:25 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

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

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

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

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

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

IMPALA-4664: Unexpected string conversion in Shell

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

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

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


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

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

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

[Kim Jin Chul (Code Review)]

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

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

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

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

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

IMPALA-4664: Unexpected string conversion in Shell

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

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

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


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

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

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 12: Code-Review-1

Let me take a look at the failure and provide a new patch set.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 02:23:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3942: Fix unexpected conversion of string literal in front-end

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-3942: Fix unexpected conversion of string literal in 
front-end
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8818/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@49
PS1, Line 49: boolean useSingleQuote = true;
> Just considering what options we have here:
Your suggestion is possible. Actually I tried what you suggested firstly which 
might be same as Jim's proposal, but I had to handle some corner cases because 
value_ contains can have non of/single/double quotes. I still have the ongoing 
code locally, but the change is bigger than this and I am not sure any hole 
still exists. If you and Jim still think "keeping a given string literal at  
value_", please let me know pros and cons compared with this version.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
Gerrit-Change-Number: 8818
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 15 Dec 2017 00:10:23 +
Gerrit-HasComments: Yes

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

[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 5:

(3 comments)

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

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


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

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


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

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



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

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

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

[Kim Jin Chul (Code Review)]

Hello John Russell, Alex Behm, Vuk Ercegovac,

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

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

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

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

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

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

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


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

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

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

[Kim Jin Chul (Code Review)]

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

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

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

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

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

IMPALA-4664: Unexpected string conversion in Shell

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

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

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


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

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

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

[Kim Jin Chul (Code Review)]

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

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


Abandoned

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

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

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 6:

(3 comments)

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

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


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

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


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

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



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

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

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

[Kim Jin Chul (Code Review)]

Hello John Russell, Alex Behm, Vuk Ercegovac,

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

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

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

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

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

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

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


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

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

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

[Kim Jin Chul (Code Review)]

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

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

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

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

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

IMPALA-2640: Make a given command case-sensitive

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

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShell.test_var_substitution

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


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

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

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 10:

(6 comments)

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

http://gerrit.cloudera.org:8080/#/c/8762/6//COMMIT_MSG@7
PS6, Line 7: IMPALA-2640: Make a given command case-sensitive
In the fix of 
IMPALA-4664(https://gerrit.cloudera.org/#/c/8639/4/shell/impala_shell.py), you 
thought a right fix is to hack code for lower-casting. I think this change for 
IMPALA-2640 can solve the lower-casting problem properly.

The result of your example looks fine to me. Would you let me know what the 
problem is in the example? I guess the lowering of column description is 
intended, right?

> sElEcT 'FoOoOo';
Query: select 'FoOoOo'
Query submitted at: 2017-12-13 22:52:08 (Coordinator: http://ubuntu:25000)
Query progress can be monitored at: 
http://ubuntu:25000/query_plan?query_id=20454e49cb053379:538981e9
+--+
| 'fo' |
+--+
| FoOoOo   |
+--+


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

http://gerrit.cloudera.org:8080/#/c/8762/6/shell/impala_shell.py@572
PS6, Line 572: # is necessary to find
> To me it looks a bit weird to pass the command string to each command.
I think this intention, passing the command, is still valid for me.
do_* methods are always not invoked here. See 
https://github.com/apache/impala/blob/master/shell/impala_shell.py#L208

Due to the above case, we should

We may get the original command from


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

http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@305
PS9, Line 305: """Original command should be stored before running the 
method. The method is usually
> Long line
Done


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@310
PS9, Line 310:   print_to_stderr("Unexpected error: Failed to execute query 
due to command "\
> Long line.
Done


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@555
PS9, Line 555:
> Can you include attribution for where the copied code came from (e.g. see b
I added some comment for the coping. The PSF license is already included in 
LICENSE.txt.


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@571
PS9, Line 571: ne c
> _ on the end of a local variable is a bit weird to me - can we call it comm
Done



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

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

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for 
pcg-cpp
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
File be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp:

http://gerrit.cloudera.org:8080/#/c/8829/2/be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp@599
PS2, Line 599:  * warning: expansion of date or time macro is not reproducible
The struct is not used in our code and pcg-cpp's sample and test codes. I 
thought comment out is better than removal of the code because someone needs 
this later.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 13 Dec 2017 13:38:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

[Kim Jin Chul (Code Review)]

Hello Jim Apple,

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

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

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

Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for 
pcg-cpp
..

IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

In the commit 4feb4f3a, the third party library pcg-cpp was excluded
from the clang-tidy check. It could make unexpected side effect, so
fixing some warnings from clang-tidy is better than avoidance of the check.

Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
---
M be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
M bin/run_clang_tidy.sh
2 files changed, 8 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
Gerrit-Change-Number: 8829
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR] IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

[Kim Jin Chul (Code Review)]

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


Change subject: IMPALA-5754: Rollback the exclusion of clang-tidy check for 
pcg-cpp
..

IMPALA-5754: Rollback the exclusion of clang-tidy check for pcg-cpp

In the commit 4feb4f3a, the third party library pcg-cpp was excluded
from the clang-tidy check. It could make unexpected side effect, so
fixing some warnings from clang-tidy is better than avoidance of the check.

Change-Id: I591d30373cb13f0eb89afbe16d81b1d3fb783365
---
M be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
M bin/run_clang_tidy.sh
2 files changed, 4 insertions(+), 3 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 22:

@Jim, I pushed a fix for rollback of the exclusion for the third party check:
https://gerrit.cloudera.org/#/c/8829/

 > You'll notice that be/src/thirdparty/squeasel and mustache are
 > sometimes modified independent of their source, and one of the
 > warnings about reproducability can prevent support nightmares.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 22
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 13 Dec 2017 13:27:53 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 22:

@Jim, Michael and Attila, I appreciate your reviews!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 22
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 13 Dec 2017 13:26:29 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

Hello John Russell, Alex Behm, Vuk Ercegovac,

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

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

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

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

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

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

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


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

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

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

[Kim Jin Chul (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 2:

(4 comments)

Thanks a lot for your kind guidance and finding some missing points such as 
toSql and test enhancements.

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

http://gerrit.cloudera.org:8080/#/c/8676/2//COMMIT_MSG@7
PS2, Line 7: Adopt
> The jira says "adopt", but we're really not making this style the default.
Done.


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

http://gerrit.cloudera.org:8080/#/c/8676/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@44
PS2, Line 44:
> These tests confirm that the oracle style hints are indeed added as expecte
Done. Thanks for your kind guidance.


http://gerrit.cloudera.org:8080/#/c/8676/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@410
PS2, Line 410: "shuffle"
> This difference is preventing you from having one loop and factoring lines
Thanks for the guidance. I applied refactoring these code.


http://gerrit.cloudera.org:8080/#/c/8676/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@426
PS2, Line 426: ParsesOk
> pls add a TestUpsertHints method that is similar to TestInsertHints so that
You're right. I introduced TestUpsertHints.



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

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

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

[Kim Jin Chul (Code Review)]

Hello John Russell, Alex Behm, Vuk Ercegovac,

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

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

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

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

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

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

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


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

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

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 20:

Thanks for the information. I would like to exclude the third party library 
because the modification of the library seems not to be maintainable and 
delivery of the modification to the upstream might not be acceptable. When I 
checked the logs, there are trivial issues only:
- warning: 'PCG_USE_ZEROCHECK_ROTATE_IDIOM' is not defined, evaluates to 0
- warning: expansion of date or time macro is not reproducible

 > I think you may have missed this comment. There were two failed
 > jobs. You have addressed one of them - thank you! Can you take a
 > look at the other one?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 20
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Wed, 13 Dec 2017 03:13:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Michael Ho, Jim Apple, Attila Jeges, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
pcg32 of third party library shows better randomness than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M LICENSE.txt
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A be/src/thirdparty/pcg-cpp-0.98/LICENSE.txt
A be/src/thirdparty/pcg-cpp-0.98/README.md
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_random.hpp
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_uint128.hpp
M bin/rat_exclude_files.txt
M bin/run_clang_tidy.sh
M testdata/workloads/functional-query/queries/QueryTest/alloc-fail-init.test
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
13 files changed, 3,458 insertions(+), 32 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 20
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 2:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8676/1/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/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@376
PS1, Line 376:   "join %sshuffle%s functional.alltypes e 
using(string_col)",
> instead of repeating the queries, how about we vary the hint location. I'm
Done. Thanks for your kind example.


http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@392
PS1, Line 392: estInsertHint
> include jira issue for bug fixes, not new features.
Thanks for the information. I didn't know about that.


http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@392
PS1, Line 392: s(Str
> nit: remove the "Adopt".
Done


http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@409
PS1, Line 409: o
> we can remove redundancy here as well.
Done


http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@414
PS1, Line 414: inal String o
> same here
Done


http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@483
PS1, Line 483:
> and remove redundancy here as well.
Done


http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@499
PS1, Line 499:
> same here
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 12 Dec 2017 10:59:12 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5237: Support a quoted string in date/time format
..


Patch Set 6: Code-Review-1

(2 comments)

Currently I count not add the test case as you mentioned at the following 
change due to the issue at IMPALA-3942:
https://gerrit.cloudera.org/#/c/8508/6/be/src/exprs/expr-test.cc

We have two options. Which is better for you? Or any other option? Thanks.
1. Deliver a complete fix. Resolve IMPALA-3942 and then deliver this fix with a 
test case for mixing single and double quotes.
2. Deliver a first part of the fix which does not include the test case. 
Resolve IMPALA-3942 and then deliver a second part of the fix which includes 
the test case.

I prefer the second one because we can deliver fixes incrementally. There will 
be some time for discussion and providing a solution to resolve blocker.

http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.h
File be/src/runtime/timestamp-parse-util.h:

http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.h@190
PS6, Line 190:   //  nullptr if input does not have unclosed quote
> My suggestion would be:
Let me reflect it on the next patch set.


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

http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@178
PS6, Line 178:   return quoted_string_begin + 1;
> Yes I do.
Okay, I accept your approach.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 12 Dec 2017 03:31:29 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 9:

(3 comments)

Thanks for your advice.

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

http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@580
PS9, Line 580: lower()
I copied the code from the super(ImpalaShell, self).onecmd() because to 
override the lowering the command. I don't want early modification of the given 
command because it might make a bug like this.


http://gerrit.cloudera.org:8080/#/c/8762/9/shell/impala_shell.py@581
PS9, Line 581: self.orig_cmd = cmd_
In the previous patch set, the code was placed at precmd(), but this is not a 
proper place. For example, "source test.txt" and test.txt has "select 
version()".

I assumed there are two calls(i.e. source and select) for precmd(), but it 
happened only once for source. The reason is that,

do_source ->
self.execute_query_list -> self.onecmd(...) directly in loop without a call of 
precmd

onecmd can be invoked without precmd, so I would onecmd is a right place to 
keep the original command.


http://gerrit.cloudera.org:8080/#/c/8762/8/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/8762/8/tests/shell/test_shell_interactive.py@402
PS8, Line 402:
> Note that "select \"second command\";" is also unnecessary here.
Thanks for your advice. Let me add some test cases and the above case is 
changed to more meaningful test.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 9
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Mon, 11 Dec 2017 12:46:10 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

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

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

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

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

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

IMPALA-2640: Make a given command case-sensitive

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

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShell.test_var_substitution

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


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

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

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8762/7/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/8762/7/tests/shell/test_shell_commandline.py@585
PS7, Line 585:   def run_simple_command(self, command):
 : run_impala_shell_cmd('-q "%s;"' % command)
 :
 :   def test_commands(self):
 : """Testing commands which have not been tested"""
 : self.run_simple_command("help")
> Why is this test added with this change?
These methods are unnecessary. They were introduced due to the code change for 
do_help at the patch set #5. See the line 1168 in
https://gerrit.cloudera.org/#/c/8762/5..7/shell/impala_shell.py


http://gerrit.cloudera.org:8080/#/c/8762/7/tests/shell/test_shell_interactive.py
File tests/shell/test_shell_interactive.py:

http://gerrit.cloudera.org:8080/#/c/8762/7/tests/shell/test_shell_interactive.py@401
PS7, Line 401:   # IMPALA-5416: Test that a command following 'source' 
won't be run twice.
 :   result = run_impala_shell_interactive(
 : "source shell_case_sensitive.cmds;select \"second 
command\";")
> The IMPALA-5416 part is a leftover from copy-pasting test_source_file. "sel
Thanks for finding this. It's my mistake. Let me remove the invalid comment.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Mon, 11 Dec 2017 10:59:46 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

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

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

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

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

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

IMPALA-2640: Make a given command case-sensitive

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

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShell.test_var_substitution

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


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

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

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5237: Support a quoted string in date/time format
..


Patch Set 6:

(6 comments)

Thanks for your answers.

http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.h
File be/src/runtime/timestamp-parse-util.h:

http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.h@190
PS6, Line 190:   //  nullptr if input does not have unclosed quote
> Before you call this function the str parameter points to the opening quote
I see. Your point is that the pointer can be changeable in the function. What 
do you think about if I will add a comment like this? Please feel free to 
modify the comment or suggest one.

The pointer of the str can be changeable because of advancing the str parameter 
to the closing quote.


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

http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@161
PS6, Line 161: const char*& str, int& position_of_string_literal, int& 
length_of_string_literal) {
> Sure, thx for the explanation. I'm not sure we should prepare a function fo
Thanks for your understanding.


http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@174
PS6, Line 174:   DCHECK(length_of_string_literal >= 0);
> Still I feel that making a CHECK on the same thing twice is an overkill. We
Right, it should be a redundant. Let me remove this on the next patch set.


http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@176
PS6, Line 176:   DCHECK(position_of_string_literal >= 0);
Duplicate. Let me remove this on the next patch set.


http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@178
PS6, Line 178:   return quoted_string_begin + 1;
> In fact you have 3 different values returned from this function. 2 of them
Do you think the function name(i.e. Get...) is still valid even though return 
value will move to an output parameter? I mean logically the function returns 
three kinds of outputs, but I guess a reader may think the function name looks 
weird because it should be void function.


http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@198
PS6, Line 198:   if (UNLIKELY(!string_literal_begin)) return false;
> Could you explain me why we have DCHECKs for position_of_string_literal, an
As I mentioned at line#174 and line#176, they are not necessary. The checks at 
SaveDateTimeToken are enough. Let me remove them.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 11 Dec 2017 10:45:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 17:

Fixed. Build failure happened due to missing license information at 
rat_exclude_files.txt.
> 22:45:02 + bin/check-rat-report.py bin/rat_exclude_files.txt rat.xml
> 22:45:02 NO APPROVALS; notice: be/src/thirdparty/pcg-cpp-0.98/LICENSE.txt
> 22:45:02 UNAPPROVED: be/src/thirdparty/pcg-cpp-0.98/README.md


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 17
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Mon, 11 Dec 2017 09:31:05 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Michael Ho, Jim Apple, Attila Jeges, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
pcg32 of third party library shows better randomness than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M LICENSE.txt
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A be/src/thirdparty/pcg-cpp-0.98/LICENSE.txt
A be/src/thirdparty/pcg-cpp-0.98/README.md
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_random.hpp
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_uint128.hpp
M bin/rat_exclude_files.txt
M testdata/workloads/functional-query/queries/QueryTest/alloc-fail-init.test
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
12 files changed, 3,456 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/17
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 17
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-5993: Fix the file offset in value parsing error

[Kim Jin Chul (Code Review)]

Hello Lars Volker,

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

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

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

Change subject: IMPALA-5993: Fix the file offset in value parsing error
..

IMPALA-5993: Fix the file offset in value parsing error

This is to fix the file offset in value parsing error
messages when scanning text files. When the text scanner
hit an error, it always prints the end of the file as the
offset, even if the error occurs in the middle of the file.

This change also contains:
- Print errors column-wise instead of row-wise
- Adopt more user friendly message format

Testing:
Add two test cases:
- TestWrongFileOffset.test_parsing_wrong_text
- TestWrongFileOffset.test_parsing_wrong_gzip_text

Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
---
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 testdata/data/README
A testdata/data/problematic_rows_impala_5993.csv
A testdata/data/problematic_rows_impala_5993.csv.gz
M tests/query_test/test_scanners.py
8 files changed, 300 insertions(+), 53 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
Gerrit-Change-Number: 8747
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 6:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8762/6/shell/impala_shell.py@303
PS6, Line 303: beewax
> spelling issue: it's beeswax, not beewax
Done


http://gerrit.cloudera.org:8080/#/c/8762/6/shell/impala_shell.py@551
PS6, Line 551: This may be overridden, but should not normally need to be;
 : see the precmd() and postcmd() methods for useful execution 
hooks.
 : The return value is a flag indicating whether interpretation 
of
 : commands by the interpreter should stop.
> This comment shouldn't be here
Removed the method.


http://gerrit.cloudera.org:8080/#/c/8762/6/shell/impala_shell.py@572
PS6, Line 572: return func(arg, cmd_)
> To me it looks a bit weird to pass the command string to each command.
Done.



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

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

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

[Kim Jin Chul (Code Review)]

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

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

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

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

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

IMPALA-2640: Make a given command case-sensitive

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

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShell.test_var_substitution

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
---
M shell/impala_shell.py
A tests/shell/shell_case_sensitive.cmds
A tests/shell/shell_case_sensitive2.cmds
M tests/shell/test_shell_commandline.py
M tests/shell/test_shell_interactive.py
5 files changed, 72 insertions(+), 48 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-5993: Fix the file offset in value parsing error

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5993: Fix the file offset in value parsing error
..


Patch Set 4: Code-Review-1

Lars, as I mentioned in the JIRA, let me provide a new patch set.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
Gerrit-Change-Number: 8747
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Sun, 10 Dec 2017 12:07:04 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8639/4/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8639/4/shell/impala_shell.py@337
PS4, Line 337:   def sanitise_input(self, args):
> I feel like all this string manipulation is very hard to reason about, 
> leading to bugs like this.

I agree. The current string manipulation logic has the following problems.
1. May have hidden bugs
2. Code maintenance is not easy because it has some special handling such as 
making lower-case command(IMPALA-2640)
3. Not to readable

Currently I would like to fix the problem with a minimal change. You may feel 
there is inefficient code. We should need to clarify the logic on an another 
JIRA ticket.

> It seems like if we're calling sqlparse.parse() to construct Statement 
> instances, we should return those Statements, which will be easier to operate 
> on, instead of converting it back to strings.
Instead we're translating tokens back to strings, joining then, then 
resplitting them with sqlparse.split().

sqlparse.parse() can return multiple statements.
(e.g. select 'foo'; select 'boo';)
Actually, I would like to rely on sqlparse.parse because it is used widely and 
confirmed by many users than our own parser logic. By the way, I am not sure 
the parse can work for all the Impala syntax. As far as I guarantee, it can 
parse command appropriately. If we will adopt the parse function widely, I need 
to get an answer for the following curiosity. You know SQL syntax in DBMSes and 
sql-on-hadoop(e.g. Impala, Hive) are slightly different each other. The systems 
try to follow up SQL standard, but, in fact, there are own specialized syntax 
on each system.

Here is my assumption. I guess we need to customize sqlparse to consume Impala 
syntax and sqlparse can recognize Impala's version to decide a set of available 
syntax. sqlparse can take some arguments a system name and a version to apply 
custom syntax. I am not sure this feature is already in sqlparse, but this is 
necessary to push upstream.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
Gerrit-Change-Number: 8639
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 07 Dec 2017 02:11:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Michael Ho, Jim Apple, Attila Jeges, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
pcg32 of third party library shows better randomness than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M LICENSE.txt
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A be/src/thirdparty/pcg-cpp-0.98/LICENSE.txt
A be/src/thirdparty/pcg-cpp-0.98/README.md
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_random.hpp
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_uint128.hpp
M testdata/workloads/functional-query/queries/QueryTest/alloc-fail-init.test
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
11 files changed, 3,454 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/16
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 16
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..


Patch Set 15:

The third party library is officially released recently: version 0.98
(http://www.pcg-random.org/download.html)

I didn't include some unnecessary files such as sample and tests. Please look 
around PCG repo: https://github.com/imneme/pcg-cpp


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Thu, 07 Dec 2017 00:36:42 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5754: Improve randomness of rand()/random()

[Kim Jin Chul (Code Review)]

Hello Michael Ho, Jim Apple, Attila Jeges, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-5754: Improve randomness of rand()/random()
..

IMPALA-5754: Improve randomness of rand()/random()

Currently implementation of rand/random built-in functions
use rand_r of C library. We recognized its randomness was poor.
pcg32 of third party library shows better randomness than rand_r.

Testing:
Revise unit test in expr-test
Add E2E test to random.test

Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
---
M LICENSE.txt
M be/src/exprs/expr-test.cc
M be/src/exprs/math-functions-ir.cc
A be/src/thirdparty/pcg-cpp-0.98/LICENSE.txt
A be/src/thirdparty/pcg-cpp-0.98/README.md
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_extras.hpp
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_random.hpp
A be/src/thirdparty/pcg-cpp-0.98/include/pcg_uint128.hpp
M testdata/workloads/functional-query/queries/QueryTest/alloc-fail-init.test
A testdata/workloads/functional-query/queries/QueryTest/random.test
M tests/query_test/test_queries.py
11 files changed, 3,454 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8355/15
--
To view, visit http://gerrit.cloudera.org:8080/8355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idafdd5fe7502ff242c76a91a815c565146108684
Gerrit-Change-Number: 8355
Gerrit-PatchSet: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

Hello Gabor Kaszab, Attila Jeges, Tim Armstrong,

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

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

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

Change subject: IMPALA-5237: Support a quoted string in date/time format
..

IMPALA-5237: Support a quoted string in date/time format

Impala does not represent a quoted string at date/time format.
For example, addtional quoted string between the patterns
(e.g. HH\'H\' => 11H). Hive supports this feature, so user wants
to get a same result from Impala. By the way, Impala returns
a different result as below.

* Hive
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08H 39M 24S

* Impala
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08'8' 39'4' 24'0'

The change affects the format pattern for
from_unixtime/from_timestamp/unix_timestamp.

In unix_timestamp, user can also specify a quoted string like this.

> select unix_timestamp('\'Epoch time: \'19700101',
> '\'Epoch time: \'MMdd');
0

By the way, the quoted strings between input and format should be
exactly same and internally string comparison is case-sensitive.

There is one intentional difference between Hive and Impala.
In Impala, the format string should have any date or time patten
as below. Throwing the error/warning is better if Impala cannot
optimize the expression. User must rewrite the query and don't pay
the function call.

* Hive
> select from_unixtime(0, '\'Hello world!\'');
Hello world!

* Impala
> select from_unixtime(0, '\'Hello world!\'');
Bad date/time conversion format: 'Hello world!'

Testing:
Add unit tests for working and nonworking cases

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6262: Always initialize runtime profile for DataSink

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-6262: Always initialize runtime profile for DataSink
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8770/2/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

http://gerrit.cloudera.org:8080/#/c/8770/2/be/src/exec/data-sink.cc@49
PS2, Line 49: state->
I guess you may assume state cannot be nullptr. If so, why don't you pass state 
using reference type instead of pointer? I could see several argument passing 
in this change. Reference type is more intuitive to guarantee the variable 
points an object.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2a683000ef180027b929dbebe78bc2a530a4767e
Gerrit-Change-Number: 8770
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 06 Dec 2017 14:30:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5237: Support a quoted string in date/time format
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h
File be/src/runtime/timestamp-parse-util.h:

http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h@183
PS4, Line 183:   /// dt_ctx -- date/time format context
> Actually I don't want leave meaningless comments. I think the parameter nam
I meant "the parameter names" are in new code in the recent patch set. Please 
feel free to tell me if you have a better name or more informative comment.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 06 Dec 2017 11:44:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5237: Support a quoted string in date/time format
..


Patch Set 3:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc@5746
PS3, Line 5746:   TestValue("unix_timestamp('\\'Epoch time: \\'1970|01|01 
00|00|00',\
> Thanks for the explanation!
I guess your question is that the result seems to be wrong even though the 
format is find and everything is okey to run.  Timezone can affect epoch 
timestamp. Impala interprets date and time with UTC timezone by default due to 
avoidance of undesired results by local timezone.
- Regarding default timezone: 
https://github.com/apache/impala/blob/master/docs/topics/impala_timestamp.xml#L89
- Regarding data and time interpretation at unix_timestamp: 
https://github.com/apache/impala/blob/master/docs/topics/impala_datetime_functions.xml#L2463
- Test case for epoch: 
https://github.com/apache/impala/blob/master/be/src/exprs/expr-test.cc#L5617

Therefore, I think the result should be zero.


http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h
File be/src/runtime/timestamp-parse-util.h:

http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h@182
PS4, Line 182: Reset(fmt, fmt_len);
> 1: This comment doesn't add  any further info as it is basically the functi
Thanks for your kind comment.
1,2: I agree. Actually I thought I should have to add the meaningless comment, 
but I didn't imagine more meaningful comment and I think the function name 
looks self-descriptive.
3. More detailed explanation is added to return description instead of the word 
"parse".
4. I agree. Reader expects a string literal due to "Get", so the revised 
function will return a pointer of string literal. The function is split into 
two parts: GetStringLiteralBetweenQuotes and SaveDateTimeToken. I think 
SaveDateTimeToken will be used generally when refactoring ParseFormatTokens or 
adding a new code.

I fully agree an unit function should have a minimal purpose and responsibility.


http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h@183
PS4, Line 183:   }
> Again, this is simply writing the type of the parameter with spaces.
Actually I don't want leave meaningless comments. I think the parameter names 
are self-explainable, but please leave a comment as a new reader if you think 
any additional information should be helpful.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 06 Dec 2017 11:41:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

Hello Gabor Kaszab, Attila Jeges, Tim Armstrong,

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

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

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

Change subject: IMPALA-5237: Support a quoted string in date/time format
..

IMPALA-5237: Support a quoted string in date/time format

Impala does not represent a quoted string at date/time format.
For example, addtional quoted string between the patterns
(e.g. HH\'H\' => 11H). Hive supports this feature, so user wants
to get a same result from Impala. By the way, Impala returns
a different result as below.

* Hive
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08H 39M 24S

* Impala
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08'8' 39'4' 24'0'

The change affects the format pattern for
from_unixtime/from_timestamp/unix_timestamp.

In unix_timestamp, user can also specify a quoted string like this.

> select unix_timestamp('\'Epoch time: \'19700101',
> '\'Epoch time: \'MMdd');
0

By the way, the quoted strings between input and format should be
exactly same and internally string comparison is case-sensitive.

There is one intentional difference between Hive and Impala.
In Impala, the format string should have any date or time patten
as below. Throwing the error/warning is better if Impala cannot
optimize the expression. User must rewrite the query and don't pay
the function call.

* Hive
> select from_unixtime(0, '\'Hello world!\'');
Hello world!

* Impala
> select from_unixtime(0, '\'Hello world!\'');
Bad date/time conversion format: 'Hello world!'

Testing:
Add unit tests for working and nonworking cases

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2640: Maintain the command given by an user

[Kim Jin Chul (Code Review)]

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

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

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

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

Change subject: IMPALA-2640: Maintain the command given by an user
..

IMPALA-2640: Maintain the command given by an user

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

Testing:
TestImpalaShell.test_var_substitution already has a testcase

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
---
M shell/impala_shell.py
1 file changed, 84 insertions(+), 71 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-2640: Maintain the command given by an user

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-2640: Maintain the command given by an user
..


Patch Set 2: Code-Review-1

I found that "ctrl + d" (EOF) does not work on my change. Let me take a look at 
this and then provide a new patch set.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Wed, 06 Dec 2017 08:28:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5237: Support a quoted string in date/time format
..


Patch Set 3:

(6 comments)

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

http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc@5746
PS3, Line 5746:   TestValue("unix_timestamp('\\'Epoch time: \\'1970|01|01 
00|00|00',\
> I checked and Hive in fact returns NULL for this input.
I guess you may expect "Epoch time: 1970|01|01 00|00|00" as a result, 
unix_timestamp is not suitable because it returns bigint. I think two kinds of 
results are only available: zero or NULL

As you mentioned, Hive returns NULL because the quoted string might be 
recognized as an unexpected format string.

The string literal in the quotes is meaningless in unix_timestamp function, but 
I think an user wants to add some description and it should not affect 
returning a result. The flexibility might be helpful to an user even though a 
difference happens between Impala and Hive. Therefore, the result should be 
zero in Impala.


http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc@5756
PS3, Line 5756:   // 2) Not allowd unclosed quoted string
> nit: typo: allowed
Done


http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc@5764
PS3, Line 5764:   TestValue("unix_timestamp('\\'Epoch time: \\'1970|01|01 
00|00|00',\
> There is one more thing I don't understand: Regardless of giving this forma
As I explained above, I think returning NULL for the mismatched case is enough. 
I would like to prefer NULL instead of throwing an error.

There are two issues.
1) The expected result is NULL because the input does not fit to the 
format(e.g. Epoch and Enoch). I think returning NULL is enough for happening a 
mismatch internally.

2) I should have to use TestIsNull instead of TestValue. It's my mistake when I 
copied from the above. Let me fix this. By the way, there is a different 
problem in our test framework. I guess the following test should fail: 
TestValue(cast(null as int), TYPE_INT, 0)
As you know, NULL cannot be compared with any value because it is unknown, so 
it should not be accept. I filed the issue on IMPALA-6283.


http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/runtime/timestamp-parse-util.h
File be/src/runtime/timestamp-parse-util.h:

http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/runtime/timestamp-parse-util.h@77
PS3, Line 77: /// IMPALA-5237: Support a quoted string in date/time format
> I have the feeling that copying the whole commit message here is a bit of a
Done


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

http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/runtime/timestamp-parse-util.cc@163
PS3, Line 163: if (*str == '\'') {
> I would find this part of code a bit more self-descriptive if it was in the
Done


http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/runtime/timestamp-parse-util.cc@173
PS3, Line 173:   if (len == 0) len = 1;
> The naming of len, val and pos aren't that self-explanatory for me. Would i
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178
Gerrit-Change-Number: 8508
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 06 Dec 2017 07:56:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5237: Support a quoted string in date/time format

[Kim Jin Chul (Code Review)]

Hello Gabor Kaszab, Attila Jeges, Tim Armstrong,

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

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

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

Change subject: IMPALA-5237: Support a quoted string in date/time format
..

IMPALA-5237: Support a quoted string in date/time format

Impala does not represent a quoted string at date/time format.
For example, addtional quoted string between the patterns
(e.g. HH\'H\' => 11H). Hive supports this feature, so user wants
to get a same result from Impala. By the way, Impala returns
a different result as below.

* Hive
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08H 39M 24S

* Impala
> select from_unixtime(1492677564, 'HH\'H\' mm\'M\' ss\'S\'');
08'8' 39'4' 24'0'

The change affects the format pattern for
from_unixtime/from_timestamp/unix_timestamp.

In unix_timestamp, user can also specify a quoted string like this.

> select unix_timestamp('\'Epoch time: \'19700101',
> '\'Epoch time: \'MMdd');
0

By the way, the quoted strings between input and format should be
exactly same and internally string comparison is case-sensitive.

There is one intentional difference between Hive and Impala.
In Impala, the format string should have any date or time patten
as below. Throwing the error/warning is better if Impala cannot
optimize the expression. User must rewrite the query and don't pay
the function call.

* Hive
> select from_unixtime(0, '\'Hello world!\'');
Hello world!

* Impala
> select from_unixtime(0, '\'Hello world!\'');
Bad date/time conversion format: 'Hello world!'

Testing:
Add unit tests for working and nonworking cases

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


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

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

[Impala-ASF-CR] IMPALA-2640: Maintain the command given by an user

[Kim Jin Chul (Code Review)]

Hello John Russell, Andre Araujo,

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

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

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

Change subject: IMPALA-2640: Maintain the command given by an user
..

IMPALA-2640: Maintain the command given by an user

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

Testing:
TestImpalaShell.test_var_substitution already has a testcase

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
---
M shell/impala_shell.py
1 file changed, 83 insertions(+), 70 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-2640: Maintain the command given by an user

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-2640: Maintain the command given by an user
..


Patch Set 1:

Confirmed green on local for the test: /tests/run-tests.py shell


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 05 Dec 2017 14:22:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5993: Fix the file offset in value parsing error

[Kim Jin Chul (Code Review)]

Hello Lars Volker,

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

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

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

Change subject: IMPALA-5993: Fix the file offset in value parsing error
..

IMPALA-5993: Fix the file offset in value parsing error

This is to fix the file offset in value parsing error
messages when scanning text files. When the text scanner
hit an error, it always prints the end of the file as the
offset, even if the error occurs in the middle of the file.

Testing:

Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
---
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 testdata/data/README
5 files changed, 186 insertions(+), 52 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
Gerrit-Change-Number: 8747
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-5993: Fix the file offset in value parsing error

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5993: Fix the file offset in value parsing error
..


Patch Set 2: Code-Review-1

(6 comments)

Preparing a testcase is still ongoing locally.

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

http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.h@390
PS1, Line 390:   ///  - error_in_row is an out bool.  It is set to true if any 
field had parse errors
> Can you add comments for the new parameters here?
Done


http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.h@405
PS1, Line 405:   /// TODO: revisit this
> nit: the line wrapping looks odd, can you wrap at 90 chars?
Yes, the line 406 cannot be appended to the line 405 due to the limitation. Let 
me change it like below:

uint8_t* error_in_row, int64_t* error_offset, int64_t* curr_offset)
WARN_UNUSED_RESULT;


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

http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.cc@251
PS1, Line 251: error_offsets[i] = *curr_offset;
> isn't fields[i].start the same as curr_offset?
fields[i].start might be used to calculate offset, but fields[i].start and 
curr_offset are not completely same. I think we don't have a start pointer of a 
file. We may need a ptr variable to keep the start of a file.
Do you think the following is better than this?

int64_t* start_of_file) {
...
error_offsets[i] = fields[i].start - start_of_file;

This is can be applied on this file, but it cannot be applied to sequence 
scanner impl. because sequence scanner reads a record(see line 313) and the 
calculation of pointer seems to be invalid:
https://gerrit.cloudera.org/#/c/8747/1/be/src/exec/hdfs-sequence-scanner.cc


http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.cc@668
PS1, Line 668: seError() {
> I am confused because this function still exists unchanged. If your code do
My change uses LogColumnParseError instead of LogRowParseError, but 
LogRowParseError is still necessary. I think it cannot be replaced by 
LogColumnParseError. Please see
https://github.com/cloudera/Impala/blob/cdh5-trunk/be/src/exec/hdfs-text-scanner.cc#L806


http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.cc@674
PS1, Line 674: Error(const int line,
> I agree that it is much easier to understand. One nit: we should print out
Done. Yes, I agree "file offset" is more clear.


http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.cc@683
PS1, Line 683: char* data, int len) {
 :   if (state_->LogHasSpace() || state_->abort_on_error()) {
 : stringstream ss;
> I don't think this change adds a big improvement, but I'm also OK with keep
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
Gerrit-Change-Number: 8747
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Tue, 05 Dec 2017 00:18:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5993: Fix the file offset in value parsing error

[Kim Jin Chul (Code Review)]

Hello Lars Volker,

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

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

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

Change subject: IMPALA-5993: Fix the file offset in value parsing error
..

IMPALA-5993: Fix the file offset in value parsing error

This is to fix the file offset in value parsing error
messages when scanning text files. When the text scanner
hit an error, it always prints the end of the file as the
offset, even if the error occurs in the middle of the file.

Testing:

Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
---
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 testdata/data/README
5 files changed, 62 insertions(+), 12 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
Gerrit-Change-Number: 8747
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-5993: Fix the file offset in value parsing error

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5993: Fix the file offset in value parsing error
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.cc@668
PS1, Line 668: stream_->file_offset()
> I think there are two approaches to fix:
Sorry there was my mistake in the last sentence.I meant that please let me know 
some reasons if you would prefer the first one than the second one. The fix for 
the first one will read stream line by line.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
Gerrit-Change-Number: 8747
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Mon, 04 Dec 2017 21:20:12 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5993: Fix the file offset in value parsing error

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5993: Fix the file offset in value parsing error
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.cc@668
PS1, Line 668: stream_->file_offset()
> Yes, that was the main reason for opening the Jira. Are you going to fix th
I think there are two approaches to fix:
1. Still rely on the function by reading stream incrementally
2. Track offset instead of using the function
My proposal adopts the second one because I thought ahead of the full scanning 
was intended such as better performace. I implemented the second one on this 
patch set. If you think there is no reason reading the stream incremenally, let 
me try to fix the problem with the first one. Is there your preference? Please 
let me know some reasons if you prefer the second one than the first one.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
Gerrit-Change-Number: 8747
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Mon, 04 Dec 2017 21:12:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5993: Fix the file offset in value parsing error

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5993: Fix the file offset in value parsing error
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.cc@264
PS1, Line 264: // define i1 @WriteCompleteTuple(%"class.impala::HdfsScanner"* 
%this,
 : //   %"class.impala::MemPool"* %pool,
 : //   
%"struct.impala::FieldLocation"* %fields,
 : //   %"class.impala::Tuple"* %tuple,
 : //   %"class.impala::TupleRow"* 
%tuple_row,
 : //   %"class.impala::Tuple"* 
%template,
 : //   i8* %error_fields, i8* 
%error_in_row) {
 : // entry:
 : //   %tuple_ptr = bitcast %"class.impala::Tuple"* %tuple
 : //to <{ %"struct.impala::StringValue", i8 }>*
 : //   %tuple_ptr1 = bitcast %"class.impala::Tuple"* %template
 : // to <{ %"struct.impala::StringValue", i8 }>*
 : //   %int8_ptr = bitcast <{ %"struct.impala::StringValue", i8 
}>* %tuple_ptr to i8*
 : //   %null_bytes_ptr = getelementptr i8, i8* %int8_ptr, i32 16
 : //   call void @llvm.memset.p0i8.i64(i8* %null_bytes_ptr, i8 0, 
i64 1, i32 0, i1 false)
 : //   %0 = bitcast %"class.impala::TupleRow"* %tuple_row
 : //to <{ %"struct.impala::StringValue", i8 }>**
 : //   %1 = getelementptr <{ %"struct.impala::StringValue", i8 }>*,
 : //  <{ %"struct.impala::StringValue", i8 
}>** %0, i32 0
 : //   store <{ %"struct.impala::StringValue", i8 }>* %tuple_ptr,
 : // <{ %"struct.impala::StringValue", i8 }>** %1
 : //   br label %parse
 : //
 : // parse:; preds = 
%entry
 : //  %data_ptr = getelementptr %"struct.impala::FieldLocation",
 : //%"struct.impala::FieldLocation"* 
%fields, i32 0, i32 0
 : //  %len_ptr = getelementptr %"struct.impala::FieldLocation",
 : //   %"struct.impala::FieldLocation"* 
%fields, i32 0, i32 1
 : //  %slot_error_ptr = getelementptr i8, i8* %error_fields, i32 0
 : //  %data = load i8*, i8** %data_ptr
 : //  %len = load i32, i32* %len_ptr
 : //  %2 = call i1 @WriteSlot(<{ %"struct.impala::StringValue", i8 
}>* %tuple_ptr,
 : //  i8* %data, i32 %len)
 : //  %slot_parse_error = xor i1 %2, true
 : //  %error_in_row2 = or i1 false, %slot_parse_error
 : //  %3 = zext i1 %slot_parse_error to i8
 : //  store i8 %3, i8* %slot_error_ptr
 : //  %4 = call %"class.impala::ScalarExprEvaluator"* 
@GetConjunctCtx(
 : //%"class.impala::HdfsScanner"* %this, i32 0)
 : //  %conjunct_eval = call i16 
@"impala::Operators::Eq_StringVal_StringValWrapper"(
 : //%"class.impala::ScalarExprEvaluator"* %4, 
%"class.impala::TupleRow"* %tuple_row)
 : //  %5 = ashr i16 %conjunct_eval, 8
 : //  %6 = trunc i16 %5 to i8
 : //  %val = trunc i8 %6 to i1
 : //  br i1 %val, label %parse3, label %eval_fail
 : //
 : // parse3:   ; preds = 
%parse
 : //   %7 = zext i1 %error_in_row2 to i8
 : //   store i8 %7, i8* %error_in_row
 : //   ret i1 true
 : //
 : // eval_fail:; preds = 
%parse
 : //   ret i1 false
 : // }
Should I maintain this? When I dump IR, this comment has not been updated for a 
while. I would like to remove the old IR completely because it is meaningless 
if we spend our precious time.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
Gerrit-Change-Number: 8747
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Mon, 04 Dec 2017 15:16:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5993: Fix the file offset in value parsing error

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5993: Fix the file offset in value parsing error
..


Patch Set 1: Code-Review-1

(4 comments)

As I commented, this change does not include a test which is ongoing locally. 
By the way, I would like to get your early review for the kernel code change. 
Please check my comments. Thanks.

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

http://gerrit.cloudera.org:8080/#/c/8747/1//COMMIT_MSG@14
PS1, Line 14: Testing:
A custom test will be introduced at tests/custom_cluster/test_impala_5993.py. 
The test contains some hdfs commands which come from tests/util/hdfs_util.py 
and table will be created by 'create table stored as textfile location' query.


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

http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.cc@668
PS1, Line 668: stream_->file_offset()
The offset can point to EOF because the data is read ahead of time via 
ScannerContext :: Stream :: GetBuffer in 
impala::HdfsTextScanner::FillByteBuffer. Therefore, file_offset cannot be used 
to print the problematic offset.


http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.cc@674
PS1, Line 674: Error parsing row: file: $0, (line: $1, pos: $2, offset: $3)
I think the refined message format is more user friendly. Let me provide 
another change if you think this improvement cannot be in a bug fix.


http://gerrit.cloudera.org:8080/#/c/8747/1/be/src/exec/hdfs-scanner.cc@683
PS1, Line 683: "Error converting column at the index: "
 :<< desc->col_pos() - scan_node_->num_partition_keys()
 :<< " (type: " << desc->type() << ")";
I think the refined message format is more user friendly. Let me provide 
another change if you think this improvement cannot be in a bug fix.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
Gerrit-Change-Number: 8747
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Mon, 04 Dec 2017 15:09:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5993: Fix the file offset in value parsing error

[Kim Jin Chul (Code Review)]

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


Change subject: IMPALA-5993: Fix the file offset in value parsing error
..

IMPALA-5993: Fix the file offset in value parsing error

This is to fix the file offset in value parsing error
messages when scanning text files. When the text scanner
hit an error, it always prints the end of the file as the
offset, even if the error occurs in the middle of the file.

Testing:

Change-Id: I4a4ac3199ffe12fcce0bf792b3e6ce529b9af61f
---
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
4 files changed, 49 insertions(+), 12 deletions(-)



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

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

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 1:

Thanks for the information. Let me deliver documentation on a separate change.


--
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: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 04 Dec 2017 07:53:04 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 1:

@John, as far as I know you're in charge of the documentation. Would you please 
answer my question?


--
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: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 04 Dec 2017 07:45:52 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 1:

May I add the new syntax to docs?


--
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: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 04 Dec 2017 01:59:57 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

Kim Jin Chul has uploaded this change for review. ( 
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/exec/hdfs-scanner.cc
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
6 files changed, 22 insertions(+), 7 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/10/8710/1
--
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: newchange
Gerrit-Change-Id: I986e1bae82d5ca7a82fc1bab2e412c4733928dce
Gerrit-Change-Number: 8710
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
..


Patch Set 7:

@Jim and Philip, I appreciate your review.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Fri, 01 Dec 2017 00:28:54 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

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


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

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

testing:
Add unit tests to ParserTest#TestPlanHints

Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
---
M fe/src/main/cup/sql-parser.cup
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
2 files changed, 68 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/8676/1
--
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: newchange
Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20
Gerrit-Change-Number: 8676
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 

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

[Kim Jin Chul (Code Review)]

Hello Tim Armstrong, Zach Amsden,

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

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

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

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

IMPALA-4664: Unexpected string conversion in Shell

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

sqlparse libraries is upgraded from 0.1.14 to
0.2.4 due to bug fixes and syntax extension.
sqlparse 0.1.14 cannot parse assignment properly.
(e.g. set variable=value)

Testing:
Add tests to tests/shell/test_shell_interactive.py

Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
---
M LICENSE.txt
M README.md
M bin/rat_exclude_files.txt
M bin/set-pythonpath.sh
M shell/.gitignore
R shell/ext-py/egg/prettytable-0.7.1/CHANGELOG
R shell/ext-py/egg/prettytable-0.7.1/COPYING
R shell/ext-py/egg/prettytable-0.7.1/MANIFEST.in
R shell/ext-py/egg/prettytable-0.7.1/PKG-INFO
R shell/ext-py/egg/prettytable-0.7.1/README
R shell/ext-py/egg/prettytable-0.7.1/prettytable.py
R shell/ext-py/egg/prettytable-0.7.1/setup.cfg
R shell/ext-py/egg/prettytable-0.7.1/setup.py
R shell/ext-py/egg/sasl-0.1.1/MANIFEST.in
R shell/ext-py/egg/sasl-0.1.1/PKG-INFO
R shell/ext-py/egg/sasl-0.1.1/sasl/__init__.py
R shell/ext-py/egg/sasl-0.1.1/sasl/saslwrapper.cpp
R shell/ext-py/egg/sasl-0.1.1/sasl/saslwrapper.h
R shell/ext-py/egg/sasl-0.1.1/sasl/saslwrapper.py
R shell/ext-py/egg/sasl-0.1.1/sasl/saslwrapper_wrap.cxx
R shell/ext-py/egg/sasl-0.1.1/setup.cfg
R shell/ext-py/egg/sasl-0.1.1/setup.py
D shell/ext-py/sqlparse-0.1.14/AUTHORS
D shell/ext-py/sqlparse-0.1.14/CHANGES
D shell/ext-py/sqlparse-0.1.14/bin/sqlformat
D shell/ext-py/sqlparse-0.1.14/docs/source/changes.rst
D shell/ext-py/sqlparse-0.1.14/pytest.ini
D shell/ext-py/sqlparse-0.1.14/setup.cfg
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/__init__.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/filter.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/grouping.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/exceptions.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/filters.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/formatter.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/functions.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/lexer.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/pipeline.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/sql.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/tokens.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/utils.py
D shell/ext-py/sqlparse-0.1.14/tests/test_filters.py
D shell/ext-py/sqlparse-0.1.14/tests/test_format.py
D shell/ext-py/sqlparse-0.1.14/tests/test_functions.py
D shell/ext-py/sqlparse-0.1.14/tests/test_grouping.py
D shell/ext-py/sqlparse-0.1.14/tests/test_parse.py
D shell/ext-py/sqlparse-0.1.14/tests/test_pipeline.py
D shell/ext-py/sqlparse-0.1.14/tests/test_regressions.py
D shell/ext-py/sqlparse-0.1.14/tests/test_split.py
D shell/ext-py/sqlparse-0.1.14/tests/test_tokenize.py
D shell/ext-py/sqlparse-0.1.14/tests/utils.py
D shell/ext-py/sqlparse-0.1.14/tox.ini
A shell/ext-py/whl/sqlparse-0.2.4/AUTHORS
A shell/ext-py/whl/sqlparse-0.2.4/CHANGELOG
R shell/ext-py/whl/sqlparse-0.2.4/LICENSE
R shell/ext-py/whl/sqlparse-0.2.4/MANIFEST.in
R shell/ext-py/whl/sqlparse-0.2.4/PKG-INFO
R shell/ext-py/whl/sqlparse-0.2.4/README.rst
R shell/ext-py/whl/sqlparse-0.2.4/TODO
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/analyzing.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/api.rst
A shell/ext-py/whl/sqlparse-0.2.4/docs/source/changes.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/conf.py
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/index.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/indices.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/intro.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/source/ui.rst
R shell/ext-py/whl/sqlparse-0.2.4/docs/sqlformat.1
A shell/ext-py/whl/sqlparse-0.2.4/setup.cfg
R shell/ext-py/whl/sqlparse-0.2.4/setup.py
R shell/ext-py/whl/sqlparse-0.2.4/sqlparse/__init__.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/__main__.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/cli.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/compat.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/engine/__init__.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/engine/filter_stack.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/engine/grouping.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/engine/statement_splitter.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/exceptions.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/__init__.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/aligned_indent.py
A shell/ext-py/whl/sqlparse-0.2.4/sqlparse/filters/others.py
A 

[Impala-ASF-CR] IMPALA-5146: Fix inconsitent results at FROM UNIXTIME()

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5146: Fix inconsitent results at FROM_UNIXTIME()
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8629/4/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

http://gerrit.cloudera.org:8080/#/c/8629/4/testdata/workloads/functional-query/queries/QueryTest/exprs.test@2518
PS4, Line 2518: 'NULL'
> I think it's an artifact of a problem with the test infrastructure and stri
Filed the issue: https://issues.apache.org/jira/browse/IMPALA-6260



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3a5e9a9cb39d32993fa2c7f725be44d8b9ce9f2
Gerrit-Change-Number: 8629
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 29 Nov 2017 02:38:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8613/4/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/4/shell/impala_shell.py@1440
PS4, Line 1440: if call(['klist', '-s']) != 0:
> This looks wrong to me.
You're right. Thanks for finding the potential issue.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 29 Nov 2017 01:54:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

[Kim Jin Chul (Code Review)]

Hello Jim Apple, Philip Zeyliger,

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

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

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

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
..

IMPALA-4506: Do not display some intro message if --quiet is set

Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
---
M shell/impala_shell.py
1 file changed, 28 insertions(+), 14 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-5146: Fix inconsitent results at FROM UNIXTIME()

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5146: Fix inconsitent results at FROM_UNIXTIME()
..


Patch Set 4:

(2 comments)

Thanks Dan for the notification.

http://gerrit.cloudera.org:8080/#/c/8629/4/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:

http://gerrit.cloudera.org:8080/#/c/8629/4/testdata/workloads/functional-query/queries/QueryTest/exprs.test@2518
PS4, Line 2518: 'NULL'
I am wondering why the test expects 'NULL' string instead of NULL. As you know, 
'NULL' is totally different to NULL. I think it's misleading. Is there any 
historical reason? I found several cases when return type is string.


http://gerrit.cloudera.org:8080/#/c/8629/4/testdata/workloads/functional-query/queries/QueryTest/exprs.test@2553
PS4, Line 2553: 'NULL'
  :  TYPES
  : STRING
Expects 'NULL' literal string



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3a5e9a9cb39d32993fa2c7f725be44d8b9ce9f2
Gerrit-Change-Number: 8629
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 29 Nov 2017 01:42:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

[Kim Jin Chul (Code Review)]

Hello Jim Apple, Philip Zeyliger,

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

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

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

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
..

IMPALA-4506: Do not display some intro message if --quiet is set

Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
---
M shell/impala_shell.py
1 file changed, 35 insertions(+), 22 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1353
PS3, Line 1353: def get_intro(options):
> Please add a docstring to new functions.
Done


http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1359
PS3, Line 1359: "not secured by TLS.\nALL PASSWORDS WILL BE 
SENT IN THE CLEAR TO IMPALA.\n")
> nit: long line
Done


http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1362
PS3, Line 1362:   intro += REFRESH_AFTER_CONNECT_DEPRECATION_WARNING
> nit, not your bug (already present in code): Can you make the last characte
Done, last character should not be a return.


http://gerrit.cloudera.org:8080/#/c/8613/3/shell/impala_shell.py@1364
PS3, Line 1364: else:
  : return ""
> You could remove a level of indentation for the bulk of the function by mak
Done. It's better readability.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Mon, 27 Nov 2017 15:15:10 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3784: Fixed broken status-benchmark

[Kim Jin Chul (Code Review)]

Hello Jim Apple,

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

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

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

Change subject: IMPALA-3784: Fixed broken status-benchmark
..

IMPALA-3784: Fixed broken status-benchmark

A default constructed boost::optional is empty - it does
not contain a value, so get() cannot be called. The value
check should be required before using it.

Change-Id: I2fcdbdb4c5bc432a4d224dda821de132e3bca611
---
M be/src/benchmarks/status-benchmark.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2fcdbdb4c5bc432a4d224dda821de132e3bca611
Gerrit-Change-Number: 8641
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR] IMPALA-5146: Fix inconsitent results at FROM UNIXTIME()

[Kim Jin Chul (Code Review)]

Hello Jim Apple, Dan Hecht,

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

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

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

Change subject: IMPALA-5146: Fix inconsitent results at FROM_UNIXTIME()
..

IMPALA-5146: Fix inconsitent results at FROM_UNIXTIME()

The FROM_UNIXTIME(epoch) and FROM_UNIXTIME(epoch, format) produce
different results when epoch is out of range of TimestampValue.
The former produces an empty string, while the latter gives NULL.

The fix is to harmonize the results to NULL.

Testing:
Add unit tests to ExprTest.TimestampFunctions.

Change-Id: Ie3a5e9a9cb39d32993fa2c7f725be44d8b9ce9f2
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
2 files changed, 5 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie3a5e9a9cb39d32993fa2c7f725be44d8b9ce9f2
Gerrit-Change-Number: 8629
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
..


Patch Set 3:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8613/2//COMMIT_MSG@9
PS2, Line 9: Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
> Please elide these messages about specific patch sets by folding the most r
Done


http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py@1481
PS1, Line 1481:   # Override query_options from config file with those 
specified on the command line.
> It seems like intro can still be no-empty at lines 1484 and 1488 in PS2.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Mon, 27 Nov 2017 08:39:24 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

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


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

IMPALA-4664: Unexpected string conversion in Shell

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

Using sqlparse library in infra instead of
shell/ext-py/sqlparse-0.1.14. The sqlparse
library in infra is upgraded from 0.1.15 to
0.2.4 due to bug fixes and syntax extension.

Testing:
Add tests to tests/shell/test_shell_interactive.py

Change-Id: Iba6fa4e9ac570d22135ba51b844db8f9be965ca3
---
M LICENSE.txt
M README.md
M bin/rat_exclude_files.txt
M infra/python/deps/requirements.txt
M shell/.gitignore
D shell/ext-py/sqlparse-0.1.14/AUTHORS
D shell/ext-py/sqlparse-0.1.14/CHANGES
D shell/ext-py/sqlparse-0.1.14/COPYING
D shell/ext-py/sqlparse-0.1.14/MANIFEST.in
D shell/ext-py/sqlparse-0.1.14/PKG-INFO
D shell/ext-py/sqlparse-0.1.14/README.rst
D shell/ext-py/sqlparse-0.1.14/TODO
D shell/ext-py/sqlparse-0.1.14/bin/sqlformat
D shell/ext-py/sqlparse-0.1.14/docs/source/analyzing.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/api.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/changes.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/conf.py
D shell/ext-py/sqlparse-0.1.14/docs/source/index.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/indices.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/intro.rst
D shell/ext-py/sqlparse-0.1.14/docs/source/ui.rst
D shell/ext-py/sqlparse-0.1.14/docs/sqlformat.1
D shell/ext-py/sqlparse-0.1.14/pytest.ini
D shell/ext-py/sqlparse-0.1.14/setup.cfg
D shell/ext-py/sqlparse-0.1.14/setup.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/__init__.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/__init__.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/filter.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/engine/grouping.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/exceptions.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/filters.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/formatter.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/functions.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/keywords.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/lexer.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/pipeline.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/sql.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/tokens.py
D shell/ext-py/sqlparse-0.1.14/sqlparse/utils.py
D shell/ext-py/sqlparse-0.1.14/tests/__init__.py
D shell/ext-py/sqlparse-0.1.14/tests/files/_Make_DirEntry.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/begintag.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/begintag_2.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/dashcomment.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/function.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/function_psql.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/function_psql2.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/function_psql3.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/huge_select.sql
D shell/ext-py/sqlparse-0.1.14/tests/files/test_cp1251.sql
D shell/ext-py/sqlparse-0.1.14/tests/test_filters.py
D shell/ext-py/sqlparse-0.1.14/tests/test_format.py
D shell/ext-py/sqlparse-0.1.14/tests/test_functions.py
D shell/ext-py/sqlparse-0.1.14/tests/test_grouping.py
D shell/ext-py/sqlparse-0.1.14/tests/test_parse.py
D shell/ext-py/sqlparse-0.1.14/tests/test_pipeline.py
D shell/ext-py/sqlparse-0.1.14/tests/test_regressions.py
D shell/ext-py/sqlparse-0.1.14/tests/test_split.py
D shell/ext-py/sqlparse-0.1.14/tests/test_tokenize.py
D shell/ext-py/sqlparse-0.1.14/tests/utils.py
D shell/ext-py/sqlparse-0.1.14/tox.ini
M shell/impala_shell.py
M tests/shell/test_shell_interactive.py
63 files changed, 37 insertions(+), 6,715 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-5146: Fix inconsitent results at FROM UNIXTIME()

[Kim Jin Chul (Code Review)]

Hello Dan Hecht,

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

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

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

Change subject: IMPALA-5146: Fix inconsitent results at FROM_UNIXTIME()
..

IMPALA-5146: Fix inconsitent results at FROM_UNIXTIME()

The FROM_UNIXTIME(epoch) and FROM_UNIXTIME(epoch, format) produce
different results when epoch is out of range of TimestampValue.
The former produces an empty string, while the latter gives NULL.

The fix is to harmonize the results to NULL.

Testing:
Add unit tests to ExprTest.TimestampFunctions.

Change-Id: Ie3a5e9a9cb39d32993fa2c7f725be44d8b9ce9f2
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions-ir.cc
2 files changed, 5 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie3a5e9a9cb39d32993fa2c7f725be44d8b9ce9f2
Gerrit-Change-Number: 8629
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dan Hecht 

[Impala-ASF-CR] IMPALA-2250: Make multiple COUNT(DISTINCT) message state workarounds

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-2250: Make multiple COUNT(DISTINCT) message state 
workarounds
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8614/3/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@249
PS3, Line 249: if 
(distinctAggExprs.get(i).getFnName().getFunction().equalsIgnoreCase(
 : "count")) {
> In my example, it does not make sense to print an error message that uses N
Okay, let me reflect your proposal.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5084be10946d68f3ec0760c2b7e698635df26a89
Gerrit-Change-Number: 8614
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 22 Nov 2017 08:01:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2250: Make multiple COUNT(DISTINCT) message state workarounds

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-2250: Make multiple COUNT(DISTINCT) message state 
workarounds
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8614/3/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@249
PS3, Line 249: if 
(distinctAggExprs.get(i).getFnName().getFunction().equalsIgnoreCase(
 : "count")) {
> That's correct, but that error message is also thrown for non-count(distinc
1. Do you mean the guidance of NDV is also printed on your example? Why should 
we do this? If a given query does not have any use case of  COUNT(DISTINCT), we 
don't have to display this. It could make an user confuse. May I know your 
intention why we should include this?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5084be10946d68f3ec0760c2b7e698635df26a89
Gerrit-Change-Number: 8614
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 22 Nov 2017 05:48:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2250: Make multiple COUNT(DISTINCT) message state workarounds

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-2250: Make multiple COUNT(DISTINCT) message state 
workarounds
..


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8614/2/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@250
PS2, Line 250: + "Replace " + distinctAggExprs.get(0).toSql()
> * Just replacing one function may not work, so suggesting to replace a sing
Please see : 
https://gerrit.cloudera.org/#/c/8614/3/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java


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

http://gerrit.cloudera.org:8080/#/c/8614/3/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@249
PS3, Line 249: + "; deviating function: " + 
distinctAggExprs.get(i).toSql() + "\n"
 : + "Replace
Conversion to NDV() is only available for the case: COUNT(DISTINCT).


http://gerrit.cloudera.org:8080/#/c/8614/3/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@252
PS3, Line 252: + ") if estimated counts are okay. Enable the 
APPX_COUNT_DISTINCT"
 : + " query option to perform this rewrite 
automatically.");
I think it is better to show a specific object instead of constant.
Do you still prefer the constant general string like COUNT(DISTINCT)?

> select count(distinct x), count(distinct y), count(distinct z) from t3;
ERROR: AnalysisException: All DISTINCT aggregate functions need to have the 
same set of parameters as count(DISTINCT x); deviating function: count(DISTINCT 
y)
Consider using NDV(y) instead of count(DISTINCT y) if estimated counts are 
acceptable. Enable the APPX_COUNT_DISTINCT query option to perform this rewrite 
automatically.

> select count(distinct x), ndv(distinct y), count(distinct z) from t3;
ERROR: AnalysisException: All DISTINCT aggregate functions need to have the 
same set of parameters as count(DISTINCT x); deviating function: count(DISTINCT 
z)
Consider using NDV(z) instead of count(DISTINCT z) if estimated counts are 
acceptable. Enable the APPX_COUNT_DISTINCT query option to perform this rewrite 
automatically.

> select count(distinct x), ndv(distinct y), ndv(distinct z) from t3;
--> works



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5084be10946d68f3ec0760c2b7e698635df26a89
Gerrit-Change-Number: 8614
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 22 Nov 2017 05:20:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2250: Make multiple COUNT(DISTINCT) message state workarounds

[Kim Jin Chul (Code Review)]

Hello Alex Behm,

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

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

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

Change subject: IMPALA-2250: Make multiple COUNT(DISTINCT) message state 
workarounds
..

IMPALA-2250: Make multiple COUNT(DISTINCT) message state workarounds

Change-Id: I5084be10946d68f3ec0760c2b7e698635df26a89
---
M fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java
1 file changed, 12 insertions(+), 4 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5084be10946d68f3ec0760c2b7e698635df26a89
Gerrit-Change-Number: 8614
Gerrit-PatchSet: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-2250: Make multiple COUNT(DISTINCT) message state workarounds

[Kim Jin Chul (Code Review)]

Hello Alex Behm,

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

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

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

Change subject: IMPALA-2250: Make multiple COUNT(DISTINCT) message state 
workarounds
..

IMPALA-2250: Make multiple COUNT(DISTINCT) message state workarounds

Change-Id: I5084be10946d68f3ec0760c2b7e698635df26a89
---
M fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java
1 file changed, 6 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5084be10946d68f3ec0760c2b7e698635df26a89
Gerrit-Change-Number: 8614
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 

[Impala-ASF-CR] IMPALA-2250: Make multiple COUNT(DISTINCT) message state workarounds

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-2250: Make multiple COUNT(DISTINCT) message state 
workarounds
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8614/1/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@246
PS1, Line 246: throw new AnalysisException(
> include this in the exception message, most users will not dig into the log
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5084be10946d68f3ec0760c2b7e698635df26a89
Gerrit-Change-Number: 8614
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 22 Nov 2017 01:27:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4506: Do not display some intro message if --quiet is set

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-4506: Do not display some intro message if --quiet is set
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8613/1/shell/impala_shell.py@1481
PS1, Line 1481:   intro = get_welcome_string(options.verbose)
> Others may have a different opinion, but I don't think we should show the '
Your idea has been introduced by PS#2. Let's wait for other's opinions.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I19c6d00dfbbe805ee9c525b72eb5703840e2f582
Gerrit-Change-Number: 8613
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 21 Nov 2017 05:52:04 +
Gerrit-HasComments: Yes