Skip to content

fix: honor maxRowCount in Druid JDBC Statement (setMaxRows) - #19946

Open
waterWang wants to merge 1 commit into
apache:masterfrom
waterWang:fix/avatica-setmaxrows
Open

fix: honor maxRowCount in Druid JDBC Statement (setMaxRows)#19946
waterWang wants to merge 1 commit into
apache:masterfrom
waterWang:fix/avatica-setmaxrows

Conversation

@waterWang

Copy link
Copy Markdown

Problem

Statement.setMaxRows() was ignored for ordinary JDBC Statements because DruidJdbcStatement.execute() hardcoded Long.MAX_VALUE instead of forwarding the maxRowCount parameter. Additionally, ResultFetcher only marked a frame complete when the underlying yielder was exhausted, so reaching the row limit produced empty non-terminal frames indefinitely.

See issue #19918 for the full analysis.

Changes

  1. DruidJdbcStatement: pass maxRowCount through to DruidJdbcResultSet instead of Long.MAX_VALUE
  2. DruidJdbcResultSet.ResultFetcher: mark frame done when the row limit is reached (offset >= limit), not only when the yielder is exhausted
  3. Tests: added testMaxRowCountDirect (single frame) and testMaxRowCountOverMultipleFramesDirect (cross-frame limit)

Key insight

The maxRowCount was already correctly threaded through the PreparedStatement path (DruidJdbcPreparedStatement). The bug was specific to the ordinary Statement path (DruidJdbcStatement.execute()).

Fixes #19918

Ordinary JDBC Statement.setMaxRows() was ignored because DruidJdbcStatement.execute()
hardcoded Long.MAX_VALUE instead of forwarding the maxRowCount parameter. Additionally,
ResultFetcher only marked a frame complete when the underlying yielder was exhausted,
so reaching the row limit produced empty non-terminal frames indefinitely.

- DruidJdbcStatement: pass maxRowCount through to DruidJdbcResultSet
- DruidJdbcResultSet.ResultFetcher: mark frame done when the limit is reached
- Add tests verifying maxRowCount limits rows in single and multiple frames

Fixes apache#19918

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity Findings
P0 0
P1 1
P2 0
P3 0
Total 1

Reviewed 3 of 3 changed files.

Validation: git diff --check passed; no builds or tests were run.


This is an automated review by Codex GPT-5.6-Luna(max)

}

final Meta.Frame result = new Meta.Frame(offset, yielder.isDone(), rows);
final Meta.Frame result = new Meta.Frame(offset, yielder.isDone() || offset >= limit, rows);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Row-limit completion checks the pre-fetch offset

When the current frame reaches the configured limit, offset has not yet been incremented, so done remains false and the client must request an extra empty frame before the result set terminates. Check offset + rowCount >= limit, or increment before checking, so the final frame is terminal.

@kdelay

kdelay commented Aug 12, 2026

Copy link
Copy Markdown

Disclosure first: I have an open PR for the same issue (#19941), so please read this as an interested party's report rather than a neutral review. Everything below is reproducible.

Neither of the two new tests has run yet, because druid-sql test compilation fails.

On this branch as-is (0867233), JDK 25 on macOS:

mvn test -pl sql -am -Dtest=DruidStatementTest -Dsurefire.failIfNoSpecifiedTests=false -Pskip-static-checks -Dweb.console.skip=true
[ERROR] .../sql/avatica/DruidStatementTest.java:[370,40] cannot find symbol
[ERROR]   symbol:   method size()
[ERROR]   location: variable rows of type java.lang.Iterable<java.lang.Object>

The same error repeats at 392 and 399. Meta.Frame.rows is Iterable<Object>, so frame.rows.size() does not compile.

Merged with current master there is a second failure on top of that one. DruidStatementTest was migrated to JUnit 5 in b1b9683 (#19909, 2026-08-08) and org.junit.Assert is no longer imported there, so every Assert.assertX in the added tests becomes an unresolved symbol. That is what the red jobs on this PR report: Compilation failure ... on project druid-sql in test-jdk25-[D*] and the other unit test shards.

Once it compiles, both new tests fail. I changed only the three frame.rows.size() calls to Lists.newArrayList(frame.rows).size() (Lists is already imported in the file), left everything else untouched, and re-ran the same command:

[ERROR] Tests run: 16, Failures: 2, Errors: 0, Skipped: 0
[ERROR] DruidStatementTest.testMaxRowCountDirect:371
[ERROR] DruidStatementTest.testMaxRowCountOverMultipleFramesDirect:400

Both failures are the assertTrue(frame.done) line. This is the same defect that was already flagged inline on DruidJdbcResultSet: offset is incremented after the frame is constructed, so yielder.isDone() || offset >= limit tests the pre-fetch offset, and the frame that reaches the limit still reports done = false. The row-count assertions on those same frames pass, so the limit itself does take effect; it is only the termination flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Druid Avatica JDBC driver does not honor Statement.setMaxRows()

3 participants