[Impala-ASF-CR] test-with-docker: work with git worktree

[Kim Jin Chul (Code Review)]

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

Change subject: test-with-docker: work with git worktree
..


Patch Set 1:

Why don't you put JIRA on the commit msg?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9186e0b6f068aacc25f8d691508165c04329fa8b
Gerrit-Change-Number: 10335
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 10 May 2018 00:34:48 +
Gerrit-HasComments: No

[Impala-ASF-CR] test-with-docker: exit properly on failures

[Kim Jin Chul (Code Review)]

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

Change subject: test-with-docker: exit properly on failures
..


Patch Set 1:

Why don't you put JIRA on the commit msg?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I285e2f4d07e34898d73beba857e9ac325ed4e6db
Gerrit-Change-Number: 10318
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 10 May 2018 00:34:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6923:Update scripts in benchmark folder to store workload and few minor updates

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-6923:Update scripts in benchmark folder to store 
workload and few minor updates
..


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10100/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10100/8//COMMIT_MSG@7
PS8, Line 7: Update
Please add a space in front of this.


http://gerrit.cloudera.org:8080/#/c/10100/8/tests/benchmark/report_benchmark_results.py
File tests/benchmark/report_benchmark_results.py:

http://gerrit.cloudera.org:8080/#/c/10100/8/tests/benchmark/report_benchmark_results.py@141
PS8, Line 141: Dont
I think either "Don't" or "Do not" is better than Dont. This is used for user's 
helper message.


http://gerrit.cloudera.org:8080/#/c/10100/8/tests/benchmark/report_benchmark_results.py@1058
PS8, Line 1058:   if exec_summaries[0] is None:
  : return ""
I think this is a redundant special handling.  I believe L1054-1055 can be 
merged into the L1058-1059 because there is no symbol dependent issue.

e.g.)
if options.hive_results or exec_summaries[0] is None:
  return ""



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica7c00ad59963d466bae9e607a4692af0138962c
Gerrit-Change-Number: 10100
Gerrit-PatchSet: 8
Gerrit-Owner: Nithya Janarthanan 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Nithya Janarthanan 
Gerrit-Comment-Date: Thu, 10 May 2018 00:12:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Fix diagnostics path to not include the parent dir structure

[Kim Jin Chul (Code Review)]

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

Change subject: Fix diagnostics path to not include the parent dir structure
..


Patch Set 2:

Why don't you put JIRA in your commit message?
As far as I know, most changes include JIRA in the commit message. But some of 
the merged changes did not exist in JIRA. I think it's a good idea to include 
JIRA consistently even with small changes. What do you think?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I540f6c228a0315780d45cf11961f124478b5dd0c
Gerrit-Change-Number: 10347
Gerrit-PatchSet: 2
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 09 May 2018 23:36:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6635: Enable disk spill encryption by default

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-6635: Enable disk spill encryption by default
..


Patch Set 3:

Is this change a part of the fix for 6635? I was not seen the user scenario for 
Kudu's decimal in the commit.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee4be2a95d689f66c3663d99e4df0fb3968893a9
Gerrit-Change-Number: 10345
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 09 May 2018 04:37:13 +
Gerrit-HasComments: No

[Impala-ASF-CR] Fix diagnostics path to not include the parent dir structure

[Kim Jin Chul (Code Review)]

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

Change subject: Fix diagnostics path to not include the parent dir structure
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10347/1/bin/diagnostics/collect_diagnostics.py
File bin/diagnostics/collect_diagnostics.py:

http://gerrit.cloudera.org:8080/#/c/10347/1/bin/diagnostics/collect_diagnostics.py@451
PS1, Line 451: arcname=os.path.basename(self.collection_root_dir))
Some comments will be helpful to understand why an alternative name is required 
instead of the original name.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I540f6c228a0315780d45cf11961f124478b5dd0c
Gerrit-Change-Number: 10347
Gerrit-PatchSet: 1
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Wed, 09 May 2018 04:29:13 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6995: avoid DCHECK in TimestampParse::Parse()

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-6995: avoid DCHECK in TimestampParse::Parse()
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/10349/1/be/src/runtime/timestamp-parse-util.cc@399
PS1, Line 399: *d = boost::gregorian::date();
 : *t = 
boost::posix_time::time_duration(boost::posix_time::not_a_date_time);
>From a code refactoring perspective, I would suggest you make a new static 
>function to set default values. L371-372 and L399-400 can utilize the function.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I02a18ffd8893fe74f5830144300f745ce31477b1
Gerrit-Change-Number: 10349
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 09 May 2018 02:56:39 +
Gerrit-HasComments: Yes

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

Sorry for the delay. I am facing a minor problem in my local development 
environment. I am willing to deliver this change.


--
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: 19
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 30 Apr 2018 22:47:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3282: [DOCS] 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/10174


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

IMPALA-3282: [DOCS] Adds regexp_escape built-in function

Change-Id: Ied8e757c1b3012dd170b05da190d1598004d12cf
---
M docs/topics/impala_string_functions.xml
1 file changed, 43 insertions(+), 0 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..


Patch Set 2:

@Gabor and Attila, thanks for your review. I have been busy recently an now I 
have room again. Let me provide a new patch set with some comments soon.
@Tim, thanks for the information.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If3df97d685af3e81a40ce8a4f277a5c543d3ea57
Gerrit-Change-Number: 9391
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 10 Apr 2018 10:56:30 +
Gerrit-HasComments: No

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

@Alex, thank you for the comment. The problem was always reproducible on the 
patch set #12. As far as I remember, the problem was obvious with a debugger. 
Let me try to check the issue is still valid or not on the latest patch set.


--
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: 19
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 10 Apr 2018 10:53:26 +
Gerrit-HasComments: No

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

@Csaba, thanks for your review. I have been busy recently an now I have room 
again. Let me provide a new patch set soon. Have a good day!


--
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: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Tue, 10 Apr 2018 10:49:46 +
Gerrit-HasComments: No

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

(2 comments)

Hi Alex,

Sorry for the late response. I have a concern to rollback the code snippet in 
ImpaladTestCatalog.java. Would you please check my comment? Thanks a lot!

http://gerrit.cloudera.org:8080/#/c/8851/19/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8851/19/be/src/service/client-request-state.cc@269
PS19, Line 269:   DCHECK(names.size() == comments.size());
> no need for this, already checked in SetResultSet()
Done


http://gerrit.cloudera.org:8080/#/c/8851/19/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
File fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8851/19/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java@128
PS19, Line 128:   public Table getTable(String dbName, String tableName) {
> I think we should remove these overrides because they can be super hard to
I am not sure I could catch what you meant exactly, but I felt these code could 
affect test behavior
As far as I understand, FE unit tests should run without the loading table 
metadata explicitly, right? The one of reasons is the change could make 
unexpected different result.

Before rolling back the code snippet, I would like to you figure out why I 
added the code and give a guidance for a better solution. In PS#12, the timed 
out issue happened while running unit tests. I realized the cause was some of 
tables were IncompleteTable in getTables, so I added the code snippet to load 
metadata explicitly. When I compared the behavior with Impala kernel w/o all my 
change, I didn't see IncompleteTable in getTables on debugger.



--
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: 19
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 03 Apr 2018 02:54:50 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4168: [DOCS] 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/9030 )

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


Patch Set 2: Code-Review+1

(1 comment)

@Rodoni, sorry for the late.

