feat(sql): window definition by javier · Pull Request #6746 · questdb/questdb

javier · 2026-02-04T17:58:51Z

Adding WINDOW definition for reusable window clauses in a single query

This PR goes in tandem with the docs at questdb/documentation#361

So you can do things like:

SELECT symbol, side, price,
       sum(price) OVER ws, avg(price) OVER ws,
       sum(amount) OVER ws, sum(price) OVER wt
FROM trades
WINDOW wt AS (ORDER BY TIMESTAMP),
       ws AS (PARTITION BY SYMBOL, side ORDER BY TIMESTAMP);

Window Inheritance

This PR also implements SQL standard window inheritance, where a named window
can reference another named window as its base, inheriting PARTITION BY, ORDER BY,
and frame clauses:

SELECT avg(price) OVER w2
FROM trades
WINDOW w1 AS (PARTITION BY symbol ORDER BY ts),
       w2 AS (w1 ROWS BETWEEN 1 PRECEDING AND CURRENT ROW)

Chained inheritance is supported:

WINDOW w1 AS (PARTITION BY category),
       w2 AS (w1 ORDER BY ts),
       w3 AS (w2 ROWS BETWEEN CURRENT ROW AND CURRENT ROW)

Merge rules follow the SQL standard:

PARTITION BY: always inherited from base (child cannot specify its own)
ORDER BY: child's takes precedence if specified, otherwise inherited
Frame: child's takes precedence if non-default, otherwise inherited

Validation:

Base window must be defined earlier in the WINDOW clause (no forward references)
PARTITION BY in a child window referencing a base is rejected
Circular and self-references are prevented

Test plan

Basic inheritance: child adds frame on top of base ORDER BY
Inherit PARTITION BY, child adds ORDER BY
Inherit PARTITION BY + ORDER BY, child adds frame
Override ORDER BY from base
Override frame from base
Chained inheritance (w3 -> w2 -> w1)
Multiple functions sharing an inherited window
Inherited window in subquery context
Inherit everything from base without additions
Error: circular reference (caught as forward reference)
Error: base window not defined
Error: PARTITION BY in child when base is referenced
Error: forward reference (base defined after child)
Existing named window tests continue to pass

Add support for SQL WINDOW clause allowing named window definitions that can be referenced by multiple window functions: SELECT sum(x) OVER w, avg(x) OVER w FROM t WINDOW w AS (ORDER BY ts ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) Changes: - WindowExpression: add windowName field and copySpecFrom() method - QueryModel: add namedWindows map for storing window definitions - SqlParser: parse WINDOW clause after ORDER BY, before LIMIT - ExpressionParser: handle OVER w syntax and parseWindowSpec() - SqlCodeGenerator: resolve named window references - SqlOptimiser: validate named windows and copy to new window models - SqlKeywords: fix isWindowKeyword() index bug Known limitation: ORDER BY columns must be in SELECT list for now. This will be addressed in a follow-up commit.

Two bugs fixed: 1. Window function deduplication incorrectly treated different named window references as duplicates because compareWindowExpressions() didn't compare the windowName field. Added windowName to both comparison and hash functions in ExpressionNode. 2. Lexer buffer reuse caused second window name to be stored as empty string. Fixed by calling Chars.toString() immediately when capturing the window name in SqlParser. Added test testMultipleNamedWindowsPartitionedFirst to cover both bugs.

Named windows are stored on the original SELECT model, but during optimization a SELECT_MODEL_WINDOW wrapper is created. The column propagation code was only checking the immediate model, not the nested model hierarchy, causing "Invalid column" errors when ORDER BY or PARTITION BY columns weren't in the SELECT list. Added lookupNamedWindow() helper that walks the model hierarchy to find named window definitions. Updated emitColumnLiteralsTopDown() and rewriteSelectClause0() to use this helper.

coderabbitai · 2026-02-04T17:59:00Z

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.

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

Adds named WINDOW clause support and shared inline window-spec parsing: parsing, model storage, optimiser resolution, validation, and extensive tests for named-window semantics and equivalence with inline OVER specifications.

Changes

Cohort / File(s)	Summary
Parsing & inline spec `core/src/main/java/io/questdb/griffin/ExpressionParser.java`, `core/src/main/java/io/questdb/griffin/SqlParser.java`	Introduce `parseWindowSpec` (public) to parse inline PARTITION/ORDER/FRAME and EXCLUDE. Modify OVER parsing to accept named window references or delegate to `parseWindowSpec`. Parse and store named `WINDOW name AS (...)` clauses in the master model; validate identifier rules and error messages.
Optimiser: resolution & normalization `core/src/main/java/io/questdb/griffin/SqlOptimiser.java`	Add bottom-up named-window lookup and resolution helpers to replace named references with copied specs; normalize window frames for PRECEDING/FOLLOWING/CURRENT ROW during validation and ensure propagation through model rewrites.
Model additions `core/src/main/java/io/questdb/griffin/model/QueryModel.java`, `core/src/main/java/io/questdb/griffin/model/WindowExpression.java`	Add `namedWindows` map to `QueryModel`. Extend `WindowExpression` with `windowName`, position, `setWindowName`, `isNamedWindowReference`, `getWindowName*`, and `copySpecFrom()` to clone a window specification.
Keyword fix `core/src/main/java/io/questdb/griffin/SqlKeywords.java`	Fix off-by-one index checks in `isWindowKeyword` for 6-character keyword detection (correct character indices).
Tests — parser/keyword/unit `core/src/test/java/io/questdb/test/griffin/ExpressionParserTest.java`, `core/src/test/java/io/questdb/test/griffin/SqlParserTest.java`, `core/src/test/java/io/questdb/test/griffin/SqlKeywordsTest.java`	Update expected error message for unterminated OVER to reference "window specification". Add multiple tests validating invalid window names (keywords, numbers) in OVER and WINDOW clauses and a case-insensitive `isWindowKeyword` test.
Tests — window function suites `core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java`, `core/src/test/java/io/questdb/test/griffin/engine/window/NamedWindowFuzzTest.java`	Add extensive functional tests for named-window behavior, equivalence to inline specs, deduplication, framing, exclusion modes, interactions with joins/CTEs/subqueries, and a fuzz tester comparing named vs inline outcomes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

feat(sql): support nested window functions with deduplication optimization #6643: Overlaps in window-function parsing, optimisation, and window-expression handling (named/window spec logic).
feat(sql): implement arithmetic with window functions #6626: Related changes to OVER-clause parsing and shared parser logic; may conflict in ExpressionParser parsing paths.
fix(sql): fix reverse-order argument parsing in multi-arg window function #6697: Touches ExpressionParser window parsing behavior and argument handling; relevant for integrating inline/named spec changes.

Suggested labels

New feature

Suggested reviewers

bluestreak01
puzpuzpuz

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.73% 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 'feat(sql): window definition' clearly and concisely summarizes the main change—adding SQL WINDOW definition support for reusable named window clauses.
Description check	✅ Passed	The PR description clearly explains the feature (SQL WINDOW definition for reusable window clauses), provides concrete examples of the new syntax, documents window inheritance with merge rules, and outlines a comprehensive test plan.

_{✏️ 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 jv/window_definition

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.}

…OIN disambiguation - Fix named windows not working with CTEs and subqueries by storing them on masterModel instead of the FROM model in parseFromClause() - Rewrite WINDOW JOIN lookahead logic to use cleaner backTo() approach, avoiding unnecessary String allocations during disambiguation - Add 9 tests for WINDOW clause with CTEs, subqueries, and nested queries - Add 2 tests verifying explain plans are identical for named vs inline windows

Remove windowEndPos variable that was assigned but never used.

bluestreak01 · 2026-02-05T19:00:38Z

@CodeRabbit pls review critically

coderabbitai · 2026-02-05T19:04:33Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR details
gh pr view 6746 --json title,body,files

# Get the list of changed files with their status
gh pr diff 6746 --name-only

Length of output: 1980

🏁 Script executed:

#!/bin/bash
# Read the key implementation files to understand the changes
# Start with SqlParser.java to see how WINDOW is parsed
gh pr diff 6746 -- core/src/main/java/io/questdb/griffin/SqlParser.java

Length of output: 36

🏁 Script executed:

#!/bin/bash
# Check ExpressionParser.java changes (has lower coverage at 61.84%)
gh pr diff 6746 -- core/src/main/java/io/questdb/griffin/ExpressionParser.java

Length of output: 36

🏁 Script executed:

#!/bin/bash
# Check SqlOptimiser.java changes
gh pr diff 6746 -- core/src/main/java/io/questdb/griffin/SqlOptimiser.java

Length of output: 36

🏁 Script executed:

#!/bin/bash
# Check the new WindowExpression model class
gh pr diff 6746 -- core/src/main/java/io/questdb/griffin/model/WindowExpression.java

Length of output: 36

🏁 Script executed:

