Repository navigation
HIVE-30241: Beeline -f corrupts statements from script files - #6947
dengzhhu653 wants to merge 1 commit into
Conversation
utafrali
left a comment
There was a problem hiding this comment.
The fix targets a real bug but uses a jline3 internal impl class as the solution, which creates a fragile dependency. Additionally, hardcoding UTF-8 can silently corrupt scripts in other encodings. No automated regression test covers this path, and the PR description's 'What changes were proposed' section is blank, making it impossible to verify intent from the diff alone.
| import org.jline.reader.LineReaderBuilder; | ||
| import org.jline.reader.impl.DefaultParser; | ||
| import org.jline.reader.impl.LineReaderImpl; | ||
| import org.jline.reader.impl.history.DefaultHistory; |
There was a problem hiding this 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:
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.
| 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(), |
There was a problem hiding this 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.
There was a problem hiding this comment.
For beeline today, getCommands(File) already uses "UTF-8", other beeline I/O also standardizes on UTF-8 (password file contents, OutputFile with "UTF-8"). Explicit UTF-8 aligns -f with existing file handling.
We can support something like getOpts().getScriptCharset() in the future.
| 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. |
There was a problem hiding this 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.
cbdf1a9 to
db247f1
Compare
|
@utafrali I don't add the regression test due to, I want to kept the diff small, 500 lines of a script file is noisy compared to few lines of fix, and this issue might depend on the environment. The jira provides a script file I can show the case locally, after patching the fix, issue is gone. |
|
There was a problem hiding this comment.
🟡 Changes recommended
Existing tests bypass the changed terminal-construction path, so a regression test is needed to verify that real provider selection preserves every line of large scripts, including UTF-8 text and a final line without a newline.
1 open issue
What changed in this PR
This PR addresses statement corruption when Beeline reads script files with -f, changing JLine terminal selection to avoid native pseudo-terminals.
Changes:
- Forces the exec provider and dumb terminal type for script input.
- Explicitly uses UTF-8 for script decoding.
- Leaves interactive system-terminal construction unchanged.
| File | Description |
|---|---|
| beeline/src/java/org/apache/hive/beeline/BeeLine.java | Configures a non-PTY, UTF-8 terminal for script input to prevent statement corruption. |
Review effort: Balanced
| .provider(TerminalBuilder.PROP_PROVIDER_EXEC) | ||
| .type(Terminal.TYPE_DUMB) | ||
| .streams(inputStream, getErrorStream()) | ||
| .encoding(StandardCharsets.UTF_8) |




What changes were proposed in this pull request?
Avoid TerminalBuilder.build() for script input so JLine does not select PosixPtyTerminal via native providers’ newTerminal, so it reads bytes/lines matching the input file.
Why are the changes needed?
Statement sent to HS2 gets corrupted
Does this PR introduce any user-facing change?
No
How was this patch tested?
Re-run the scripts in my macOS laptop.