http://gerrit.cloudera.org:8080/#/c/9030/2/docs/topics/impala_upsert.xml
File docs/topics/impala_upsert.xml:

http://gerrit.cloudera.org:8080/#/c/9030/2/docs/topics/impala_upsert.xml@71
PS2, Line 71: [hint_clause]
> Just to confirm: unlike with INSERT, is it correct that the hint for UPSERT
No, the optional hints can be placed between UPSERT and INTO. I think the 
current document is fine to me.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I43e0a782087c2e67f2e012424fb9261be445efc9
Gerrit-Change-Number: 9030
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 28 Mar 2018 11:26:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..


Patch Set 2:

Sure! Thank you.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If3df97d685af3e81a40ce8a4f277a5c543d3ea57
Gerrit-Change-Number: 9391
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 06 Mar 2018 18:40:46 +
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 9:

(1 comment)

Thanks for your comment.

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

http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc@175
PS9, Line 175:   // quote due to the compatibility with Hive's behavior.
> I think we still need a comment like my above one, since we are still treat
I had thought "'foo''bar'" should be broken into two tokens as you mentioned 
until I realized foo'bar should be expected string(not foobar). Now I believe 
the escaped single quotes should be in a token for single quoted string.

Why do we display a single quote in the middle of the string if we have to 
split "'foo''bar'" into 'foo' and 'bar'?

Could you please explain why we cannot convert into a single token without 
creating a new string and copying the data? If we need a pair of alloc and 
memcpy for new tokens, this is not related to the number of broken tokens.



--
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: 9
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, 05 Mar 2018 00:45:39 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3651: [DOCS] Doc for MURMUR HASH() function

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-3651: [DOCS] Doc for MURMUR_HASH() function
..


Patch Set 4:

Thank you all for the reviews.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I784a5a080d7d13192aac2fca67f841d2d19fc99b
Gerrit-Change-Number: 9031
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 05 Mar 2018 00:17:59 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

Hello Alex Behm,

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

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

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

Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..

IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC

Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.

* Add the following fields to EXTRACT and DATE_PART:

  WEEK
  DOW
  DOY
  SECOND/SECONDS
  MICROSECOND/MICROSECONDS
  NANOSECOND/NANOSECONDS

* Add the following field to TRUNC:

  SS

* Testing:
  Add unit tests to expr-test

Change-Id: If3df97d685af3e81a40ce8a4f277a5c543d3ea57
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 215 insertions(+), 88 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If3df97d685af3e81a40ce8a4f277a5c543d3ea57
Gerrit-Change-Number: 9391
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 

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

Thanks for your comment. I've resolved the conflicts and tested locally.


--
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: 19
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 26 Feb 2018 13:26:53 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

Hello Dimitris Tsirogiannis, Impala Public Jenkins,

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

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

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

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.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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 fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/authorization/test_grant_revoke.py
M tests/metadata/test_hms_integration.py
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
23 files changed, 337 insertions(+), 216 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/19
--
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: 19
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-5607: Add additional units to EXTRACT, DATE PART, TRUNC

[Kim Jin Chul (Code Review)]

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


Change subject: IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC
..

IMPALA-5607: Add additional units to EXTRACT, DATE_PART, TRUNC

Return type of EXTRACT/DATE_PART is changed from INT to BIGINT
because INT data type cannot cover NANOSECOND's value.

* Add the following fields to EXTRACT and DATE_PART:

  WEEK
  DOW
  DOY
  SECOND/SECONDS
  MICROSECOND/MICROSECONDS
  NANOSECOND/NANOSECONDS

* Add the following field to TRUNC:

  SS

* Testing:
  Add unit tests to expr-test

Change-Id: If3df97d685af3e81a40ce8a4f277a5c543d3ea57
---
M be/src/exprs/expr-test.cc
M be/src/exprs/udf-builtins-ir.cc
M be/src/exprs/udf-builtins.cc
M be/src/exprs/udf-builtins.h
M common/function-registry/impala_functions.py
M common/thrift/Exprs.thrift
6 files changed, 211 insertions(+), 84 deletions(-)



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

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

