Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-07-06 Thread Vihang Karajgaonkar 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/#review179819
---


Fix it, then Ship it!





common/src/java/org/apache/hive/common/util/HiveStringUtils.java
Lines 1149-1150 (patched)


I didn't know that BeeLine supported "#" as a comment prefix (and I don't 
see any documentation for that on wiki as well). While BeeLine may be 
supporting it and we should probably not change that now, I am not sure if we 
should keep it as well here since now it will apply to all the other JDBC 
clients. Should we rather stick to the SQL92 comment prefix only here and let 
BeeLine keep using "#" in addition to "--"? What do you think?



common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java
Lines 112 (patched)


nit, spellcheck comments


- Vihang Karajgaonkar


On July 5, 2017, 6:09 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60445/
> ---
> 
> (Updated July 5, 2017, 6:09 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Bugs: HIVE-16935
> https://issues.apache.org/jira/browse/HIVE-16935
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> We strip sql comments from a command string. The stripped command is use to 
> determine which
> CommandProcessor will execute the command. If the CommandProcessorFactory 
> does not select a special
> CommandProcessor then we execute the original unstripped command so that the 
> sql parser can remove comments.
> Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine 
> to call it from there
> Add a better test with separate tokens for "set role" in 
> TestCommandProcessorFactory.
> Add a test case for comment removal in set_processor_namespaces.q using an 
> indented comment as
> unindented comments are removed by the test driver.
> 
> Change-Id: I166dc1e7588ec9802ba373d88e69e716aecd33c2
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 
> 3b2d72ed79771e6198e62c47060a7f80665dbcb2 
>   beeline/src/test/org/apache/hive/beeline/TestCommands.java 
> 04c939a04c7a56768286743c2bb9c9797507e3aa 
>   cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 
> 27fd66d35ea89b0de0d17763625fbf564584fcca 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
> 4a6413a7c376ffb4de6d20d24707ac5bf89ebc0c 
>   common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
> 6bd7037152c6f809daec8af42708693c05fe00cf 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  21bdcf44436a02b11f878fa439e916d4b55ac63d 
>   ql/src/test/queries/clientpositive/set_processor_namespaces.q 
> 612807f0c871b1881446d088e1c2c399d1afe970 
>   ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
> c05ce4d61d00a9ee6671d97f2fd178f18d44cc8c 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  2dd90b69b3bf789b1a3928129cf801b17884033f 
> 
> 
> Diff: https://reviews.apache.org/r/60445/diff/4/
> 
> 
> Testing
> ---
> 
> Added new test case.
> Hand tested with Hue and Jdbc.
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-07-06 Thread Sergio Pena 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/#review179821
---


Ship it!




Great, this looks very good.

- Sergio Pena


On July 5, 2017, 6:09 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60445/
> ---
> 
> (Updated July 5, 2017, 6:09 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Bugs: HIVE-16935
> https://issues.apache.org/jira/browse/HIVE-16935
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> We strip sql comments from a command string. The stripped command is use to 
> determine which
> CommandProcessor will execute the command. If the CommandProcessorFactory 
> does not select a special
> CommandProcessor then we execute the original unstripped command so that the 
> sql parser can remove comments.
> Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine 
> to call it from there
> Add a better test with separate tokens for "set role" in 
> TestCommandProcessorFactory.
> Add a test case for comment removal in set_processor_namespaces.q using an 
> indented comment as
> unindented comments are removed by the test driver.
> 
> Change-Id: I166dc1e7588ec9802ba373d88e69e716aecd33c2
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 
> 3b2d72ed79771e6198e62c47060a7f80665dbcb2 
>   beeline/src/test/org/apache/hive/beeline/TestCommands.java 
> 04c939a04c7a56768286743c2bb9c9797507e3aa 
>   cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 
> 27fd66d35ea89b0de0d17763625fbf564584fcca 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
> 4a6413a7c376ffb4de6d20d24707ac5bf89ebc0c 
>   common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
> 6bd7037152c6f809daec8af42708693c05fe00cf 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  21bdcf44436a02b11f878fa439e916d4b55ac63d 
>   ql/src/test/queries/clientpositive/set_processor_namespaces.q 
> 612807f0c871b1881446d088e1c2c399d1afe970 
>   ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
> c05ce4d61d00a9ee6671d97f2fd178f18d44cc8c 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  2dd90b69b3bf789b1a3928129cf801b17884033f 
> 
> 
> Diff: https://reviews.apache.org/r/60445/diff/4/
> 
> 
> Testing
> ---
> 
> Added new test case.
> Hand tested with Hue and Jdbc.
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-07-05 Thread Andrew Sherman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/
---

(Updated July 5, 2017, 6:09 p.m.)


Review request for hive and Sahil Takiar.


Changes
---

Use Guava's Splitter


Bugs: HIVE-16935
https://issues.apache.org/jira/browse/HIVE-16935


Repository: hive-git


Description
---

We strip sql comments from a command string. The stripped command is use to 
determine which
CommandProcessor will execute the command. If the CommandProcessorFactory does 
not select a special
CommandProcessor then we execute the original unstripped command so that the 
sql parser can remove comments.
Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine to 
call it from there
Add a better test with separate tokens for "set role" in 
TestCommandProcessorFactory.
Add a test case for comment removal in set_processor_namespaces.q using an 
indented comment as
unindented comments are removed by the test driver.

Change-Id: I166dc1e7588ec9802ba373d88e69e716aecd33c2


Diffs (updated)
-

  beeline/src/java/org/apache/hive/beeline/Commands.java 
3b2d72ed79771e6198e62c47060a7f80665dbcb2 
  beeline/src/test/org/apache/hive/beeline/TestCommands.java 
04c939a04c7a56768286743c2bb9c9797507e3aa 
  cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 
27fd66d35ea89b0de0d17763625fbf564584fcca 
  common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
4a6413a7c376ffb4de6d20d24707ac5bf89ebc0c 
  common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
6bd7037152c6f809daec8af42708693c05fe00cf 
  
ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
 21bdcf44436a02b11f878fa439e916d4b55ac63d 
  ql/src/test/queries/clientpositive/set_processor_namespaces.q 
612807f0c871b1881446d088e1c2c399d1afe970 
  ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
c05ce4d61d00a9ee6671d97f2fd178f18d44cc8c 
  
service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
 2dd90b69b3bf789b1a3928129cf801b17884033f 


Diff: https://reviews.apache.org/r/60445/diff/4/

Changes: https://reviews.apache.org/r/60445/diff/3-4/


Testing
---

Added new test case.
Hand tested with Hue and Jdbc.


Thanks,

Andrew Sherman

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-07-02 Thread Sahil Takiar 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/#review179480
---


Fix it, then Ship it!




LGTM, one minor comment.


common/src/java/org/apache/hive/common/util/HiveStringUtils.java
Lines 1087 (patched)


would recommend using Guava's Splitter


- Sahil Takiar


On June 30, 2017, 4:18 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60445/
> ---
> 
> (Updated June 30, 2017, 4:18 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Bugs: HIVE-16935
> https://issues.apache.org/jira/browse/HIVE-16935
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> We strip sql comments from a command string. The stripped command is use to 
> determine which
> CommandProcessor will execute the command. If the CommandProcessorFactory 
> does not select a special
> CommandProcessor then we execute the original unstripped command so that the 
> sql parser can remove comments.
> Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine 
> to call it from there
> Add a better test with separate tokens for "set role" in 
> TestCommandProcessorFactory.
> Add a test case for comment removal in set_processor_namespaces.q using an 
> indented comment as
> unindented comments are removed by the test driver.
> 
> Change-Id: I166dc1e7588ec9802ba373d88e69e716aecd33c2
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 
> 3b2d72ed79771e6198e62c47060a7f80665dbcb2 
>   beeline/src/test/org/apache/hive/beeline/TestCommands.java 
> 04c939a04c7a56768286743c2bb9c9797507e3aa 
>   cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 
> 27fd66d35ea89b0de0d17763625fbf564584fcca 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
> 4a6413a7c376ffb4de6d20d24707ac5bf89ebc0c 
>   common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
> 6bd7037152c6f809daec8af42708693c05fe00cf 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  21bdcf44436a02b11f878fa439e916d4b55ac63d 
>   ql/src/test/queries/clientpositive/set_processor_namespaces.q 
> 612807f0c871b1881446d088e1c2c399d1afe970 
>   ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
> c05ce4d61d00a9ee6671d97f2fd178f18d44cc8c 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  2dd90b69b3bf789b1a3928129cf801b17884033f 
> 
> 
> Diff: https://reviews.apache.org/r/60445/diff/3/
> 
> 
> Testing
> ---
> 
> Added new test case.
> Hand tested with Hue and Jdbc.
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-06-30 Thread Peter Vary 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/#review179380
---


Ship it!




Ship It!

- Peter Vary


On June 30, 2017, 4:18 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60445/
> ---
> 
> (Updated June 30, 2017, 4:18 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Bugs: HIVE-16935
> https://issues.apache.org/jira/browse/HIVE-16935
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> We strip sql comments from a command string. The stripped command is use to 
> determine which
> CommandProcessor will execute the command. If the CommandProcessorFactory 
> does not select a special
> CommandProcessor then we execute the original unstripped command so that the 
> sql parser can remove comments.
> Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine 
> to call it from there
> Add a better test with separate tokens for "set role" in 
> TestCommandProcessorFactory.
> Add a test case for comment removal in set_processor_namespaces.q using an 
> indented comment as
> unindented comments are removed by the test driver.
> 
> Change-Id: I166dc1e7588ec9802ba373d88e69e716aecd33c2
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 
> 3b2d72ed79771e6198e62c47060a7f80665dbcb2 
>   beeline/src/test/org/apache/hive/beeline/TestCommands.java 
> 04c939a04c7a56768286743c2bb9c9797507e3aa 
>   cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 
> 27fd66d35ea89b0de0d17763625fbf564584fcca 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
> 4a6413a7c376ffb4de6d20d24707ac5bf89ebc0c 
>   common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
> 6bd7037152c6f809daec8af42708693c05fe00cf 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  21bdcf44436a02b11f878fa439e916d4b55ac63d 
>   ql/src/test/queries/clientpositive/set_processor_namespaces.q 
> 612807f0c871b1881446d088e1c2c399d1afe970 
>   ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
> c05ce4d61d00a9ee6671d97f2fd178f18d44cc8c 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  2dd90b69b3bf789b1a3928129cf801b17884033f 
> 
> 
> Diff: https://reviews.apache.org/r/60445/diff/3/
> 
> 
> Testing
> ---
> 
> Added new test case.
> Hand tested with Hue and Jdbc.
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-06-30 Thread Andrew Sherman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/
---

(Updated June 30, 2017, 4:18 p.m.)


Review request for hive and Sahil Takiar.


Bugs: HIVE-16935
https://issues.apache.org/jira/browse/HIVE-16935


Repository: hive-git


Description (updated)
---

We strip sql comments from a command string. The stripped command is use to 
determine which
CommandProcessor will execute the command. If the CommandProcessorFactory does 
not select a special
CommandProcessor then we execute the original unstripped command so that the 
sql parser can remove comments.
Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine to 
call it from there
Add a better test with separate tokens for "set role" in 
TestCommandProcessorFactory.
Add a test case for comment removal in set_processor_namespaces.q using an 
indented comment as
unindented comments are removed by the test driver.

Change-Id: I166dc1e7588ec9802ba373d88e69e716aecd33c2


Diffs (updated)
-

  beeline/src/java/org/apache/hive/beeline/Commands.java 
3b2d72ed79771e6198e62c47060a7f80665dbcb2 
  beeline/src/test/org/apache/hive/beeline/TestCommands.java 
04c939a04c7a56768286743c2bb9c9797507e3aa 
  cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 
27fd66d35ea89b0de0d17763625fbf564584fcca 
  common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
4a6413a7c376ffb4de6d20d24707ac5bf89ebc0c 
  common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
6bd7037152c6f809daec8af42708693c05fe00cf 
  
ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
 21bdcf44436a02b11f878fa439e916d4b55ac63d 
  ql/src/test/queries/clientpositive/set_processor_namespaces.q 
612807f0c871b1881446d088e1c2c399d1afe970 
  ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
c05ce4d61d00a9ee6671d97f2fd178f18d44cc8c 
  
service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
 2dd90b69b3bf789b1a3928129cf801b17884033f 


Diff: https://reviews.apache.org/r/60445/diff/3/

Changes: https://reviews.apache.org/r/60445/diff/2-3/


Testing
---

Added new test case.
Hand tested with Hue and Jdbc.


Thanks,

Andrew Sherman

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-06-30 Thread Peter Vary 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/#review179369
---



Thanks for the fast update! LGTM, just one little nit :)

Peter


common/src/java/org/apache/hive/common/util/HiveStringUtils.java
Lines 1101-1103 (original), 1101-1103 (patched)


nit: I think your new javadoc is good, so we can remove this. What do you 
think?


- Peter Vary


On June 30, 2017, 12:30 a.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60445/
> ---
> 
> (Updated June 30, 2017, 12:30 a.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Bugs: HIVE-16935
> https://issues.apache.org/jira/browse/HIVE-16935
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> We strip sql comments from a command string. The stripped command is use to 
> determine which
> CommandProcessor will execute the command. If the CommandProcessorFactory 
> does not select a special
> CommandProcessor then we execute the original unstripped command so that the 
> sql parser can remove comments.
> Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine 
> to call it from there
> Add a better test with separate tokens for "set role" in 
> TestCommandProcessorFactory.
> Add a test case for comment removal in set_processor_namespaces.q  using an 
> indented comment as
> unindented comments are removed by the test driver.
> 
> Change-Id: I166dc1e7588ec9802ba373d88e69e716aecd33c2
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 
> 3b2d72ed79771e6198e62c47060a7f80665dbcb2 
>   beeline/src/test/org/apache/hive/beeline/TestCommands.java 
> 04c939a04c7a56768286743c2bb9c9797507e3aa 
>   cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 
> 27fd66d35ea89b0de0d17763625fbf564584fcca 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
> 4a6413a7c376ffb4de6d20d24707ac5bf89ebc0c 
>   common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
> 6bd7037152c6f809daec8af42708693c05fe00cf 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  21bdcf44436a02b11f878fa439e916d4b55ac63d 
>   ql/src/test/queries/clientpositive/set_processor_namespaces.q 
> 612807f0c871b1881446d088e1c2c399d1afe970 
>   ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
> c05ce4d61d00a9ee6671d97f2fd178f18d44cc8c 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  2dd90b69b3bf789b1a3928129cf801b17884033f 
> 
> 
> Diff: https://reviews.apache.org/r/60445/diff/2/
> 
> 
> Testing
> ---
> 
> Added new test case.
> Hand tested with Hue and Jdbc.
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-06-29 Thread Andrew Sherman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/
---

(Updated June 30, 2017, 12:30 a.m.)


Review request for hive and Sahil Takiar.


Bugs: HIVE-16935
https://issues.apache.org/jira/browse/HIVE-16935


Repository: hive-git


Description (updated)
---

We strip sql comments from a command string. The stripped command is use to 
determine which
CommandProcessor will execute the command. If the CommandProcessorFactory does 
not select a special
CommandProcessor then we execute the original unstripped command so that the 
sql parser can remove comments.
Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine to 
call it from there
Add a better test with separate tokens for "set role" in 
TestCommandProcessorFactory.
Add a test case for comment removal in set_processor_namespaces.q  using an 
indented comment as
unindented comments are removed by the test driver.

Change-Id: I166dc1e7588ec9802ba373d88e69e716aecd33c2


Diffs (updated)
-

  beeline/src/java/org/apache/hive/beeline/Commands.java 
3b2d72ed79771e6198e62c47060a7f80665dbcb2 
  beeline/src/test/org/apache/hive/beeline/TestCommands.java 
04c939a04c7a56768286743c2bb9c9797507e3aa 
  cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 
27fd66d35ea89b0de0d17763625fbf564584fcca 
  common/src/java/org/apache/hive/common/util/HiveStringUtils.java 
4a6413a7c376ffb4de6d20d24707ac5bf89ebc0c 
  common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
6bd7037152c6f809daec8af42708693c05fe00cf 
  
ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
 21bdcf44436a02b11f878fa439e916d4b55ac63d 
  ql/src/test/queries/clientpositive/set_processor_namespaces.q 
612807f0c871b1881446d088e1c2c399d1afe970 
  ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
c05ce4d61d00a9ee6671d97f2fd178f18d44cc8c 
  
service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
 2dd90b69b3bf789b1a3928129cf801b17884033f 


Diff: https://reviews.apache.org/r/60445/diff/2/

Changes: https://reviews.apache.org/r/60445/diff/1-2/


Testing
---

Added new test case.
Hand tested with Hue and Jdbc.


Thanks,

Andrew Sherman

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-06-29 Thread Andrew Sherman 


> On June 28, 2017, 10:39 a.m., Peter Vary wrote:
> > Hi,
> > 
> > Thanks for the change.
> > Overall looks good.
> > After running Yetus, it come up with the following warnings:
> > 
> > ./common/src/java/org/apache/hive/common/util/HiveStringUtils.java:1077:  
> > /**: warning: First sentence should end with a period.
> > ./common/src/java/org/apache/hive/common/util/HiveStringUtils.java:1088:
> > int[] startQuote = { -1 };:25: warning: '{' is followed by whitespace.
> > ./common/src/java/org/apache/hive/common/util/HiveStringUtils.java:1105:
> > if (line == null || line.isEmpty()): warning: 'if' construct must use '{}'s.
> > ./common/src/java/org/apache/hive/common/util/HiveStringUtils.java:1107:
> > if (startQuote[0] == -1 && isComment(line)): warning: 'if' construct must 
> > use '{}'s.
> > ./cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java:126:String 
> > cmd_trimmed = removeComments(cmd).trim();:12: warning: Name 'cmd_trimmed' 
> > must match pattern '^[a-z][a-zA-Z0-9]*$'.
> > 
> > I personally do not like to import methods:
> > https://stackoverflow.com/questions/420791/what-is-a-good-use-case-for-static-import-of-methods
> > 
> > Thanks for the patch,
> > Peter

Thanks for the review.
Thanks for letting me know about Yetus. I was able to follow the instructiona 
at https://cwiki.apache.org/confluence/display/Hive/Running+Yetus
I have fixed the checkstyle problems except for the name 'cmd_trimmed'. 
Changing this causes cascading checkstyle changes which expand the scope of the 
fix too far.
I reduced the usage of static imports but kept it in the test where it makes 
the test more readable.


> On June 28, 2017, 10:39 a.m., Peter Vary wrote:
> > common/src/java/org/apache/hive/common/util/HiveStringUtils.java
> > Lines 1104 (patched)
> > 
> >
> > nit: do we need this as public?

Yes, it is called from org.apache.hive.beeline.Commands


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/#review179088
---


On June 26, 2017, 8:35 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60445/
> ---
> 
> (Updated June 26, 2017, 8:35 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Bugs: HIVE-16935
> https://issues.apache.org/jira/browse/HIVE-16935
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> We strip sql comments from a command string. The stripped command is use to 
> determine which
> CommandProcessor will execute the command. If the CommandProcessorFactory 
> does not select a special
> CommandProcessor then we execute the original unstripped command so that the 
> sql parser can remove comments.
> Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine 
> to call it from there
> Add a better test with separate tokens for "set role" in 
> TestCommandProcessorFactory.
> Add a test case for comment removal in set_processor_namespaces.q using an 
> indented comment as
> unindented comments are removed by the test driver.
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 3b2d72ed79 
>   beeline/src/test/org/apache/hive/beeline/TestCommands.java 04c939a04c 
>   cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 27fd66d35e 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 4a6413a7c3 
>   common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
> 6bd7037152 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  21bdcf4443 
>   ql/src/test/queries/clientpositive/set_processor_namespaces.q 612807f0c8 
>   ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
> c05ce4d61d 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  2dd90b69b3 
> 
> 
> Diff: https://reviews.apache.org/r/60445/diff/1/
> 
> 
> Testing
> ---
> 
> Added new test case.
> Hand tested with Hue and Jdbc.
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-06-29 Thread Andrew Sherman 


> On June 28, 2017, 5:18 a.m., Sahil Takiar wrote:
> > common/src/java/org/apache/hive/common/util/HiveStringUtils.java
> > Lines 1083 (patched)
> > 
> >
> > is this copied from somewhere, or is this logic new?

This code is new.


> On June 28, 2017, 5:18 a.m., Sahil Takiar wrote:
> > common/src/java/org/apache/hive/common/util/HiveStringUtils.java
> > Lines 1104 (patched)
> > 
> >
> > are there any changes to this method, or was it just moved from 
> > `Commands.java`

This code was moved. Peter requests updated javadoc so I will change that in  
the next revision.


> On June 28, 2017, 5:18 a.m., Sahil Takiar wrote:
> > common/src/java/org/apache/hive/common/util/HiveStringUtils.java
> > Lines 1136 (patched)
> > 
> >
> > new logic, or copied from beeline?

Moved from beeline


> On June 28, 2017, 5:18 a.m., Sahil Takiar wrote:
> > common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java
> > Lines 113-115 (patched)
> > 
> >
> > nit: remove extra lines

Fixed in next patch


- Andrew


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/#review179073
---


On June 26, 2017, 8:35 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60445/
> ---
> 
> (Updated June 26, 2017, 8:35 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Bugs: HIVE-16935
> https://issues.apache.org/jira/browse/HIVE-16935
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> We strip sql comments from a command string. The stripped command is use to 
> determine which
> CommandProcessor will execute the command. If the CommandProcessorFactory 
> does not select a special
> CommandProcessor then we execute the original unstripped command so that the 
> sql parser can remove comments.
> Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine 
> to call it from there
> Add a better test with separate tokens for "set role" in 
> TestCommandProcessorFactory.
> Add a test case for comment removal in set_processor_namespaces.q using an 
> indented comment as
> unindented comments are removed by the test driver.
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 3b2d72ed79 
>   beeline/src/test/org/apache/hive/beeline/TestCommands.java 04c939a04c 
>   cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 27fd66d35e 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 4a6413a7c3 
>   common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
> 6bd7037152 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  21bdcf4443 
>   ql/src/test/queries/clientpositive/set_processor_namespaces.q 612807f0c8 
>   ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
> c05ce4d61d 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  2dd90b69b3 
> 
> 
> Diff: https://reviews.apache.org/r/60445/diff/1/
> 
> 
> Testing
> ---
> 
> Added new test case.
> Hand tested with Hue and Jdbc.
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-06-28 Thread Peter Vary 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/#review179088
---



Hi,

Thanks for the change.
Overall looks good.
After running Yetus, it come up with the following warnings:

./common/src/java/org/apache/hive/common/util/HiveStringUtils.java:1077:  /**: 
warning: First sentence should end with a period.
./common/src/java/org/apache/hive/common/util/HiveStringUtils.java:1088:
int[] startQuote = { -1 };:25: warning: '{' is followed by whitespace.
./common/src/java/org/apache/hive/common/util/HiveStringUtils.java:1105:if 
(line == null || line.isEmpty()): warning: 'if' construct must use '{}'s.
./common/src/java/org/apache/hive/common/util/HiveStringUtils.java:1107:if 
(startQuote[0] == -1 && isComment(line)): warning: 'if' construct must use 
'{}'s.
./cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java:126:String 
cmd_trimmed = removeComments(cmd).trim();:12: warning: Name 'cmd_trimmed' must 
match pattern '^[a-z][a-zA-Z0-9]*$'.

I personally do not like to import methods:
https://stackoverflow.com/questions/420791/what-is-a-good-use-case-for-static-import-of-methods

Thanks for the patch,
Peter


common/src/java/org/apache/hive/common/util/HiveStringUtils.java
Lines 1101-1103 (patched)


nit: Could we change it to valid javadoc?



common/src/java/org/apache/hive/common/util/HiveStringUtils.java
Lines 1104 (patched)


nit: do we need this as public?


- Peter Vary


On June 26, 2017, 8:35 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60445/
> ---
> 
> (Updated June 26, 2017, 8:35 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Bugs: HIVE-16935
> https://issues.apache.org/jira/browse/HIVE-16935
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> We strip sql comments from a command string. The stripped command is use to 
> determine which
> CommandProcessor will execute the command. If the CommandProcessorFactory 
> does not select a special
> CommandProcessor then we execute the original unstripped command so that the 
> sql parser can remove comments.
> Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine 
> to call it from there
> Add a better test with separate tokens for "set role" in 
> TestCommandProcessorFactory.
> Add a test case for comment removal in set_processor_namespaces.q using an 
> indented comment as
> unindented comments are removed by the test driver.
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 3b2d72ed79 
>   beeline/src/test/org/apache/hive/beeline/TestCommands.java 04c939a04c 
>   cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 27fd66d35e 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 4a6413a7c3 
>   common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
> 6bd7037152 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  21bdcf4443 
>   ql/src/test/queries/clientpositive/set_processor_namespaces.q 612807f0c8 
>   ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
> c05ce4d61d 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  2dd90b69b3 
> 
> 
> Diff: https://reviews.apache.org/r/60445/diff/1/
> 
> 
> Testing
> ---
> 
> Added new test case.
> Hand tested with Hue and Jdbc.
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Re: Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-06-27 Thread Sahil Takiar 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/#review179073
---



overall, looks good, just a few questions


common/src/java/org/apache/hive/common/util/HiveStringUtils.java
Lines 1083 (patched)


is this copied from somewhere, or is this logic new?



common/src/java/org/apache/hive/common/util/HiveStringUtils.java
Lines 1104 (patched)


are there any changes to this method, or was it just moved from 
`Commands.java`



common/src/java/org/apache/hive/common/util/HiveStringUtils.java
Lines 1136 (patched)


new logic, or copied from beeline?



common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java
Lines 113-115 (patched)


nit: remove extra lines


- Sahil Takiar


On June 26, 2017, 8:35 p.m., Andrew Sherman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60445/
> ---
> 
> (Updated June 26, 2017, 8:35 p.m.)
> 
> 
> Review request for hive and Sahil Takiar.
> 
> 
> Bugs: HIVE-16935
> https://issues.apache.org/jira/browse/HIVE-16935
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> We strip sql comments from a command string. The stripped command is use to 
> determine which
> CommandProcessor will execute the command. If the CommandProcessorFactory 
> does not select a special
> CommandProcessor then we execute the original unstripped command so that the 
> sql parser can remove comments.
> Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine 
> to call it from there
> Add a better test with separate tokens for "set role" in 
> TestCommandProcessorFactory.
> Add a test case for comment removal in set_processor_namespaces.q using an 
> indented comment as
> unindented comments are removed by the test driver.
> 
> 
> Diffs
> -
> 
>   beeline/src/java/org/apache/hive/beeline/Commands.java 3b2d72ed79 
>   beeline/src/test/org/apache/hive/beeline/TestCommands.java 04c939a04c 
>   cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 27fd66d35e 
>   common/src/java/org/apache/hive/common/util/HiveStringUtils.java 4a6413a7c3 
>   common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
> 6bd7037152 
>   
> ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
>  21bdcf4443 
>   ql/src/test/queries/clientpositive/set_processor_namespaces.q 612807f0c8 
>   ql/src/test/results/clientpositive/set_processor_namespaces.q.out 
> c05ce4d61d 
>   
> service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
>  2dd90b69b3 
> 
> 
> Diff: https://reviews.apache.org/r/60445/diff/1/
> 
> 
> Testing
> ---
> 
> Added new test case.
> Hand tested with Hue and Jdbc.
> 
> 
> Thanks,
> 
> Andrew Sherman
> 
>

Review Request 60445: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run.

2017-06-26 Thread Andrew Sherman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60445/
---

Review request for hive and Sahil Takiar.


Bugs: HIVE-16935
https://issues.apache.org/jira/browse/HIVE-16935


Repository: hive-git


Description
---

We strip sql comments from a command string. The stripped command is use to 
determine which
CommandProcessor will execute the command. If the CommandProcessorFactory does 
not select a special
CommandProcessor then we execute the original unstripped command so that the 
sql parser can remove comments.
Move BeeLine's comment stripping code to HiveStringUtils and change BeeLine to 
call it from there
Add a better test with separate tokens for "set role" in 
TestCommandProcessorFactory.
Add a test case for comment removal in set_processor_namespaces.q using an 
indented comment as
unindented comments are removed by the test driver.


Diffs
-

  beeline/src/java/org/apache/hive/beeline/Commands.java 3b2d72ed79 
  beeline/src/test/org/apache/hive/beeline/TestCommands.java 04c939a04c 
  cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java 27fd66d35e 
  common/src/java/org/apache/hive/common/util/HiveStringUtils.java 4a6413a7c3 
  common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java 
6bd7037152 
  
ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
 21bdcf4443 
  ql/src/test/queries/clientpositive/set_processor_namespaces.q 612807f0c8 
  ql/src/test/results/clientpositive/set_processor_namespaces.q.out c05ce4d61d 
  
service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
 2dd90b69b3 


Diff: https://reviews.apache.org/r/60445/diff/1/


Testing
---

Added new test case.
Hand tested with Hue and Jdbc.


Thanks,

Andrew Sherman