utafrali commented on code in PR #6947:
URL: https://github.com/apache/hive/pull/6947#discussion_r4478382509
##########
beeline/src/java/org/apache/hive/beeline/BeeLine.java:
##########
@@ -1543,7 +1545,9 @@ private InputStream prepareInputStream(InputStream
inputStream) {
protected Terminal buildTerminal(InputStream inputStream) throws IOException
{
Terminal terminal;
if (inputStream != null) { // typically when there is a file script to
read from
- terminal = TerminalBuilder.builder().streams(inputStream,
getErrorStream()).build();
+ // An ExternalTerminal is a plain stream pump and reads the script
faithfully.
Review Comment:
The comment "a plain stream pump" is not accurate for `ExternalTerminal`.
According to jline3 source, `ExternalTerminal` spawns a background thread to
process input and handle signals (e.g. Ctrl+C). It is designed for SSH/telnet
connections, not file reading. This comment will mislead future readers about
what `ExternalTerminal` actually does. If the goal is simply to suppress
terminal capability detection, please describe that instead.
##########
beeline/src/java/org/apache/hive/beeline/BeeLine.java:
##########
@@ -121,6 +122,7 @@
import org.jline.reader.impl.history.DefaultHistory;
Review Comment:
Importing from `org.jline.terminal.impl` couples Beeline to jline3's
internal implementation package. Classes in `impl` packages are not part of the
stable public API and can be removed or changed in patch releases of jline3
without notice.
The same behavior can be achieved through the public `TerminalBuilder` API:
```java
terminal = TerminalBuilder.builder()
.streams(inputStream, getErrorStream())
.type(Terminal.TYPE_DUMB)
.encoding(StandardCharsets.UTF_8)
.build();
```
This avoids the `impl` import entirely while still forcing a dumb,
encoding-fixed terminal for file input.
##########
beeline/src/java/org/apache/hive/beeline/BeeLine.java:
##########
@@ -1543,7 +1545,9 @@ private InputStream prepareInputStream(InputStream
inputStream) {
protected Terminal buildTerminal(InputStream inputStream) throws IOException
{
Terminal terminal;
if (inputStream != null) { // typically when there is a file script to
read from
- terminal = TerminalBuilder.builder().streams(inputStream,
getErrorStream()).build();
+ // An ExternalTerminal is a plain stream pump and reads the script
faithfully.
+ terminal = new ExternalTerminal("beeline", Terminal.TYPE_DUMB,
inputStream, getErrorStream(),
Review Comment:
Hardcoding `StandardCharsets.UTF_8` here is a silent regression for users
whose script files are in a different encoding (e.g. Latin-1, GBK, Shift-JIS).
The original `TerminalBuilder` path would have derived the encoding from the
JVM default or terminal negotiation.
Consider reading the charset from an existing Beeline option (e.g.
`getOpts().getScriptCharset()` or a system property fallback), or at minimum
document the UTF-8 requirement prominently, since any non-UTF-8 script will now
be silently misread rather than just having its statements reordered.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
