-----------------------------------------------------------
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)
<https://reviews.apache.org/r/60445/#comment253527>
nit: Could we change it to valid javadoc?
common/src/java/org/apache/hive/common/util/HiveStringUtils.java
Lines 1104 (patched)
<https://reviews.apache.org/r/60445/#comment253528>
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
>
>
