hive-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sp...@apache.org
Subject hive git commit: HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to run. (Andrew Sherman, reviewed by Sahil Takiar, Sergio Pena, Peter Vary)
Date Thu, 06 Jul 2017 20:43:32 GMT
Repository: hive
Updated Branches:
  refs/heads/master 555f00114 -> 5feb499d0


HIVE-16935: Hive should strip comments from input before choosing which CommandProcessor to
run. (Andrew Sherman, reviewed by Sahil Takiar, Sergio Pena, Peter Vary)


Project: http://git-wip-us.apache.org/repos/asf/hive/repo
Commit: http://git-wip-us.apache.org/repos/asf/hive/commit/5feb499d
Tree: http://git-wip-us.apache.org/repos/asf/hive/tree/5feb499d
Diff: http://git-wip-us.apache.org/repos/asf/hive/diff/5feb499d

Branch: refs/heads/master
Commit: 5feb499d07a5e409056c227c6e2774a9161febdf
Parents: 555f001
Author: Andrew Sherman <asherman@cloudera.com>
Authored: Thu Jul 6 15:42:50 2017 -0500
Committer: Sergio Pena <sergio.pena@cloudera.com>
Committed: Thu Jul 6 15:42:50 2017 -0500

----------------------------------------------------------------------
 .../java/org/apache/hive/beeline/Commands.java  | 32 +-------
 .../org/apache/hive/beeline/TestCommands.java   | 29 ++++----
 .../org/apache/hadoop/hive/cli/CliDriver.java   | 10 ++-
 .../hive/common/util/HiveStringUtils.java       | 77 ++++++++++++++++++++
 .../hive/common/util/TestHiveStringUtils.java   | 51 +++++++++++++
 .../processors/TestCommandProcessorFactory.java |  2 +
 .../clientpositive/set_processor_namespaces.q   |  8 ++
 .../set_processor_namespaces.q.out              |  2 +
 .../operation/ExecuteStatementOperation.java    |  8 +-
 9 files changed, 171 insertions(+), 48 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/hive/blob/5feb499d/beeline/src/java/org/apache/hive/beeline/Commands.java
----------------------------------------------------------------------
diff --git a/beeline/src/java/org/apache/hive/beeline/Commands.java b/beeline/src/java/org/apache/hive/beeline/Commands.java
index 3b2d72e..7897e22 100644
--- a/beeline/src/java/org/apache/hive/beeline/Commands.java
+++ b/beeline/src/java/org/apache/hive/beeline/Commands.java
@@ -60,10 +60,10 @@ import org.apache.hadoop.hive.conf.SystemVariables;
 import org.apache.hadoop.hive.conf.VariableSubstitution;
 import org.apache.hadoop.io.IOUtils;
 import org.apache.hive.beeline.logs.BeelineInPlaceUpdateStream;
+import org.apache.hive.common.util.HiveStringUtils;
 import org.apache.hive.jdbc.HiveStatement;
 import org.apache.hive.jdbc.Utils;
 import org.apache.hive.jdbc.Utils.JdbcConnectionParams;
