fix(sql): fix EMA, VWEMA and KSUM failures in combined window queries#7030
Conversation
Implement pass1 materialization for EMA, VWEMA, and KSUM zero-pass variants, and clarify the WindowFunction pass-count contract. Add cached-window regression coverage for reordered EMA, VWEMA, and KSUM. Fixes #7006
|
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:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
core/src/test/java/io/questdb/test/griffin/engine/window/EmaWindowFunctionTest.java (2)
547-548: Method is out of alphabetical order.Per the project convention,
testEmaPeriodCachedWindowPartitionedReorderedReplayshould be placed beforetestEmaPeriodMode(line 472), not aftertestEmaPeriodOne(line 523).As per coding guidelines: "Group Java class members by kind (static vs. instance) and visibility, and sort them alphabetically. When adding new methods or fields, insert them in the correct alphabetical position among existing members of the same kind."
🤖 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/engine/window/EmaWindowFunctionTest.java` around lines 547 - 548, Move the test method testEmaPeriodCachedWindowPartitionedReorderedReplay so it appears alphabetically among the other instance test methods: place it before testEmaPeriodMode and not after testEmaPeriodOne; adjust its position within the class so all instance test methods remain grouped and sorted alphabetically per the project's member ordering convention.
551-556: Consolidate into a single multi-rowINSERT.As per coding guidelines: "Use a single INSERT statement to insert multiple rows in tests."
♻️ Proposed change
- execute("insert into tab values (1::timestamp, 2, 0, 10.0)"); - execute("insert into tab values (2::timestamp, 2, 1, 100.0)"); - execute("insert into tab values (3::timestamp, 1, 0, 20.0)"); - execute("insert into tab values (4::timestamp, 3, 1, 300.0)"); - execute("insert into tab values (5::timestamp, 3, 0, 30.0)"); - execute("insert into tab values (6::timestamp, 1, 1, 200.0)"); + execute(""" + INSERT INTO tab VALUES + (1::timestamp, 2, 0, 10.0), + (2::timestamp, 2, 1, 100.0), + (3::timestamp, 1, 0, 20.0), + (4::timestamp, 3, 1, 300.0), + (5::timestamp, 3, 0, 30.0), + (6::timestamp, 1, 1, 200.0) + """);🤖 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/engine/window/EmaWindowFunctionTest.java` around lines 551 - 556, Multiple separate execute("insert into tab values ...") calls violate the guideline to use a single multi-row INSERT in tests; update the test in EmaWindowFunctionTest by replacing the six individual execute(...) insert statements that target table "tab" with one execute call that issues a single INSERT INTO tab VALUES statement containing all six row tuples in the same order (preserve timestamps and columns) so the test uses one multi-row insert instead of multiple statements.core/src/test/java/io/questdb/test/griffin/engine/window/VwemaWindowFunctionTest.java (2)
558-559: Method is out of alphabetical order.
testVwemaPeriodCachedWindowPartitionedReorderedReplayshould sit neartestVwemaPeriodMode(line 450), not in the middle of the time-weighted block.As per coding guidelines: "Group Java class members by kind (static vs. instance) and visibility, and sort them alphabetically. When adding new methods or fields, insert them in the correct alphabetical position among existing members of the same kind."
🤖 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/engine/window/VwemaWindowFunctionTest.java` around lines 558 - 559, The test method testVwemaPeriodCachedWindowPartitionedReorderedReplay is out of alphabetical order in VwemaWindowFunctionTest; move the entire method declaration so it sits near testVwemaPeriodMode (i.e., within the same grouping of test methods) to maintain alphabetical ordering among instance test methods—ensure you cut the method starting at "public void testVwemaPeriodCachedWindowPartitionedReorderedReplay" and paste it in the correct alphabetical position adjacent to testVwemaPeriodMode, preserving imports, annotations (`@Test`) and existing formatting.
562-567: Consolidate into a single multi-rowINSERT.As per coding guidelines: "Use a single INSERT statement to insert multiple rows in tests."
♻️ Proposed change
- execute("insert into tab values ('2024-01-01T00:00:00.000000Z'::timestamp, 2, 'A', 10.0, 3.0)"); - execute("insert into tab values ('2024-01-01T00:00:01.000000Z'::timestamp, 2, 'B', 100.0, 1.0)"); - execute("insert into tab values ('2024-01-01T00:00:02.000000Z'::timestamp, 1, 'A', 20.0, 1.0)"); - execute("insert into tab values ('2024-01-01T00:00:03.000000Z'::timestamp, 3, 'B', 300.0, 4.0)"); - execute("insert into tab values ('2024-01-01T00:00:04.000000Z'::timestamp, 3, 'A', 30.0, 1.0)"); - execute("insert into tab values ('2024-01-01T00:00:05.000000Z'::timestamp, 1, 'B', 200.0, 2.0)"); + execute(""" + INSERT INTO tab VALUES + ('2024-01-01T00:00:00.000000Z'::timestamp, 2, 'A', 10.0, 3.0), + ('2024-01-01T00:00:01.000000Z'::timestamp, 2, 'B', 100.0, 1.0), + ('2024-01-01T00:00:02.000000Z'::timestamp, 1, 'A', 20.0, 1.0), + ('2024-01-01T00:00:03.000000Z'::timestamp, 3, 'B', 300.0, 4.0), + ('2024-01-01T00:00:04.000000Z'::timestamp, 3, 'A', 30.0, 1.0), + ('2024-01-01T00:00:05.000000Z'::timestamp, 1, 'B', 200.0, 2.0) + """);🤖 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/engine/window/VwemaWindowFunctionTest.java` around lines 562 - 567, Replace the six separate execute(...) insert calls in the VwemaWindowFunctionTest with a single execute call that performs a multi-row INSERT INTO tab VALUES (...),(...),(...),(...),(...),(...); keeping the exact timestamp literals and ::timestamp casts and the same order of columns/values; update the test method in VwemaWindowFunctionTest that contains the repeated execute calls so it uses one execute("insert into tab values (...),(...),..."); instead of multiple execute(...) invocations.
🤖 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/test/java/io/questdb/test/griffin/engine/window/EmaWindowFunctionTest.java`:
- Around line 547-574: The test only covers EmaOverPartitionFunction.pass1; add
three additional cached-window tests mirroring
testEmaPeriodCachedWindowPartitionedReorderedReplay to exercise the other pass1
variants: EmaTimeWeightedOverPartitionFunction.pass1,
EmaOverUnboundedRowsFrameFunction.pass1, and
EmaTimeWeightedOverUnboundedRowsFrameFunction.pass1. For each new test create
the same table shape and use queries that trigger the specific branch
(partitioned time-weighted: use OVER (partition by i order by sort_key) with
time-weighted EMA; non-partitioned fixed-alpha: use avg(val,'alpha',...) OVER
(order by sort_key); non-partitioned time-weighted: use time-weighted OVER
(order by sort_key)), follow the reordered-replay insert pattern from
testEmaPeriodCachedWindowPartitionedReorderedReplay, and assert using
assertQueryNoLeakCheck the expected output to ensure the
UnsupportedOperationException→computeNext+putDouble fix is exercised for those
factories.
In
`@core/src/test/java/io/questdb/test/griffin/engine/window/VwemaWindowFunctionTest.java`:
- Around line 558-585: Add three new tests mirroring
testVwemaPeriodCachedWindowPartitionedReorderedReplay to exercise the other
pass1 variants: create methods named e.g.
testVwemaTimeWeightedCachedWindowPartitionedReorderedReplay,
testVwemaPeriodCachedWindowUnboundedReorderedReplay, and
testVwemaTimeWeightedCachedWindowUnboundedReorderedReplay that use the same
table/data setup and assertQueryNoLeakCheck pattern; for
VwemaTimeWeightedOverPartitionFunction use avg(price, 'time', 3, volume) over
(partition by sym order by sort_key), for VwemaOverUnboundedRowsFrameFunction
use avg(price, 'period', 3, volume) over (order by sort_key) (no partition), and
for VwemaTimeWeightedOverUnboundedRowsFrameFunction use avg(price, 'time', 3,
volume) over (order by sort_key); keep the same ordering column ("ts") and the
same expected-result formatting as the existing test to ensure cached-window
(pass1) code paths are exercised.
In
`@core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java`:
- Around line 5117-5138: Add two tests mirroring testKSumRangeCachedWindow to
exercise the cached-window pass1 materialization for EMA and VWEMA: create
testEmaRangeCachedWindow and testVwemaRangeCachedWindow that use the same table
setup and rows, run queries "select ts, sym, val, ema(val) over (order by ts
range between 1 second preceding and current row) as ema_val, avg(val) over ()
as avg_all from tab" and similarly for vwema(val) (use the same window/frame and
expected output pattern), and assert results like the KSUM test; this will
exercise EmaDoubleWindowFunctionFactory and VwemaDoubleWindowFunctionFactory
pass1 paths analogous to testKSumRangeCachedWindow.
- Around line 5119-5137: In the assertMemoryLeak block in WindowFunctionTest
where you call executeWithRewriteTimestamp/create table and multiple
execute("insert into tab values ..."), replace the multiple single-row execute
inserts with one multi-row INSERT (single execute call containing all rows),
change numeric timestamp literals to underscore-separated forms (1_000_000,
2_000_000, 3_000_000, 4_000_000), and replace the final assertSql(...) call with
assertQueryNoLeakCheck(...) (keeping the same SQL and expected result string);
locate and update the code around executeWithRewriteTimestamp, the insert
execute calls, and the assertSql invocation in this test.
- Around line 4818-4835: Replace the use of assertSql(...) with
assertQueryNoLeakCheck(...) for the query assertion (change the call that
currently wraps the "select ts, sym, val, ksum(val) over..." verification),
collapse the four separate execute("insert ...") calls into a single multi-row
execute(...) INSERT statement (i.e. one INSERT with multiple value tuples) and
update numeric timestamp literals in the CREATE/INSERT SQL to use underscore
separators for numbers with 5+ digits (e.g. 1_000_000, 2_000_000, 3_000_000) so
the test uses assertQueryNoLeakCheck, a single multi-row INSERT via
execute(...), and underscore-separated numeric literals.
---
Nitpick comments:
In
`@core/src/test/java/io/questdb/test/griffin/engine/window/EmaWindowFunctionTest.java`:
- Around line 547-548: Move the test method
testEmaPeriodCachedWindowPartitionedReorderedReplay so it appears alphabetically
among the other instance test methods: place it before testEmaPeriodMode and not
after testEmaPeriodOne; adjust its position within the class so all instance
test methods remain grouped and sorted alphabetically per the project's member
ordering convention.
- Around line 551-556: Multiple separate execute("insert into tab values ...")
calls violate the guideline to use a single multi-row INSERT in tests; update
the test in EmaWindowFunctionTest by replacing the six individual execute(...)
insert statements that target table "tab" with one execute call that issues a
single INSERT INTO tab VALUES statement containing all six row tuples in the
same order (preserve timestamps and columns) so the test uses one multi-row
insert instead of multiple statements.
In
`@core/src/test/java/io/questdb/test/griffin/engine/window/VwemaWindowFunctionTest.java`:
- Around line 558-559: The test method
testVwemaPeriodCachedWindowPartitionedReorderedReplay is out of alphabetical
order in VwemaWindowFunctionTest; move the entire method declaration so it sits
near testVwemaPeriodMode (i.e., within the same grouping of test methods) to
maintain alphabetical ordering among instance test methods—ensure you cut the
method starting at "public void
testVwemaPeriodCachedWindowPartitionedReorderedReplay" and paste it in the
correct alphabetical position adjacent to testVwemaPeriodMode, preserving
imports, annotations (`@Test`) and existing formatting.
- Around line 562-567: Replace the six separate execute(...) insert calls in the
VwemaWindowFunctionTest with a single execute call that performs a multi-row
INSERT INTO tab VALUES (...),(...),(...),(...),(...),(...); keeping the exact
timestamp literals and ::timestamp casts and the same order of columns/values;
update the test method in VwemaWindowFunctionTest that contains the repeated
execute calls so it uses one execute("insert into tab values (...),(...),...");
instead of multiple execute(...) invocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb833b9e-42cf-467b-b2aa-e60b6d6bc8b0
📒 Files selected for processing (7)
core/src/main/java/io/questdb/griffin/engine/functions/window/EmaDoubleWindowFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/window/KSumDoubleWindowFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/functions/window/VwemaDoubleWindowFunctionFactory.javacore/src/main/java/io/questdb/griffin/engine/window/WindowFunction.javacore/src/test/java/io/questdb/test/griffin/engine/window/EmaWindowFunctionTest.javacore/src/test/java/io/questdb/test/griffin/engine/window/VwemaWindowFunctionTest.javacore/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java
|
a CI hiccup: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/src/test/java/io/questdb/test/griffin/engine/window/VwemaWindowFunctionTest.java (1)
468-468: Drop the banner section heading.This file already relies on alphabetical method ordering, and these banner comments are explicitly disallowed in QuestDB Java sources.
As per coding guidelines, "
Never insert // === or // --- banner comments as section headings in any Java file" and "Methods are sorted alphabetically and will not stay grouped by category."🤖 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/engine/window/VwemaWindowFunctionTest.java` at line 468, Remove the banner comment line "// ========================= Period and Time-Weighted Mode Tests =========================" from VwemaWindowFunctionTest (delete the standalone section-heading comment in the class) so the file relies on the existing alphabetical method ordering; ensure no other // === or // --- style banner comments remain in that test class.core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java (1)
4692-4707: Use Java text blocks for long SQL statements.Both tests build long INSERT/SELECT SQL via concatenation; converting to multiline strings will align with the test SQL conventions and improve readability.
♻️ Suggested refactor (apply similarly in both tests)
- execute("insert into tab values " + - "(1_000_000::timestamp, 'A', 10.0), " + - "(2_000_000::timestamp, 'B', 100.0), " + - "(2_000_000::timestamp, 'A', 30.0), " + - "(3_000_000::timestamp, 'B', 200.0)"); + execute(""" + insert into tab values + (1_000_000::timestamp, 'A', 10.0), + (2_000_000::timestamp, 'B', 100.0), + (2_000_000::timestamp, 'A', 30.0), + (3_000_000::timestamp, 'B', 200.0) + """); - "select ts, sym, val, ksum(val) over (partition by sym order by ts range between 1 second preceding and current row) as ksum_val, avg(val) over () as avg_all " + - "from tab", + """ + select ts, sym, val, + ksum(val) over (partition by sym order by ts range between 1 second preceding and current row) as ksum_val, + avg(val) over () as avg_all + from tab + """,As per coding guidelines: "Use multiline strings for longer SQL statements (multiple INSERT rows, complex queries), as well as to assert multiline query results in tests."
Also applies to: 5037-5052
🤖 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/engine/window/WindowFunctionTest.java` around lines 4692 - 4707, Replace the concatenated SQL strings in the test (the call to execute(...) that inserts rows and the assertQueryNoLeakCheck(...) select/expected strings using replaceTimestampSuffix) with Java text blocks (multiline string literals) to improve readability; specifically convert the long INSERT statement passed to execute and the expected-result and SELECT SQL passed to assertQueryNoLeakCheck into """...""" text blocks, preserving the same SQL content and spacing, and apply the same change to the other occurrence noted (around lines 5037-5052) so both tests use multiline text blocks instead of string concatenation.
🤖 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/test/java/io/questdb/test/griffin/engine/window/VwemaWindowFunctionTest.java`:
- Around line 35-55: The test method testVwemaAlphaEqualsOne (and the other test
blocks around lines referenced for the same file) currently call
assertQuery(...) — replace those calls with assertQueryNoLeakCheck(...) and wrap
the test body in assertMemoryLeak(() -> { ... }) so the query assertions perform
the package's leak/factory-property checks; update all similar uses in this file
(including the block covering the comment's 143-186 range) to use
assertQueryNoLeakCheck and assertMemoryLeak in the same pattern as the
cached-window regression tests.
---
Nitpick comments:
In
`@core/src/test/java/io/questdb/test/griffin/engine/window/VwemaWindowFunctionTest.java`:
- Line 468: Remove the banner comment line "// ========================= Period
and Time-Weighted Mode Tests =========================" from
VwemaWindowFunctionTest (delete the standalone section-heading comment in the
class) so the file relies on the existing alphabetical method ordering; ensure
no other // === or // --- style banner comments remain in that test class.
In
`@core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java`:
- Around line 4692-4707: Replace the concatenated SQL strings in the test (the
call to execute(...) that inserts rows and the assertQueryNoLeakCheck(...)
select/expected strings using replaceTimestampSuffix) with Java text blocks
(multiline string literals) to improve readability; specifically convert the
long INSERT statement passed to execute and the expected-result and SELECT SQL
passed to assertQueryNoLeakCheck into """...""" text blocks, preserving the same
SQL content and spacing, and apply the same change to the other occurrence noted
(around lines 5037-5052) so both tests use multiline text blocks instead of
string concatenation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d921088-36d1-428f-aa2b-7b55ac53fc3c
📒 Files selected for processing (3)
core/src/test/java/io/questdb/test/griffin/engine/window/EmaWindowFunctionTest.javacore/src/test/java/io/questdb/test/griffin/engine/window/VwemaWindowFunctionTest.javacore/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/io/questdb/test/griffin/engine/window/EmaWindowFunctionTest.java
bluestreak01
left a comment
There was a problem hiding this comment.
Verified the fix and the cached-window contract change. Empirically confirmed the new tests catch the regression by reverting one pass1() to throw — test fails as expected. Nice fix.
[PR Coverage check]😍 pass : 20 / 20 (100.00%) file detail
|
bluestreak01
left a comment
There was a problem hiding this comment.
Approved. Small, focused, correct fix for #7006 — the new pass1() implementations match the established pattern used by Avg/Max/StdDev/RowNumber/Rank ZERO_PASS factories. Verified all 10 modified variants are reachable via the cached executor when combined with a non-ZERO_PASS function, and hand-checked the new test expectations.
Three minor (non-blocking) nits:
VwemaOverUnboundedRowsFrameFunction.pass1()andVwemaTimeWeightedOverUnboundedRowsFrameFunction.pass1()usegetDouble(record)(which recomputesnumerator/denominator) instead of a cached field. Inconsistent with the partitioned variants that storevwema. Functionally correct, just an extra divide per row.- New VWEMA cached-path tests run only in micros mode because
VwemaWindowFunctionTestis not parametrized overTestTimestampType. Pre-existing pattern in the file, but the gap is now wider versus EMA/KSUM cached tests. - Pre-existing empty
reopen()override inKSumOverPartitionRowsFrameFunction(lines ~822-824) — drop in a future cleanup.
Brings in 4 upstream commits (c0c5638..8735521): * feat(core): add local parquet metadata sidecar file for optimized query planning (#6913). Introduces the `_pm` sidecar produced by `ParquetMetadataWriter`, the qdb-parquet-meta Rust crate, the pm_generate / pm_inspect binaries, Mig940 to backfill existing partitions, and a refactor of attachPartition / O3 parquet paths to derive parquet state from the sidecar. * fix(sql): EMA, VWEMA and KSUM failures in combined window queries (#7030). * chore(core): another posting index fix avoiding corrupt results (#7062). * docs(core): bump Java requirement to 25 in core/README (#7036). Conflict 1: core/src/main/resources/io/questdb/bin/linux-x86-64/libquestdbr.so is a prebuilt native library; took master's version (same resolution as prior 046c874 / 4302e10 merges). Other native libs updated cleanly. Conflict 2: MemoryTag.java tag-name registration. Took both - the errand-side NATIVE_MEMFD_STORAGE entry and the new master-side MMAP_PARQUET_METADATA_READER entry are independent. Semantic merge: TableWriter.attachPartition. Master refactored the method to detect parquet partitions from the post-rename `_pm` file (`parquetFileSize > -1`) and dropped the local `boolean isParquet`. Auto-merge silently kept errand's three `isParquet` references (skip attachValidateMetadata, upsert column tops instead of iterateDir, skip configureAppendPosition) without their declaration. Restored `boolean isParquet` and the early `PARQUET_PARTITION_NAME` probe so errand's parquet-attach optimisations stay intact alongside master's new sidecar generation block. Path is reset via trimTo after the detection probe. Also relocated the parquet-testing nested submodule from core/rust/qdbr/parquet2/testing/parquet-testing to core/rust/parquet2/testing/parquet-testing per the upstream rename; removed the now-empty leftover directory under core/rust/qdbr/parquet2. Build: mvn package -DskipTests -pl core -am succeeds. Tests not run in this commit.
Fixes #7006
Fixes valid window queries using
EMA,VWEMA, orKSUMthat could fail when the planner chose cached window execution.These functions already worked in streaming execution, but some query shapes require cached execution, for example when an incremental window function is used together with another window expression. In those cases, users could hit an execution failure even though the SQL was valid.