fix(sql): fix empty result set from limit queries#6504
Conversation
WalkthroughThe PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)core/src/main/java/io/questdb/griffin/SqlCodeGenerator.javacore/src/test/java/io/questdb/test/griffin/ExplainPlanTest.javaThanks 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 |
bluestreak01
added
Bug
|
@CodeRabbit review |
✅ 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: 1
🧹 Nitpick comments (2)
core/src/main/java/io/questdb/cairo/IntervalBwdPartitionFrameCursor.java (1)
121-123: Underflow guard forintervalLoincalculateSizelooks correct; consider mirroring innext()for consistencyThe new ternary:
long lo = intervalLo > Long.MIN_VALUE ? timestampFinder.findTimestamp(intervalLo - 1, 0, hi) : 0;correctly prevents
intervalLo - 1from underflowing whenintervalLo == Long.MIN_VALUE, and mapping that specific case tolo = 0matches the intended semantics for an open‑ended lower bound (start from the first row in the partition). Given thatcalculateSizeonly uses local copies of the cursor state, this is a safe, localized fix for size estimation in LIMIT scenarios.There is a very similar use of
intervalLo - 1innext()(Line 212) without the same guard. IfintervalLocan ever beLong.MIN_VALUEalong that path as well, you may want to apply an analogous check there for robustness and symmetry, even if it doesn’t currently manifest as a bug.Also applies to: 206-213
core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java (1)
6218-6233: Use of physical column name in table-query metadata looks correctSwitching the
TableColumnMetadataname tometadata.getColumnName(columnIndex)aligns result metadata with the physical table schema, which is consistent with how other parts of the generator treat base-table projections. The rest of the metadata (type, indexing, symbol flags, capacities) is still sourced from the samemetadatainstance, so behavior remains coherent.If you want to micro-optimize, you could cache
metadata.getColumnMetadata(columnIndex)into a local to avoid repeated calls, but it’s not critical.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/io/questdb/cairo/FullPartitionFrameCursorFactory.java(0 hunks)core/src/main/java/io/questdb/cairo/IntervalBwdPartitionFrameCursor.java(1 hunks)core/src/main/java/io/questdb/cairo/IntervalPartitionFrameCursorFactory.java(0 hunks)core/src/main/java/io/questdb/cairo/sql/PartitionFrameCursorFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java(1 hunks)core/src/test/java/io/questdb/test/griffin/ClickBenchTest.java(2 hunks)core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java(1 hunks)core/src/test/java/io/questdb/test/griffin/engine/join/AsOfJoinTest.java(4 hunks)
💤 Files with no reviewable changes (2)
- core/src/main/java/io/questdb/cairo/FullPartitionFrameCursorFactory.java
- core/src/main/java/io/questdb/cairo/IntervalPartitionFrameCursorFactory.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T12:21:00.062Z
Learnt from: jerrinot
Repo: questdb/questdb PR: 6413
File: core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java:11982-12002
Timestamp: 2025-11-19T12:21:00.062Z
Learning: QuestDB Java tests use a deterministic random seed. The test utilities (e.g., io.questdb.test.tools.TestUtils and io.questdb.std.Rnd) produce reproducible sequences, so rnd_* functions (including rnd_uuid4) yield deterministic outputs across runs. Do not flag tests in core/src/test/** that assert against values produced by rnd_* as flaky due to randomness.
Applied to files:
core/src/test/java/io/questdb/test/griffin/ClickBenchTest.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). (36)
- 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 Running tests with cover on linux-griffin-root)
- 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 (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (Trigger Enterprise CI Trigger Enterprise Pipeline)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (Check Changes Check changes)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
core/src/test/java/io/questdb/test/griffin/ClickBenchTest.java (2)
231-239: Plan expectation now matches actual column nameUpdating
values: [min(Eventdate),max(Eventdate)]to useEventdatealigns the Q6 plan expectation with the column name defined in the DDL and what the planner emits. Looks correct.
802-803: Good use ofrecordforTestCasehelperSwitching
TestCaseto aprivate recordkeeps the semantics (name/query/expectedPlan triple) while reducing boilerplate and enforcing immutability. No issues spotted.core/src/main/java/io/questdb/cairo/sql/PartitionFrameCursorFactory.java (1)
49-63: Switch-expression refactor keeps ordering semantics intactThe new switch expressions in
nameOf/reversepreserve the mapping of order constants to names and back, and handle unexpected values by falling back to"any"/ORDER_ANY. This is a clean, readable refactor.core/src/test/java/io/questdb/test/griffin/engine/join/AsOfJoinTest.java (2)
866-882: Explain-plan expectation updated to correct symbol nameThe expected plan for
testAsOfJoinLinearSearchHintnow checksfilter: market_data_symbol='sym_1', matching themarket_data_symbolcolumn defined in themarket_datatable. This aligns the assertion with the real plan output.
1605-1618: Consistent symbol casing in second linear-hint plan expectationSimilarly, the
Filtered AsOf Join Fastplan expectation now usesfilter: market_data_symbol='sym_1', keeping the explain-text assertion consistent with the table schema and the first EXPLAIN in this test.
[PR Coverage check]😍 pass : 5 / 10 (50.00%) file detail
|
3 participants
fixes #6501