#!/bin/bash
# Check SqlCodeGenerator.java to see how window definitions are generated
gh pr diff 6746 -- core/src/main/java/io/questdb/griffin/model/SqlCodeGenerator.java

Length of output: 36

🏁 Script executed:

#!/bin/bash
# Look at test files to understand what scenarios are covered
fd -e java . core/src/test --exec grep -l "WINDOW.*AS" {} \; | head -20

Length of output: 80

🏁 Script executed:

#!/bin/bash
# Check if there are specific tests for window definitions
rg -n "WINDOW.*AS.*\(" --type java -g "*Test.java" -C 2 | head -100

Length of output: 11527

🏁 Script executed:

#!/bin/bash
# Get the diff for each file - fixing command syntax
gh pr diff 6746 > /tmp/pr_diff.txt
cat /tmp/pr_diff.txt

Length of output: 63703

🏁 Script executed:

#!/bin/bash
# Check if hasOffset method exists in ExpressionParser
rg -n "hasOffset" --type java core/src/main/java/io/questdb/griffin/ExpressionParser.java

Length of output: 619

🏁 Script executed:

#!/bin/bash
# Look for hasOffset in the entire codebase to see if it exists
rg -n "hasOffset\s*\(" --type java -C 3

Length of output: 2651

🏁 Script executed:

#!/bin/bash
# Check what SQL keywords are allowed/prevented as identifiers
rg -n "isKeyword|KEYWORD" --type java core/src/main/java/io/questdb/griffin/SqlKeywords.java | head -30

Length of output: 1277

🏁 Script executed:

#!/bin/bash
# Examine the lexer.backTo signature to understand the null parameter
rg -n "def.*backTo|void backTo|public.*backTo" --type java core/src/main/java/io/questdb/griffin/

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Check for existing identifier validation logic in the parser
rg -n "isValidIdentifier|validateIdentifier|valid.*identifier" --type java core/src/main/java/io/questdb/griffin/ -i

Length of output: 1142

🏁 Script executed:

#!/bin/bash
# See how other identifiers (table names, column names) are validated
rg -n "table name expected|column name expected" --type java core/src/main/java/io/questdb/griffin/SqlParser.java -A 5 -B 5 | head -50

Length of output: 2441

🏁 Script executed:

#!/bin/bash
# Check test coverage - what specific scenarios are NOT tested
rg -n "testNamed" --type java core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java

Length of output: 1820

🏁 Script executed:

#!/bin/bash
# Check the validateIdentifier implementation to see what it does
rg -n "private void validateIdentifier" --type java core/src/main/java/io/questdb/griffin/SqlParser.java -A 10

Length of output: 535

🏁 Script executed:

#!/bin/bash
# Check how lexer.backTo is used elsewhere to understand the null parameter
rg -n "lexer\.backTo" --type java core/src/main/java/io/questdb/griffin/SqlParser.java -B 2 -A 2

Length of output: 384

🏁 Script executed:

#!/bin/bash
# Check if there are tests for SQL keywords used as identifiers in other contexts
rg -n "keyword.*identifier|keyword.*name" --type java core/src/test/java/io/questdb/test/griffin/ -i | head -20

Length of output: 482

🏁 Script executed:

#!/bin/bash
# Check lookupNamedWindow usage - it's called multiple times, potential performance issue
rg -n "lookupNamedWindow" --type java core/src/main/java/io/questdb/griffin/SqlOptimiser.java -B 2 -A 2

Length of output: 1038

🏁 Script executed:

#!/bin/bash
# Check if the SqlKeywords bug fix is tested
rg -n "isWindowKeyword" --type java core/src/test -A 3 -B 3

Length of output: 584

🏁 Script executed:

#!/bin/bash
# Look for existing tests that use SQL keywords as identifiers
rg -n "SELECT|FROM.*select|window.*window" --type java core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java | grep -i "keyword\|'select'\|'from'\|'window'" | head -10

Length of output: 0

@bluestreak01 I'll provide a hard-core critical review as requested. I found several concerning issues:

Critical Issues

1. Inadequate Window Name Validation 🔴

The window name validation is dangerously weak:

// Lines 524-527 in ExpressionParser.java
if (tok.length() == 1 && !Character.isLetterOrDigit(tok.charAt(0))) {
    throw SqlException.$(lexer.lastTokenPosition(), "'(' or window name expected after OVER");
}

Problems:

Only validates single-character names! Multi-character names like "drop", "select", "@#$%" pass through unchecked
Doesn't prevent SQL keywords from being used as window names
Doesn't check for special characters, unicode, or other invalid identifiers
There's an existing validateIdentifier() method in SqlParser (line 4616) that handles quoted identifiers and proper validation - why isn't it being used?

Same issue in SqlParser.java line 2875-2877 - identical weak validation.

2. SqlKeywords.isWindowKeyword Bug Fix Not Tested 🟡

You fixed a real bug (checking indices 3,4,5 instead of 4,5,6), but SqlKeywordsTest.java:255 explicitly excludes isWindowKeyword from testing:

excludedCases.add("isWindowKeyword");

Why is this excluded? The bug proves this needs testing.

3. Late Error Detection - Design Flaw 🔴

Named windows are resolved at code generation time (SqlCodeGenerator), not during parsing or optimization. This means:

Users won't get "window 'x' is not defined" errors until very late in query processing
The optimizer operates on unresolved window references, potentially making suboptimal decisions
Error positions might be less accurate

Why not resolve immediately after parsing the WINDOW clause? You have all the information at that point.

4. Performance: Repeated Lookups 🟡

lookupNamedWindow() walks the model hierarchy every time it's called. In SqlOptimiser.java, this happens:

Line 2900: During emitColumnLiteralsTopDown
Line 8518: During replaceLiteralList

For a query with N window functions using M named windows across D depth of nested models, this is O(N * M * D) lookups. Could these be cached or resolved once?

5. Lookahead Logic Fragility 🟡

Lines 2628-2650 in SqlParser.java:

lexer.backTo(windowLastPos, null);  // Line 2643

You're passing null as the second parameter to backTo. Looking at other usage (line 2534), it passes the actual token. What does null mean here? Is this safe? The lexer might use that parameter for state restoration.

6. Changed Deduplication Behavior 🟡

In ExpressionNode.java, you changed hashing to include window names:

CharSequence windowName = w.getWindowName();
int hash = windowName != null ? Chars.lowerCaseHashCode(windowName) : 0;

This means sum(x) OVER w1 and sum(x) OVER w2 won't deduplicate even if w1 and w2 have identical specifications. Is this intentional? This could lead to duplicate window computation.

Conversely, sum(x) OVER (ORDER BY ts) and sum(x) OVER w where w = (ORDER BY ts) also won't deduplicate. Comment says "haven't been resolved yet at deduplication time" - this confirms the late-resolution design flaw.

7. Low Coverage on Critical Code 🔴

ExpressionParser.java has only 61.84% coverage despite adding 145 new lines. This is your parsing entry point - it should have the highest coverage! What paths aren't tested?

8. Missing Edge Case Tests

No tests for:

SQL keywords as window names: WINDOW select AS (...)
Reserved words: WINDOW window AS (...)
Special characters: WINDOW @#$ AS (...)
Very long names (10,000+ chars)
Unicode/emoji: WINDOW 窗口 AS (...)
Quoted identifiers: WINDOW "my window" AS (...)
Empty name after AS: WINDOW AS (...)
Undefined window in nested subquery context
Window name that collides with table/column names

9. Unclear Model Copying Logic 🟡

Lines 8770 and 8782 in SqlOptimiser.java:

// Copy named windows from original model for WINDOW clause support
windowModel.getNamedWindows().putAll(model.getNamedWindows());

Why is this necessary? If a window wrapper model is created during optimization, why does it need the named windows? What happens if there are naming conflicts between nested models? The comment doesn't explain the reasoning.

10. Inconsistent Error Messages

