Walkthrough

The pull request fixes an issue where aliases hide original column names in CASE expressions with window functions. A new helper method replaceWindowFunctionOrLiteral is introduced to SqlOptimiser.java that attempts window function replacement and falls back to literal column reference handling with proper propagation through the window model. Three regression tests are added to validate that both original column names and aliases remain accessible in CASE branches when window functions are present.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• fix(sql): resolve 'Invalid column' error in WINDOW JOIN with aliased columns #6701: Modifies SqlOptimiser's literal/alias replacement logic for window contexts to ensure non-window column references propagate correctly through window models.
• feat(sql): support nested window functions with deduplication optimization #6643: Modifies SqlOptimiser's window-function replacement and emission path to change how window functions and literal column references are handled and propagated.

Suggested reviewers

• puzpuzpuz

No actionable comments were generated in the recent review. 🎉

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 @coderabbitai help to get the list of available commands and usage tips.}

[PR Coverage check]

😍 pass : 12 / 12 (100.00%)

file detail

@CodeRabbit review critically

#!/bin/bash
# Fetch PR details
gh pr view 6775 --json title,body,files,additions,deletions,changedFiles

# Get the diff for the PR
gh pr diff 6775

# Look for the main changed file
fd -t f "SqlOptimiser.java" --exec cat {}

# Find related test files
fd -t f "WindowFunctionTest.java" --exec cat {}

#!/bin/bash
# Let's examine the full context around the changes and look for other call sites
# that might have the same issue

# Find all calls to replaceIfWindowFunction
echo "=== All calls to replaceIfWindowFunction ==="
rg -n "replaceIfWindowFunction\(" --type java

# Look at the emitWindowFunctions method context
echo -e "\n=== emitWindowFunctions method ==="
ast-grep --pattern 'private void emitWindowFunctions($$$) {
  $$$
}'

# Check if there are other places where window functions and CASE interact
echo -e "\n=== Places where CASE and window functions might interact ==="
rg -n "CASE|case" core/src/main/java/io/questdb/griffin/SqlOptimiser.java | head -20

@bluestreak01 Here’s a critical, line-cited review with concrete follow-ups.

What looks good

• Line 3098, 3112, 3128: Replacing replaceIfWindowFunction(...) with replaceWindowFunctionOrLiteral(...) inside emitWindowFunctions(...) directly addresses the propagation gap that caused #6769 by ensuring non-window column refs get emitted during the same traversal pass.
• Line 6201–6235: The helper short-circuits to replaceIfWindowFunction(...) first and only falls back to replaceLiteral(...), minimizing behavior changes for “true” window nodes. The passthrough to windowModel via addBottomUpColumnIfNotExists(nextColumn(alias)) is a pragmatic way to make the column visible to outer models without re-traversing.
• Tests: The three additions reproduce the reported bug and also cover the mixed usage of alias vs. original column name in CASE THEN branches, plus the “not selected but used in CASE” case.

Push-back and risks

1. Potential duplicate/definition drift

• I see replaceIfWindowFunction(...) declared around Line 6117 and referenced at Line 6216 from inside replaceWindowFunctionOrLiteral(...). That’s expected. However, because this file is very large and has multiple similar helpers, please confirm there is exactly one definition of replaceWindowFunctionOrLiteral(...) in SqlOptimiser.java. A duplicate signature would break compilation or mask future edits. If there are two, remove the older one and keep the variant introduced at Line 6201–6235.

2. Alias collision and windowModel passthrough

• Line 6222–6231: When replaceLiteral(...) returns a literal alias n.token, you add a passthrough with windowModel.addBottomUpColumnIfNotExists(nextColumn(alias)). Unlike createColumnAlias(...), nextColumn(alias) keeps the same alias. That’s correct for passthrough, but:
  • If windowModel already contains a different definition under the same alias (e.g., a function column), addBottomUpColumnIfNotExists won’t add, and the alias will point to the existing column. If that existing column doesn’t represent the same underlying literal, the outer model may read the wrong value. Please verify that alias collisions at this layer cannot occur, or guard with a compare-by-AST before early return.
  • Consider using ensureAliasUniqueness(windowModel, nextColumn(alias)) if there’s any realistic path for a conflict, then rewrite the reference accordingly.

3. Ambiguous columns across joins

• Because replaceLiteral(...) is now executed from the window pass for non-window nodes, it can trigger alias creation/validation earlier than before. If a query has ambiguous bare column names in CASE branches in the presence of joins, the earlier emission may surface ambiguous column errors at a different phase, or worse, resolve to a different model than intended. Please confirm behavior w.r.t.:
  • SELECT ... FROM t1 JOIN t2 USING (price) ... CASE WHEN price > lag(price) OVER (...) THEN price END
  • Expected: still ambiguous unless qualified, error position preserved. If not, we need a test.

4. Frame/window spec safety

• The helper is only invoked on expression subtrees being walked by emitWindowFunctions(...). That visit includes CASE argument nodes. Good. Please make sure the helper is not applied to window spec expressions where other invariants must hold:
  • Partition/Order/Frame nodes are already handled via emitLiterals(...)/replaceLiteralList(...) elsewhere; ensure this new helper is not reachable from those spec traversals (I didn’t see a conflicting path, just calling it out).

5. Performance considerations