[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 (#8).

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 printing errors column-wise instead
of row-wise.

Testing:
Add two test cases:
- TestWrongFileOffset.test_invalid_int
- TestWrongFileOffset.test_invalid_int_gzip

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/invalid_int.csv
A testdata/data/invalid_int.csv.gz
M tests/query_test/test_scanners.py
8 files changed, 177 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8747/8
--
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: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 

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

(5 comments)

Thanks for your comments. I've revised the algorithm to find escaped single 
quotes. I think this is more intuitive and readable than previous version.

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

http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/exprs/expr-test.cc@6631
PS9, Line 6631:   // Test that pairs of single quotes are treated as an escape 
sequence.
> Can you add a comment, e.g.
Done


http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/exprs/expr-test.cc@6632
PS9, Line 6632:   TestStringValue(R"(from_unixtime(0, 'H\'foo\'\'bar\'H'))", 
"0foo'bar0");
> Can you also test the case when the escaped quote is at the end of the stri
Done


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

http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc@86
PS9, Line 86:
> nit: remove extra line
Done


http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc@175
PS9, Line 175:   // e.g. '''foo''bar''' => 'foo'bar'
> This is a little tricky and needs some explanation. I'd suggest something l
Let me provide a more intuitive algorithm to find escaped single quotes between 
single quotes.


http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc@177
PS9, Line 177:  q
> nit: spaces around +, i.e. should be " + 1"
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: 10
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: Thu, 22 Feb 2018 02:31:21 +
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 (#10).

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: ExprTest.QuotedStringForDateTimeFormat

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, 144 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8508/10
--
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: 10
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-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 18:

Applied the update for the expected results


--
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: 18
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 19 Feb 2018 07:00:35 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

Hello Dimitris Tsirogiannis, Impala Public Jenkins,

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

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

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

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.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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 fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/authorization/test_grant_revoke.py
M tests/metadata/test_hms_integration.py
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
23 files changed, 351 insertions(+), 231 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/18
--
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: 18
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 

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

(10 comments)

Thanks for the comments. I've applied the update.

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

http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/exprs/expr-test.cc@6622
PS8, Line 6622: int expected_var_begin, const map& 
expected_offsets) {
> I found a case where your implementation is inconsistent with hive. It look
Done. Thanks for finding the corner case. The case is added.


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

http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.h@187
PS8, Line 187:   /// len -- length of the string to parse (must be > 0)
> This might change if you fix the inconsistency I mentioned.
As you mentioned in the corner case, I've fixed the inconsistency on the last 
patch set. See 
https://gerrit.cloudera.org/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc 
(#line 177)


http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.h@188
PS8, Line 188:   /// dt_ctx -- date/time format context (must contain valid 
tokens)
> nit: use /// for function comments.
Done


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

http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@150
PS8, Line 150: ParseFormatTokens
> I think this should be a method of DateTimeFormatContext instead of Timesta
Done


http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@150
PS8, Line 150: ParseFormatTokens
> Maybe DateTimeFormatContext::AppendToken()
Done


http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@155
PS8, Line 155: const
> Use DCHECK_GE macro to get better info on failure.
Done


http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@156
PS8, Line 156: tr_end =
> You can use emplace_back(token_type, position_of_token, ...) instead.
Done


http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@213
PS8, Line 213: while (curr_tok_chr < str_end) {
> Can you convert these other locations in this function to use your new help
Done


http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@518
PS8, Line 518: boost::unordered_map::const_iterator 
iter =
> Use DCHECK_LE instead.
Done


http://gerrit.cloudera.org:8080/#/c/8508/8/be/src/runtime/timestamp-parse-util.cc@519
PS8, Line 519: TH_INDE
> I think using memcmp() would be better, since strncmp() is meant for null-t
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: 1
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, 19 Feb 2018 04:38:29 +
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 (#9).

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: ExprTest.QuotedStringForDateTimeFormat

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, 135 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8508/9
--
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: 9
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-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 17: Code-Review-1

Let me look into the failure.


--
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: 17
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 14 Feb 2018 04:59:18 +
Gerrit-HasComments: No

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

(1 comment)

Applied the update.

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
File fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java@81
PS15, Line 81: getLoadedTable
> I think this function should be called loadAndAddTable. The current name im
Done



--
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: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 13 Feb 2018 00:34:35 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Dimitris Tsirogiannis, Impala Public Jenkins,

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

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

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

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.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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 fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
20 files changed, 338 insertions(+), 218 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/16
--
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: 16
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
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 15:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/main/java/org/apache/impala/service/MetadataOp.java@279
PS15, Line 279: for (Table table: fe.getTables(db.getName(), 
tablePatternMatcher, user)) {
  :   String tabName = table.getName();
> Are we certain that this code doesn't introduce the same infinite wait for
I confirmed the problem should be solved with my change. The cause of the 
problem is JUnit test requires tables loading explicitly if they are not 
loaded. This is the reason why I introduce a new code at 
ImpaladTestCatalog.java.


http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
File fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8851/15/fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java@107
PS15, Line 107: /**
  :* Overrides ImpaladCatalog.getTables to load the table 
metadata if it is missing.
  :*/
  :   @Override
  :   public List getTables(String dbName, PatternMatcher 
matcher)
  :   throws DatabaseNotFoundException {
  : List tables = super.getTables(dbName, matcher);
  :
  : // The table was not yet loaded. Load it in to the catalog 
and try getTable()
  : // again.
  : for (int i = 0; i < tables.size(); ++i) {
  :   Table table = tables.get(i);
  :   // Table doesn't exist or is already loaded. Just return 
it.
  :   if (table != null && !table.isLoaded()) {
  : try {
  :   tables.set(i, getLoadedTable(dbName, 
table.getName()));
  : } catch (CatalogException e) {
  :   LOG.error(String.format("Error loading table: %s.%s", 
dbName, table.getName()),
  :   e);
  : }
  :   }
  : }
  : return tables;
  :   }
> Where is the ImpaladTestCatalog::getTables() called?
During JUnit test, it should be called at 
"impaladCatalog_.get().getTables(dbName, matcher)". See 
https://gerrit.cloudera.org/#/c/8851/15/fe/src/main/java/org/apache/impala/service/Frontend.java
 at 625.



--
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: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 12 Feb 2018 23:17:02 +
Gerrit-HasComments: Yes

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

(1 comment)

Applied the update.

I realized that table loading should be required if a table has not been loaded 
on JUnit test. In getTables, some of tables can be IncompleteTable. Thus, these 
tables should be loaded explicitly. Please see my change in ImpaladTestCatalog.

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

http://gerrit.cloudera.org:8080/#/c/8851/14/fe/src/main/java/org/apache/impala/service/MetadataOp.java@283
PS14, Line 283: else {
  : continue;
  :   }
> Are you sure this is correct? If table is loaded and is not an instance of
Sorry, I should have thought it deeply.



--
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: 14
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Mon, 12 Feb 2018 12:42:54 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Dimitris Tsirogiannis, Impala Public Jenkins,

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

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

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

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.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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 fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
20 files changed, 338 insertions(+), 218 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/15
--
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: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
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 13:

Applied the update.


--
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: 13
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Fri, 09 Feb 2018 00:32:03 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

Hello Dimitris Tsirogiannis, Impala Public Jenkins,

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

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

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

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.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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 testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
19 files changed, 296 insertions(+), 206 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/14
--
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: 14
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 

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

Hi Lars,

You may be busy. It would be great if you review this change again. Thanks!

Regards,
Jinchul


--
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: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 09 Feb 2018 00:06:38 +
Gerrit-HasComments: No

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

(1 comment)

The timed out issue at Jenkins should be resolved on this patch set.

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

http://gerrit.cloudera.org:8080/#/c/8851/13/fe/src/main/java/org/apache/impala/service/MetadataOp.java@281
PS13, Line 281:   try {
  : table = catalog.getTable(db.getName(), tabName);
  :   } catch (TableLoadingException e) {
  : // Ignore exception (this table will be skipped).
  :   }
  :   if (table == null) continue;
The removal of the code causes the timed out issue. The problem is infinite 
retries for not loaded table at Frontend.requestTblLoadAndWait.

I thought the code should be duplicate. By the way, there are some conditions 
to check that a table is loaded, table type is come from IncompleteType and 
TableLoadingException happens. I prefer to revive the code instead of copy of 
essential code snippet due to avoidance of any side effect. Here is the 
relevant code for the condition: 
https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java#L250



--
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: 13
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 08 Feb 2018 13:53:45 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Dimitris Tsirogiannis, Impala Public Jenkins,

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

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

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

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.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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 testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
19 files changed, 290 insertions(+), 200 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/13
--
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: 13
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
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 12: Code-Review-1

My change seems to be related with the timed out of the parallel test run in 
Jenkins. Let me look into this.


--
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: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 01 Feb 2018 09:07:55 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 9:

Thank you all for the reviews.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84c3e0ded26f6eb20794c38b75be9b25cd111e4b
Gerrit-Change-Number: 8900
Gerrit-PatchSet: 9
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 01 Feb 2018 05:42:15 +
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 8:

(5 comments)

Applied the update.

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

http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.h@189
PS7, Line 189:   // function finishes str pointer is changed to point 
to the closing quote.
> This sentence is not needed in my opinion.
Done


http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.h@190
PS7, Line 190:   //  position_of_string_literal -- start position of the string 
literal.
> str is assumed to point to...
Done


http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.h@192
PS7, Line 192:   //  string_literal_begin -- start pointer of the string 
literal if there is a pair of
> some separator between the param name and it's description would be great.
Done


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

http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.cc@161
PS7, Line 161: const char** str, int* position_of_string_literal, int* 
length_of_string_literal,
> One thing I learned recently, that in Impala the out parameters are preferr
You're right. Actually, Tim already answered the similar question on this 
change: https://gerrit.cloudera.org/#/c/8770/2/be/src/exec/data-sink.cc


http://gerrit.cloudera.org:8080/#/c/8508/7/be/src/runtime/timestamp-parse-util.cc@203
PS7, Line 203:   // str is to point to the closing quote and the 
incrementing points to the first
> It might worth a comment here that incrementing this would result in the st
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: 8
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, 30 Jan 2018 09:19:01 +
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 (#8).

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: ExprTest.QuotedStringForDateTimeFormat

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, 128 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/8508/8
--
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: 8
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-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 (#10).

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.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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 testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
M www/catalog.tmpl
19 files changed, 290 insertions(+), 208 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/10
--
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: 10
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
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 9:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8851/9/be/src/catalog/catalog-server.cc@368
PS9, Line 368: if (table_info.__isset.comment) {
 : Value table_comment(table_info.comment.c_str(), 
document->GetAllocator());
 : table_obj.AddMember("comment", table_comment, 
document->GetAllocator());
 :   }
> Did you verify that this shows up in the web UI of impalad and catalogd?  I
Done. The comment column is added to the catalog page like this: 
https://image.ibb.co/cSjog6/2018_01_30_2_35_55.png



--
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: 9
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 30 Jan 2018 05:40:59 +
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 7:

(5 comments)

Applied the update.

@Gabor, I am sorry to make you review it again after a long time. Now we can 
deliver some test cases for the mix of single and double quotes as you advised!

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:   //  str is to point to the opening quote when the function is 
called. Once the function
> Let me reflect it on the next patch set.
Done


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@174
PS6, Line 174:   length_of_string_literal = str - quoted_string_begin - 1;
> Right, it should be a redundant. Let me remove this on the next patch set.
Done


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


http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@177
PS6, Line 177:   if (length_of_string_literal == 0) length_of_string_literal = 
1;
> Okay, let me reflect this on the next patch set.
Done


http://gerrit.cloudera.org:8080/#/c/8508/6/be/src/runtime/timestamp-parse-util.cc@178
PS6, Line 178:   position_of_string_literal = str - str_begin - 
length_of_string_literal;
> Okay, I accept your approach.
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: 7
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: Fri, 26 Jan 2018 06:09:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4168: [DOCS] 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/9030 )

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


Patch Set 2:

> In addition to the text that was actually added, I'll look in
 > impala_hints.xml and see if there is info in there that also needs
 > to change.
I added some examples for Oracle-style hint placement for INSERT/UPSERT into 
impala_hints.xml. I guess some examples would be redundant. They might be 
overkill. Please give me your advice for my concern. I didn't mention about the 
new placement in impala_hints because I think this part is to address for hint 
itself.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I43e0a782087c2e67f2e012424fb9261be445efc9
Gerrit-Change-Number: 9030
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Fri, 26 Jan 2018 01:03:59 +
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 (#9).

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.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.test_table_comment_visibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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 testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
18 files changed, 288 insertions(+), 208 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/9
--
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: 9
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-3651: [DOCS] Doc for MURMUR HASH() function

[Kim Jin Chul (Code Review)]

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

Change subject: IMPALA-3651: [DOCS] Doc for MURMUR_HASH() function
..


Patch Set 1:

(5 comments)

Applied the update.

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

http://gerrit.cloudera.org:8080/#/c/9031/1//COMMIT_MSG@7
PS1, Line 7: [DOCS]
> Let's include a reference to the code JIRA:
Done


http://gerrit.cloudera.org:8080/#/c/9031/1/docs/topics/impala_math_functions.xml
File docs/topics/impala_math_functions.xml:

http://gerrit.cloudera.org:8080/#/c/9031/1/docs/topics/impala_math_functions.xml@862
PS1, Line 862: 2.12.0
> rev="IMPALA-3651 2.12.0"
Done


http://gerrit.cloudera.org:8080/#/c/9031/1/docs/topics/impala_math_functions.xml@871
PS1, Line 871: https://en.wikipedia.org/wiki/MurmurHash
> Could you add an entry in ../impala_keydefs.ditamap for this wiki page, fol
Done


http://gerrit.cloudera.org:8080/#/c/9031/1/docs/topics/impala_math_functions.xml@878
PS1, Line 878: You might use the return value
> Is there some aspect of performance, collisions, or similar that would infl
Done


http://gerrit.cloudera.org:8080/#/c/9031/1/docs/topics/impala_math_functions.xml@907
PS1, Line 907: relatively low entropy
> Is this statement really true for murmur_hash()? For fnv_hash(), the corres
Done. Sorry, it's my mistake.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I784a5a080d7d13192aac2fca67f841d2d19fc99b
Gerrit-Change-Number: 9031
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 25 Jan 2018 09:40:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3651: [DOCS] Doc for MURMUR HASH() function

[Kim Jin Chul (Code Review)]

Hello John Russell, Tim Armstrong,

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

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

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

Change subject: IMPALA-3651: [DOCS] Doc for MURMUR_HASH() function
..

IMPALA-3651: [DOCS] Doc for MURMUR_HASH() function

Change-Id: I784a5a080d7d13192aac2fca67f841d2d19fc99b
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_math_functions.xml
2 files changed, 76 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I784a5a080d7d13192aac2fca67f841d2d19fc99b
Gerrit-Change-Number: 9031
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Tim Armstrong 

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

(12 comments)

Applied update.

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/catalog/catalog.h@79
PS7, Line 79: Returns all matching table names and table comments, per Hive's
:   /// "SHOW TABLES ". Each table name of TTableInfo 
returned is unqualified.
:   /// Comment cannot be set when table is not loaded. If pattern 
is NULL, match all
:   /// tables otherwise match only those tables that match the 
pattern string. Patterns
:   /// are "p1|p2|p3" where | denotes choice, and each pN may 
contain wildcards denoted
:   /// by '*' which match all strings. Table comments can be empty 
if table is not loaded.
> Let's fix this comment a bit:
Done


http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/client-request-state.cc@280
PS7, Line 280: if (!tbl_info.__isset.comment) DCHECK(tbl_info.comment.empty());
 : comments.push_back(tbl_info.comment);
> This check is a bit confusing. Ideally, you want to ensure that names and c
Sorry, actually I didn't catch your intention for adding DCHECK in the previous 
comment. I still don't understand your last sentence. Why should we consider 
this(i.e. check)?
As far as I understand, comment should be either empty(table is not loaded case 
or a given empty string as a comment) or not empty. Hence, we just put it to 
comments. Anyway, we should guarantee that the number of names should be equal 
to the number of comments.

Let me remove the DCHECK code line which seems to be meaningless now.


http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.h
File be/src/service/frontend.h:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.h@57
PS7, Line 57: /// Returns all matching table names and table comments.
:   /// If pattern is NULL, match all tables otherwise match only 
those tables that
:   /// match the pattern string. Patterns are "p1|p2|p3" where | 
denotes choice,
:   /// and each pN may contain wildcards denoted by '*' which 
match all strings.
:   /// The TSessionState parameter is used to filter results of 
metadata operations when
:   /// authorization is enabled. If this is a user initiated 
request, it should
:   /// be set to the user's current session. If this is an Impala 
internal request,
:   /// the session should be set to NULL which will skip privilege 
checks returning all
:   /// results. Table comments can be empty if table is not loaded.
> See previous comment about this comment :).
Done


http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.cc
File be/src/service/frontend.cc:

http://gerrit.cloudera.org:8080/#/c/8851/7/be/src/service/frontend.cc@145
PS7, Line 145: return JniUtil::CallJniMethod(fe_, get_tables_info_id_, params,
 :   tables_info_result);
> nit: check if it fits in a single line
Done


http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@91
PS7, Line 91: Comment needs to indicate when and if this is set. For instance,
:   // it's not clear if this is set if the table is loaded but 
there is no comment.
> I believe this was my comment. The point was to explicitly specify the beha
Done. Sorry, it's my mistake. I was confusing table 'comment' and your 
'comment'.


http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@96
PS7, Line 96: // getTablesInfo returns a list of table names and a list of 
table comment.
> comments is not consistent with the struct definition, plz update
Done


http://gerrit.cloudera.org:8080/#/c/8851/7/common/thrift/Frontend.thrift@98
PS7, Line 98: tableinfos
> tables_info
Done


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

http://gerrit.cloudera.org:8080/#/c/8851/6/fe/src/main/java/org/apache/impala/catalog/Table.java@407
PS6, Line 407: blic
> My thinking is that we're not particularly consistent with using final in o
Thanks for sharing your idea. It's just my curiosity. I want to know the reason 
why you thought. Actually I didn't find the use of final keyword elsewhere 
except for class member variable. I would like to follow other authors' 
preference.

[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 (#8).

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.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test: TestShowTables.testTableCommentVisibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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 testdata/workloads/functional-query/queries/QueryTest/show.test
M tests/metadata/test_metadata_query_statements.py
18 files changed, 304 insertions(+), 197 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/8
--
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: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-3942: Fix wrongly 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 wrongly escaped string literal in front-end
..


Patch Set 11:

Thanks you all for the reviews!


--
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: 11
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 23 Jan 2018 20:53:22 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3942: Fix wrongly 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 wrongly escaped string literal in front-end
..


Patch Set 10:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8818/6/tests/query_test/test_queries.py@310
PS6, Line 310:
> I just realized this comment has not been addressed, sorry. Can you move th
As you mentioned in the previous comment, our unit test can fully cover the 
issue, so I just remove the E2E test.



--
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: 10
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 23 Jan 2018 15:28:33 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Thomas Tauber-Marshall, 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 (#10).

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

IMPALA-3942: Fix wrongly 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:
- toSql() always returns strings that are single quoted, resulting in
improper escaping in the output if the original string was actually
double quoted.
- It is not always possible to determine if a string "should" be single
or double quoted. e.g. concat('a', "b")

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)
to single quotes.

Testing:
Added unit tests into expr-test, ToSqlTest

Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
3 files changed, 93 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/8818/10
--
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: 10
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

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

(22 comments)

Applied the update.

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

http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@13
PS6, Line 13: table's comment can be displayed if catalog table
: owns Hive metastore table(HMS) object
> There is no notion of ownership here. A loaded table is always associated w
Done


http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@14
PS6, Line 14: There is not any HMS call
: due to performance concern, so table's comment can be empty even
: though table has a comment.
> Rewrite as: "If the table is not loaded, an empty comment is returned."
Done


http://gerrit.cloudera.org:8080/#/c/8851/6//COMMIT_MSG@18
PS6, Line 18: There is a change in thrift part: a list of table comments is 
added to
: TGetTablesResult struct as an optional element.
> Remove this sentence.
Done


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

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog-server.cc@381
PS6, Line 381: for (const TTableInfo& table_info: 
get_tables_info_result.tableinfos) {
 :   Value table_obj(kObjectType);
 :   Value fq_name(Substitute("$0.$1", db.db_name, 
table_info.tbl_name).c_str(),
 :   document->GetAllocator());
 :   table_obj.AddMember("fqtn", fq_name, 
document->GetAllocator());
 :   Value table_name(table_info.tbl_name.c_str(), 
document->GetAllocator());
 :   table_obj.AddMember("name", table_name, 
document->GetAllocator());
 :   table_array.PushBack(table_obj, document->GetAllocator());
 : }
> Why not returning the table comments as well?
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.h@83
PS6, Line 83: Hive metastore was not loaded for a table
> "table is not loaded".
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.cc
File be/src/catalog/catalog.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/catalog/catalog.cc@140
PS6, Line 140:   return JniUtil::CallJniMethod(catalog_, get_table_names_id_, 
params, tables_info_result);
> long line
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/client-request-state.cc@275
PS6, Line 275: for (const TTableInfo& tbl_info: tables_info_result.tableinfos) {
 : names.push_back(tbl_info.tbl_name);
 : comments.push_back(tbl_info.comment);
 :   }
 :   SetResultSet(names, comments);
> This code will result to DCHECK being hit if some table doesn't have the co
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/frontend.cc
File be/src/service/frontend.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/frontend.cc@139
PS6, Line 139: Status Frontend::GetTableNames(const string& db, const string* 
pattern,
 : const TSessionState* session, TGetTablesInfoResult* 
tables_info_result) {
 :   TGetTablesInfoParams params;
 :   params.__set_db(db);
 :   if (pattern != NULL) params.__set_pattern(*pattern);
 :   if (session != NULL) params.__set_session(*session);
 :   return JniUtil::CallJniMethod(fe_, get_table_names_id_, 
params, tables_info_result);
 : }
 :
 : Status Frontend::GetTableNamesAndComments(const string& db, 
const string* pattern,
 : const TSessionState* session, TGetTablesInfoResult* 
tables_info_result) {
 :   TGetTablesInfoParams params;
 :   params.__set_db(db);
 :   if (pattern != NULL) params.__set_pattern(*pattern);
 :   if (session != NULL) params.__set_session(*session);
 :   return JniUtil::CallJniMethod(fe_, 
get_table_names_and_comments_id_, params,
 :   tables_info_result);
 : }
> Should be a single call Frontend::GetTablesInfo()
Done


http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/impala-http-handler.cc
File be/src/service/impala-http-handler.cc:

http://gerrit.cloudera.org:8080/#/c/8851/6/be/src/service/impala-http-handler.cc@522
PS6, Line 522: for (const TTableInfo& tbl_info: 
get_tables_info_result.tableinfos) {
 :   Value table_obj(kObjectType);
 :   Value fq_name(Substitute("$0.$1", db.db_name, 
tbl_info.tbl_name).c_str(),
   

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

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.

On this change, table's comment is displayed if the table is
loaded. If the table is not loaded, an empty comment is returned.

Testing:
Adds E2E test. TestShowTables.testTableCommentVisibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
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/metadata/test_metadata_query_statements.py
17 files changed, 190 insertions(+), 90 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/7
--
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: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 

[Impala-ASF-CR] IMPALA-3942: Fix wrongly 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 wrongly escaped string literal in front-end
..


Patch Set 8:

(1 comment)

Applied the update.

http://gerrit.cloudera.org:8080/#/c/8818/8/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/8/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@92
PS8, Line 92:*  String literal can come directly from the SQL of a query or 
from rewrites like
> Some grammar issues, suggest this correction:
Done



--
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: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 23 Jan 2018 04:19:16 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Thomas Tauber-Marshall, 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 (#9).

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

IMPALA-3942: Fix wrongly 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:
- toSql() always returns strings that are single quoted, resulting in
improper escaping in the output if the original string was actually
double quoted.
- It is not always possible to determine if a string "should" be single
or double quoted. e.g. concat('a', "b")

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)
to single quotes.

Testing:
Added unit tests into expr-test, ToSqlTest
Added E2E tests into test_queries

Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M tests/query_test/test_queries.py
4 files changed, 113 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/8818/9
--
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: 9
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

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

[Kim Jin Chul (Code Review)]

Hello Tianyi Wang, Jim Apple, Philip Zeyliger, Tim Armstrong,

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

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

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

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

IMPALA-3282: Adds regexp_escape built-in function

Escapes the following special characters in RE2 library:
.\+*?[^]$(){}=!<>|:-

Testing:
Add some unit tests into ExprTest.StringRegexpFunctions
Add some E2E tests into exprs.test

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
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
5 files changed, 91 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/8900/7
--
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: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 5:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8900/5/be/src/exprs/string-functions-ir.cc@623
PS5, Line 623:   const strings::CharSet to_escape(".\\+*?[^]$(){}=!<>|:-");
> This is a constant (perhaps "REGEX_ESCAPE_CHARACTERS") and could be declare
Thanks for the suggestion. The name is changed and static keyword is also 
added. By the way, I'd like to leave it in the function because it is just used 
in this function.


http://gerrit.cloudera.org:8080/#/c/8900/5/be/src/exprs/string-functions-ir.cc@637
PS5, Line 637:   DCHECK_GE(result.len, 0);
> DCHECK_GE(result.len, str.len) should be true too, no?
Correct. the length of a new generated string should be greater or equal than 
the length of the original string. Your idea is more close to what I intended.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84c3e0ded26f6eb20794c38b75be9b25cd111e4b
Gerrit-Change-Number: 8900
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 23 Jan 2018 00:57:24 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Tianyi Wang, Jim Apple, Philip Zeyliger, Tim Armstrong,

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

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

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

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

IMPALA-3282: Adds regexp_escape built-in function

Escapes the following special characters in RE2 library:
.\+*?[^]$(){}=!<>|:-

Testing:
Add some unit tests into ExprTest.StringRegexpFunctions
Add some E2E tests into exprs.test

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
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
5 files changed, 92 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/8900/6
--
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: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3942: Fix wrongly 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 wrongly escaped string literal in front-end
..


Patch Set 7:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8818/7/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/7/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@87
PS7, Line 87: return BaseSemanticAnalyzer.unescapeSQLString("'" + 
getNormalizedValue()
> whitespace
Done


http://gerrit.cloudera.org:8080/#/c/8818/7/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@91
PS7, Line 91:   // Assumes a string literal from single quotes and escapes it 
because:
> What does this function do and return? Please consider these snippets which
Done


http://gerrit.cloudera.org:8080/#/c/8818/7/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@100
PS7, Line 100:   private String getNormalizedValue() {
> nit: we typically use /* */ style comment for class and function comments
Done


http://gerrit.cloudera.org:8080/#/c/8818/7/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@101
PS7, Line 101: final int lengthOfValue = value_.length();
> len?
Done


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

http://gerrit.cloudera.org:8080/#/c/8818/7/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@468
PS7, Line 468:   public void escapeStringLiteralTest() {
> normalizeStringLiteralsTest
Done


http://gerrit.cloudera.org:8080/#/c/8818/7/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@471
PS7, Line 471: testToSql("select \"'\"", "SELECT '\\''");
> add a few tests with single quotes that show they are not transformed
Done



--
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: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 22 Jan 2018 15:11:58 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Thomas Tauber-Marshall, 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 (#8).

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

IMPALA-3942: Fix wrongly 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:
- toSql() always returns strings that are single quoted, resulting in
improper escaping in the output if the original string was actually
double quoted.
- It is not always possible to determine if a string "should" be single
or double quoted. e.g. concat('a', "b")

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)
to single quotes.

Testing:
Added unit tests into expr-test, ToSqlTest
Added E2E tests into test_queries

Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M tests/query_test/test_queries.py
4 files changed, 113 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/8818/8
--
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: 8
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 4:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8900/4/be/src/exprs/string-functions-ir.cc@624
PS4, Line 624:   ostringstream ss;
> We should just preallocate a StringVal of 2 * str.len for the output, inste
Done


http://gerrit.cloudera.org:8080/#/c/8900/4/be/src/exprs/string-functions-ir.cc@625
PS4, Line 625:   for (char const *c = start_ptr; c < end_ptr; c++) {
> Calling non-IR functions from other modules from IR generally works - when
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84c3e0ded26f6eb20794c38b75be9b25cd111e4b
Gerrit-Change-Number: 8900
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 17 Jan 2018 05:54:36 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Tianyi Wang, Jim Apple, Philip Zeyliger, Tim Armstrong,

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

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

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

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

IMPALA-3282: Adds regexp_escape built-in function

Escapes the following special characters in RE2 library:
.\+*?[^]$(){}=!<>|:-

Testing:
Add some unit tests into ExprTest.StringRegexpFunctions
Add some E2E tests into exprs.test

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
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
5 files changed, 92 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/8900/5
--
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: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

(25 comments)

Hi Lars,

Sorry I thought I already replied your comments, but I didn't submit a botton 
when I answered them. Now the latest patch set is ready for your review. Thanks 
for your notification.

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

http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.h@368
PS6, Line 368:   /// to the error.
> Explain what error_file_offsets does in comment
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.h@382
PS6, Line 382: r. Ret
> Wouldn't this report an invalid value (column inside row)?
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.h@382
PS6, Line 382:   /// readers to exercise various failure paths in Parquet 
scanner. Returns the status
> It seems confusing to me that there is now a LogColumnParseError and Report
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.h@391
PS6, Line 391:
> Start comments with three /// like the other lines.
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.h@393
PS6, Line 393:  tuple (e.g. fields[0] ma
> No need to mention its type since it's the same as in the declaration itsel
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.h@408
PS6, Line 408:   /// the reason that the out error parameters are typed uint8_t 
instead of bool. We need
> This looks like it could have some performance impact. Did you do any perf
Done, the computation is moved to the error reporting function. By the way, 
HdfsSequenceScanner::ProcessRange needs the computation because I could not 
find a way to apply the computation logic. See line#337 
https://gerrit.cloudera.org/#/c/8747/7/be/src/exec/hdfs-sequence-scanner.cc


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

http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.cc@233
PS6, Line 233: uint8_t* error_fields, uint8_t* error_in_row) {
> nit: wrap at 90 characters.
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.cc@254
PS6, Line 254:   return EvalConjuncts(tuple_row);
> Is +1 for the delimiter? If so, can you add a brief comment?
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.cc@578
PS6, Line 578:   DCHECK_EQ(replaced, 1);
> Please indent function calls 4 characters, here and elsewhere.
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.cc@759
PS6, Line 759:
> nit: line wrapping.
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.cc@761
PS6, Line 761:
> Why does this not check state_->HasLogSpace() but the method below does?
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/be/src/exec/hdfs-scanner.cc@768
PS6, Line 768:
> I think the message was clearer before the change.
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/testdata/data/README
File testdata/data/README:

http://gerrit.cloudera.org:8080/#/c/8747/6/testdata/data/README@136
PS6, Line 136: signed_integer_logical_types
> Can you think of a name that captures the nature of the problems? Most of t
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/tests/query_test/test_scanners.py
File tests/query_test/test_scanners.py:

http://gerrit.cloudera.org:8080/#/c/8747/6/tests/query_test/test_scanners.py@30
PS6, Line 30: from parquet.ttypes import ConvertedType
> I think elsewhere we just use "os.path.join()" when we need it, so for cons
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/tests/query_test/test_scanners.py@841
PS6, Line 841:
 :   @classmethod
> nit: missing word?
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/tests/query_test/test_scanners.py@842
PS6, Line 842:   @classmethod
 :   def get_workload(cls):
> Rephrase to say what it should do, not what the problem was.
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/tests/query_test/test_scanners.py@852
PS6, Line 852:   self.hdfs_client = get_hdfs_client_from_conf(hdfs_conf)
> If you change this to "if p.c.o...: " it will also cover the empty string.
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/tests/query_test/test_scanners.py@860
PS6, Line 860:   "invalid_int":
> I think it might be cleaner if every test prepared their own test data sepa
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/tests/query_test/test_scanners.py@862
PS6, Line 862: "dir_path": os.path.join(tmpdir.strpath, "invalid_int"),
> This will break with parallel test runs. Use a tmpdir fixture for pytest in
Done


http://gerrit.cloudera.org:8080/#/c/8747/6/tests/query_test/test_scanners.py@876
PS6, Line 876: put
> nit: putting
Done

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 7:

Docs: https://gerrit.cloudera.org/#/c/9031/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14d56ffb8fab256f3f66a2669271fd4b3c50cc29
Gerrit-Change-Number: 8893
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 16 Jan 2018 13:00:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] [DOCS] Doc for MURMUR HASH() function

[Kim Jin Chul (Code Review)]

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


Change subject: [DOCS] Doc for MURMUR_HASH() function
..

[DOCS] Doc for MURMUR_HASH() function

Change-Id: I784a5a080d7d13192aac2fca67f841d2d19fc99b
---
M docs/topics/impala_math_functions.xml
1 file changed, 65 insertions(+), 0 deletions(-)



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

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

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

docs: https://gerrit.cloudera.org/#/c/9030/


--
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: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 16 Jan 2018 12:16:36 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

Hello Thomas Tauber-Marshall, 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 (#7).

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

IMPALA-3942: Fix wrongly 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:
- toSql() always returns strings that are single quoted, resulting in
improper escaping in the output if the original string was actually
double quoted.
- It is not always possible to determine if a string "should" be single
or double quoted. e.g. concat('a', "b")

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)
to single quotes.

Testing:
Added unit tests into expr-test, ToSqlTest
Added E2E tests into test_queries

Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
---
M be/src/exprs/expr-test.cc
M fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M tests/query_test/test_queries.py
4 files changed, 110 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/8818/7
--
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: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

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

(11 comments)

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

http://gerrit.cloudera.org:8080/#/c/8818/6//COMMIT_MSG@7
PS6, Line 7: wronly
> wrongly
Done


http://gerrit.cloudera.org:8080/#/c/8818/6//COMMIT_MSG@15
PS6, Line 15:  and that's why we just choose one
: to always normalize to
> remove this, covered by the paragraph below.
Done


http://gerrit.cloudera.org:8080/#/c/8818/6//COMMIT_MSG@19
PS6, Line 19: .
> ... to single quotes
Done


http://gerrit.cloudera.org:8080/#/c/8818/6/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/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@41
PS6, Line 41:  It is not always possible to determine if a string "should" be
:   // single or double quoted(e.g. concat('a', "b")) and that's 
why we just choose one
:   // to always normalize to.
> This second sentence is not necessary, this is explained better in the larg
Done


http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@44
PS6, Line 44:   private final String escaped_single_quoted_value_;
> Let's not store this value and instead compute it on-the-fly.
Done. By the way, would you tell me the reason why you don't want to keep this? 
You may know I intended to avoid any change of the redundant computation. I 
think the escaped value is always valid since value_ is immutable.


http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@99
PS6, Line 99: .
:   // Here is a background why we should deploy single quotes as a 
default
> Its nice to keep comments short, so this can just be reduced to 'because:'
Done


http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@103
PS6, Line 103: It is easy to know if a given string is wrapped by single/double 
quotes, but
> Similarly, in the interest of being concise, this can be removed.
Done


http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@110
PS6, Line 110:   private String getEscapedSingleQuotedValue() {
> getNormalizedValue()
Done


http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@211
PS6, Line 211: + escaped_single_quoted_value_ + "' to number.");
> let's keep value_ here because that was the original value the user passed
Done


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

http://gerrit.cloudera.org:8080/#/c/8818/6/tests/query_test/test_queries.py@273
PS6, Line 273: result = self.execute_query("select 'a\"b'")
> My understanding is that these tests do not cover your new changes. These a
Done


http://gerrit.cloudera.org:8080/#/c/8818/6/tests/query_test/test_queries.py@310
PS6, Line 310:   def test_escaping_string_literal(self, unique_database):
> We should have a more focused test for this in ToSqlTest.java. This fix aff
I agree with your suggestion. ToSqlTest mainly checks the problem. By the way, 
I think the E2E tests are still meaningful to reproduce the reported issues. I 
would like we have the tests are in the both parts.



--
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: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 16 Jan 2018 11:52:04 +
Gerrit-HasComments: Yes

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

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8851/5/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:

http://gerrit.cloudera.org:8080/#/c/8851/5/common/thrift/Frontend.thrift@70
PS5, Line 70: Arguments to getTableNamesAndComments
> I think we can clean up the API a bit. Instead of having getTableNames and
Done



--
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: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Tue, 16 Jan 2018 02:20:00 +
Gerrit-HasComments: Yes

[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 (#6).

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.

On this change, table's comment can be displayed if catalog table
owns Hive metastore table(HMS) object. There is not any HMS call
due to performance concern, so table's comment can be empty even
though table has a comment.

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

Testing:
Adds E2E test. TestShowTables.testTableCommentVisibility

Change-Id: I44f814af05db6f3c027718ade9f474f8b8153bcd
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
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
16 files changed, 206 insertions(+), 70 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/6
--
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: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 

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

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 printing errors column-wise instead
of row-wise.

Testing:
Add two test cases:
- TestWrongFileOffset.test_invalid_int
- TestWrongFileOffset.test_invalid_int_gzip

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/invalid_int.csv
A testdata/data/invalid_int.csv.gz
M tests/query_test/test_scanners.py
8 files changed, 177 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/8747/7
--
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: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 7:

I appreciate all your reviews!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14d56ffb8fab256f3f66a2669271fd4b3c50cc29
Gerrit-Change-Number: 8893
Gerrit-PatchSet: 7
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 11 Jan 2018 04:32:38 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 4: Code-Review-1

(1 comment)

Performance comparison: linear scan vs. using bit-map

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

http://gerrit.cloudera.org:8080/#/c/8900/4/be/src/exprs/string-functions-ir.cc@625
PS4, Line 625:   for (char const *c = start_ptr; c < end_ptr; c++) {
> I think this is a re-implementation of be/src/gutil/strings/escaping.cc:Bac
Thanks for the point. Let me check performance and the feasibility.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84c3e0ded26f6eb20794c38b75be9b25cd111e4b
Gerrit-Change-Number: 8900
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 10 Jan 2018 13:40:51 +
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 12:

I appreciate all your reviews!
@John, let me provide a draft of the doc soon.


--
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: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 10 Jan 2018 13:35:15 +
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 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8818/5/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/5/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@120
PS5, Line 120: sb.append("\"");
> Just asking: Wouldn't this be more readable to say sb.append('"')
You're right. It's more readable! Thanks.


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

http://gerrit.cloudera.org:8080/#/c/8818/5/tests/query_test/test_queries.py@272
PS5, Line 272:   def prepare(self, unique_database):
> nit: as the only usage of prepare() is in the last test of this file, I'd m
Done


http://gerrit.cloudera.org:8080/#/c/8818/5/tests/query_test/test_queries.py@297
PS5, Line 297: result = self.execute_query("select '\\\''")
> I'm a bit confused of all these backslashes and single-double quotes to be
I still think the current result is valid. The single quote is already escaped, 
so there is no additional escaping.The result "a single quote character' should 
be a result of select '\''

Regarding the line 301, the single quote should be escaped explicitly. The 
result should be same as above. Currently Impala returns empty result for the 
query in the line 301.



--
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: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 10 Jan 2018 13:32:44 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Thomas Tauber-Marshall, 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 (#6).

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:
- toSql() always returns strings that are single quoted, resulting in
improper escaping in the output if the original string was actually
double quoted.
- It is not always possible to determine if a string "should" be single
or double quoted, eg concat('a', "b") and that's why we just choose one
to always normalize to.

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, 110 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/8818/6
--
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: 6
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

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

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8851/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java
File fe/src/main/java/org/apache/impala/service/JniFrontend.java:

http://gerrit.cloudera.org:8080/#/c/8851/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java@304
PS4, Line 304: for (Table table: tables) {
 : org.apache.hadoop.hive.metastore.api.Table msTable
 :   = table.getMetaStoreTable(msClient);
 : if (msTable != null) {
 :   String comment = 
msTable.getParameters().get("comment");
 :   comments.add(comment != null ? comment : "");
 : }
 :   }
> 1. You can guarantee that by forcing table metadata to be loaded for a tabl
Okay, thanks for the answer.



--
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: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Wed, 10 Jan 2018 12:54:11 +
Gerrit-HasComments: Yes

[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 (#5).

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.

On this change, table's comment can be displayed if catalog table
owns Hive metastore table(HMS) object. There is not any HMS call
due to performance concern, so table's comment can be empty even
though table has a comment.

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

Testing:
Adds 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, 149 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8851/5
--
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: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kim Jin Chul 

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

[Kim Jin Chul (Code Review)]

Hello Thomas Tauber-Marshall, 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 (#5).

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:
- toSql() always returns strings that are single quoted, resulting in
improper escaping in the output if the original string was actually
double quoted.
- It is not always possible to determine if a string "should" be single
or double quoted, eg concat('a', "b") and that's why we just choose one
to always normalize to.

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, 115 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/8818/5
--
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: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

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

(7 comments)

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

http://gerrit.cloudera.org:8080/#/c/8818/2//COMMIT_MSG@7
PS2, Line 7: wronly
> wrongly
Done


http://gerrit.cloudera.org:8080/#/c/8818/2//COMMIT_MSG@10
PS2, Line 10: There are some holes in escaping the string literal
> Can you expand on this a little more, eg mention that:
Done


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@39
PS2, Line 39:
> Add a comment explaining what this is.
Done


http://gerrit.cloudera.org:8080/#/c/8818/2/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@104
PS2, Line 104: ression rewritt
> You might name it something like "getEscapedSingleQuotedValue" to make it m
Done


http://gerrit.cloudera.org:8080/#/c/8818/2/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@105
PS2, Line 105:  2. Single quotes are closer to the SQL standard.
> Since getEscapedValue() is called in the constructor, this will always be t
Done


http://gerrit.cloudera.org:8080/#/c/8818/2/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@121
PS2, Line 121:   s
> nit: ++i
Done


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@315
PS2, Line 315: result = self.execute_query("select concat(\"a'b\", 
'c\"d')")
> We should use the unique_database test fixture instead of this approach. E.
Done



--
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: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 08 Jan 2018 02:31:43 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Thomas Tauber-Marshall, 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 (#4).

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:
- toSql() always returns strings that are single quoted, resulting in
improper escaping in the output if the original string was actually
double quoted.
- It is not always possible to determine if a string "should" be single
or double quoted, eg concat('a', "b") and that's why we just choose one
to always normalize to.

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, 113 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/8818/4
--
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: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

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

[Kim Jin Chul (Code Review)]

Hello Thomas Tauber-Marshall, 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 (#3).

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:
- toSql() always returns strings that are single quoted, resulting in
improper escaping in the output if the original string was actually
double quoted.
- It is not always possible to determine if a string "should" be single
or double quoted, eg concat('a', "b") and that's why we just choose one
to always normalize to.

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, 115 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/8818/3
--
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: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

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

Tho examples for INSERT/UPSERT are added to the commit message.

 > Can you also an example of two of the new allowed syntax? Thanks


--
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: 9
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Sun, 07 Jan 2018 07:41:39 +
Gerrit-HasComments: No

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

[Kim Jin Chul (Code Review)]

Hello Attila Jeges, Tim Armstrong, Alex Behm, Impala Public Jenkins,

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

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

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

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

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

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

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

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


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/8893/5
--
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: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 4:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8893/4/testdata/workloads/functional-query/queries/QueryTest/exprs.test@2922
PS4, Line 2922: select murmur_hash(NULL)
> Tests like this belong in expr-test.cc
Done, thanks for your advice.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14d56ffb8fab256f3f66a2669271fd4b3c50cc29
Gerrit-Change-Number: 8893
Gerrit-PatchSet: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sun, 07 Jan 2018 07:11:39 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Tianyi Wang, Jim Apple, Tim Armstrong,

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

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

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

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

IMPALA-3282: Adds regexp_escape built-in function

Escapes the following special characters in RE2 library:
.\+*?[^]$(){}=!<>|:-

Testing:
Add some unit tests into ExprTest.StringRegexpFunctions
Add some E2E tests into exprs.test

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
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
5 files changed, 86 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/8900/4
--
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: 4
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 2:

(7 comments)

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

http://gerrit.cloudera.org:8080/#/c/8900/2//COMMIT_MSG@10
PS2, Line 10: ".*\\+?^[](){}$!=:-#\n\r\t\v "
> Where does this list come from? Impala uses RE2 syntax, which does not esca
I've collected the special characters again.


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

http://gerrit.cloudera.org:8080/#/c/8900/2/be/src/exprs/expr-test.cc@4157
PS2, Line 4157:   TestStringValue("regexp_escape('Hello.world')", 
"Hello\\.world");
> I think the examples in this file could be easier for a reader to understan
Done. Thanks for the information.


http://gerrit.cloudera.org:8080/#/c/8900/2/be/src/exprs/expr-test.cc@4159
PS2, Line 4159:   TestStringValue("regexp_escape('Helloworld')", 
"Helloworld");
> It seems that the parameter to the regexp_escape function is escaped once s
Done. I've added a comment.


http://gerrit.cloudera.org:8080/#/c/8900/2/be/src/exprs/expr-test.cc@4185
PS2, Line 4185:   
TestStringValue("regexp_escape('a.b*cd+e?f^g[h]i(j)k{l}m$n!o=p:q-r#s\nt\ru\tv\vw
 x"
> We should also directly test that the escaping is correct for our other reg
Done. I've added some mixed case with other regexp_*.


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

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


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


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



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84c3e0ded26f6eb20794c38b75be9b25cd111e4b
Gerrit-Change-Number: 8900
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 06 Jan 2018 13:10:19 +
Gerrit-HasComments: Yes

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

[Kim Jin Chul (Code Review)]

Hello Tianyi Wang, Jim Apple, Tim Armstrong,

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

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

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

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

IMPALA-3282: Adds regexp_escape built-in function

Escapes the following special characters:
.\+*?[^]$(){}=!<>|:-

Testing:
Add some unit tests into ExprTest.StringRegexpFunctions
Add some E2E tests into exprs.test

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
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
5 files changed, 81 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/00/8900/3
--
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: 3
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

[Kim Jin Chul (Code Review)]

Hello Attila Jeges, Tim Armstrong,

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

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

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

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

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

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

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

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


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

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

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 2:

(3 comments)

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

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


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

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


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



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

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

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

[Kim Jin Chul (Code Review)]

Hello Attila Jeges, Tim Armstrong,

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

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

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

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

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

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

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

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


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

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

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

[Kim Jin Chul (Code Review)]

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

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

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

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

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

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

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

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


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

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

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

[Kim Jin Chul (Code Review)]

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

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


Patch Set 8:

(4 comments)

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

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


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


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


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

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



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

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

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

[Kim Jin Chul (Code Review)]

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

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


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

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

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

[Kim Jin Chul (Code Review)]

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


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

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

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



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

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

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

[Kim Jin Chul (Code Review)]

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

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

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

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


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

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