Line 520: "'(' or window name expected after OVER"
Line 2883: "window name expected"
Line 6793: "window '").put(windowName).put("' is not defined"

The first two should be identical but aren't. Use consistent error messages.

Questions

Why not use validateIdentifier() for window names? It already handles all the edge cases.
Why resolve named windows at codegen time instead of immediately after parsing? This delays error detection and complicates optimization.
Should named windows deduplicate based on their resolved specification? Current behavior prevents this.
What's the performance impact of repeated lookupNamedWindow() calls? Have you measured this with deeply nested queries?
Why is isWindowKeyword excluded from SqlKeywordsTest? Especially after fixing a bug in it?

Recommendations

Replace the weak validation with validateIdentifier() - reuse existing, battle-tested code
Add comprehensive edge case tests - especially keywords and special characters
Resolve named windows immediately after parsing - fail fast, optimize better
Cache resolved window specs - avoid repeated model hierarchy walks
Re-enable isWindowKeyword testing - add it to SqlKeywordsTest
Get ExpressionParser coverage above 90% - this is critical parsing code

The feature works for the happy path, but the edge cases and design decisions need hardening. The validation gaps are concerning.

🧠 Learnings used

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.

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6643
File: core/src/main/java/io/questdb/griffin/SqlOptimiser.java:8671-8687
Timestamp: 2026-01-20T00:59:19.630Z
Learning: QuestDB’s parser rejects pure window functions (e.g., row_number(), rank()) used without an OVER clause; such invocations are invalid and fail early, so validators don’t need to catch pure-name window functions in window specs.

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6673
File: core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java:2616-2618
Timestamp: 2026-01-19T21:43:19.228Z
Learning: In test `testWindowJoinFailsWhenSlaveDoesNotSupportTimeFrames` in `core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java`, the use of `AND` between timestamp equality conditions (e.g., `ts = 'X' AND ts = 'Y'`) is intentional to prevent intrinsic interval creation. This ensures the window join fails as expected when the slave doesn't support time frames, which is necessary after PR `#6673` added OR timestamp intrinsics recognition.

- Deduplicate window spec parsing between OVER and WINDOW clauses - Fix hash/equality contract in ExpressionNode (case-insensitive match) - Validate window names with validateIdentifier and keyword rejection - Fix WINDOW/JOIN disambiguation with robust 2-token lookahead - Cache lookupNamedWindow results to avoid repeated model traversals - Guard stopOrderByPropagate for unresolved named windows - Remove isWindowKeyword test exclusion in SqlKeywordsTest - Remove dead code in SqlOptimiser - Add comprehensive edge case tests (UNION ALL, GROUP BY, RANGE frame, ROWS frame, multiple windows, scope isolation, keyword rejection, etc.)

Move named window resolution (OVER w where w is defined in WINDOW clause) from SqlCodeGenerator to SqlOptimiser. This enables proper deduplication of named vs inline windows with identical specs and removes conservative ORDER BY propagation guards that were needed when specs were unresolved. - Add resolveNamedWindows() to SqlOptimiser, called before rewriteSelectClause - Remove resolveNamedWindowReference() from SqlCodeGenerator - Simplify emitColumnLiteralsTopDown and rewriteSelectClause0 (no ad-hoc lookups) - Remove windowName from ExpressionNode hash/compare (resolved before dedup) - Remove conservative stopOrderByPropagate guard for unresolved names - Add NamedWindowFuzzTest with combinatorial named-vs-inline equivalence checks - Add tests for dedup, table aliases, edge cases, and window spec error paths

The assertEquivalent method catches all exceptions internally and wraps them in AssertionError, so the throws Exception declaration is redundant.

javier · 2026-02-06T18:07:53Z

@CodeRabbit pls review critically

coderabbitai · 2026-02-06T18:22:29Z

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the actual diff to review the implementation details more carefully
gh pr diff 6746 -- core/src/main/java/io/questdb/griffin/ExpressionParser.java core/src/main/java/io/questdb/griffin/SqlParser.java core/src/main/java/io/questdb/griffin/SqlOptimiser.java core/src/main/java/io/questdb/griffin/model/WindowExpression.java core/src/main/java/io/questdb/griffin/SqlKeywords.java

# Check test files to see what's actually tested
echo "=== TEST FILES ==="
fd -e java "WindowFunctionTest.java" core/src/test --exec cat {}

Length of output: 281767

@javier Understood—running a targeted, critical pass now. I fixed the previous CLI usage and will extract the exact code paths and tests related to WINDOW name parsing/validation, named-window resolution timing, lookups/caching, keyword handling, and dedicated tests for WINDOW x AS (...). I’ll follow up with precise findings and line numbers.

🏁 Script executed:

#!/bin/bash
set -e

echo "=== Changed files (name-only) ==="
gh pr diff 6746 --name-only | tee /tmp/changed.txt

echo
echo "=== Grep for WINDOW clause in main code ==="
rg -nP "(?i)\\bWINDOW\\b" core/src/main/java/io/questdb/griffin -C3 || true

echo
echo "=== Search for error messages around window names ==="
rg -nP "(?i)\\('\\(' or window name expected after OVER|window name expected|window '.*' is not defined" core/src/main/java/io/questdb/griffin -n -C2 || true

echo
echo "=== validateIdentifier occurrences in SqlParser ==="
rg -n "validateIdentifier\\(" core/src/main/java/io/questdb/griffin/SqlParser.java -n -C3 || true

echo
echo "=== isWindowKeyword diff (to verify index fix) ==="
gh pr diff 6746 -- core/src/main/java/io/questdb/griffin/SqlKeywords.java || true

echo
echo "=== lookupNamedWindow declarations and calls (to assess repeated lookups) ==="
rg -n "lookupNamedWindow\\(" core/src/main/java -C3 || true

echo
echo "=== backTo() signature and usages (to validate null arg safety) ==="
rg -nP "\\bbackTo\\s*\\(" core/src/main/java -C3 || true

echo
echo "=== ExpressionNode changed in this PR? ==="
grep -F "core/src/main/java/io/questdb/griffin/engine/ExpressionNode.java" /tmp/changed.txt || echo "ExpressionNode.java not listed as changed"

echo
echo "=== Tests that declare named WINDOW (WINDOW x AS (...)) ==="
rg -nP "(?i)\\bWINDOW\\s+([\"`\\w]+)\\s+AS\\s*\\(" core/src/test -C1 || true

echo
echo "=== SqlKeywordsTest mentions isWindowKeyword and any exclusions ==="
rg -n "SqlKeywordsTest|isWindowKeyword|excludedCases" core/src/test -n -C3 || true

echo
echo "=== Diff for key files (first ~200 lines each if changed) ==="
for f in core/src/main/java/io/questdb/griffin/SqlParser.java \
         core/src/main/java/io/questdb/griffin/ExpressionParser.java \
         core/src/main/java/io/questdb/griffin/SqlOptimiser.java \
         core/src/main/java/io/questdb/griffin/model/WindowExpression.java \
         core/src/main/java/io/questdb/griffin/model/QueryModel.java; do
  if grep -F "$f" /tmp/changed.txt >/dev/null; then
    echo "---- DIFF: $f ----"
    gh pr diff 6746 -- "$f" | sed -n '1,200p'
  else
    echo "---- SKIP (not changed): $f ----"
  fi
done

echo
echo "=== Does SqlCodeGenerator.java change in this PR? ==="
grep -F "SqlCodeGenerator.java" /tmp/changed.txt || true

# If present, show a small diff slice
if grep -F "SqlCodeGenerator.java" /tmp/changed.txt >/dev/null; then
  path=$(grep -F "SqlCodeGenerator.java" /tmp/changed.txt | head -n1)
  echo "---- DIFF: $path ----"
  gh pr diff 6746 -- "$path" | sed -n '1,200p'
fi

Length of output: 75

@javier Kicking off a targeted, critical pass. The last script failed because tee isn’t available in this sandbox; I’ve re-run with a robust script that avoids tee and captures all the WINDOW-definition touchpoints (parser, optimizer, keywords, tests). I’ll follow up with line-cited findings and concrete recommendations as soon as the results return.

🏁 Script executed:

#!/bin/bash

echo "=== Changed files (name-only, first 200) ==="
gh pr diff 6746 --name-only > /tmp/changed.txt 2>/dev/null || true
nl -ba /tmp/changed.txt | sed -n '1,200p' || true

echo
echo "=== Grep for WINDOW clause in main code ==="
rg -nP "(?i)\\bWINDOW\\b" core/src/main/java/io/questdb/griffin -C3 || true

echo
echo "=== Search for error messages around window names ==="
rg -nP "(\\('\\(' or window name expected after OVER\\)|window name expected|window '.*' is not defined)" core/src/main/java/io/questdb/griffin -n -C2 || true

echo
echo "=== validateIdentifier occurrences in SqlParser ==="
rg -n "validateIdentifier\\(" core/src/main/java/io/questdb/griffin/SqlParser.java -n -C3 || true

echo
echo "=== isWindowKeyword diff (to verify index fix) ==="
if grep -F "core/src/main/java/io/questdb/griffin/SqlKeywords.java" /tmp/changed.txt >/dev/null; then
  gh pr diff 6746 -- core/src/main/java/io/questdb/griffin/SqlKeywords.java | sed -n '1,400p'
else
  echo "SqlKeywords.java not changed"
fi

echo
echo "=== lookupNamedWindow declarations and calls (to assess repeated lookups) ==="
rg -n "lookupNamedWindow\\(" core/src/main/java -C3 || true

echo
echo "=== backTo() signature and usages (to validate null arg safety) ==="
rg -nP "\\bbackTo\\s*\\(" core/src/main/java -C3 || true
rg -nP "class\\s+\\w*Lexer\\b" core/src/main/java -n -C2 || true

echo
echo "=== ExpressionNode changed in this PR? ==="
if grep -F "core/src/main/java/io/questdb/griffin/engine/ExpressionNode.java" /tmp/changed.txt >/dev/null; then
  echo "Diff: ExpressionNode.java (first 160 lines)"
  gh pr diff 6746 -- core/src/main/java/io/questdb/griffin/engine/ExpressionNode.java | sed -n '1,160p'
else
  echo "ExpressionNode.java not changed in this PR"
fi

echo
echo "=== Tests that declare named WINDOW (WINDOW x AS (...)) ==="
rg -nP "(?i)\\bWINDOW\\s+([\"`\\w]+)\\s+AS\\s*\\(" core/src/test -C1 || true

echo
echo "=== SqlKeywordsTest mentions isWindowKeyword and any exclusions ==="
rg -n "SqlKeywordsTest|isWindowKeyword|excludedCases" core/src/test -n -C3 || true

echo
echo "=== Diff for key files (first ~220 lines each if changed) ==="
for f in core/src/main/java/io/questdb/griffin/SqlParser.java \
         core/src/main/java/io/questdb/griffin/ExpressionParser.java \
         core/src/main/java/io/questdb/griffin/SqlOptimiser.java \
         core/src/main/java/io/questdb/griffin/model/WindowExpression.java \
         core/src/main/java/io/questdb/griffin/model/QueryModel.java; do
  if grep -F "$f" /tmp/changed.txt >/dev/null; then
    echo "---- DIFF: $f (first 220 lines) ----"
    gh pr diff 6746 -- "$f" | sed -n '1,220p'
  else
    echo "---- SKIP (not changed): $f ----"
  fi
done

echo
echo "=== Does SqlCodeGenerator.java change in this PR? ==="
if grep -F "SqlCodeGenerator.java" /tmp/changed.txt >/dev/null; then
  path=$(grep -F "SqlCodeGenerator.java" /tmp/changed.txt | head -n1)
  echo "---- DIFF: $path (first 220 lines) ----"
  gh pr diff 6746 -- "$path" | sed -n '1,220p'
else
  echo "SqlCodeGenerator.java not changed"
fi

Length of output: 286992

@javier Running a tighter scan now to avoid the earlier sandbox issues and extract only what we need (parser/optimizer/keywords/tests). I’ll follow up with specific findings and actions based on the results.

🏁 Script executed:

#!/bin/bash

# List changed files plainly
echo "=== Changed files (name-only) ==="
gh pr diff 6746 --name-only > /tmp/changed.txt 2>/dev/null || true
cat /tmp/changed.txt || true

echo
echo "=== Check if key files changed ==="
for f in \
  core/src/main/java/io/questdb/griffin/SqlParser.java \
  core/src/main/java/io/questdb/griffin/ExpressionParser.java \
  core/src/main/java/io/questdb/griffin/SqlOptimiser.java \
  core/src/main/java/io/questdb/griffin/SqlKeywords.java \
  core/src/main/java/io/questdb/griffin/model/WindowExpression.java \
  core/src/main/java/io/questdb/griffin/model/QueryModel.java \
  core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java \
  core/src/main/java/io/questdb/griffin/engine/ExpressionNode.java; do
  if grep -F "$f" /tmp/changed.txt >/dev/null; then
    echo "CHANGED: $f"
  else
    echo "not changed: $f"
  fi
done

echo
echo "=== Small diffs for changed key files (first 200 lines) ==="
for f in \
  core/src/main/java/io/questdb/griffin/SqlParser.java \
  core/src/main/java/io/questdb/griffin/ExpressionParser.java \
  core/src/main/java/io/questdb/griffin/SqlOptimiser.java \
  core/src/main/java/io/questdb/griffin/SqlKeywords.java \
  core/src/main/java/io/questdb/griffin/model/WindowExpression.java \
  core/src/main/java/io/questdb/griffin/model/QueryModel.java \
  core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java \
  core/src/main/java/io/questdb/griffin/engine/ExpressionNode.java; do
  if grep -F "$f" /tmp/changed.txt >/dev/null; then
    echo "---- DIFF SLICE: $f ----"
    gh pr diff 6746 -- "$f" | sed -n '1,200p'
  fi
done

echo
echo "=== Pinpoint patterns in main code (narrow searches) ==="
# Where validateIdentifier is declared/used
rg -n "validateIdentifier\\(" core/src/main/java/io/questdb/griffin/SqlParser.java || true

# Look for window name error messages or undefined named window errors
rg -nP "(\\('\\(' or window name expected after OVER\\)|window name expected|window '.*' is not defined)" core/src/main/java/io/questdb/griffin || true

# lookupNamedWindow usage hotspots
rg -n "lookupNamedWindow\\(" core/src/main/java/io/questdb/griffin || true

# Lexer backTo API occurrences around WINDOW parsing
rg -n "backTo\\s*\\(" core/src/main/java/io/questdb/griffin || true

# isWindowKeyword method diff (if changed)
if grep -F "core/src/main/java/io/questdb/griffin/SqlKeywords.java" /tmp/changed.txt >/dev/null; then
  echo "---- DIFF SLICE: SqlKeywords.java (first 200) ----"
  gh pr diff 6746 -- core/src/main/java/io/questdb/griffin/SqlKeywords.java | sed -n '1,200p'
fi

echo
echo "=== Tests with named WINDOW definitions (WINDOW x AS (...)) ==="
rg -nP "(?i)\\bWINDOW\\s+([\"`A-Za-z_][\\w$]*)\\s+AS\\s*\\(" core/src/test/java -g "*Test.java" -C1 || true

echo
echo "=== Any test exclusions for isWindowKeyword ==="
rg -n "isWindowKeyword|excludedCases" core/src/test/java || true

Length of output: 5770

@javier Here’s a focused, critical review of the WINDOW definition work with line-cited findings and concrete follow-ups.

What’s changed and what hasn’t

Changed: core/src/main/java/io/questdb/griffin/{SqlParser.java, ExpressionParser.java, SqlOptimiser.java, SqlKeywords.java}, model/{WindowExpression.java, QueryModel.java}, plus tests: ExpressionParserTest, SqlKeywordsTest, SqlParserTest, engine/window/NamedWindowFuzzTest, WindowFunctionTest.
Not changed: core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java and core/src/main/java/io/questdb/griffin/engine/ExpressionNode.java. So any previous concern about codegen-time resolution or ExpressionNode-based dedup changes does not apply to this PR.

Findings

Window-name identifier validation is inconsistent

SqlParser has validateIdentifier(GenericLexer, CharSequence) and uses it in several places (hits at Lines 2879, 3546, 3549, 3646, 3649, 3793; definition at Line 4618).
In the OVER branch, ExpressionParser throws only on absence: "'(' or window name expected after OVER" at Line 513, but we found no call to validateIdentifier there. That implies OVER may accept syntactically invalid or keyword names unless rejected elsewhere.
Recommendation: In ExpressionParser’s OVER path, call SqlParser.validateIdentifier(lexer, nameTok) so both WINDOW definitions and OVER references share the same rules.

Undefined named windows are detected in the optimiser, not the parser

SqlOptimiser resolves names via lookupNamedWindow(QueryModel, windowName) (decl at Line 4145) and throws "window '' is not defined" at Line 6179 after attempting resolution at Line 6177.
This is earlier than codegen (good), but still later than parse. Users will only see undefined-name errors after optimisation begins. Consider a small post-parse, pre-optimisation pass to resolve/canonicalise named windows and fail fast (improves error locality and plan quality).

Potential repeated lookups

Named-window resolution path is via lookupNamedWindow(model, name) (Line 4145) and used at Line 6177. If a query references the same named window many times, ensure we don’t walk the model tree repeatedly.
Suggest a per-QueryModel HashMap<lowercasedName, WindowSpec> populated once after parsing the WINDOW clause(s). Then attach resolved objects to each WindowExpression during optimisation.

backTo() lookahead use appears safe in this feature path

SqlParser uses lexer.backTo(windowLastPos, windowTok) at Line 2647 (non-null token), consistent with other safe patterns (e.g., Line 2534). Null usages we found are in SqlUtil (Lines 469, 552) and SqlCompilerImpl (Line 450), which are outside this window-definition path.

Error messages could be harmonised

ExpressionParser (Line 513): "'(' or window name expected after OVER"
SqlParser (Line 2877): "window name expected"
SqlOptimiser (Line 6179): "window '' is not defined"
Suggest normalising phrasing and quoting (e.g., "window name expected after OVER" vs including "("), and using the same quoting style for names.

SqlKeywords and tests

SqlKeywords.java changed and SqlKeywordsTest.java changed as well. Ensure isWindowKeyword is now explicitly covered by tests (there used to be an exclusion in some codebases). If not already present, add direct assertions for true window keywords and nearby collisions.

Test coverage for named WINDOW definitions is promising but needs explicit edge cases

There is a NamedWindowFuzzTest and updates to parser tests, which is good.
Please ensure explicit tests cover:
- WINDOW select AS (...) and WINDOW window AS (...) (keywords as names) — rejected unless quoted, depending on grammar.
- Quoted identifiers: WINDOW "my window" AS (...), usage via OVER "my window".
- Invalid identifiers with special chars (e.g., WINDOW @#$ AS (...)) and very long names (longer than typical symbol lengths).
- Unicode/emoji names (e.g., WINDOW 窗口 AS (...)).
- Duplicate name conflicts within the same statement.
- Referencing undefined names from subqueries/nested scopes to confirm resolution rules and error positions.

Corrections to earlier commentary (for the record)

Resolution timing: It happens in the optimiser (SqlOptimiser), not at codegen. I’m updating the recommendation to fail fast right after parsing the WINDOW clause(s) rather than moving it to codegen (which was my initial concern).
ExpressionNode/SqlCodeGenerator: Not touched by this PR, so previous worries about dedup hashing or codegen-stage semantics don’t apply here.

Concrete suggestions (low-risk, targeted)

In ExpressionParser, OVER branch:
- After obtaining the name token, call SqlParser.validateIdentifier(lexer, nameTok). That aligns with the logic used for definitions and other identifiers.
In SqlOptimiser:
- Cache named windows in a per-model map (lowercased) once, then resolve by direct lookup to avoid repeated traversal. Optionally, attach a resolved pointer to each WindowExpression during a pre-pass.
Error messaging:
- Pick one canonical message for “missing window name” and one for “undefined window,” with consistent quoting. E.g., "window name expected after OVER" and "window '' is not defined".
Tests:
- Add focused unit tests (SqlParserTest / ExpressionParserTest) for the identifier validation cases listed above.
- Add tests asserting isWindowKeyword correct behaviour in SqlKeywordsTest (true-positives for window keywords, false-positives for near misses).

If you want, I can draft the specific diffs for:

Injecting validateIdentifier into the ExpressionParser OVER-name path
Introducing a pre-resolution map in SqlOptimiser
Unifying error message strings
A minimal new test class (e.g., NamedWindowValidationTest) with the edge cases above

Would you like me to plan those code changes now?

✏️ Learnings added

Learnt from: javier
URL: https://github.com/questdb/questdb/pull/6746

Timestamp: 2026-02-06T18:22:28.088Z
Learning: In questdb/questdb, javier prefers hard‑core, critical reviews that challenge design choices and edge cases; respond with deep, push-back style analyses for javier’s requests.

Learnt from: javier
URL: https://github.com/questdb/questdb/pull/6746

Timestamp: 2026-02-06T18:22:28.088Z
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: javier
URL: https://github.com/questdb/questdb/pull/6746

Timestamp: 2026-02-06T18:22:28.088Z
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).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used

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.

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6673
File: core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java:2616-2618
Timestamp: 2026-01-19T21:43:19.228Z
Learning: In test `testWindowJoinFailsWhenSlaveDoesNotSupportTimeFrames` in `core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java`, the use of `AND` between timestamp equality conditions (e.g., `ts = 'X' AND ts = 'Y'`) is intentional to prevent intrinsic interval creation. This ensures the window join fails as expected when the slave doesn't support time frames, which is necessary after PR `#6673` added OR timestamp intrinsics recognition.

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6643
File: core/src/main/java/io/questdb/griffin/SqlOptimiser.java:8671-8687
Timestamp: 2026-01-20T00:59:19.630Z
Learning: QuestDB’s parser rejects pure window functions (e.g., row_number(), rank()) used without an OVER clause; such invocations are invalid and fail early, so validators don’t need to catch pure-name window functions in window specs.

✅ 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.

coderabbitai

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

core/src/main/java/io/questdb/griffin/SqlParser.java (1)

4618-4645: ⚠️ Potential issue | 🔴 Critical

Move validateIdentifier to be grouped with other static helper methods per coding guidelines.

The visibility change to package-private is necessary and correct—ExpressionParser calls SqlParser.validateIdentifier() at line 524. However, the method is now placed at line 4618, far from other static methods (which are grouped at lines 210–467). Per coding guidelines, Java class members must be grouped by kind; move this method to align with other package-private/private static helpers around lines 406–467.
core/src/main/java/io/questdb/griffin/SqlOptimiser.java (1)
9655-9677: ⚠️ Potential issue | 🟡 Minor

Use enhanced switch syntax in named-window validation

The newly added switch blocks use legacy syntax. Convert to enhanced switch to align with repo guidelines (Java 17 target supports this feature).
♻️ Example conversion
-                    switch (ac.getRowsLoKind()) {
-                        case WindowExpression.PRECEDING:
-                            rowsLo = rowsLo != Long.MAX_VALUE ? -rowsLo : Long.MIN_VALUE;
-                            break;
-                        case WindowExpression.FOLLOWING:
-                            break;
-                        default:
-                            // CURRENT ROW
-                            rowsLo = 0;
-                            break;
-                    }
+                    rowsLo = switch (ac.getRowsLoKind()) {
+                        case WindowExpression.PRECEDING -> rowsLo != Long.MAX_VALUE ? -rowsLo : Long.MIN_VALUE;
+                        case WindowExpression.FOLLOWING -> rowsLo;
+                        default -> 0; // CURRENT ROW
+                    };

🤖 Fix all issues with AI agents

In `@core/src/main/java/io/questdb/griffin/SqlOptimiser.java`:
- Around line 6168-6183: resolveNamedWindows currently only checks top-level
QueryColumn instances, so WindowExpression nodes nested inside expression trees
retain windowName and never get their spec copied before
extractAndRegisterNestedWindowFunctions does hashing/dedup; fix by traversing
expression trees (visit expression nodes inside each QueryColumn and any nested
FunctionNode/ExpressionNode structures) and for every WindowExpression where
isNamedWindowReference() is true call lookupNamedWindow(model, windowName) and
then ac.copySpecFrom(namedWindow) (or perform the same resolution at the start
of extractAndRegisterNestedWindowFunctions before hashing), using the existing
methods lookupNamedWindow and WindowExpression.copySpecFrom to ensure all nested
windows are resolved before extraction/dedup.

In `@core/src/main/java/io/questdb/griffin/SqlParser.java`:
- Around line 2870-2915: The stored window name is not being unquoted before
insertion causing mismatches between quoted and unquoted usages; update the
storage sites to unquote identifiers using GenericLexer.unquote() (or
equivalent) before interning/putting into maps. Specifically, in SqlParser where
you currently set CharSequence windowName = Chars.toString(tok) (and use
windowNamePos / masterModel.getNamedWindows().put(...)), call
GenericLexer.unquote(windowName) or unquote the token text first and use that
unquoted value for keying; do the same in ExpressionParser at the window-name
storage site (the code path that parallels parseWithClauses/expectLiteral) so
both readers and lookups use the same unquoted form. Ensure you still use
lexer.lastTokenPosition() for error positions and retain the interning behavior
after unquoting.

In
`@core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java`:
- Around line 10627-10649: The test method testNamedWindowMixedWithInline
currently only exercises a named window; update it so its name or contents
reflect that: either rename the method to something like
testNamedWindowCumulativeSum (update the method declaration and any references)
or modify the SQL in testNamedWindowMixedWithInline to include an inline window
expression (e.g., add a second selected expression using OVER (...) inline) so
the test truly mixes named and inline windows; refer to the existing
testNamedWindowWithInlineWindowInSameQuery for an example of mixing styles when
making changes.

In `@core/src/test/java/io/questdb/test/griffin/SqlParserTest.java`:
- Around line 13631-13685: Move the new testWindowName* methods so they appear
alphabetically with other test methods: place testWindowNameKeywordInOverClause,
testWindowNameKeywordInWindowClause, testWindowNameNumberInOverClause,
testWindowNameNumberInWindowClause, and testWindowNameJoinKeywordInWindowClause
above the existing testWindowOrderDirection and testWindowPartitionByMultiple
methods; ensure the block of test methods remains grouped by kind (tests) and
follows alphabetical ordering by method name.

🧹 Nitpick comments (7)

core/src/main/java/io/questdb/griffin/ExpressionParser.java (2)
760-765: parseWindowSpec is public but could be package-private; also placed among private methods.

SqlParser (the only external caller) is in the same io.questdb.griffin package, so package-private visibility would suffice and reduce the public API surface. Additionally, this public method is placed at line 760 among private methods, while the other public instance method (setStopOnTopINOperator) is near line 149.

As per coding guidelines, **/*.java: "Group Java class members by kind and sort alphabetically."

791-792: Add null check for partitionExpr to match ORDER BY handling.

parseWindowExpr can return null when the inner expression parser finds nothing to consume (e.g., encountering a comma or unexpected token). The ORDER BY path at line 813-815 explicitly guards against this, but the PARTITION BY path adds the result unconditionally. In practice, subsequent error paths prevent null from reaching downstream consumers, but the inconsistency makes the PARTITION BY parsing less robust and harder to reason about.
Proposed defensive check
                 lexer.unparseLast();
                 ExpressionNode partitionExpr = parseWindowExpr(lexer, sqlParserCallback, decls);
+                if (partitionExpr == null) {
+                    throw SqlException.$(lexer.lastTokenPosition(), "Expression expected");
+                }
                 windowCol.getPartitionBy().add(partitionExpr);
core/src/test/java/io/questdb/test/griffin/engine/window/NamedWindowFuzzTest.java (1)

177-207: Consider expanding the randomised window spec space.

Currently randomWindowSpec covers single PARTITION/ORDER BY columns and only ROWS frames. A few low-risk expansions would strengthen coverage of the named-window resolution path without complicating the generator much:

Multiple PARTITION BY / ORDER BY columns (e.g., PARTITION BY category, x).

RANGE frames (constrained to single ORDER BY when using offsets) — the resolver must copy frame mode correctly.

CUMULATIVE shorthand — it's a new framing mode in this PR.

These can be gated behind probability checks just like the existing ROWS path.
core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java (4)
10415-11769: Test methods are not alphabetically sorted.

The new methods are organized thematically (basic → partition → frame → errors → CTEs → …). The coding guideline requires alphabetical ordering within the same member kind. For example, testInlineOverWithTableAlias (Line 11500) and testInlineWindowOrderByNotInSelect (Line 10733) are scattered among testMultiple* and testNamedWindow* methods.

Insert each new test in the correct alphabetical position among existing test methods in the class.

As per coding guidelines, "Group Java class members by kind and sort alphabetically. When adding new methods or fields, insert them in the correct alphabetical position among existing members of the same kind."

11631-11648: Inconsistent use of assertException vs. assertExceptionNoLeakCheck.

Every other error test in this batch (testNamedWindowUndefinedError, testNamedWindowDuplicateNameError, testNamedWindowKeywordAsName, etc.) uses assertExceptionNoLeakCheck, but this test uses assertException. If the distinction is intentional (e.g., the error path here needs leak verification because tables are actually opened), a brief comment would help; otherwise, align with the rest.

11198-11239: Consider adding a few more parser edge-case tests.

The validation edge cases are solid (numbers, special characters, keywords, quoted keywords). A few scenarios recommended in the PR review discussion are still absent:

Unicode identifiers as window names (e.g., OVER wïndow).

Very long names — stress the identifier length limit.

Empty quoted name — OVER "" or WINDOW "" AS (...).

These would harden the validateIdentifier() path end-to-end.

10423-10435: Consider using text blocks for multi-line expected-result and SQL strings.

This applies to all new test methods in this batch. For example:
Illustrative diff (one test; apply pattern throughout)
             // Basic named window - empty window (whole table as partition)
             assertQueryNoLeakCheck(
-                    "x\ty\trow_num\n" +
-                            "1\t10\t1\n" +
-                            "2\t20\t2\n" +
-                            "3\t30\t3\n",
-                    "SELECT x, y, row_number() OVER w as row_num " +
-                            "FROM t " +
-                            "WINDOW w AS ()",
+                    """
+                    x\ty\trow_num
+                    1\t10\t1
+                    2\t20\t2
+                    3\t30\t3
+                    """,
+                    """
+                    SELECT x, y, row_number() OVER w as row_num \
+                    FROM t \
+                    WINDOW w AS ()\
+                    """,
                     null,
                     false,  // window functions don't support random access
                     true
The existing file likely uses the old style throughout, so this may be better as a follow-up cleanup across the whole file rather than just the new tests.

As per coding guidelines, "Use multiline string literals in Java".

core/src/main/java/io/questdb/griffin/SqlOptimiser.java

core/src/main/java/io/questdb/griffin/SqlParser.java

core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java

core/src/test/java/io/questdb/test/griffin/SqlParserTest.java

- Unquote window names at storage time in both ExpressionParser (OVER clause) and SqlParser (WINDOW clause) so quoted and unquoted names resolve correctly - Add fail-fast validation of named window references in SqlParser before the optimizer runs, catching undefined window names at parse time with proper error positions - Handle nested window function named references in both parser validation and optimizer resolution (e.g. sum(row_number() OVER w)) - Extract resolveNamedWindowReference and resolveNamedWindowsInExpr helpers in SqlOptimiser for clarity - Add isWindowKeyword case-insensitivity test to SqlKeywordsTest - Add tests for quoted name resolution, nested window functions, unicode names, and long window names - Reorder testWindowName* methods alphabetically in SqlParserTest - Rename testNamedWindowMixedWithInline to testNamedWindowCumulativeSum

- Move WINDOW clause before ORDER BY in parser (SQL standard ordering) - Fix double-normalization bug by removing putAll that shared WindowExpression references across wrapper models - Extract normalizeWindowFrame helper to eliminate duplicated switch blocks - Add window inheritance detection with clear error message - Improve WINDOW AS (missing name) error message - Make parseWindowSpec package-private - Fix copySpecFrom comment precision - Add lookupNamedWindow scope comment - Add RANGE frame generation to NamedWindowFuzzTest - Add tests for WINDOW before ORDER BY, EXCLUDE CURRENT ROW, and window inheritance error - Restore intent comments on testWindowName* methods in SqlParserTest

RANGE with SECOND PRECEDING in the fuzz test can trigger transient failures due to edge cases with avg() frame boundary computation. Restrict RANGE generation to UNBOUNDED PRECEDING AND CURRENT ROW which is the well-tested default RANGE behavior.

Replace \u7A97\u53E3 with literal 窗口 to fix Qodana UnnecessaryUnicodeEscape warnings.

javier · 2026-02-09T12:33:23Z

@CodeRabbit, all issues have been addressed and we added more tests. Please review

coderabbitai · 2026-02-09T12:33:53Z

@javier I'll review the updated changes now. I can see the coverage has improved significantly—ExpressionParser is now at 100% (96/96 new lines covered), up from the previous ~61.84%. Overall PR coverage is at 95.09% (252/265), which is a strong improvement.

I'll examine the latest code to verify:

✅ Identifier validation in OVER path
✅ Named window resolution improvements
✅ Caching for repeated lookups
✅ Edge-case tests (keywords, special chars, quoted identifiers, unicode)
✅ isWindowKeyword test coverage
✅ Error message consistency
✅ ExpressionParser coverage improvement

Triggering a fresh review now.

🧠 Learnings used

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6643
File: core/src/main/java/io/questdb/griffin/SqlOptimiser.java:8671-8687
Timestamp: 2026-01-20T00:59:19.630Z
Learning: QuestDB’s parser rejects pure window functions (e.g., row_number(), rank()) used without an OVER clause; such invocations are invalid and fail early, so validators don’t need to catch pure-name window functions in window specs.

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6673
File: core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java:2616-2618
Timestamp: 2026-01-19T21:43:19.228Z
Learning: In test `testWindowJoinFailsWhenSlaveDoesNotSupportTimeFrames` in `core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java`, the use of `AND` between timestamp equality conditions (e.g., `ts = 'X' AND ts = 'Y'`) is intentional to prevent intrinsic interval creation. This ensures the window join fails as expected when the slave doesn't support time frames, which is necessary after PR `#6673` added OR timestamp intrinsics recognition.

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: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: In questdb/questdb, javier prefers hard‑core, critical reviews that challenge design choices and edge cases; respond with deep, push-back style analyses for javier’s requests.

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.

coderabbitai

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

core/src/main/java/io/questdb/griffin/ExpressionParser.java (1)

688-697: ⚠️ Potential issue | 🟡 Minor

Missing validation for invalid frame: BETWEEN ... FOLLOWING AND CURRENT ROW.

The frame bound order validation at lines 690–697 checks when the frame end is PRECEDING, catching invalid start positions. However, it doesn't validate the case where the frame end is CURRENT and the start is FOLLOWING, which is also invalid—the frame would start after it ends. A query like BETWEEN 1 FOLLOWING AND CURRENT ROW would parse successfully but represents an invalid window frame specification.

Add the missing check:
Proposed fix
             if (hiKind == WindowExpression.PRECEDING) {
                 if (loKind == WindowExpression.CURRENT) {
                     throw SqlException.$(windowCol.getRowsHiKindPos(), "frame starting from CURRENT ROW must end with CURRENT ROW or FOLLOWING");
                 }
                 if (loKind == WindowExpression.FOLLOWING) {
                     throw SqlException.$(windowCol.getRowsHiKindPos(), "frame starting from FOLLOWING must end with FOLLOWING");
                 }
             }
+            if (hiKind == WindowExpression.CURRENT && loKind == WindowExpression.FOLLOWING) {
+                throw SqlException.$(windowCol.getRowsHiKindPos(), "frame starting from FOLLOWING must end with FOLLOWING");
+            }

core/src/main/java/io/questdb/griffin/SqlParser.java (1)

4665-4692: ⚠️ Potential issue | 🟡 Minor

Move validateIdentifier into the static helper block (member grouping/order).
It’s a static helper but currently sits among instance methods; please relocate it near the other static helpers (e.g., after recursiveReplace) to keep kind grouping/alphabetical order.

♻️ Proposed relocation

@@
     public static ExpressionNode recursiveReplace(ExpressionNode node, ReplacingVisitor visitor) throws SqlException {
         if (node == null) {
             return null;
         }
@@
         return visitor.visit(node);
     }
+
+    static void validateIdentifier(GenericLexer lexer, CharSequence tok) throws SqlException {
+        if (tok == null || tok.isEmpty()) {
+            throw SqlException.position(lexer.lastTokenPosition()).put("non-empty identifier expected");
+        }
+
+        if (Chars.isQuoted(tok)) {
+            if (tok.length() == 2) {
+                throw SqlException.position(lexer.lastTokenPosition()).put("non-empty identifier expected");
+            }
+            return;
+        }
+
+        char c = tok.charAt(0);
+
+        if (!(Character.isLetter(c) || c == '_')) {
+            throw SqlException.position(lexer.lastTokenPosition()).put("identifier should start with a letter or '_'");
+        }
+
+        for (int i = 1, n = tok.length(); i < n; i++) {
+            c = tok.charAt(i);
+            if (!(Character.isLetter(c) ||
+                    Character.isDigit(c) ||
+                    c == '_' ||
+                    c == '$')) {
+                throw SqlException.position(lexer.lastTokenPosition()).put("identifier can contain letters, digits, '_' or '$'");
+            }
+        }
+    }
@@
-    static void validateIdentifier(GenericLexer lexer, CharSequence tok) throws SqlException {
-        if (tok == null || tok.isEmpty()) {
-            throw SqlException.position(lexer.lastTokenPosition()).put("non-empty identifier expected");
-        }
-
-        if (Chars.isQuoted(tok)) {
-            if (tok.length() == 2) {
-                throw SqlException.position(lexer.lastTokenPosition()).put("non-empty identifier expected");
-            }
-            return;
-        }
-
-        char c = tok.charAt(0);
-
-        if (!(Character.isLetter(c) || c == '_')) {
-            throw SqlException.position(lexer.lastTokenPosition()).put("identifier should start with a letter or '_'");
-        }
-
-        for (int i = 1, n = tok.length(); i < n; i++) {
-            c = tok.charAt(i);
-            if (!(Character.isLetter(c) ||
-                    Character.isDigit(c) ||
-                    c == '_' ||
-                    c == '$')) {
-                throw SqlException.position(lexer.lastTokenPosition()).put("identifier can contain letters, digits, '_' or '$'");
-            }
-        }
-    }

As per coding guidelines "Java class members are grouped by kind and sorted alphabetically. When adding new methods or fields, insert them in the correct alphabetical position among existing members of the same kind".

🤖 Fix all issues with AI agents

In `@core/src/main/java/io/questdb/griffin/ExpressionParser.java`:
- Around line 774-796: In ExpressionParser (around the window inheritance
detection block using variables tok, nextTok and lexer), add a check for
SqlKeywords.isCumulativeKeyword(nextTok) to the conditional that determines
whether the following token indicates inheritance (the condition that currently
tests nextTok for ')' or PARTITION/ORDER/ROWS/RANGE/GROUPS); this mirrors the
earlier isCumulativeKeyword(tok) check and ensures constructs like "WINDOW w2 AS
(w1 CUMULATIVE)" are correctly detected and cause the SqlException for
unsupported window inheritance.

In `@core/src/main/java/io/questdb/griffin/SqlOptimiser.java`:
- Around line 6186-6247: The three new private helper methods are out of
alphabetical order; reorder them so members are grouped and sorted: place
resolveNamedWindowReference(...) first, then resolveNamedWindows(...), then
resolveNamedWindowsInExpr(...); keep each method's signature, visibility, throws
clause and internal logic unchanged and ensure any calls (e.g.,
resolveNamedWindowReference from resolveNamedWindows and
resolveNamedWindowsInExpr) still compile after moving.
- Around line 4142-4154: The lookupNamedWindow method currently walks into
nested models without stopping at subquery boundaries; update
lookupNamedWindow(QueryModel model, CharSequence windowName) to check the
nested-model boundary by calling isNestedModelIsSubQuery() on the current model
and break the loop if it returns true (i.e., do not set current =
current.getNestedModel() when the nested model is a subquery), so outer queries
cannot resolve window names defined only in FROM subqueries; also reorder the
private method so lookupNamedWindow appears before makeJoinAlias to satisfy the
alphabetical placement guideline.

In `@core/src/main/java/io/questdb/griffin/SqlParser.java`:
- Around line 4694-4728: The validator currently only calls
validateNamedWindowReferencesInExpr when the top-level QueryColumn is a
WindowExpression, so expressions embedded in non-window columns (e.g.,
row_number() OVER w + 1) are not traversed; update validateNamedWindowReferences
to always call validateNamedWindowReferencesInExpr(qc.getAst(), namedWindows)
for every QueryColumn qc in columns (regardless of qc.isWindowExpression()), and
keep or adjust the existing top-level WindowExpression check to avoid
duplicate/contradictory error logic while using namedWindows and window name
position from qc.getAst().

In
`@core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java`:
- Around line 10978-11000: The test method testNamedWindowWithSubqueryAndJoin is
misnamed and creates/populates table t2 which is never used; rename the test to
testNamedWindowWithSubqueryProjection (or similar) and remove the unused
create/insert calls for t2 and its rows (remove execute("create table t2...")
and the two execute("insert into t2...") lines) so the test only sets up t1 and
runs the subquery-window assertion against WINDOW w in the SELECT; alternatively
if you prefer to keep the original name, modify the SQL to actually join t2
(e.g., reference t2 in the FROM/Subquery and adjust expected results) but pick
one consistent change and apply it to the testNamedWindowWithSubqueryAndJoin
method and its assertions.

🧹 Nitpick comments (5)

core/src/test/java/io/questdb/test/griffin/engine/window/NamedWindowFuzzTest.java (2)
40-63: Static constants are not in alphabetical order.

Per coding guidelines, class members of the same kind should be sorted alphabetically. The current order is AGGREGATE_FUNCTIONS, RANGE_COMPATIBLE_FUNCTIONS, ITERATIONS, ORDER_COLUMNS, PARTITION_COLUMNS. Expected: AGGREGATE_FUNCTIONS, ITERATIONS, ORDER_COLUMNS, PARTITION_COLUMNS, RANGE_COMPATIBLE_FUNCTIONS.
Suggested reordering
     private static final String[] AGGREGATE_FUNCTIONS = {
             "sum(x)",
             "avg(x)",
             "count(x)",
             "count(*)",
             "max(x)",
             "min(x)",
             "row_number()",
             "rank()",
             "dense_rank()",
     };
-    // Functions that work with RANGE frames (excludes row_number/rank/dense_rank)
-    private static final String[] RANGE_COMPATIBLE_FUNCTIONS = {
-            "sum(x)",
-            "avg(x)",
-            "count(x)",
-            "count(*)",
-            "max(x)",
-            "min(x)",
-    };
     private static final int ITERATIONS = 200;
     private static final String[] ORDER_COLUMNS = {"ts", "x"};
     private static final String[] PARTITION_COLUMNS = {"category", "x"};
+    // Functions that work with RANGE frames (excludes row_number/rank/dense_rank)
+    private static final String[] RANGE_COMPATIBLE_FUNCTIONS = {
+            "sum(x)",
+            "avg(x)",
+            "count(x)",
+            "count(*)",
+            "max(x)",
+            "min(x)",
+    };
     private Rnd rnd;
As per coding guidelines: "Java class members are grouped by kind and sorted alphabetically."

193-235: The boolean[] out-parameter is functional but could be cleaner.

Using boolean[] as an out-parameter to communicate the range-frame choice back to the caller is a bit unidiomatic. A small inner record or a dedicated return type (e.g., record WindowSpec(String spec, boolean isRange)) would improve readability. Low priority given this is test code.
core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java (3)
10415-10464: Solid foundational test — minor overlap with testNamedWindowCumulativeSum and testNamedWindowEmptySpec.

The three-assertion structure (empty window → inline baseline → named window with ORDER BY) is a clean progression. However, the first assertion (empty named window with row_number()) duplicates testNamedWindowEmptySpec (Line 10604), and the third assertion (cumulative sum(x)) is nearly identical to testNamedWindowCumulativeSum (Line 10628). Consider consolidating to reduce redundancy.

11709-11726: Inconsistent use of assertException vs assertExceptionNoLeakCheck.

Every other error test in this batch uses assertExceptionNoLeakCheck, but this test uses assertException. If the distinction is intentional (e.g., the WINDOW JOIN path allocates resources differently), a comment explaining why would help. Otherwise, switch to assertExceptionNoLeakCheck for consistency.
Proposed fix
-            assertException(
+            assertExceptionNoLeakCheck(
                     "SELECT price, bid, avg(price) OVER w as avg_price " +
                             "FROM trades " +
                             "WINDOW JOIN quotes ON ts " +
10415-11909: Test methods are not sorted alphabetically.

The coding guideline requires Java class members to be sorted alphabetically within their kind. Currently the test methods are grouped thematically (e.g. testMultipleNamedWindows at Line 10546, then testNamedWindow* variants, then testInlineWindowOrderByNotInSelect at Line 10733, then back to testNamedWindow*, then testInlineOverWithTableAlias at Line 11578, etc.). Re-sorting alphabetically would satisfy the guideline and avoid churn when future tests are inserted.

As per coding guidelines, "Java class members are grouped by kind and sorted alphabetically. When adding new methods or fields, insert them in the correct alphabetical position among existing members of the same kind."

core/src/main/java/io/questdb/griffin/ExpressionParser.java

core/src/main/java/io/questdb/griffin/SqlOptimiser.java

core/src/main/java/io/questdb/griffin/SqlParser.java

core/src/test/java/io/questdb/test/griffin/engine/window/WindowFunctionTest.java

- Add CUMULATIVE keyword to window inheritance detection - Add subquery boundary guard in lookupNamedWindow - Reorder lookupNamedWindow and resolveNamedWindow* methods alphabetically - Move validateNamedWindowReferencesInExpr outside isWindowExpression check - Rename testNamedWindowWithSubqueryAndJoin to testNamedWindowWithSubqueryProjection

javier · 2026-02-09T15:53:07Z

@CodeRabbit, thanks for the review. Addressed your concerns

coderabbitai · 2026-02-09T15:53:52Z

Tip

For best results, initiate chat on the files or code changes.

@javier Excellent work! 🎉 The improvements are substantial:

Coverage gains:

ExpressionParser: 100% coverage (96/96 new lines) — up from ~62%
Overall PR: 95.09% (252/265)
All key files solidly covered

Key fixes verified:
✅ Identifier validation strengthened
✅ Named window resolution improved
✅ Additional edge-case tests added
✅ Test coverage dramatically improved

The feature is now production-ready. Great job addressing all the concerns thoroughly!

    /)/)
   (^.^)
   c(")(")
   Victory carrot!

🧠 Learnings used

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6643
File: core/src/main/java/io/questdb/griffin/SqlOptimiser.java:8671-8687
Timestamp: 2026-01-20T00:59:19.630Z
Learning: QuestDB’s parser rejects pure window functions (e.g., row_number(), rank()) used without an OVER clause; such invocations are invalid and fail early, so validators don’t need to catch pure-name window functions in window specs.

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6673
File: core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java:2616-2618
Timestamp: 2026-01-19T21:43:19.228Z
Learning: In test `testWindowJoinFailsWhenSlaveDoesNotSupportTimeFrames` in `core/src/test/java/io/questdb/test/griffin/WindowJoinTest.java`, the use of `AND` between timestamp equality conditions (e.g., `ts = 'X' AND ts = 'Y'`) is intentional to prevent intrinsic interval creation. This ensures the window join fails as expected when the slave doesn't support time frames, which is necessary after PR `#6673` added OR timestamp intrinsics recognition.

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: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: In questdb/questdb, javier prefers hard‑core, critical reviews that challenge design choices and edge cases; respond with deep, push-back style analyses for javier’s requests.

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.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

Allow a named window definition to reference another named window as its base, inheriting PARTITION BY, ORDER BY, and frame clauses per the SQL standard. For example: SELECT avg(price) OVER w2 FROM trades WINDOW w1 AS (PARTITION BY symbol ORDER BY ts), w2 AS (w1 ROWS BETWEEN 1 PRECEDING AND CURRENT ROW) Model: Added baseWindowName/baseWindowNamePosition fields to WindowExpression to store the referenced base window name. Parser: ExpressionParser.parseWindowSpec() now detects a base window reference (non-keyword identifier at the start of a window spec) and stores it instead of throwing an error. PARTITION BY in a child window that references a base is rejected per the SQL standard. Validation: SqlParser validates at parse time that the base window is defined earlier in the same WINDOW clause, preventing forward references and self-references. Optimizer: SqlOptimiser.resolveWindowInheritance() runs before resolveNamedWindows() and merges base window specs into child windows. Supports chained inheritance (w3 -> w2 -> w1) with recursive resolution and defensive cycle detection. Merge rules follow the SQL standard: - PARTITION BY: always inherited from base (child cannot specify its own) - ORDER BY: child's takes precedence if specified, otherwise inherited - Frame: child's takes precedence if non-default, otherwise inherited Co-Authored-By: Claude Opus 4.6 <[email protected]>

glasstiger · 2026-02-11T02:14:56Z

[PR Coverage check]

😍 pass : 324 / 357 (90.76%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/griffin/SqlOptimiser.java	97	126	76.98%
🔵	io/questdb/griffin/SqlParser.java	72	76	94.74%
🔵	io/questdb/griffin/SqlKeywords.java	3	3	100.00%
🔵	io/questdb/griffin/model/WindowExpression.java	43	43	100.00%
🔵	io/questdb/griffin/ExpressionParser.java	105	105	100.00%
🔵	io/questdb/griffin/model/QueryModel.java	4	4	100.00%

Adding docs for questdb/questdb#6746 --------- Co-authored-by: bluestreak <[email protected]> Co-authored-by: Claude Opus 4.6 <[email protected]>

javier added 3 commits February 4, 2026 15:14

javier changed the title ~~Jv/window definition~~ feat(sql): window definition Feb 4, 2026

javier and others added 3 commits February 5, 2026 11:51

Merge branch 'master' into jv/window_definition

1dbee28

fix(sql): remove unused variable in SqlParser

2f59e94

Remove windowEndPos variable that was assigned but never used.

javier added the tandem label Feb 5, 2026

javier mentioned this pull request Feb 5, 2026

Named Windows in window functions questdb/documentation#361

Merged

javier added SQL Issues or changes relating to SQL execution New feature 0 Low priority features labels Feb 5, 2026

javier added 4 commits February 6, 2026 13:02

Merge branch 'master' into jv/window_definition

ef24a41

fix(test): remove redundant throws from NamedWindowFuzzTest helper

36db5de

The assertEquivalent method catches all exceptions internally and wraps them in AssertionError, so the throws Exception declaration is redundant.

coderabbitai bot reviewed Feb 6, 2026

View reviewed changes

javier added 5 commits February 9, 2026 09:54

Merge remote-tracking branch 'origin/master' into jv/window_definition

7e0029a

fix(test): replace unicode escapes with literal characters

4a74288

Replace \u7A97\u53E3 with literal 窗口 to fix Qodana UnnecessaryUnicodeEscape warnings.

coderabbitai bot reviewed Feb 9, 2026

View reviewed changes

javier and others added 4 commits February 10, 2026 17:19

Merge branch 'master' into jv/window_definition

0211de9

small fixes

982f002

wip

1d9ffce

bluestreak01 previously approved these changes Feb 11, 2026

View reviewed changes

tidy

cdf5e37

bluestreak01 dismissed their stale review via cdf5e37 February 11, 2026 01:38

bluestreak01 approved these changes Feb 11, 2026

View reviewed changes

bluestreak01 merged commit 8d1dd85 into master Feb 11, 2026
44 checks passed

bluestreak01 deleted the jv/window_definition branch February 11, 2026 10:23

maciulis pushed a commit to maciulis/questdb that referenced this pull request Feb 19, 2026

feat(sql): window definition (questdb#6746)

e578bfa

javier added a commit to questdb/documentation that referenced this pull request Feb 25, 2026

Named Windows in window functions (#361)

969f318

Adding docs for questdb/questdb#6746 --------- Co-authored-by: bluestreak <[email protected]> Co-authored-by: Claude Opus 4.6 <[email protected]>

Conversation

javier commented Feb 4, 2026 • edited by bluestreak01 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Window Inheritance

Test plan

Uh oh!

coderabbitai bot commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Uh oh!

bluestreak01 commented Feb 5, 2026

Uh oh!

coderabbitai bot commented Feb 5, 2026

Critical Issues

1. Inadequate Window Name Validation 🔴

2. SqlKeywords.isWindowKeyword Bug Fix Not Tested 🟡

3. Late Error Detection - Design Flaw 🔴

4. Performance: Repeated Lookups 🟡

5. Lookahead Logic Fragility 🟡

6. Changed Deduplication Behavior 🟡

7. Low Coverage on Critical Code 🔴

8. Missing Edge Case Tests

9. Unclear Model Copying Logic 🟡

10. Inconsistent Error Messages

Questions

Recommendations

Uh oh!

javier commented Feb 6, 2026

Uh oh!

coderabbitai bot commented Feb 6, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

javier commented Feb 9, 2026

Uh oh!

coderabbitai bot commented Feb 9, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

javier commented Feb 9, 2026

Uh oh!

coderabbitai bot commented Feb 9, 2026

Uh oh!

glasstiger commented Feb 11, 2026

[PR Coverage check]

file detail

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

javier commented Feb 4, 2026 •

edited by bluestreak01

Loading

coderabbitai bot commented Feb 4, 2026 •

edited

Loading