fix(sql): crash when using parallel ORDER BY with LIMIT and JIT filter#6482
fix(sql): crash when using parallel ORDER BY with LIMIT and JIT filter#6482bluestreak01 merged 3 commits intomasterfrom
Conversation
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 WalkthroughThis PR refactors bind variable memory preparation by extracting Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
|
@coderabbitai 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
🧹 Nitpick comments (1)
core/src/main/java/io/questdb/griffin/engine/table/AsyncFilterUtils.java (1)
120-189: Per‑type bind‑var serialization matches expected layout; consider tiny error‑message tweakThe switch over
ColumnType.tagOf(function.getType())covers the expected scalar and geo types, writes fixed‑size representations intoMemoryCARW(with the special FLOAT handling and 128‑bit UUID), and fails fast for unsupported types. The SYMBOL branch correctly ensures initialization withsymbolTableSource/executionContextbefore reading the int ID.As a minor polish, you might consider using
ColumnType.nameOf(columnType)instead ofcolumnTypeTagin the default case to preserve any additional type information in the error message, but this is non‑blocking.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/io/questdb/griffin/engine/table/AsyncFilterUtils.java(2 hunks)core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByAtom.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByNotKeyedAtom.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/table/AsyncJitFilteredRecordCursorFactory.java(1 hunks)core/src/main/java/io/questdb/griffin/engine/table/AsyncTopKAtom.java(2 hunks)core/src/test/java/io/questdb/test/cairo/fuzz/ParallelTopKFuzzTest.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-10T14:28:48.329Z
Learnt from: mtopolnik
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2025-11-10T14:28:48.329Z
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/griffin/engine/table/AsyncJitFilteredRecordCursorFactory.java
🧬 Code graph analysis (4)
core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByNotKeyedAtom.java (1)
core/src/main/java/io/questdb/griffin/engine/table/AsyncFilterUtils.java (1)
AsyncFilterUtils(48-190)
core/src/main/java/io/questdb/griffin/engine/table/AsyncTopKAtom.java (1)
core/src/main/java/io/questdb/griffin/engine/table/AsyncFilterUtils.java (1)
AsyncFilterUtils(48-190)
core/src/main/java/io/questdb/griffin/engine/table/AsyncJitFilteredRecordCursorFactory.java (1)
core/src/main/java/io/questdb/griffin/engine/table/AsyncFilterUtils.java (1)
AsyncFilterUtils(48-190)
core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByAtom.java (1)
core/src/main/java/io/questdb/griffin/engine/table/AsyncFilterUtils.java (1)
AsyncFilterUtils(48-190)
⏰ 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). (35)
- GitHub Check: New pull request (Coverage Report Coverage Report)
- 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 (Trigger Enterprise CI Trigger Enterprise Pipeline)
- 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 Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- 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 Griffin tests on linux-arm64)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (Check Changes Check changes)
🔇 Additional comments (12)
core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByNotKeyedAtom.java (1)
55-55: LGTM!Import correctly updated to use the centralized
AsyncFilterUtils.prepareBindVarMemory. The call site at line 248 remains functionally equivalent.core/src/main/java/io/questdb/griffin/engine/table/AsyncGroupByAtom.java (1)
69-69: LGTM!Import correctly updated to use the centralized
AsyncFilterUtils.prepareBindVarMemory, consistent with the refactor pattern across other atoms.core/src/main/java/io/questdb/griffin/engine/table/AsyncTopKAtom.java (2)
54-55: LGTM!Static import correctly added for the centralized bind variable memory preparation utility.
257-261: Key fix for the crash - properly initializes bind variable memory for parallel TopK queries.This addition correctly addresses the segmentation fault by ensuring bind variable memory is initialized before JIT-compiled filters access it. The pattern matches the initialization in
AsyncGroupByAtomandAsyncGroupByNotKeyedAtom.core/src/test/java/io/questdb/test/cairo/fuzz/ParallelTopKFuzzTest.java (4)
29-31: LGTM!Required imports added for bind variable test functionality.
151-163: Good test coverage for the fix.This test directly exercises the crash scenario from the PR description: parallel ORDER BY + LIMIT with a bind variable in the filter. The expected result correctly matches
testParallelTopKFilterwhich uses the literal'k0'.
165-167: Clean refactoring of the test helper.The delegation pattern keeps existing tests unchanged while adding bind variable support. The initializer is called early in the test setup (before table creation), which ensures bind variables are available throughout the test execution.
Also applies to: 169-178
211-213: LGTM!Simple and appropriate functional interface for test initialization.
core/src/main/java/io/questdb/griffin/engine/table/AsyncJitFilteredRecordCursorFactory.java (2)
60-60: LGTM!Import correctly updated to use the centralized
AsyncFilterUtils.prepareBindVarMemory, completing the refactor from this class.
372-377: Good refactor - AsyncJitFilterAtom.init() now uses the centralized utility.The initialization pattern remains consistent with other atoms while eliminating code duplication. The bind variable memory preparation is correctly called after
Function.init().core/src/main/java/io/questdb/griffin/engine/table/AsyncFilterUtils.java (2)
27-37: New imports align with added bind‑var handling helpersThe additional imports (ColumnType, SymbolTableSource, SqlException, SqlExecutionContext, CompiledFilterSymbolBindVariable) are all used by the new helper methods and look consistent with their responsibilities.
104-118: Bind‑var memory preparation is straightforward and side‑effect safeThe logic to truncate
bindVarMemoryonly whenbindVarFunctions.size() > 0and then serialize each function viawriteBindVarFunctionis clean and avoids unnecessary work when there are no bind variables. This should correctly initialize the bind‑var buffer before compiled filter execution.
…vars_in_parallel_topk
[PR Coverage check]😍 pass : 47 / 52 (90.38%) file detail
|
3 participants
Parallel ORDER BY + LIMIT queries that involve a JIT-compiled filter like the following one
were producing segfaults due to missing bind variable memory initialization.
The crash occurs both when bind variables are used explicitly, but also when the filter involves not-yet-existing symbols, e.g.
As a mitigation of this bug,
cairo.sql.parallel.topk.enabled=falsecan be set. This will disable the problematic execution plan.