fix(core): fix occasional errors in result set size calculation#6351
Merged
bluestreak01 merged 24 commits intomasterfrom Nov 11, 2025
Merged
fix(core): fix occasional errors in result set size calculation#6351bluestreak01 merged 24 commits intomasterfrom
bluestreak01 merged 24 commits intomasterfrom
Conversation
WalkthroughAdds per-frame remaining-row tracking to PageFrameCursor implementations, removes a static RecordCursor.calculateSize utility, and updates size-calculation flows to account for remaining rows; expands related tests and introduces new calculateSize implementations for interval partition cursors. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PageFrameRecordCursorImpl as PFImpl
participant FrameCursor
participant Counter
Caller->>PFImpl: calculateSize(counter)
PFImpl->>FrameCursor: clear() %% reset current frame cursor state
PFImpl->>FrameCursor: getRemainingRowsInInterval()
FrameCursor-->>PFImpl: remainingRows
PFImpl->>Counter: add(remainingRows)
PFImpl->>FrameCursor: calculateSize(counter) %% delegate remaining traversal
FrameCursor-->>Counter: add(..) %% frameCursor updates counter further
PFImpl-->>Caller: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Member
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
ideoma
reviewed
Nov 6, 2025
core/src/main/java/io/questdb/griffin/engine/functions/table/ReadParquetPageFrameCursor.java
Show resolved
Hide resolved
core/src/main/java/io/questdb/griffin/engine/table/FwdTableReaderPageFrameCursor.java
Outdated
Show resolved
Hide resolved
...src/test/java/io/questdb/test/griffin/engine/table/PageFrameRecordCursorImplFactoryTest.java
Show resolved
Hide resolved
...src/test/java/io/questdb/test/griffin/engine/table/PageFrameRecordCursorImplFactoryTest.java
Show resolved
Hide resolved
ideoma
previously approved these changes
Nov 7, 2025
glasstiger
added
the
DO NOT MERGE
glasstiger
removed
the
DO NOT MERGE
Member
|
@CodeRabbit there were more places where size wasn't calculated correctly, can you review the new changes? |
|
@bluestreak01: I'll review the new changes where additional size calculation issues were fixed. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
core/src/main/java/io/questdb/griffin/engine/join/HashJoinLightRecordCursorFactory.java (1)
197-214: Logic is correct for remaining-size calculation, but consider documenting side effects.The addition of lines 199-202 correctly accounts for remaining slave-chain entries before counting matches from subsequent master records. This properly computes the remaining result-set size (not the total size from the beginning), which aligns with the PR's objective of fixing size-calculation errors when cursors are partially consumed.
However, the method has significant side effects that should be documented:
Destructive operation:
calculateSizeexhausts bothslaveChainCursorandmasterCursorby advancing them to completion. SubsequenthasNext()calls will return false unlesstoTop()is called.Non-idempotent: Multiple calls to
calculateSizewill yield different results (first call counts remaining records, second call returns 0).Naming ambiguity: The method name suggests a read-only calculation, but it mutates cursor state. Consider adding a JavaDoc comment clarifying that this method computes the remaining size and advances the cursors as a side effect.
Consider adding JavaDoc to clarify the semantics:
/** * Calculates the number of records remaining in this cursor. * <p> * WARNING: This method advances both the slave-chain cursor and master cursor * to completion as a side effect. After calling this method, hasNext() will * return false unless toTop() is called first. * * @param circuitBreaker circuit breaker for long-running operations * @param counter accumulator for the count */ @Override public void calculateSize(SqlExecutionCircuitBreaker circuitBreaker, Counter counter) {core/src/test/java/io/questdb/test/AbstractCairoTest.java (1)
271-285: Good addition; guard against long→int overflow when picking skip.If count > Integer.MAX_VALUE,
(int) counttruncates. Clamp the bound before rnd.nextInt.- final int skip = rnd.nextBoolean() ? rnd.nextInt((int) count) : 0; + final int skipBound = count > Integer.MAX_VALUE ? Integer.MAX_VALUE : (int) count; + final int skip = rnd.nextBoolean() ? rnd.nextInt(skipBound) : 0;core/src/test/java/io/questdb/test/griffin/engine/table/PageFrameRecordCursorImplFactoryTest.java (1)
1065-1158: Add DESC interval tests to exercise backward size-calculation.You cover interval paths in ASC; mirror a few with ORDER_DESC to hit IntervalBwdPartitionFrameCursor.calculateSize.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
core/src/main/java/io/questdb/cairo/AbstractIntervalPartitionFrameCursor.java(0 hunks)core/src/main/java/io/questdb/cairo/IntervalBwdPartitionFrameCursor.java(2 hunks)core/src/main/java/io/questdb/cairo/IntervalFwdPartitionFrameCursor.java(2 hunks)core/src/main/java/io/questdb/cairo/RecordArray.java(3 hunks)core/src/main/java/io/questdb/cairo/sql/PageFrameCursor.java(3 hunks)core/src/main/java/io/questdb/griffin/engine/functions/catalogue/PgAttrDefFunctionFactory.java(5 hunks)core/src/main/java/io/questdb/griffin/engine/functions/table/ReadParquetPageFrameCursor.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/join/HashJoinLightRecordCursorFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/table/BwdTableReaderPageFrameCursor.java(3 hunks)core/src/main/java/io/questdb/griffin/engine/table/FwdTableReaderPageFrameCursor.java(3 hunks)core/src/main/java/io/questdb/griffin/engine/table/PageFrameRecordCursorImpl.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/table/SelectedRecordCursorFactory.java(1 hunks)core/src/test/java/io/questdb/test/AbstractCairoTest.java(2 hunks)core/src/test/java/io/questdb/test/cairo/wal/WalTableSqlTest.java(34 hunks)core/src/test/java/io/questdb/test/griffin/engine/table/PageFrameRecordCursorImplFactoryTest.java(6 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/io/questdb/cairo/AbstractIntervalPartitionFrameCursor.java
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/io/questdb/griffin/engine/table/FwdTableReaderPageFrameCursor.java
- core/src/main/java/io/questdb/griffin/engine/table/BwdTableReaderPageFrameCursor.java
- core/src/main/java/io/questdb/cairo/sql/PageFrameCursor.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-10T14:28:48.307Z
Learnt from: mtopolnik
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2025-11-10T14:28:48.307Z
Learning: In AsOfJoinDenseRecordCursorFactoryBase.java, the `backwardScanExhausted` flag is intentionally NOT reset in `toTop()` because backward scan results are reusable across cursor rewinds. The backward scan caches historical matches that remain valid when the cursor is rewound.
Applied to files:
core/src/main/java/io/questdb/cairo/IntervalBwdPartitionFrameCursor.javacore/src/test/java/io/questdb/test/AbstractCairoTest.javacore/src/main/java/io/questdb/griffin/engine/join/HashJoinLightRecordCursorFactory.javacore/src/main/java/io/questdb/cairo/RecordArray.javacore/src/test/java/io/questdb/test/griffin/engine/table/PageFrameRecordCursorImplFactoryTest.java
⏰ 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). (29)
- GitHub Check: New pull request (Coverage Report Coverage Report)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- 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 (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz 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 Griffin And Fuzz tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (7)
core/src/test/java/io/questdb/test/cairo/wal/WalTableSqlTest.java (1)
125-130: LGTM! Excellent refactoring to text blocks.The conversion from string concatenation to Java text blocks significantly improves readability and maintainability of test expectations and SQL statements. The functional behavior remains unchanged.
Also applies to: 174-180, 218-222, 262-266, 303-307, 431-436, 523-527, 543-548, 590-593, 620-623, 635-638, 649-652, 664-667, 767-770, 801-809, 836-840, 864-871, 901-911, 953-956, 1133-1140, 1147-1150, 1434-1437, 1570-1577, 1583-1587, 1628-1634, 1681-1684, 1723-1727, 1749-1753, 1813-1817, 1839-1843, 1900-1906, 1916-1922, 1956-1964, 2115-2119, 2178-2184, 2350-2359, 2361-2366, 2373-2378, 2383-2386
core/src/main/java/io/questdb/griffin/engine/table/SelectedRecordCursorFactory.java (1)
342-346: Delegation is correct.Propagating getRemainingRowsInInterval() keeps wrappers in sync with base behavior.
core/src/main/java/io/questdb/griffin/engine/functions/table/ReadParquetPageFrameCursor.java (1)
97-101: Reasonable default for Parquet.Returning 0 avoids double-counting and matches the non-interval nature of this cursor.
core/src/main/java/io/questdb/cairo/RecordArray.java (1)
94-99: put(Record) looks good.The write path is correct and pairs with beginRecord()/recordSink.copy().
core/src/test/java/io/questdb/test/griffin/engine/table/PageFrameRecordCursorImplFactoryTest.java (2)
69-155: Solid functional test for symbol index + full partition path.Covers the projection/indexed row-cursor factory path well.
881-927: Helper test path is clean and asserts skip + calculateSize correctly.Good to validate both ASC and DESC orders via the same helper.
core/src/main/java/io/questdb/griffin/engine/table/PageFrameRecordCursorImpl.java (1)
84-87: Verification complete: all PageFrameCursor implementations define getRemainingRowsInInterval().The codebase contains 4 concrete implementations, all with the method defined:
- BwdTableReaderPageFrameCursor (line 97)
- FwdTableReaderPageFrameCursor (line 98)
- SelectedPageFrameCursor (line 343)
- ReadParquetPageFrameCursor (line 98)
The method is properly declared in the PageFrameCursor interface, and TablePageFrameCursor correctly extends it. The code addition at lines 84-87 is sound and complete.
core/src/main/java/io/questdb/cairo/IntervalBwdPartitionFrameCursor.java
Show resolved
Hide resolved
core/src/main/java/io/questdb/cairo/IntervalFwdPartitionFrameCursor.java
Show resolved
Hide resolved
core/src/main/java/io/questdb/cairo/IntervalFwdPartitionFrameCursor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/questdb/griffin/engine/functions/catalogue/PgAttrDefFunctionFactory.java
Show resolved
Hide resolved
bluestreak01
approved these changes
Nov 10, 2025
Contributor
Author
[PR Coverage check]😍 pass : 117 / 147 (79.59%) file detail
|
Member
|
CI was OOM killed perhaps: |
Contributor
Author
|
Looks like disk was full: ParallelCsvFileImporterTest.testIndexChunksInBigCsvByDay() failed first: Searched for That |
Member
|
yeah |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The web console displayed incorrect result set size, when the size of the first partition of the query's result exceeded the configured maximum page frame size.
Modified the size calculation in page frame cursors to account for the remaining size of the backing partition.
In addition also fixed few other size calculation related bugs in this PR.
Some factories did not account for the rows already consumed from the cursor.
Also fixes
PgAttrDefFunctionFactory, the cursor was resetting automatically without callingtoTop().