-import com.google.common.annotations.VisibleForTesting;
 import org.apache.hive.jdbc.logs.InPlaceUpdateStream;
 
 public class Commands {
@@ -1068,39 +1068,13 @@ public class Commands {
     return true;
   }
 
-  //startQuote use array type in order to pass int type as input/output parameter.
-  //This method remove comment from current line of a query.
-  //It does not remove comment like strings inside quotes.
-  @VisibleForTesting
-  String removeComments(String line, int[] startQuote) {
-    if (line == null || line.isEmpty()) return line;
-    if (startQuote[0] == -1 && beeLine.isComment(line)) return "";  //assume # can
only be used at the beginning of line.
-    StringBuilder builder = new StringBuilder();
-    for (int index = 0; index < line.length(); index++) {
-      if (startQuote[0] == -1 && index < line.length() - 1 && line.charAt(index)
== '-' && line.charAt(index + 1) =='-') {
-        return builder.toString().trim();
-      }
-
-      char letter = line.charAt(index);
-      if (startQuote[0] == letter && (index == 0 || line.charAt(index -1) != '\\')
) {
-        startQuote[0] = -1; // Turn escape off.
-      } else if (startQuote[0] == -1 && (letter == '\'' || letter == '"') &&
(index == 0 || line.charAt(index -1) != '\\')) {
-        startQuote[0] = letter; // Turn escape on.
-      }
-
-      builder.append(letter);
-    }
-
-    return builder.toString().trim();
-  }
-
   /*
    * Check if the input line is a multi-line command which needs to read further
    */
   public String handleMultiLineCmd(String line) throws IOException {
     //When using -e, console reader is not initialized and command is always a single line
     int[] startQuote = {-1};
-    line = removeComments(line,startQuote);
+    line = HiveStringUtils.removeComments(line, startQuote);
     while (isMultiLine(line) && beeLine.getOpts().isAllowMultiLineCommand()) {
       StringBuilder prompt = new StringBuilder(beeLine.getPrompt());
       if (!beeLine.getOpts().isSilent()) {
@@ -1125,7 +1099,7 @@ public class Commands {
       if (extra == null) { //it happens when using -f and the line of cmds does not end with
;
         break;
       }
-      extra = removeComments(extra,startQuote);
+      extra = HiveStringUtils.removeComments(extra, startQuote);
       if (extra != null && !extra.isEmpty()) {
         line += "\n" + extra;
       }

http://git-wip-us.apache.org/repos/asf/hive/blob/5feb499d/beeline/src/test/org/apache/hive/beeline/TestCommands.java
----------------------------------------------------------------------
diff --git a/beeline/src/test/org/apache/hive/beeline/TestCommands.java b/beeline/src/test/org/apache/hive/beeline/TestCommands.java
index 04c939a..80c01ff 100644
--- a/beeline/src/test/org/apache/hive/beeline/TestCommands.java
+++ b/beeline/src/test/org/apache/hive/beeline/TestCommands.java
@@ -20,29 +20,28 @@ package org.apache.hive.beeline;
 
 import org.junit.Test;
 
+import static org.apache.hive.common.util.HiveStringUtils.removeComments;
 import static org.junit.Assert.assertEquals;
 
 public class TestCommands {
 
   @Test
   public void testLinesEndingWithComments() {
-    BeeLine beeline = new BeeLine();
-    Commands commands = new Commands(beeline);
     int[] escape = {-1};
-    assertEquals("show tables;", commands.removeComments("show tables;",escape));
-    assertEquals("show tables;", commands.removeComments("show tables; --comments",escape));
-    assertEquals("show tables;", commands.removeComments("show tables; -------comments",escape));
-    assertEquals("show tables;", commands.removeComments("show tables; -------comments;one;two;three;;;;",escape));
-    assertEquals("show", commands.removeComments("show-- tables; -------comments",escape));
-    assertEquals("show", commands.removeComments("show --tables; -------comments",escape));
-    assertEquals("s", commands.removeComments("s--how --tables; -------comments",escape));
-    assertEquals("", commands.removeComments("-- show tables; -------comments",escape));
+    assertEquals("show tables;", removeComments("show tables;",escape));
+    assertEquals("show tables;", removeComments("show tables; --comments",escape));
+    assertEquals("show tables;", removeComments("show tables; -------comments",escape));
+    assertEquals("show tables;", removeComments("show tables; -------comments;one;two;three;;;;",escape));
+    assertEquals("show", removeComments("show-- tables; -------comments",escape));
+    assertEquals("show", removeComments("show --tables; -------comments",escape));
+    assertEquals("s", removeComments("s--how --tables; -------comments",escape));
+    assertEquals("", removeComments("-- show tables; -------comments",escape));
 
-    assertEquals("\"show tables\"", commands.removeComments("\"show tables\" --comments",escape));
-    assertEquals("\"show --comments tables\"", commands.removeComments("\"show --comments
tables\" --comments",escape));
-    assertEquals("\"'show --comments' tables\"", commands.removeComments("\"'show --comments'
tables\" --comments",escape));
-    assertEquals("'show --comments tables'", commands.removeComments("'show --comments tables'
--comments",escape));
-    assertEquals("'\"show --comments tables\"'", commands.removeComments("'\"show --comments
tables\"' --comments",escape));
+    assertEquals("\"show tables\"", removeComments("\"show tables\" --comments",escape));
+    assertEquals("\"show --comments tables\"", removeComments("\"show --comments tables\"
--comments",escape));
+    assertEquals("\"'show --comments' tables\"", removeComments("\"'show --comments' tables\"
--comments",escape));
+    assertEquals("'show --comments tables'", removeComments("'show --comments tables' --comments",escape));
+    assertEquals("'\"show --comments tables\"'", removeComments("'\"show --comments tables\"'
--comments",escape));
   }
 }
 

http://git-wip-us.apache.org/repos/asf/hive/blob/5feb499d/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java
----------------------------------------------------------------------
diff --git a/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java b/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java
index 27fd66d..cf06582 100644
--- a/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java
+++ b/cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java
@@ -79,6 +79,7 @@ import org.apache.hadoop.hive.ql.processors.CommandProcessorResponse;
 import org.apache.hadoop.hive.ql.session.SessionState;
 import org.apache.hadoop.hive.ql.session.SessionState.LogHelper;
 import org.apache.hadoop.io.IOUtils;
+import org.apache.hive.common.util.HiveStringUtils;
 import org.apache.hive.common.util.ShutdownHookManager;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -122,7 +123,7 @@ public class CliDriver {
 
     // Flush the print stream, so it doesn't include output from the last command
     ss.err.flush();
-    String cmd_trimmed = cmd.trim();
+    String cmd_trimmed = HiveStringUtils.removeComments(cmd).trim();
     String[] tokens = tokenizeCmd(cmd_trimmed);
     int ret = 0;
 
@@ -181,7 +182,12 @@ public class CliDriver {
     }  else { // local mode
       try {
         CommandProcessor proc = CommandProcessorFactory.get(tokens, (HiveConf) conf);
-        ret = processLocalCmd(cmd, proc, ss);
+        if (proc instanceof Driver) {
+          // Let Driver strip comments using sql parser
+          ret = processLocalCmd(cmd, proc, ss);
+        } else {
+          ret = processLocalCmd(cmd_trimmed, proc, ss);
+        }
       } catch (SQLException e) {
         console.printError("Failed processing command " + tokens[0] + " " + e.getLocalizedMessage(),
           org.apache.hadoop.util.StringUtils.stringifyException(e));

http://git-wip-us.apache.org/repos/asf/hive/blob/5feb499d/common/src/java/org/apache/hive/common/util/HiveStringUtils.java
----------------------------------------------------------------------
diff --git a/common/src/java/org/apache/hive/common/util/HiveStringUtils.java b/common/src/java/org/apache/hive/common/util/HiveStringUtils.java
index 4a6413a..71d11bd 100644
--- a/common/src/java/org/apache/hive/common/util/HiveStringUtils.java
+++ b/common/src/java/org/apache/hive/common/util/HiveStringUtils.java
@@ -40,6 +40,7 @@ import java.util.Properties;
 import java.util.StringTokenizer;
 import java.util.regex.Pattern;
 
+import com.google.common.base.Splitter;
 import com.google.common.collect.Interner;
 import com.google.common.collect.Interners;
 
@@ -1073,4 +1074,80 @@ public class HiveStringUtils {
   
     return null;
   }
+
+  /**
+   * Strip comments from a sql statement, tracking when the statement contains a string literal.
+   *
+   * @param statement the input string
+   * @return a stripped statement
+   */
+  public static String removeComments(String statement) {
+    if (statement == null) {
+      return null;
+    }
+    Iterator<String> iterator = Splitter.on("\n").omitEmptyStrings().split(statement).iterator();
+    int[] startQuote = {-1};
+    StringBuilder ret = new StringBuilder(statement.length());
+    while (iterator.hasNext()) {
+      String lineWithComments = iterator.next();
+      String lineNoComments = removeComments(lineWithComments, startQuote);
+      ret.append(lineNoComments);
+      if (iterator.hasNext() && !lineNoComments.isEmpty()) {
+        ret.append("\n");
+      }
+    }
+    return ret.toString();
+  }
+
+  /**
+   * Remove comments from the current line of a query.
+   * Avoid removing comment-like strings inside quotes.
+   * @param line a line of sql text
+   * @param startQuote The value -1 indicates that line does not begin inside a string literal.
+   *                   Other values indicate that line does begin inside a string literal
+   *                   and the value passed is the delimiter character.
+   *                   The array type is used to pass int type as input/output parameter.
+   * @return the line with comments removed.
+   */
+  public static String removeComments(String line, int[] startQuote) {
+    if (line == null || line.isEmpty()) {
+      return line;
+    }
+    if (startQuote[0] == -1 && isComment(line)) {
+      return "";  //assume # can only be used at the beginning of line.
+    }
+    StringBuilder builder = new StringBuilder();
+    for (int index = 0; index < line.length(); index++) {
+      if (startQuote[0] == -1 && index < line.length() - 1 && line.charAt(index)
== '-'
+          && line.charAt(index + 1) == '-') {
+        return builder.toString().trim();
+      }
+
+      char letter = line.charAt(index);
+      if (startQuote[0] == letter && (index == 0 || line.charAt(index - 1) != '\\'))
{
+        startQuote[0] = -1; // Turn escape off.
+      } else if (startQuote[0] == -1 && (letter == '\'' || letter == '"') &&
(index == 0
+          || line.charAt(index - 1) != '\\')) {
+        startQuote[0] = letter; // Turn escape on.
+      }
+
+      builder.append(letter);
+    }
+
+    return builder.toString();
+  }
+
+  /**
+   * Test whether a line is a comment.
+   *
+   * @param line the line to be tested
+   * @return true if a comment
+   */
+  private static boolean isComment(String line) {
+    // SQL92 comment prefix is "--"
+    // beeline also supports shell-style "#" prefix
+    String lineTrimmed = line.trim();
+    return lineTrimmed.startsWith("#") || lineTrimmed.startsWith("--");
+  }
+
 }

http://git-wip-us.apache.org/repos/asf/hive/blob/5feb499d/common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java
----------------------------------------------------------------------
diff --git a/common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java b/common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java
index 6bd7037..45d0dee 100644
--- a/common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java
+++ b/common/src/test/org/apache/hive/common/util/TestHiveStringUtils.java
@@ -18,6 +18,7 @@
 
 package org.apache.hive.common.util;
 
+import static org.apache.hive.common.util.HiveStringUtils.removeComments;
 import static org.junit.Assert.*;
 
 import java.util.Arrays;
@@ -61,4 +62,54 @@ public class TestHiveStringUtils {
     assertTrue(Arrays.toString(expectedResults) + " == " + Arrays.toString(testResults),
         Arrays.equals(expectedResults, testResults));
   }
+
+  @Test
+  public void testStripComments() throws Exception {
+    assertNull(removeComments(null));
+    assertUnchanged("foo");
+    assertUnchanged("select 1");
+    assertUnchanged("insert into foo (values('-----')");
+    assertUnchanged("insert into foo (values('abc\n\'xyz')");
+    assertUnchanged("create database if not exists testDB; set hive.cli.print.current.db=true;use\ntestDB;\nuse
default;drop if exists testDB;");
+
+    assertEquals("foo", removeComments("foo\n"));
+    assertEquals("foo", removeComments("\nfoo"));
+    assertEquals("foo", removeComments("\n\nfoo\n\n"));
+    assertEquals("insert into foo (values('-----')", removeComments("--comment\ninsert into
foo (values('-----')"));
+    assertEquals("insert into foo (values('----''-')", removeComments("--comment\ninsert
into foo (values('----''-')"));
+    assertEquals("insert into foo (values(\"----''-\")", removeComments("--comment\ninsert
into foo (values(\"----''-\")"));
+    assertEquals("insert into foo (values(\"----\"\"-\")", removeComments("--comment\ninsert
into foo (values(\"----\"\"-\")"));
+    assertEquals("insert into foo (values('-\n--\n--')", removeComments("--comment\ninsert
into foo (values('-\n--\n--')"));
+    assertEquals("insert into foo (values('-\n--\n--')", removeComments("--comment\n\ninsert
into foo (values('-\n--\n--')"));
+    assertEquals("insert into foo (values(\"-\n--\n--\")", removeComments("--comment\n\ninsert
into foo (values(\"-\n--\n--\")"));
+    assertEquals("insert into foo (values(\"-\n--\n--\")", removeComments("\n\n--comment\n\ninsert
into foo (values(\"-\n--\n--\")\n\n"));
+    assertEquals("insert into foo (values('abc');\ninsert into foo (values('def');", removeComments(
"insert into foo (values('abc');\n--comment\ninsert into foo (values('def');"));
+  }
+
+  @Test
+  public void testLinesEndingWithComments() {
+    int[] escape = {-1};
+    assertEquals("show tables;", removeComments("show tables;",escape));
+    assertEquals("show tables;", removeComments("show tables; --comments",escape));
+    assertEquals("show tables;", removeComments("show tables; -------comments",escape));
+    assertEquals("show tables;", removeComments("show tables; -------comments;one;two;three;;;;",escape));
+    assertEquals("show", removeComments("show-- tables; -------comments",escape));
+    assertEquals("show", removeComments("show --tables; -------comments",escape));
+    assertEquals("s", removeComments("s--how --tables; -------comments",escape));
+    assertEquals("", removeComments("-- show tables; -------comments",escape));
+
+    assertEquals("\"show tables\"", removeComments("\"show tables\" --comments",escape));
+    assertEquals("\"show --comments tables\"", removeComments("\"show --comments tables\"
--comments",escape));
+    assertEquals("\"'show --comments' tables\"", removeComments("\"'show --comments' tables\"
--comments",escape));
+    assertEquals("'show --comments tables'", removeComments("'show --comments tables' --comments",escape));
+    assertEquals("'\"show --comments tables\"'", removeComments("'\"show --comments tables\"'
--comments",escape));
+  }
+
+  /**
+   * check that statement is unchanged after stripping
+   */
+  private void assertUnchanged(String statement) {
+    assertEquals("statement should not have been affected by stripping commnents", statement,
+        removeComments(statement));
+  }
 }

http://git-wip-us.apache.org/repos/asf/hive/blob/5feb499d/ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
----------------------------------------------------------------------
diff --git a/ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
b/ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
index 21bdcf4..50eaf18 100644
--- a/ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
+++ b/ql/src/test/org/apache/hadoop/hive/ql/processors/TestCommandProcessorFactory.java
@@ -46,6 +46,8 @@ public class TestCommandProcessorFactory {
       CommandProcessorFactory.getForHiveCommand(new String[]{" "}, conf));
     Assert.assertNull("Set role should have returned null",
       CommandProcessorFactory.getForHiveCommand(new String[]{"set role"}, conf));
+    Assert.assertNull("Set role should have returned null",
+      CommandProcessorFactory.getForHiveCommand(new String[]{"set", "role"}, conf));
     Assert.assertNull("SQL should have returned null",
       CommandProcessorFactory.getForHiveCommand(new String[]{"SELECT * FROM TABLE"}, conf));
     Assert.assertNull("Test only command should have returned null",

http://git-wip-us.apache.org/repos/asf/hive/blob/5feb499d/ql/src/test/queries/clientpositive/set_processor_namespaces.q
----------------------------------------------------------------------
diff --git a/ql/src/test/queries/clientpositive/set_processor_namespaces.q b/ql/src/test/queries/clientpositive/set_processor_namespaces.q
index 612807f..1f9e282 100644
--- a/ql/src/test/queries/clientpositive/set_processor_namespaces.q
+++ b/ql/src/test/queries/clientpositive/set_processor_namespaces.q
@@ -30,3 +30,11 @@ set jar=${system:maven.local.repository}/org/apache/derby/derby/${system:derby.v
 add file ${hiveconf:jar};
 delete file ${hiveconf:jar};
 list file;
+
+
+-- comment (will be removed by test driver)
+set x=1;
+set x;
+    -- an indented comment to test comment removal
+set x=2;
+set x;
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/hive/blob/5feb499d/ql/src/test/results/clientpositive/set_processor_namespaces.q.out
----------------------------------------------------------------------
diff --git a/ql/src/test/results/clientpositive/set_processor_namespaces.q.out b/ql/src/test/results/clientpositive/set_processor_namespaces.q.out
index c05ce4d..c1c8270 100644
--- a/ql/src/test/results/clientpositive/set_processor_namespaces.q.out
+++ b/ql/src/test/results/clientpositive/set_processor_namespaces.q.out
@@ -51,3 +51,5 @@ POSTHOOK: Input: default@src
 5	val_5
 5	val_5
 c=1
+x=1
+x=2

http://git-wip-us.apache.org/repos/asf/hive/blob/5feb499d/service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
----------------------------------------------------------------------
diff --git a/service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
b/service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
index 2dd90b6..52c320b 100644
--- a/service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
+++ b/service/src/java/org/apache/hive/service/cli/operation/ExecuteStatementOperation.java
@@ -22,6 +22,7 @@ import java.util.Map;
 
 import org.apache.hadoop.hive.ql.processors.CommandProcessor;
 import org.apache.hadoop.hive.ql.processors.CommandProcessorFactory;
+import org.apache.hive.common.util.HiveStringUtils;
 import org.apache.hive.service.cli.HiveSQLException;
 import org.apache.hive.service.cli.OperationType;
 import org.apache.hive.service.cli.session.HiveSession;
@@ -42,7 +43,9 @@ public abstract class ExecuteStatementOperation extends Operation {
   public static ExecuteStatementOperation newExecuteStatementOperation(HiveSession parentSession,
       String statement, Map<String, String> confOverlay, boolean runAsync, long queryTimeout)
       throws HiveSQLException {
-    String[] tokens = statement.trim().split("\\s+");
+
+    String cleanStatement = HiveStringUtils.removeComments(statement);
+    String[] tokens = cleanStatement.trim().split("\\s+");
     CommandProcessor processor = null;
     try {
       processor = CommandProcessorFactory.getForHiveCommand(tokens, parentSession.getHiveConf());
@@ -51,8 +54,9 @@ public abstract class ExecuteStatementOperation extends Operation {
     }
     if (processor == null) {
       // runAsync, queryTimeout makes sense only for a SQLOperation
+      // Pass the original statement to SQLOperation as sql parser can remove comments by
itself
       return new SQLOperation(parentSession, statement, confOverlay, runAsync, queryTimeout);
     }
-    return new HiveCommandOperation(parentSession, statement, processor, confOverlay);
+    return new HiveCommandOperation(parentSession, cleanStatement, processor, confOverlay);
   }
 }


Mime
View raw message