> 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)
> > <https://reviews.apache.org/r/60445/diff/1/?file=1763215#file1763215line1104>
> >
> > 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
>
>
