You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Andrew Sherman <as...@cloudera.com> on 2017/06/26 20:35:47 UTC
Review Request 60445: HIVE-16935: Hive should strip comments from
input before choosing which CommandProcessor to run.
-----------------------------------------------------------
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
Re: Review Request 60445: HIVE-16935: Hive should strip comments from
input before choosing which CommandProcessor to run.
Posted by Andrew Sherman <as...@cloudera.com>.
> On June 28, 2017, 5:18 a.m., Sahil Takiar wrote:
> > common/src/java/org/apache/hive/common/util/HiveStringUtils.java
> > Lines 1083 (patched)
> > <https://reviews.apache.org/r/60445/diff/1/?file=1763215#file1763215line1083>
> >
> > 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)
> > <https://reviews.apache.org/r/60445/diff/1/?file=1763215#file1763215line1104>
> >
> > 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)
> > <https://reviews.apache.org/r/60445/diff/1/?file=1763215#file1763215line1136>
> >
> > 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)
> > <https://reviews.apache.org/r/60445/diff/1/?file=1763216#file1763216line113>
> >
> > 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.
Posted by Sahil Takiar <ta...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/60445/#comment253483>
is this copied from somewhere, or is this logic new?
common/src/java/org/apache/hive/common/util/HiveStringUtils.java
Lines 1104 (patched)
<https://reviews.apache.org/r/60445/#comment253484>
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)
<https://reviews.apache.org/r/60445/#comment253485>
new logic, or copied from beeline?
common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java
Lines 113-115 (patched)
<https://reviews.apache.org/r/60445/#comment253486>
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
>
>
Re: Review Request 60445: HIVE-16935: Hive should strip comments from
input before choosing which CommandProcessor to run.
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/60445/#comment254036>
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.
Posted by Sahil Takiar <ta...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/60445/#comment254215>
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.
Posted by Vihang Karajgaonkar <vi...@cloudera.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/60445/#comment254682>
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)
<https://reviews.apache.org/r/60445/#comment254672>
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.
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
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.
Posted by Andrew Sherman <as...@cloudera.com>.
-----------------------------------------------------------
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.
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
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.
Posted by Andrew Sherman <as...@cloudera.com>.
-----------------------------------------------------------
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.
Posted by Andrew Sherman <as...@cloudera.com>.
-----------------------------------------------------------
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.
Posted by Andrew Sherman <as...@cloudera.com>.
> 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
>
>
Re: Review Request 60445: HIVE-16935: Hive should strip comments from
input before choosing which CommandProcessor to run.
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
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
>
>