ayushtkn commented on code in PR #5838:
URL: https://github.com/apache/hive/pull/5838#discussion_r2205558895
##########
beeline/pom.xml:
##########
@@ -114,6 +114,11 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-hdfs</artifactId>
+ <scope>test</scope>
+ </dependency>
Review Comment:
Why is hdfs being added as a dependency here? It is already there at L194
##########
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java:
##########
@@ -152,11 +153,17 @@ public String get(String envVar) {
public BeeLineOpts(BeeLine beeLine, Properties props) {
this.beeLine = beeLine;
- if (terminal.getWidth() > 0) {
- maxWidth = terminal.getWidth();
- }
- if (terminal.getHeight() > 0) {
- maxHeight = terminal.getHeight();
+ try {
+ Terminal terminal = TerminalBuilder.terminal();
+ if (terminal.getWidth() > 0) {
+ maxWidth = terminal.getWidth();
+ }
+ if (terminal.getHeight() > 0) {
+ maxHeight = terminal.getHeight();
+ }
+ terminal.close();
Review Comment:
Curious where was the terminal closed earlier?
##########
beeline/src/java/org/apache/hive/beeline/cli/HiveCli.java:
##########
@@ -32,11 +32,15 @@ public static void main(String[] args) throws IOException {
}
public int runWithArgs(String[] cmd, InputStream inputStream) throws
IOException {
- beeLine = new BeeLine(false);
+ beeLine = createBeeline();
try {
return beeLine.begin(cmd, inputStream);
} finally {
beeLine.close();
}
}
+
+ protected BeeLine createBeeline() {
Review Comment:
is protected needed or default access works?
##########
common/src/java/org/apache/hive/common/util/MatchingStringsCompleter.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hive.common.util;
+
+import org.jline.reader.Candidate;
+import org.jline.reader.Completer;
+import org.jline.reader.LineReader;
+import org.jline.reader.ParsedLine;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.SortedSet;
+import java.util.TreeSet;
+
+/**
+ * A matching string Completer based on JLine's StringCompleter
+ */
+public class MatchingStringsCompleter implements Completer {
+ protected SortedSet<String> candidateStrings = new TreeSet<>();
+
+ public MatchingStringsCompleter() {
+ // empty
+ }
+
+ public MatchingStringsCompleter(String... strings) {
+ this(Arrays.asList(strings));
+ }
+
+ public MatchingStringsCompleter(Iterable<String> strings) {
+ strings.forEach(candidateStrings::add);
+ }
+
+ public MatchingStringsCompleter(Candidate... candidates) {
+
Arrays.stream(candidates).map(Candidate::value).forEach(candidateStrings::add);
+ }
Review Comment:
seems unused
##########
beeline/src/java/org/apache/hive/beeline/Commands.java:
##########
@@ -1103,16 +1103,29 @@ public String handleMultiLineCmd(String line) throws
IOException {
}
String extra;
//avoid NPE below if for some reason -e argument has multi-line command
- if (beeLine.getConsoleReader() == null) {
+ if (beeLine.getLineReader() == null) {
throw new RuntimeException("Console reader not initialized. This could
happen when there "
+ "is a multi-line command using -e option and which requires
further reading from console");
}
- if (beeLine.getOpts().isSilent() && beeLine.getOpts().getScriptFile() !=
null) {
- extra = beeLine.getConsoleReader().readLine(null, mask);
- } else {
- extra = beeLine.getConsoleReader().readLine(prompt.toString());
+
+ try {
+ if (beeLine.getOpts().isSilent() && beeLine.getOpts().getScriptFile()
!= null) {
+ extra = beeLine.getLineReader().readLine(null, mask);
+ } else {
+ extra = beeLine.getLineReader().readLine(prompt.toString());
+ }
+ } catch (EndOfFileException t) {
+ /*
Review Comment:
maybe change t to eof and better to put a debug log here
##########
beeline/src/java/org/apache/hive/beeline/BeeLine.java:
##########
@@ -1385,7 +1396,17 @@ private int execute(ConsoleReader reader, boolean
exitOnError) {
} else if (line != null) {
lastExecutionResult = ERRNO_OK;
}
-
+ } catch (EndOfFileException t) {
Review Comment:
nit
can you change the variable name to `eof`, with `t` it just give me
`Throwable` vibes
##########
beeline/src/java/org/apache/hive/beeline/BeeLineDummyTerminal.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hive.beeline;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import org.jline.terminal.Terminal;
+import org.jline.terminal.impl.DumbTerminal;
+
+/**
+ * A Beeline implementation that always creates a DumbTerminal.
+ * This class resides in the production source code (not in tests) because
Beeline can serve as a
+ * dummy terminal tool without real user interaction (e.g., HiveSchemaTool),
not just in testing scenarios,
+ * although that is its primary use case.
+ */
+public class BeeLineDummyTerminal extends BeeLine {
+
+ public BeeLineDummyTerminal() {
+ this(true);
+ }
+
+ public BeeLineDummyTerminal(boolean isBeeLine) {
+ super(isBeeLine);
+ }
+
+ protected Terminal buildTerminal(InputStream inputStream) throws IOException
{
Review Comment:
nit
can you add the `override` annotation
##########
beeline/src/java/org/apache/hive/beeline/BeeLine.java:
##########
@@ -938,8 +945,8 @@ private boolean connectUsingArgs(BeelineParser
beelineParser, CommandLine cl) {
String propertyFile = cl.getOptionValue("property-file");
if (propertyFile != null) {
try {
- this.consoleReader = new ConsoleReader();
- } catch (IOException e) {
+ this.lineReader = LineReaderBuilder.builder().build();
+ } catch (IOError e) {
handleException(e);
Review Comment:
`IOException` has changed to `IOError`, can this `handleException` handle it?
##########
beeline/src/java/org/apache/hive/beeline/BeeLine.java:
##########
@@ -1270,6 +1278,7 @@ public HS2ConnectionFileParser
getHiveSiteHS2ConnectionFileParser() {
return new HiveSiteHS2ConnectionFileParser();
}
+ @VisibleForTesting
int runInit() {
Review Comment:
this is used in `Commands.java` which is prod code & that too requires this
visibility, I don't think we need ``VisibleForTesting`` here
##########
beeline/src/java/org/apache/hive/beeline/BeeLine.java:
##########
@@ -1360,16 +1369,18 @@ private int executeFile(String fileName) {
}
}
- private int execute(ConsoleReader reader, boolean exitOnError) {
+ @VisibleForTesting
+ int execute(LineReader reader, boolean exitOnError) {
int lastExecutionResult = ERRNO_OK;
Character mask = (System.getProperty("jline.terminal",
"").equals("jline.UnsupportedTerminal")) ? null
- : ConsoleReader.NULL_MASK;
+ : LineReaderImpl.NULL_MASK;
+ String line;
while (!exit) {
try {
// Execute one instruction; terminate on executing a script if there
is an error
// in silent mode, prevent the query and prompt being echoed back to
terminal
- String line = (getOpts().isSilent() && getOpts().getScriptFile() !=
null) ? reader
+ line = (getOpts().isSilent() && getOpts().getScriptFile() != null) ?
reader
Review Comment:
Why you need to extract this? It works the previous way as well
##########
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java:
##########
@@ -79,24 +79,7 @@ void setCompletions(boolean skipmeta) throws SQLException,
IOException {
: getDatabaseMetaData().getExtraNameCharacters();
// setup the completer for the database
- sqlCompleter = new ArgumentCompleter(
- new ArgumentCompleter.AbstractArgumentDelimiter() {
- // delimiters for SQL statements are any
- // non-letter-or-number characters, except
- // underscore and characters that are specified
- // by the database to be valid name identifiers.
- @Override
- public boolean isDelimiterChar(CharSequence buffer, int pos) {
- char c = buffer.charAt(pos);
- if (Character.isWhitespace(c)) {
- return true;
- }
- return !(Character.isLetterOrDigit(c))
- && c != '_'
- && extraNameCharacters.indexOf(c) == -1;
- }
- },
- new SQLCompleter(SQLCompleter.getSQLCompleters(beeLine, skipmeta)));
+ sqlCompleter = new ArgumentCompleter(new
SQLCompleter(SQLCompleter.getSQLCompleters(beeLine, skipmeta)));
Review Comment:
there is a variable above `extraNameCharacters` that goes unused with this
change, that can be dropped
##########
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java:
##########
@@ -152,11 +153,17 @@ public String get(String envVar) {
public BeeLineOpts(BeeLine beeLine, Properties props) {
this.beeLine = beeLine;
- if (terminal.getWidth() > 0) {
- maxWidth = terminal.getWidth();
- }
- if (terminal.getHeight() > 0) {
- maxHeight = terminal.getHeight();
+ try {
+ Terminal terminal = TerminalBuilder.terminal();
+ if (terminal.getWidth() > 0) {
+ maxWidth = terminal.getWidth();
+ }
+ if (terminal.getHeight() > 0) {
+ maxHeight = terminal.getHeight();
+ }
+ terminal.close();
+ } catch(IOException e) {
+ // nothing to do
Review Comment:
We should atleast log something here
##########
cli/pom.xml:
##########
@@ -121,6 +121,10 @@
<artifactId>hadoop-mapreduce-client-core</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-1.2-api</artifactId>
+ </dependency>
Review Comment:
where is this being used?
##########
beeline/src/java/org/apache/hive/beeline/BeeLineOpts.java:
##########
@@ -195,12 +202,11 @@ public File saveDir() {
@Override
- public int complete(String buf, int pos, List cand) {
+ public void complete(LineReader reader, ParsedLine line, List<Candidate>
candidates) {
Review Comment:
this looks like some behaviour change to me, can we not return the values
which were returned earlier, supposedly 0 & -1
##########
beeline/src/java/org/apache/hive/beeline/TableNameCompletor.java:
##########
@@ -38,11 +41,11 @@ class TableNameCompletor implements Completer {
}
@Override
- public int complete(String buf, int pos, List cand) {
- if (beeLine.getDatabaseConnection() == null) {
- return -1;
+ public void complete(LineReader reader, ParsedLine line, List<Candidate>
candidates) {
Review Comment:
can we return int & maintain the method signature, this is a public method,
changing signature of a public method afaik is an incompatible change
##########
beeline/src/java/org/apache/hive/beeline/Commands.java:
##########
@@ -1701,6 +1713,24 @@ public boolean connect(Properties props) throws
IOException {
}
}
+ /**
+ * Reads a line with the given prompt and optional mask character.
+ * Starting with JLine3, an EndOfFileException is intentionally thrown upon
reaching the end of the input stream.
+ * In interactive usage, this method returns the partial line entered before
the exception to preserve existing
+ * behavior. This is typically used for input prompts such as
username/password.
+ *
+ * @param prompt the prompt message displayed to the user
+ * @param mask the character used to mask the input, if any
+ * @return the full line read, or the partial input captured before the
EndOfFileException was thrown
+ */
+ private String readLineWithPrompt(String prompt, Character mask) {
+ try {
+ return beeLine.getLineReader().readLine(prompt, mask);
+ } catch (EndOfFileException e) {
Review Comment:
eof
##########
itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeelinePasswordOption.java:
##########
@@ -17,6 +17,7 @@
*/
package org.apache.hive.beeline;
+import static org.junit.Assert.assertEquals;
Review Comment:
this is unused
##########
itests/util/pom.xml:
##########
@@ -122,6 +122,10 @@
<artifactId>hive-llap-server</artifactId>
<classifier>tests</classifier>
</dependency>
+ <dependency>
+ <groupId>org.apache.hive</groupId>
+ <artifactId>hive-jdbc</artifactId>
+ </dependency>
Review Comment:
why?
##########
itests/hive-unit/src/test/java/org/apache/hive/beeline/TestBeeLineWithArgs.java:
##########
@@ -175,13 +173,40 @@ static String testCommandLineScript(List<String> argList,
InputStream inputStrea
throw new RuntimeException("Unexpected outstream type " + streamType);
}
String[] args = argList.toArray(new String[argList.size()]);
- beeLine.begin(args, inputStream);
+ beeLine.begin(args, null);
beeLine.close();
beelineOutputStream.close();
String output = os.toString("UTF8");
return output;
}
+ /*
+ * Creates a BeeLineDummyTerminalFromFile instance that skips the main
execution code path
+ * if an init script has already run. Since this unit test class executes
scripts in init (-i) or file (-f) mode,
+ * we can avoid using waiting hacks and simply return from execute() with
the original init result.
+ */
+ private static BeeLineDummyTerminalFromFile getBeeLineDummyTerminal() {
Review Comment:
is this required for JLIne3, or is it some optimisation kicking in here?
##########
itests/hive-unit/src/test/java/org/apache/hive/beeline/hs2connection/BeelineWithHS2ConnectionFileTestBase.java:
##########
@@ -75,19 +76,19 @@ public abstract class BeelineWithHS2ConnectionFileTestBase {
public String transportMode = null;
- protected class TestBeeLine extends BeeLine {
+ protected class BeelineWithConfigFileManager extends BeeLineDummyTerminal {
Review Comment:
remove the import for beeline above it goes unused after this change
##########
beeline/src/test/org/apache/hive/beeline/cli/TestHiveCli.java:
##########
@@ -80,7 +81,7 @@ public void testCommentStripping() {
@Test
public void testSetPromptValue() {
- verifyCMD("set hive.cli.prompt=MYCLI;SHOW\nTABLES;", "MYCLI> ", err, null,
+ verifyCMD("set hive.cli.prompt=MYCLI;SHOW\nTABLES;\n", "MYCLI> ", err,
null,
Review Comment:
why this \n thing has kicked in? is it something that will bother the end
user in prod as well. Moreover rather than modifying each call with \n can we
handle it in `verifyCMD` method itself?
##########
beeline/src/test/org/apache/hive/beeline/TestBeelineArgParsing.java:
##########
@@ -70,7 +70,7 @@ public TestBeelineArgParsing(String connectionString, String
driverClazzName, St
this.defaultSupported = defaultSupported;
}
- public class TestBeeline extends BeeLine {
+ public static class TestBeeline extends BeeLine {
Review Comment:
this just seems some style fix, can be avoided in this scope
##########
beeline/src/test/org/apache/hive/beeline/cli/HiveCliForTest.java:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hive.beeline.cli;
+
+import org.apache.hive.beeline.BeeLine;
+import org.apache.hive.beeline.BeeLineDummyTerminal;
+
+public class HiveCliForTest extends HiveCli {
+
+ protected BeeLine createBeeline() {
Review Comment:
add `override` annotation
##########
beeline/src/java/org/apache/hive/beeline/DatabaseConnection.java:
##########
@@ -79,24 +79,7 @@ void setCompletions(boolean skipmeta) throws SQLException,
IOException {
: getDatabaseMetaData().getExtraNameCharacters();
// setup the completer for the database
- sqlCompleter = new ArgumentCompleter(
- new ArgumentCompleter.AbstractArgumentDelimiter() {
- // delimiters for SQL statements are any
- // non-letter-or-number characters, except
- // underscore and characters that are specified
- // by the database to be valid name identifiers.
Review Comment:
are these case being taken care, I think in the new code Uses the default
delimiter logic from JLine’s ArgumentCompleter:
Usually splits on whitespace and some basic punctuation & doesn’t account
for underscores or custom DB identifier characters.
That is what my little research says, can you double check once
##########
cli/src/test/org/apache/hadoop/hive/cli/TestCliDriverMethods.java:
##########
@@ -206,24 +204,28 @@ private PrintStream headerPrintingTestDriver(Schema
mockSchema) throws CommandPr
@Test
public void testGetCommandCompletor() {
- Completer[] completors = CliDriver.getCommandCompleter();
- assertEquals(2, completors.length);
- assertTrue(completors[0] instanceof ArgumentCompleter);
- assertTrue(completors[1] instanceof Completer);
-
- List<CharSequence> testList = Arrays.asList(")");
- completors[1].complete("fdsdfsdf", 0, testList);
- assertEquals(")", testList.get(0));
- testList=new ArrayList<CharSequence>();
- completors[1].complete("len", 0, testList);
- assertTrue(testList.get(0).toString().endsWith("length("));
-
- testList=new ArrayList<CharSequence>();
- completors[0].complete("set f", 0, testList);
- assertEquals("set", testList.get(0));
-
+ Completer[] completers = CliDriver.getCommandCompleter();
+ assertEquals(2, completers.length);
+ assertTrue(completers[0] instanceof ArgumentCompleter);
+ assertNotNull(completers[1]);
+
+ final List<Candidate> candidates1 = new ArrayList<>();
+ candidates1.add(new Candidate(")"));
+ completers[1].complete(null,
CliDriver.getDefaultParser().parse("fdsdfsdf", 0), candidates1);
+ assertEquals(")", candidates1.get(0).value());
Review Comment:
can we change these `get(0)` with `getFirst()` here & below
##########
itests/hive-unit/src/test/java/org/apache/hive/beeline/BeeLineDummyTerminalFromFile.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hive.beeline;
+
+import org.apache.hive.beeline.BeeLineDummyTerminal;
Review Comment:
this ain't required
##########
cli/src/test/org/apache/hadoop/hive/cli/TestCliDriverMethods.java:
##########
@@ -206,24 +204,28 @@ private PrintStream headerPrintingTestDriver(Schema
mockSchema) throws CommandPr
@Test
public void testGetCommandCompletor() {
- Completer[] completors = CliDriver.getCommandCompleter();
- assertEquals(2, completors.length);
- assertTrue(completors[0] instanceof ArgumentCompleter);
- assertTrue(completors[1] instanceof Completer);
-
- List<CharSequence> testList = Arrays.asList(")");
- completors[1].complete("fdsdfsdf", 0, testList);
- assertEquals(")", testList.get(0));
- testList=new ArrayList<CharSequence>();
- completors[1].complete("len", 0, testList);
- assertTrue(testList.get(0).toString().endsWith("length("));
-
- testList=new ArrayList<CharSequence>();
- completors[0].complete("set f", 0, testList);
- assertEquals("set", testList.get(0));
-
+ Completer[] completers = CliDriver.getCommandCompleter();
+ assertEquals(2, completers.length);
+ assertTrue(completers[0] instanceof ArgumentCompleter);
+ assertNotNull(completers[1]);
+
+ final List<Candidate> candidates1 = new ArrayList<>();
+ candidates1.add(new Candidate(")"));
+ completers[1].complete(null,
CliDriver.getDefaultParser().parse("fdsdfsdf", 0), candidates1);
+ assertEquals(")", candidates1.get(0).value());
+
+ final List<Candidate> candidates2 = new ArrayList<>();
+ completers[1].complete(null, CliDriver.getDefaultParser().parse("length",
0), candidates2);
+ System.out.printf("--- --> %s%n", candidates2.get(0).value());
+ assertTrue(candidates2.get(0).value().endsWith("length("));
+
+ final List<Candidate> candidates3 = new ArrayList<>();
+ completers[0].complete(null, CliDriver.getDefaultParser().parse("set f",
0), candidates3);
+ assertEquals("set", candidates3.get(0).value());
}
+
+
Review Comment:
nit
avoid
##########
cli/src/java/org/apache/hadoop/hive/cli/CliDriver.java:
##########
@@ -609,47 +610,32 @@ public static Completer[] getCommandCompleter() {
candidateStrings.add(s.toLowerCase());
}
- StringsCompleter strCompleter = new StringsCompleter(candidateStrings);
-
- // Because we use parentheses in addition to whitespace
- // as a keyword delimiter, we need to define a new ArgumentDelimiter
- // that recognizes parenthesis as a delimiter.
Review Comment:
I think the new code doesn't handle this, did you try this case?
##########
beeline/src/java/org/apache/hive/beeline/BeeLine.java:
##########
@@ -1496,13 +1548,12 @@ boolean dispatch(String line) {
}
line = HiveStringUtils.removeComments(line);
+ line = line.trim();
- if (line.trim().length() == 0) {
+ if (line.length() == 0) {
Review Comment:
since you are touching it, can you change it to line.isEmpty()
##########
beeline/src/java/org/apache/hive/beeline/Commands.java:
##########
@@ -48,6 +48,7 @@
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
+import java.util.ListIterator;
Review Comment:
unused
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
