fix(sql): fix bugs, resource leaks and NPE in SQL engine#6874
Merged
bluestreak01 merged 3 commits intomasterfrom Mar 16, 2026
Merged
fix(sql): fix bugs, resource leaks and NPE in SQL engine#6874bluestreak01 merged 3 commits intomasterfrom
bluestreak01 merged 3 commits intomasterfrom
Conversation
SqlCodeGenerator: add missing break after DECIMAL cases in generateCastFunctions, preventing fall-through to BINARY that corrupted the cast function list in multi-column UNION queries. SqlCodeGenerator: fix generateFill to use getQuick(i) instead of getQuick(0), so each fill column reads its own fill value expression rather than always reading the first one. SqlCodeGenerator: fix GeoHash-to-VARCHAR cast functions to use GeoByteColumn (not GeoShortColumn) and fromType (not toTag), matching the correct pattern used by GeoHash-to-STRING casts. SqlOptimiser: fix moveClauses to increment the position counter after each matched clause, so all clauses marked for stealing are actually moved during join reordering. Without this, multi-column equi-joins on cross-joined tables created circular dependencies that broke topological sort. SqlOptimiser: fix hasGroupByFunc and hasOrderedGroupByFunc to traverse rhs and args of non-aggregate FUNCTION nodes. Previously these methods only followed lhs, missing nested aggregates like abs(sum(x)) and causing PIVOT to reject valid expressions. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
bluestreak01
added
Bug
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
SqlCodeGenerator: close filter function on all error paths in generateJoins const-filter handling. The filter leaked when it was non-boolean (threw without closing), constant true (abandoned), or constant false (returned without closing). SqlCodeGenerator: wrap generateSampleBy function allocations in try-catch. timezoneNameFunc, offsetFunc, sampleFromFunc, and sampleToFunc were created outside the outer try block and never freed on error paths from coerceRuntimeConstantType or subsequent factory construction. SqlParser: add null check for getTableTokenIfExists return value in parseCreateMatView. When WITH BASE specifies a nonexistent table not referenced in the query, the method returned null and the subsequent isView() call threw NullPointerException instead of SqlException. SqlParserTest: add test documenting known subQueryMode nesting corruption bug where nested parseAsSubQuery calls reset the flag prematurely, causing valid queries to be rejected. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
bluestreak01
changed the title
Fix generateSampleBy indentation after moving the try block up to wrap function parsing. Remove commented-out capacity overflow check in DirectLongList. Move shared block comment in PivotTest into each test method to avoid orphaned section headings. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
RaphDal
reviewed
Mar 16, 2026
Contributor
[PR Coverage check]😍 pass : 75 / 80 (93.75%) file detail
|
RaphDal
approved these changes
Mar 16, 2026
|
Well done |
maciulis
pushed a commit
to maciulis/questdb
that referenced
this pull request
Mar 16, 2026
|
@bluestreak01 Hey Bro Are You eligible for Swag |
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
5 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.
Summary
Correctness fixes
DECIMAL fall-through to BINARY in UNION casts:
generateCastFunctionslacked abreakafter the DECIMAL inner switch, causing fall-through tocase BINARY:that added a spuriousBinColumnto the cast function list. In multi-column UNION queries, this shifted subsequent column indices, causingUnsupportedOperationExceptionat runtime.moveClausesnever incrementing position counter: whenswapJoinOrder0needed to steal multiple join clauses, only the first was moved. The rest stayed in the wrong join context, creating circular dependencies that broke topological sort and caused query compilation failures on 3+ table cross joins with multi-column WHERE conditions.hasGroupByFunc/hasOrderedGroupByFuncskipping children of non-aggregate functions: theFUNCTIONcase didbreakinstead of falling through to thedefaulttraversal logic, so nested aggregates likeabs(sum(x))were invisible. This caused PIVOT to reject valid aggregate expressions with "expected aggregate function".Resource leak fixes
Const filter leaked in
generateJoins: thefilterfunction leaked when it was non-boolean (threw without closing), constant true (abandoned), or constant false (returned without closing).generateSampleByfunction leaks:timezoneNameFunc,offsetFunc,sampleFromFunc, andsampleToFuncwere created outside the outer try block and never freed on error paths.Crash fix
CREATE MATERIALIZED VIEW: whenWITH BASEspecifies a nonexistent table not referenced in the query,getTableTokenIfExists()returned null and.isView()threwNullPointerExceptioninstead of a descriptiveSqlException.Latent bug fixes (currently unreachable, fixed for correctness)
generateFillalways reading first fill value:getQuick(0)instead ofgetQuick(i)in a loop. Currently gated by the optimizer restricting multi-value FILL to a different code path.GeoHash-to-VARCHAR cast using wrong column type and parameter: all four GEO→VARCHAR casts used
toTaginstead offromType. Currently unreachable because UNION type resolution never produces VARCHAR as target for GEO sources.Known bug documented (fix deferred)
subQueryModenesting corruption: nestedparseAsSubQuerycalls reset the boolean flag prematurely, causing valid 3-level nested queries to be rejected. Both counter-based and save/restore fixes break 39 other parser tests — a proper fix requires a deeper refactor. Test added to document the issue.Test plan
UnionAllCastTest#testIntDecimalMultiColumn— multi-column INT→DECIMAL UNION ALL; fails withUnsupportedOperationExceptionbefore fixSqlOptimiserTest#testMoveClausesIncrementsPositionCounter— 3-table CROSS JOIN with multi-column WHERE; fails to compile before fixPivotTest#testPivotWithWrappedAggregate—abs(SUM(x))in PIVOT; rejected before fixPivotTest#testPivotWithCoalesceWrappedAggregate—coalesce(0, SUM(x))in PIVOT; rejected before fixPivotTest#testPivotWithCoalesceVarArgsWrappedAggregate—coalesce(NULL, NULL, SUM(x))in PIVOT; rejected before fixCreateMatViewTest#testCreateMatViewBaseTableNoReferenceAndDoesNotExist— NPE before fixSqlParserTest#testSubQueryModeNestedCorruption— documents known bugSampleByTestfull suite (283 tests) — verifies generateSampleBy leak fix has no regressions🤖 Generated with Claude Code