• The new path calls replaceLiteral(...) for every visited non-window node during the window traversal. That adds:
  • A lookup in translatingModel.getColumnNameToAliasMap()
  • Potential add to translatingModel/innerVirtualModel maps
  • For many large CASE or deep expressions, this is additional O(n) work in the window pass on top of the literal-emission pass elsewhere.
• Mitigation:
  • Micro: Short-circuit early when node.type != LITERAL (the current code already benefits from replaceLiteral’s initial type check, but consider guarding before the call to avoid function dispatch).
  • Macro: You could track a boolean “windowPassLiteralPropagated” to skip the literal pass later for the same subtree, but that’s a larger refactor. At minimum, please add a perf note in the method Javadoc and confirm via benchmark if the added map lookups are negligible for typical workloads.

6. Test coverage gaps and proposed additions

• Missing ELSE-only and searched/conditional variants:
  • CASE WHEN price > lag(price) OVER (...) THEN 1 END (no ELSE)
  • CASE WHEN lag(price) OVER (...) IS NULL THEN price END (column only in THEN)
  • Nested CASE inside THEN/ELSE mixing alias and real name
• Joins and ambiguity:
  • SELECT ts, t1.price AS p, CASE WHEN t1.price > lag(t1.price) OVER (ORDER BY ts) THEN price END FROM t1 JOIN t2 ON ...; Expect ambiguous “price” unless qualified; ensure error message and position are stable.
• Multiple window functions within a single CASE:
  • WHEN price > lag(price) OVER (...) AND price < lead(price) OVER (...) THEN price
  • Should dedup still work, and literals be propagated once.
• Non-selected source with alias-only projection:
  • SELECT ts, price AS p, CASE WHEN p > lag(p) OVER (ORDER BY ts) THEN p END FROM trades; Ensures alias path also works without the original name in SELECT.

I recommend adding these to WindowFunctionTest.java:

`@Test`
public void testCaseWhenNoElseWithWindow() throws Exception {
    assertMemoryLeak(() -> {
        execute("CREATE TABLE t(ts TIMESTAMP, price DOUBLE) TIMESTAMP(ts)");
        execute("INSERT INTO t VALUES ('2024-01-01', 100.0), ('2024-01-02', 110.0)");
        // No ELSE, column only in THEN
        assertQueryNoLeakCheck(
            "ts\tp\tcase\n" +
            "2024-01-01T00:00:00.000000Z\t100.0\tnull\n" +
            "2024-01-02T00:00:00.000000Z\t110.0\t110.0\n",
            "SELECT ts, price AS p,\n" +
            "  CASE WHEN price > lag(price) OVER (ORDER BY ts) THEN price END\n" +
            "FROM t",
            "ts", false, true
        );
    });
}

`@Test`
public void testCaseWithAliasOnlyNoBaseColumnSelected() throws Exception {
    assertMemoryLeak(() -> {
        execute("CREATE TABLE trades (ts TIMESTAMP, price DOUBLE) TIMESTAMP(ts)");
        execute(\"\"\"\n" +
            "INSERT INTO trades VALUES\n" +
            "  ('2024-01-01', 100.0),\n" +
            "  ('2024-01-02', 110.0),\n" +
            "  ('2024-01-03', 90.0)\n" +
            "\"\"\");
        assertQueryNoLeakCheck(
            "ts\tp\tcase\n" +
            "2024-01-01T00:00:00.000000Z\t100.0\tnull\n" +
            "2024-01-02T00:00:00.000000Z\t110.0\t110.0\n" +
            "2024-01-03T00:00:00.000000Z\t90.0\t-90.0\n",
            "SELECT ts, price AS p,\n" +
            "  CASE\n" +
            "    WHEN p > lag(p) OVER (ORDER BY ts) THEN p\n" +
            "    WHEN p < lag(p) OVER (ORDER BY ts) THEN -p\n" +
            "  END\n" +
            "FROM trades",
            "ts", false, true
        );
    });
}

And for join ambiguity (expected failure):

`@Test`
public void testCaseAmbiguousColumnInJoinWithWindow() {
    // Expect invalid/ambiguous column error when unqualified "price" appears with joins
    assertException(
        "SELECT ts,\n" +
        "  CASE WHEN price > lag(price) OVER (ORDER BY ts) THEN price END\n" +
        "FROM t1 JOIN t2 ON t1.ts = t2.ts",
        0, // adjust to the position where 'price' first appears
        "ambiguous column" // or the exact message your SqlException emits
    );
}

Small code hardening suggestion

• Line 6222–6231: Consider guarding the passthrough with a check that any existing windowModel column under alias either:
  • Is a LITERAL to the same alias (no-op), or
  • Otherwise, create a unique alias via ensureAliasUniqueness(...) and rewrite the reference. This avoids silent alias re-use in rare cases.

Documentation trace

• Line 6201 Javadoc explains the intent well; please augment it with a short note about why the passthrough is needed for the outer virtual model visibility and that we intentionally run literal emission during the window traversal for CASE branches.

Overall recommendation

• The approach is sound for the reported bug and the tests exercise the critical path. Before merge, please:
  • Confirm single-definition of replaceWindowFunctionOrLiteral(...)
  • Add at least the “no ELSE” and “alias-only CASE” tests above
  • Evaluate/guard the alias passthrough collision
  • Sanity-check perf (quick micro-benchmark around emitWindowFunctions on a query with deeply nested CASE)

If you’d like, I can run an additional grep to confirm the single-definition now.

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

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

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