fix(sql): between and inside sub-expression#7044
Conversation
Signed-off-by: Bruno Calza <brunoangelicalza@gmail.com>
|
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)
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 |
bluestreak01
left a comment
There was a problem hiding this comment.
Fix is correct and minimal. Verified against existing BETWEEN tests in SqlParserTest, ExpressionParserTest (295), WhereClauseParserTest (842), and IntervalFilterTest (26) — all pass. The new scopeStack.size() check is a strict superset of the old caseCount check, so CASE behavior is preserved while PAREN/BRACKET/ARRAY/CAST scopes are now handled correctly.
Non-blocking suggestions:
- PR title is missing the verb per CLAUDE.md (
fix(sql): fix BETWEEN ...). Fixes <URL>won't auto-close issue #6534; useFixes #6534.- Add
BugandSQLlabels. - Consider adding an execution-level test that asserts the resulting type-mismatch error, since the original bug surfaced as a runtime AssertionError.
Brings in 12 upstream commits (b6b3b15..c0c5638). Notable upstream changes: * feat(core): compact posting index for SYMBOL columns (#6861) plus follow-ups #7051 (seal indexes after WAL fast-lag commit) and #7052 (refresh covering sidecar mappings on reader reload). Adds DirectBitSet, bp_bitpack / FSST native helpers, and PostingIndexBenchmarkSuite. * fix(sql): preserve outer-join predicates, fix NOT in UNION, isolate parser state (#7027). * fix(sql): BETWEEN inside sub-expression (#7044). * fix(sql): JIT filter wrong results with UUID bind variables (#7049). * fix(core): avoid server-main error on WAL table rename race (#7046). * chore(ilp): fix QWP string-to-numeric parse status classification (#7038). * Build/CI chores: aux-job moved to Hetzner (#7054), latest IntelliJ for formatting (#7040), 'qwp' allowed as PR-title subtype, level switch added to review-pr skill (#7055). Conflict: core/src/main/resources/io/questdb/bin/linux-x86-64/libquestdb.so is a prebuilt native library; took master's version (same resolution as the prior 046c874 merge). Other native libs were updated cleanly without conflict. Auto-merged: SqlCompilerImpl.java, SqlCodeGenerator.java, SqlParser.java, SqlKeywords.java, TableColumnMetadata.java, TableWriter.java, AbstractCairoTest.java. Errand-specific files (MemFdFilesFacade, HybridCairoConfiguration, ArrowBulkExport, ArrowBulkIngest) untouched.
Summary
Fixes #6534
The issue here is not the malformed query. The problem is the
ANDinside a sub-expression. Currently, the parser thinks that theANDinside the sub-expression refers toBETWEEN'sANDoperator.There was a special treatment for
BETWEEN-CASE, so the problem does not arise inbut in other cases, such as
the issue arises. These are syntactically correct statements, and they should throw a type mismatch error.
The fix
We keep track of the scope depth when we reach
BETWEENon a variable calledbetweenStartScopeDepth, and whenever we see anANDwe use the current scope depth to know whether theANDrefers toBETWEENor not.This fix is identical to the treatment given to the
BETWEEN-CASE, but it's more generic. We had abetweenStartCaseCountvariable that we removed.How this was tested
This was manually tested, and a unit test was added for each one of the scope cases.