fix(sql): incorrect results produced by SAMPLE BY with mixed timestamp and aggregate expressions#6254
Conversation
…p and aggregate expressions
puzpuzpuz
added
Bug
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors SqlOptimiser temporary state to use new buffers (tempBoolList, tempIntList, tempColumns, tempColumns2) in place of prior lists, updating related methods and control paths. Adds two tests validating scenarios with timestamps mixed with aggregates: one for materialized views and one for SAMPLE BY rewrite and plans. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant P as Parser
participant O as SqlOptimiser
participant E as Execution Engine
C->>P: Submit SQL with SAMPLE BY / aggregates
P->>O: AST
rect rgba(230,245,255,0.6)
note over O: Rewrite and planning (updated buffers)
O->>O: Initialize tempBoolList / tempIntList / tempColumns / tempColumns2
O->>O: Analyze group-by / sample-by predicates
O->>O: Track timestamp and aggregate flags via tempBoolList
O->>O: Manage column/index sets via tempColumns* and tempIntList
O-->>P: Physical plan
end
P-->>E: Plan
E-->>C: Result set (sampled/aggregated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
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 |
|
@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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/io/questdb/griffin/SqlOptimiser.java (1)
5368-5375: Prevent dropping existing projection columns
needRemoveColumnsis incremented for every entry inmissingDependedTokens, even whenaddBottomUpColumnIfNotExistsdoes not add anything because the dependency column is already present. In those cases the wrapper constructed later subtracts too many columns and removes legitimate user-selected columns from the projection. Example:SELECT ts, value, ts + value FROM x SAMPLE BY ...ends up returning onlytsbecausevaluegets “removed” despite never being reintroduced.Only mark a dependency for later removal when we actually insert it:
- for (int i = 0, size = missingDependedTokens.size(); i < size; i++) { - model.addBottomUpColumnIfNotExists(nextColumn(missingDependedTokens.get(i))); - } - needRemoveColumns += missingDependedTokens.size(); + for (int i = 0, size = missingDependedTokens.size(); i < size; i++) { + final CharSequence dependency = missingDependedTokens.get(i); + if (model.getAliasToColumnMap().excludes(dependency)) { + model.addBottomUpColumn(nextColumn(dependency)); + needRemoveColumns++; + } + }This keeps existing projection columns intact while still cleaning up the temporary ones.
🧹 Nitpick comments (1)
core/src/test/java/io/questdb/test/cairo/mv/MatViewTest.java (1)
4314-4334: Don’t pre-sort the results; assert natural ordering.Issue #6239 is about unordered rows. Passing "ts" to assertQueryNoLeakCheck sorts the actual rows in the harness, masking ordering bugs. Compare without sorting to validate the engine’s natural order.
Apply this diff:
- assertQueryNoLeakCheck( - """ - ts\tCoverage - 2009-12-31T23:00:00.000000Z\t0.08333333333333333 - 2019-12-31T23:00:00.000000Z\t0.041666666666666664 - 2029-12-31T23:00:00.000000Z\t0.041666666666666664 - """, - "x_view", - "ts", - true, - true - ); + assertQueryNoLeakCheck( + """ + ts\tCoverage + 2009-12-31T23:00:00.000000Z\t0.08333333333333333 + 2019-12-31T23:00:00.000000Z\t0.041666666666666664 + 2029-12-31T23:00:00.000000Z\t0.041666666666666664 + """, + "x_view" + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/io/questdb/griffin/SqlOptimiser.java(20 hunks)core/src/test/java/io/questdb/test/cairo/mv/MatViewTest.java(1 hunks)core/src/test/java/io/questdb/test/griffin/engine/groupby/SampleByTest.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). (28)
- 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 (SelfHosted Griffin And Fuzz tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
- GitHub Check: New pull request (Hosted Running tests on windows-other)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo)
- 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)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-other)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-cairo)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-fuzz)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (Hosted Running tests with cover on linux-griffin)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
- GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (2)
core/src/test/java/io/questdb/test/cairo/mv/MatViewTest.java (1)
4294-4336: Solid regression test for mixed ts + aggregates in SAMPLE BY.Covers the reported case well and validates MV refresh/state. Looks good.
core/src/test/java/io/questdb/test/griffin/engine/groupby/SampleByTest.java (1)
6128-6162: Great regression coverage for mixed timestamp rewriteAppreciate how this locks in both the ordered results and the expected plan shape when aggregates coexist with the sampled timestamp—exactly the scenario from #6239. Solid guard against the rewrite regressing again.
This comment was spot-on. The fix is in 4a97543 |
bluestreak01
left a comment
bluestreak01
left a comment
There was a problem hiding this comment.
while the result is correct, the execution plan is highly sub-optimal. this should do sub-query extraction instead. E.g. aggregate on ts, count() and do calculations on top. Right now the whole thing degrades in performance massively.
This is not degradation as such queries were not supported by the rewrite. Better to have correct results rather than get some garbage fast. The extraction must be implemented optimally, i.e. with aggregate reuse, so I suggest merging this patch to get a couple critical bugs fixed while I'm going to start working on mixed ts + aggregate expressions support in a separate PR. WDYT? |
|
Failed test is #6266, unrelated with these changes. I can reproduce locally, so going to fix it in a separate branch. |
[PR Coverage check]😍 pass : 61 / 63 (96.83%) file detail
|
3 participants
Fixes #6239
SAMPLE BY rewrite fix 1
SAMPLE BY to parallel GROUP BY rewrite was not handling mixed timestamp and aggregate function expressions. This was leading to unordered and incorrect results returned from the query.
Example of a problematic query:
The above query was incorrectly rewritten into something like this (note that this is a simplified representation, without the actual GROUP BY details in the nested query):
The fix disables the rewrite for such queries.
SAMPLE BY rewrite fix 2
The rewrite could throw away columns from the result set in case of certain expressions involving the designated timestamp column. Here is an example:
Before this patch, the last column (
c) would not be returned from the query.