Make queued and buffered rows configurable in cli #26015
Conversation
This prevents OOM exception when retrieving huge results
There was a problem hiding this comment.
Pull Request Overview
This PR introduces configurable limits for queued and buffered rows in the CLI to avoid out-of-memory errors when processing large result sets.
- Adds new CLI options
--max-buffered-rowsand--max-queued-rowswith defaults. - Propagates these settings through
ClientOptions,Console,QueryRunner,Query, andOutputHandler. - Updates tests to supply the new constructor arguments and recognize the new options.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client/trino-cli/src/main/java/io/trino/cli/ClientOptions.java | Declares maxBufferedRows and maxQueuedRows CLI options |
| client/trino-cli/src/main/java/io/trino/cli/Console.java | Passes new options into QueryRunner construction |
| client/trino-cli/src/main/java/io/trino/cli/QueryRunner.java | Adds fields and constructor parameters for row limits |
| client/trino-cli/src/main/java/io/trino/cli/Query.java | Propagates new limits to output handlers |
| client/trino-cli/src/main/java/io/trino/cli/OutputHandler.java | Replaces static row‐limit constants with configurable fields |
| client/trino-cli/src/test/java/io/trino/cli/TestClientOptions.java | Recognizes new option names in tests |
| client/trino-cli/src/test/java/io/trino/cli/TestQueryRunner.java | Supplies new constructor parameters in test setup |
Comments suppressed due to low confidence (1)
client/trino-cli/src/test/java/io/trino/cli/TestClientOptions.java:387
- [nitpick] There are no tests validating that the new --max-buffered-rows and --max-queued-rows options actually affect buffering and queuing behavior. Consider adding tests for boundary conditions and default values.
case "maxQueuedRows":
| private void discardResults() | ||
| { | ||
| try (OutputHandler handler = new OutputHandler(new NullPrinter())) { | ||
| try (OutputHandler handler = new OutputHandler(new NullPrinter(), 100, 100)) { |
There was a problem hiding this comment.
In discardResults(), the hard-coded limits (100, 100) do not use the configured maxQueuedRows and maxBufferedRows. This will lead to inconsistent behavior compared to other paths—pass the instance fields instead of constants.
| try (OutputHandler handler = new OutputHandler(new NullPrinter(), 100, 100)) { | |
| try (OutputHandler handler = new OutputHandler(new NullPrinter(), maxQueuedRows, maxBufferedRows)) { |
| clientSession, | ||
| false); | ||
| false, | ||
| 1000, |
There was a problem hiding this comment.
[nitpick] The magic numbers (1000, 500) in this test could drift from production defaults. Consider using named constants or referencing ClientOptions defaults to keep the test in sync when defaults change.
| session, | ||
| clientOptions.debug)) { | ||
| clientOptions.debug, | ||
| clientOptions.maxQueuedRows, |
There was a problem hiding this comment.
Can we have a check for those two configs? i,e at least should greater than 0.
| case "outputFormatInteractive": | ||
| case "pager": | ||
| case "ignoreErrors": | ||
| case "editingMode": |
There was a problem hiding this comment.
Same here, can we have a unsupported test
|
@wendigo this is super technical. Can't we just monitor how big rows are and automatically adjust based on that? |
|
@losipiuk i was thinking about that but there is no easy way of calculating row size hence it's not possible to bound it |
This prevents OOM exception when retrieving huge results
Relates to #26013
Description
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: