• ✅ Full review completed - (🔄 Check again to review again)

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 : 16 / 16 (100.00%)

file detail

@CodeRabbit pls review critically, don't hallucinate and double-check each point

#!/bin/bash
# Get the PR diff and list of changed files
gh pr view 6749 --json files --jq '.files[] | .path' | head -20

# Get the full diff to see what changed
gh pr diff 6749

#!/bin/bash
# Find and read the Numbers.parseInt implementation to verify underscore support
fd -t f "Numbers.java" | head -5

#!/bin/bash
# Read the parseInt method implementation to verify underscore support
cat core/src/main/java/io/questdb/std/Numbers.java | grep -A 50 "parseInt" | head -100

#!/bin/bash
# Look for the parseInt0 implementation which is the actual implementation
rg -A 30 "private static int parseInt0" core/src/main/java/io/questdb/std/Numbers.java

#!/bin/bash
# Get the complete parseInt0 implementation
rg -A 50 "private static int parseInt0" core/src/main/java/io/questdb/std/Numbers.java | head -70

#!/bin/bash
# Check the complete addDuration implementation to see how it parses the number
rg -B 10 -A 40 "private static long addDuration" core/src/main/java/io/questdb/griffin/model/IntervalUtils.java

#!/bin/bash
# Check if there are other places that parse tick expressions that might need underscore support
rg -n "parseInt.*[Tt]ick|[Tt]ick.*parseInt" --type java | head -20

# Also search for other number parsing in interval/date related code
rg -n "Character\.isDigit|>= '0' && .* <= '9'" core/src/main/java/io/questdb/griffin/model/ | head -30

#!/bin/bash
# Check what those other number parsing locations at lines 1847 and 1882 are for
# They don't support underscores
sed -n '1830,1900p' core/src/main/java/io/questdb/griffin/model/IntervalUtils.java

#!/bin/bash
# Check if bracket expansion like [1000,2000] should support underscores
# First, let's understand what bracket expansion is used for
rg -B 5 -A 3 "bracket expansion" core/src/test/java/io/questdb/test/griffin/model/TickExprTest.java | head -40

@bluestreak01 I've conducted a thorough critical review. Here are my findings:

✅ Confirmed correct:

1. Underscore support claim verified: Numbers.parseInt() does indeed support underscores with proper validation (rejects leading, trailing, and consecutive underscores via digitCounter tracking)

2. Error position improvements are mathematically sound: The calculations like errorPos + offsetStart - lo correctly convert relative positions within the expression to absolute SQL positions for error reporting

3. Test coverage is comprehensive: Covers valid cases (basic, compact, ranges, business days, durations) and invalid cases (leading, trailing, consecutive underscores)

❌ Critical issues found:

1. Scope creep: Unrelated cosmetic refactoring

Lines 89-95 in DateExpressionEvaluator.java:

-        while (varEnd < hi) {
+        for (; varEnd < hi; varEnd++) {
             char c = expression.charAt(varEnd);
             if (c == ' ' || c == '+' || c == '-') {
                 break;
             }
-            varEnd++;
         }

This while→for conversion is purely cosmetic and unrelated to underscore support. It's not mentioned in the PR description and doesn't add value. The second while→for conversion (lines 125-132) IS justified since it's part of number parsing with underscores, but this first one is not.

Question: Why is this change included? It adds unnecessary noise to the diff.

2. Inconsistent underscore support across parsing contexts

You added underscore support to:

• ✅ Date expression numbers: $now - 10_000T
• ✅ Duration suffixes: ;1_500T

But NOT to:

• ❌ Bracket expansion: 2018-01-[10_000,20_000] (lines 1847, 1882 in IntervalUtils)

The bracket expansion still uses Character.isDigit() without underscore support. This creates inconsistency: users can write 2018-01-10;10_000T but NOT 2018-01-[10_000,20_000].

Questions:

• Is this intentional?
• Should bracket expansion also support underscores for consistency?
• If not, is there a principled reason why TICK syntax supports it but bracket expansion doesn't?

3. Edge case: What about negative numbers with underscores?

The PR doesn't test expressions like $now - -1_000T (double negative). While this might be nonsensical, should it be explicitly rejected with a clear error? The current code would fail at the operator parsing stage, but I don't see a test confirming the error message quality.

4. Minor: CLAUDE.md change seems unrelated

The change to CLAUDE.md about alphabetical sorting clarification appears unrelated to this PR's objective. Should this be in a separate commit/PR?

🤔 Design questions:

1. Why allow underscores at all? The PR says "consistent with Numbers.parseInt()", but that's a circular argument. What's the actual user benefit? Are there queries with large tick values where readability matters? (e.g., $now - 1_000_000T vs $now - 1000000T)

2. Performance: The validation now happens twice—once in the manual loop checking c == '_' || isDigit(c), then again in Numbers.parseInt(). Why not just call parseInt() on a broader range and let it handle all validation? (Minor, but worth considering)

3. Error messages: When parseInt fails on invalid underscores, users get "Invalid number in date expression". Would "Invalid underscore placement" be clearer?

📊 Verdict:

The core underscore support is correctly implemented and well-tested. However:

• Remove the unrelated cosmetic while→for change (lines 89-95)
• Address the inconsistency with bracket expansion or document why it's intentional
• Consider splitting CLAUDE.md into a separate PR

The implementation correctly delegates validation to Numbers.parseInt(), and error positions are properly calculated. With the scope creep removed, this would be a solid PR.

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.