perf(sql): breaking change 💥 - Improve query execution time by folding constant expressions#6828
perf(sql): breaking change 💥 - Improve query execution time by folding constant expressions#6828bluestreak01 merged 21 commits intomasterfrom
Conversation
|
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:
WalkthroughFunctionParser introduces safer resource handling with try/catch wrapping and new helpers for adaptive timestamp constant conversion. Multiple test files update expected output to reflect changes in constant representation and query plan formatting for arrays, numeric literals, and timestamps. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
… in switch function tests
…ry plan output for dateadd function
|
@coderabbitai review critically |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java (1)
8835-8841:⚠️ Potential issue | 🟠 MajorAdd explicit regression coverage for Infinity/-Infinity folding-to-NULL.
This segment validates finite constant folding, but the PR’s documented breaking behavior is specifically about non-finite double/float constants becoming NULL at compile time. Please add a targeted explain-plan assertion so this doesn’t regress silently.
🧪 Suggested test addition
+ `@Test` + public void testSelectWithJittedFilter22bInfinityConstantsFoldToNull() throws Exception { + assertPlan("create table tab ( d double, ts timestamp);", "select * from tab where d = 'Infinity'::double", """ + Async Filter workers: 1 + filter: d is null + PageFrame + Row forward scan + Frame forward scan on: tab + """); + + assertPlan("create table tab ( d double, ts timestamp);", "select * from tab where d = '-Infinity'::double", """ + Async Filter workers: 1 + filter: d is null + PageFrame + Row forward scan + Frame forward scan on: tab + """); + }
🧹 Nitpick comments (1)
core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java (1)
9093-9131: Test intent/comments are now stale after cast folding in plan text.These tests now assert folded literals (
l=12,s=1,l=1024L, etc.), but several method comments still frame them as “type mismatch due to cast.” Please update names/comments to reflect the current behavior and keep intent clear.Also applies to: 9220-9224, 9288-9291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java` around lines 9093 - 9131, The test comments and/or method names for testSelectWithNonJittedFilter1, testSelectWithNonJittedFilter10, testSelectWithNonJittedFilter11, and testSelectWithNonJittedFilter12 are stale (they describe “type mismatch due to cast” or similar) because cast folding now produces folded literals in the plan; update each test's JavaDoc/comment and, if desired, the test method names to reflect the new behavior (e.g., change "jit is not used due to type mismatch" / "jit filter doesn't work with type casts" / "TODO: should run with jitted filter..." to a concise description that the cast is folded and the plan shows folded literals like l=12, s=1, b=true, l=1024L), and apply the same comment/name updates to the other occurrences noted (around the tests at the referenced locations) so the intent matches the asserted plan text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/io/questdb/griffin/FunctionParser.java`:
- Around line 377-382: When performing early constant folding in the block that
calls functionToConstant(arg) inside parseFunction(), wrap the call in a
try/catch so that if functionToConstant throws you first release the arg you
just popped and also release any Functions already moved into mutableArgs before
rethrowing; specifically, after polling functionStack and popping positionStack,
enclose the functionToConstant(arg) invocation in a try, and in the catch call
the appropriate release/close method on arg and iterate mutableArgs to
release/close each Function, then rethrow the exception so parseFunction() does
not leak Function resources.
In
`@core/src/test/java/io/questdb/test/griffin/engine/functions/date/TimestampAddWithTimezoneFunctionFactoryTest.java`:
- Around line 380-383: The test asserts a TIMESTAMP_NS value using
microsecond-aligned nanoseconds which can hide precision loss; in
TimestampAddWithTimezoneFunctionFactoryTest update the explain expectation and
the ISO literal so they use a non-micro-aligned ns value (replace the
micro-aligned 1587275359886758000L::timestamp_ns and the matching ISO ending
like "...000Z" with a timestamp containing non-micro-aligned nanoseconds) so any
precision downgrade from TIMESTAMP_NS becomes detectable.
---
Nitpick comments:
In `@core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java`:
- Around line 9093-9131: The test comments and/or method names for
testSelectWithNonJittedFilter1, testSelectWithNonJittedFilter10,
testSelectWithNonJittedFilter11, and testSelectWithNonJittedFilter12 are stale
(they describe “type mismatch due to cast” or similar) because cast folding now
produces folded literals in the plan; update each test's JavaDoc/comment and, if
desired, the test method names to reflect the new behavior (e.g., change "jit is
not used due to type mismatch" / "jit filter doesn't work with type casts" /
"TODO: should run with jitted filter..." to a concise description that the cast
is folded and the plan shows folded literals like l=12, s=1, b=true, l=1024L),
and apply the same comment/name updates to the other occurrences noted (around
the tests at the referenced locations) so the intent matches the asserted plan
text.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/io/questdb/griffin/FunctionParser.javacore/src/test/java/io/questdb/test/cairo/ArrayTest.javacore/src/test/java/io/questdb/test/cairo/mv/MatViewTest.javacore/src/test/java/io/questdb/test/cutlass/pgwire/PGArraysTest.javacore/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.javacore/src/test/java/io/questdb/test/griffin/ExplainPlanTest.javacore/src/test/java/io/questdb/test/griffin/engine/functions/conditional/SwitchFunctionFactoryTest.javacore/src/test/java/io/questdb/test/griffin/engine/functions/date/TimestampAddWithTimezoneFunctionFactoryTest.java
.../questdb/test/griffin/engine/functions/date/TimestampAddWithTimezoneFunctionFactoryTest.java
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@RaphDal there are some edge cases that don't quite work still:
|
This make sense, |
|
@RaphDal we should get test coverage to 100% here:
|
…s and improve reassociation tests
[PR Coverage check]😍 pass : 102 / 102 (100.00%) file detail
|
…g constant expressions (questdb#6828)


This pull request improves query execution performance by constants folding inner functions. This is effective when a full expression cannot be folded.
Summary
FunctionParser.visit()now folds each constant argument into its evaluated constant form before passing it tocreateFunction(), eliminating redundantper-row evaluation of constant sub-expressions like
'2026-01-01'::timestampor1024.1 + 1.FunctionParseronly folded constants at the top level (line 329), when the final result ofparseFunction()was constant. Intermediate constantsub-expressions passed as arguments to non-constant parent functions were never folded. e.g., in
x < '2026-01-01'::timestamp, the ::timestamp cast remaineda
CastStrToTimestampFunctionand re-executed on every row.functionToConstant()method, which already handles closing the original function when a new constant is returned. Guardconditions mirror the top-level fold:
isConstant(),extendedOps() == null, plus aTypeConstantexclusion to avoid folding type-only marker constants (likeArrayTypeConstant) that carry no evaluable value.Constant reassociation
The constant folding above only applies when constant sub-expressions are already grouped together in the AST. Because SQL is parsed left-to-right, left-associative chains like
col + 1 + 4produce the tree(col + 1) + 4, where the two constants1and4are separated by the column reference and cannot be folded.ExpressionNode.reassociateConstants()runs a single bottom-up pass over the expression tree before function parsing and regroups constants of the same associative operator into a single subtree:(col + 1) + 4→col + (1 + 4)→col + 5(col * 2) * 3→col * (2 * 3)→col * 64 + (col + 1)→col + (4 + 1)→col + 5(commutative operators)The method handles four structural patterns for each pair of adjacent operator nodes, gated on whether the operator is associative, commutative, or both. Operators that are not associative (
-,/,%) are left unchanged.Each operator declares its
associativeandcommutativeproperties inOperatorExpression. The rewrite relinks existingExpressionNodeinstances without allocating new nodes, consistent with QuestDB's zero-GC design.Breaking change
Float and double
Infinity/-Infinityvalues are now treated as NULL at compile time.Numbers.isNull(float)andNumbers.isNull(double)already considerInfinityand-Infinityas NULL, andFloatConstant.newInstance()/DoubleConstant.newInstance()collapse non-finite values toNaN(the NULL sentinel).Before this change, expressions like
cast('Infinity' as float)remained as unevaluatedCastStrToFloatFunctionobjects at runtime, bypassing this convention.getFloat()returnedFloat.POSITIVE_INFINITYdirectly, allowing downstream consumers likeCASE/SWITCHto distinguish Infinity from NULL.With constant folding, these cast expressions are now evaluated at compile time through
FloatConstant.newInstance()/DoubleConstant.newInstance(), which applies QuestDB's NULL convention:Infinity,-Infinity, andNaNall become NULL.CASEexpressions can no longer branch onInfinityor-Infinityas distinct float/double values.Benchmark
Query executed on Apple M5 with the clickbench dataset. Measured with pgbench
18.1: 5 seconds warmup and 30 seconds execution.Query:
Query plans
Master:
Patch:
Test plan
ConstantReassociationTest— unit tests for all four reassociation patterns (A, B, Mirror A, Mirror B), multi-level chains, logical AND/OR, n-ary functions, bind variables, NULL, and negative cases (subtraction, division, modulo)ExplainPlanTest— integration tests verifying end-to-end folding in query plans: addition (d + 1 + 4→d + 5), bitwise AND (l & 3 & 5→l & 1), commutative pattern (4 + (d + 1)→d + 5), and bind variable exclusion