Skip to content

perf(sql): breaking change 💥 - Improve query execution time by folding constant expressions#6828

Merged
bluestreak01 merged 21 commits intomasterfrom
rd_constant_folding
Mar 5, 2026
Merged

perf(sql): breaking change 💥 - Improve query execution time by folding constant expressions#6828
bluestreak01 merged 21 commits intomasterfrom
rd_constant_folding

Conversation

@RaphDal
Copy link
Copy Markdown
Contributor

@RaphDal RaphDal commented Feb 27, 2026

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 to createFunction(), eliminating redundant
    per-row evaluation of constant sub-expressions like '2026-01-01'::timestamp or 1024.1 + 1.
  • Previously, FunctionParser only folded constants at the top level (line 329), when the final result of parseFunction() was constant. Intermediate constant
    sub-expressions passed as arguments to non-constant parent functions were never folded. e.g., in x < '2026-01-01'::timestamp, the ::timestamp cast remained
    a CastStrToTimestampFunction and re-executed on every row.
  • The folding reuses the existing functionToConstant() method, which already handles closing the original function when a new constant is returned. Guard
    conditions mirror the top-level fold: isConstant(), extendedOps() == null, plus a TypeConstant exclusion to avoid folding type-only marker constants (like
    ArrayTypeConstant) 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 + 4 produce the tree (col + 1) + 4, where the two constants 1 and 4 are 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) + 4col + (1 + 4)col + 5
  • (col * 2) * 3col * (2 * 3)col * 6
  • 4 + (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 associative and commutative properties in OperatorExpression. The rewrite relinks existing ExpressionNode instances without allocating new nodes, consistent with QuestDB's zero-GC design.

Breaking change

Float and double Infinity / -Infinity values are now treated as NULL at compile time.

Numbers.isNull(float) and Numbers.isNull(double) already consider Infinity and -Infinity as NULL, and FloatConstant.newInstance() / DoubleConstant.newInstance() collapse non-finite values to NaN (the NULL sentinel).

Before this change, expressions like cast('Infinity' as float) remained as unevaluated CastStrToFloatFunction objects at runtime, bypassing this convention. getFloat() returned Float.POSITIVE_INFINITY directly, allowing downstream consumers like CASE/SWITCH to 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, and NaN all become NULL. CASE expressions can no longer branch on Infinity or -Infinity as 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:

SELECT SUM(AdvEngineID), COUNT(*), AVG(ResolutionWidth)
FROM read_parquet('hits.parquet')
WHERE EventTime < '2013-07-06T00:00:00.000000Z'::timestamp;

-- master: 921ms
-- patch: 336ms

Query plans

Master:

 Async Group By workers: 10
   values: [sum(AdvEngineID),count(*),avg(ResolutionWidth)]
   filter: EventTime<'2013-07-06T00:00:00.000000Z'::timestamp
     parquet page frame scan
       columns: AdvEngineID,ResolutionWidth,EventTime

Patch:

 Async Group By workers: 10
   values: [sum(AdvEngineID),count(*),avg(ResolutionWidth)]
   filter: EventTime<2013-07-06T00:00:00.000000Z
     parquet page frame scan
       columns: AdvEngineID,ResolutionWidth,EventTime

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 + 4d + 5), bitwise AND (l & 3 & 5l & 1), commutative pattern (4 + (d + 1)d + 5), and bind variable exclusion

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 27, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3f5159d3-899d-4ac9-825a-7f2f6a1d3f00

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Walkthrough

FunctionParser 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

Cohort / File(s) Summary
Core Function Parsing & Constant Handling
core/src/main/java/io/questdb/griffin/FunctionParser.java
Added try/catch for resource cleanup in functionToConstant, early constant conversion within visitor loop, adaptive timestamp constant conversion with helper methods extractYearFromTimestamp and getAdaptiveTimestampType for nano-timestamp range detection.
Array & Plan Representation Tests
core/src/test/java/io/questdb/test/cairo/ArrayTest.java, core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java
Updated expected array subscript syntax and numeric literal representations in explain plans (simplified casts and arithmetic expressions).
Materialized View & Error Message Tests
core/src/test/java/io/questdb/test/cairo/mv/MatViewTest.java, core/src/test/java/io/questdb/test/cutlass/pgwire/PGArraysTest.java
Updated batch insert result timestamps and changed jagged array error message from "element counts don't match" to "array shapes don't match".
Plan Formatting Tests
core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java, core/src/test/java/io/questdb/test/griffin/engine/functions/date/TimestampAddWithTimezoneFunctionFactoryTest.java
Updated numeric literal formatting in explain plans (iL vs 'i'::long) and timestamp literal representation from nanosecond form to ISO 8601 UTC format.
Function Test Expectations
core/src/test/java/io/questdb/test/griffin/engine/functions/conditional/SwitchFunctionFactoryTest.java
Remapped expected output values for null-key cases in switch function tests, altering final column expectations without changing control flow.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

