fix(sql): query with limit x, y (x > 0 & y > 0) return correct `size()` by kafka1991 · Pull Request #6409 · questdb/questdb

kafka1991 · 2025-11-19T02:20:28Z

When executing LIMIT x, y queries (where both x > 0 and y > 0), LimitRecordCursorFactory.size() returns incorrect results(y -x) when the underlying base cursor has fewer rows than y - x

I'm not certain if calling countRows() (which invokes base.calculateSize()) when base.size() == -1 is optimal, but I followed the existing pattern used in other branches for consistency. If we need to preserve size()
efficiency by returning -1 in such cases, we should review all other branches and update numerous tests accordingly.

Found when testing windowJoin

coderabbitai · 2025-11-19T02:20:33Z

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The pull request refactors the LimitRecordCursor logic in LimitRecordCursorFactory to consistently use countRows() for determining row counts, replacing previous base.size()-based optimization paths in two branches. A test case is added to verify limit and offset behavior on normal tables.

Changes

Cohort / File(s)	Summary
Limit cursor logic simplification `core/src/main/java/io/questdb/griffin/engine/LimitRecordCursorFactory.java`	Modified two branching paths in LimitRecordCursor: (1) when lo > -1 and hiFunction is null, and (2) when lo >= 0 and hiFunction is not null. Both now use countRows() to populate rowCount instead of querying base.size(), computing limit as min(rowCount, lo) and max(0, min(rowCount, hi) - lo) respectively, removing the base.size()-based conditional logic.
Test coverage addition `core/src/test/java/io/questdb/test/griffin/OrderByAscRowSkippingTest.java`	Added testNormalTableSelectEithLimitOffset() test method to verify SELECT queries with WHERE, ORDER BY ts, and LIMIT OFFSET clauses; includes import reordering.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

The logic changes in LimitRecordCursorFactory are localized and follow a clear pattern: replacing base.size() checks with consistent countRows() calls
Test addition is straightforward and aligns with the refactoring
Verify that countRows() behavior correctly handles edge cases where lo/hi are at boundary values and that min/max computations match prior optimization behavior

Suggested labels

SQL

Suggested reviewers

nwoolmer

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and specifically describes the main bug fix: correcting the size() return value for LIMIT x,y queries when both x and y are positive.
Description check	✅ Passed	The pull request description clearly explains the bug being fixed: LIMIT x, y queries return incorrect size() results when the base cursor has fewer rows than y - x.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

kafka1991 · 2025-11-19T02:26:53Z

@CodeRabbit review

coderabbitai · 2025-11-19T02:26:58Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e88abf7 and 9159dc1.

📒 Files selected for processing (2)

core/src/main/java/io/questdb/griffin/engine/LimitRecordCursorFactory.java (2 hunks)
core/src/test/java/io/questdb/test/griffin/OrderByAscRowSkippingTest.java (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (34)

GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
GitHub Check: New pull request (Hosted Running tests on windows-other-2)
GitHub Check: New pull request (Hosted Running tests on windows-other-1)
GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
GitHub Check: New pull request (Hosted Running tests on mac-other)
GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
GitHub Check: New pull request (Hosted Running tests on mac-cairo)
GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
GitHub Check: New pull request (Hosted Running tests on mac-griffin)
GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
GitHub Check: New pull request (Check Changes Check changes)
GitHub Check: build

🔇 Additional comments (2)

core/src/main/java/io/questdb/griffin/engine/LimitRecordCursorFactory.java (2)

267-271: LGTM! Correct handling of first N rows.

The logic now properly counts rows before calculating the limit, ensuring size() returns the minimum of the requested rows (lo) and the actual row count. This prevents returning an incorrect size when fewer rows are available than requested.

312-318: LGTM! Critical fix for LIMIT x,y size calculation.

This change correctly fixes the bug described in the PR. By using Math.min(rowCount, hi), the code now caps the upper bound to the actual number of rows available in the base cursor, preventing size() from incorrectly returning hi - lo when fewer rows exist. The Math.max(0, ...) wrapper properly handles edge cases where lo exceeds the capped upper bound.

Example: With 5 rows available, LIMIT 2,10 now correctly computes limit = max(0, min(5, 10) - 2) = 3 instead of potentially returning 8.

core/src/test/java/io/questdb/test/griffin/OrderByAscRowSkippingTest.java

glasstiger · 2025-11-20T20:32:28Z

[PR Coverage check]

😍 pass : 4 / 4 (100.00%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/griffin/engine/LimitRecordCursorFactory.java	4	4	100.00%

fix limit size() return value

9159dc1

coderabbitai bot reviewed Nov 19, 2025

View reviewed changes

core/src/test/java/io/questdb/test/griffin/OrderByAscRowSkippingTest.java Outdated Show resolved Hide resolved

kafka1991 and others added 5 commits November 19, 2025 10:54

fix tests

ba38447

fix tests

941d3e4

Merge branch 'master' into order_by_size_fix

91d7246

Merge branch 'master' into order_by_size_fix

ce4e8e8

cleanup

28c215e

bluestreak01 approved these changes Nov 20, 2025

View reviewed changes

Merge branch 'master' into order_by_size_fix

1d6b505

bluestreak01 merged commit a29bd95 into master Nov 20, 2025
39 of 41 checks passed

bluestreak01 deleted the order_by_size_fix branch November 20, 2025 23:54

nwoolmer mentioned this pull request Dec 4, 2025

ORDER BY … DESC LIMIT returns no rows when WHERE filter matches data #6501

Closed

1 task

coderabbitai bot mentioned this pull request Dec 16, 2025

perf(sql): avoid expensive row counting in EXPLAIN query with LIMIT #6540

Merged

coderabbitai bot mentioned this pull request Dec 24, 2025

Fix HAVING clause in Cube.js driver #6578

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sql): query with limit x, y (x > 0 & y > 0) return correct `size()`#6409

fix(sql): query with limit x, y (x > 0 & y > 0) return correct `size()`#6409
bluestreak01 merged 7 commits intomasterfrom
order_by_size_fix

kafka1991 commented Nov 19, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 19, 2025 •

edited

Loading

Review skipped

Uh oh!

kafka1991 commented Nov 19, 2025

Uh oh!

coderabbitai bot commented Nov 19, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

glasstiger commented Nov 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kafka1991 commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Suggested labels

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

kafka1991 commented Nov 19, 2025

Uh oh!

coderabbitai bot commented Nov 19, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glasstiger commented Nov 20, 2025

[PR Coverage check]

file detail

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kafka1991 commented Nov 19, 2025 •

edited

Loading

coderabbitai bot commented Nov 19, 2025 •

edited

Loading