Performance, SQL

Suggested reviewers

  • bluestreak01
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.26% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: constant folding of function expressions to improve query performance, with a note about the breaking change.
Description check ✅ Passed The pull request description clearly relates to the changeset, explaining the constant folding optimization, breaking changes, and test adjustments present across multiple files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch rd_constant_folding

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@RaphDal RaphDal changed the title perf(sql): Improve query execution time by folding constant expressions perf(sql): breaking change 💥 - Improve query execution time by folding constant expressions Feb 27, 2026
@RaphDal
Copy link
Copy Markdown
Contributor Author

RaphDal commented Mar 2, 2026

@coderabbitai review critically

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 2, 2026

@RaphDal: I'll conduct a thorough, critical review of this PR. I'll examine the constant folding implementation, scrutinize the breaking change implications around Infinity/-Infinity handling, look for edge cases, verify the design decisions, and provide concrete line-cited findings with follow-up actions where needed.


🧠 Learnings used
Learnt from: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: In questdb/questdb, for PRs requested by javier, provide line-cited, push-back style reviews with concrete follow-ups (tests, diffs, and performance considerations).

Learnt from: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: For questdb/questdb, javier expects a rigorous, push-back style review; always include concrete line-cited findings and propose targeted tests/fixes when reviewing javier’s PRs.

Learnt from: jerrinot
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-04T11:18:36.492Z
Learning: For PRs by jerrinot, always use a thorough and critical review mode: question design decisions, challenge assumptions, scrutinize implementations for edge cases and potential issues, and avoid superficial validation. They prefer hard-core reviews that push back on the code.
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Add 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

📥 Commits

Reviewing files that changed from the base of the PR and between fc24a7f and 7685af7.

📒 Files selected for processing (8)
  • core/src/main/java/io/questdb/griffin/FunctionParser.java
  • core/src/test/java/io/questdb/test/cairo/ArrayTest.java
  • core/src/test/java/io/questdb/test/cairo/mv/MatViewTest.java
  • core/src/test/java/io/questdb/test/cutlass/pgwire/PGArraysTest.java
  • core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java
  • core/src/test/java/io/questdb/test/griffin/ExplainPlanTest.java
  • core/src/test/java/io/questdb/test/griffin/engine/functions/conditional/SwitchFunctionFactoryTest.java
  • core/src/test/java/io/questdb/test/griffin/engine/functions/date/TimestampAddWithTimezoneFunctionFactoryTest.java

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@bluestreak01
Copy link
Copy Markdown
Member

@RaphDal there are some edge cases that don't quite work still:

image

@RaphDal
Copy link
Copy Markdown
Contributor Author

RaphDal commented Mar 3, 2026

@RaphDal there are some edge cases that don't quite work still:

This make sense, ExpressionParser parses it as (usage_user + 1) + 4 which isn't foldable from FunctionParser perspective.
This is a bit out-of-scope for this PR which focuses on constant folding of expressions and the feature you're looking for is expression reassociate.
Added in 8f9b90a.

@bluestreak01
Copy link
Copy Markdown
Member

@RaphDal we should get test coverage to 100% here:

image

@glasstiger
Copy link
Copy Markdown
Contributor

[PR Coverage check]

😍 pass : 102 / 102 (100.00%)

file detail

path covered line new line coverage
🔵 io/questdb/griffin/model/ExpressionNode.java 61 61 100.00%
🔵 io/questdb/griffin/OperatorExpression.java 26 26 100.00%
🔵 io/questdb/griffin/ExpressionTreeBuilder.java 1 1 100.00%
🔵 io/questdb/griffin/FunctionParser.java 14 14 100.00%

@bluestreak01 bluestreak01 merged commit 42483f0 into master Mar 5, 2026
48 of 50 checks passed
@bluestreak01 bluestreak01 deleted the rd_constant_folding branch March 5, 2026 14:15
maciulis pushed a commit to maciulis/questdb that referenced this pull request Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants