perf(sql): improve double-to-decimal cast performance by mtopolnik · Pull Request #6792 · questdb/questdb

mtopolnik · 2026-02-19T20:27:25Z

Summary

Replace the Dragon4 double-to-string algorithm with Ryu (Ulf Adams, 2018), which produces minimal-length, correctly-rounded decimal representations with better performance.
Add a direct double-to-Decimal conversion path (Numbers.doubleToDecimal) that bypasses string formatting entirely, removing the double→string→decimal roundtrip in CastDoubleToDecimalFunctionFactory.
Remove the dependency on jdk.internal.math.FDBigInteger, eliminating the --add-exports java.base/jdk.internal.math compiler flag.

Test plan

New NumbersDoubleToDecimalTest with 800+ lines covering auto-inference and target-precision paths, lossy truncation, edge cases (subnormals, NaN, Infinity, zero, negative zero), and bulk fraction round-trips
New testFormatDoubleSubnormals and testFormatDoubleRoundEven in NumbersTest for subnormal formatting and round-half-to-even correctness
Existing testFormatDoubleRandom (1M random doubles) and testFormatDoubleAsRandomFloat (1M random floats) verify round-trip correctness of the new Ryu-based Numbers.append
Updated CastDoubleToDecimalFunctionFactoryTest covering runtime casts, overflow, lossy truncation, constant folding, NaN/Infinity/null handling

🤖 Generated with Claude Code

Remove the scientific-notation forcing from extractDoubleDigits that produced extra digits for display formatting purposes. The forcing (low=false, high=false when estDecExp < -3 or >= 8) was inherited from the JDK's FloatingDecimal and ensured Dragon4 produced at least 2 significant digits for values in scientific notation range. This is a string formatting concern that leaked into the Decimal conversion path, causing: - Inflated precision/scale (e.g., 0.0001 got precision 5 instead of 4) - Wider Decimal type selection than necessary - 1e-76 failing to fit in Decimal256 (scale 77 > max 76) when 1 digit gives scale 76 (exact fit) The string formatter (appendDouble00) already handles nDigits==1 by padding with '0', so Numbers.append output is unchanged. Add comprehensive test coverage for doubleToDecimal with 77 tests covering: auto-inference (type selection, precision/scale, all three Decimal widths, boundary values, Dragon4 fast/slow paths, powers of 2, bulk round-trips) and target cast (lossy truncation, scale/precision overflow, NaN/Infinity, all Decimal types). Co-Authored-By: Claude Opus 4.6 <[email protected]>

Change the Dragon4 digit extraction buffer from char[] to byte[], storing raw digit values (0-9) instead of ASCII characters ('0'-'9'). This simplifies the DECIMAL consumption path, which previously had to subtract '0' from each digit. The STRING path now adds '0' when emitting digits via a small putAsciiDigits() helper — this is not a regression since the previous bulk putAscii(char[], start, len) was already a per-character loop in the CharSink default implementation, with no specialized override in any concrete sink used by this path. Co-Authored-By: Claude Opus 4.6 <[email protected]>

Add RyuDouble.java implementing the Ryu algorithm for O(1) double-to-decimal conversion via 128-bit multiply with precomputed lookup tables. This replaces the Dragon4 algorithm (extractDoubleDigits) which required the JDK-internal FDBigInteger class and ~250 lines of complex fallback code. The two doubleToDecimal() overloads now use RyuDouble.d2d() to get significant digits as a long plus a decimal exponent, then extract digits via division — no byte[] buffer needed. The string formatting path (Numbers.append) delegates to Double.toString() for exact JDK-compatible output, since Ryu can produce shorter-but-different representations that would break backward compatibility. Removed from Numbers.java: - extractDoubleDigits, estimateDecExpDouble, insignificantDigitsForPow2, roundupDouble - appendDouble0, appendDouble00, putAsciiDigits - Dragon4 constants (LONG_5_POW, SMALL_5_POW, N_5_BITS, etc.) - FDBigInteger import and Unsafe.addExports for jdk.internal.math Removed --add-exports java.base/jdk.internal.math from core/pom.xml (compiler and javadoc configurations). Updated CastDoubleToDecimalFunctionFactory and NumbersDoubleToDecimalTest to remove byte[] digits parameter. Co-Authored-By: Claude Opus 4.6 <[email protected]>

Replace the Double.toString()-based implementation of Numbers.append(CharSink, double, int) with a Ryu-based formatter that writes directly to CharSink. This eliminates a String allocation on every call, removing GC pressure on the hot data path. The new implementation decomposes the double via RyuDouble.d2d() and formats with three branches: fixed-point with integer part (decExp 1-7), leading-zero fixed-point (decExp 0 to -2), and scientific notation (otherwise). Scale truncation is applied in fixed-point branches only, matching prior behavior. Ryu produces the shortest decimal representation that round-trips to the original double, which may differ from JDK 11's Double.toString() by ±1 in the last digit on edge cases. Update random round-trip tests to verify parse correctness rather than exact string match with Double.toString(). Co-Authored-By: Claude Opus 4.6 <[email protected]>

Use QuestDB's ThreadLocal instead of java.lang.ThreadLocal for the tlE10 field in Numbers, and fix the modifier order to the standard private static final. Make RyuDouble internal helpers and constants private since only d2d() and decimalLength17() are called from outside the class. Update stale Dragon4 references in test comments to reflect the Ryu replacement. Co-Authored-By: Claude Opus 4.6 <[email protected]>

Add a PGWire client benchmark that measures the throughput of double-to-decimal and double-to-varchar conversions. The benchmark connects to an already-running QuestDB server and exercises all three Decimal widths (64, 128, 256) plus the VARCHAR formatting path. The test table uses exp(rnd_double(0) * 46 - 11.5) to generate a log-uniform distribution of 10M doubles spanning ~20 orders of magnitude, giving fair coverage to all formatting branches in Numbers.append() and all exponent ranges in RyuDouble.d2d(). Co-Authored-By: Claude Opus 4.6 <[email protected]>

In the trailing-zeros digit-removal loops, the `vrIsTrailingZeros` flag was being checked against `lastRemovedDigit` after it had already been overwritten with the current iteration's digit. This meant the "all removed digits were zero" invariant was tested against the wrong value. Move the `lastRemovedDigit` assignment after the trailing-zeros flag updates so that `vrIsTrailingZeros &= lastRemovedDigit == 0` correctly checks the previous iteration's digit. This fixes round-half-to-even: when the removed portion is exactly 0.5 and the retained significand is even, the algorithm now correctly avoids rounding up (e.g., 1.3607615367753142E15). Add a targeted regression test for this case. Co-Authored-By: Claude Opus 4.6 <[email protected]>

When casting a very small double (e.g. 1e-25) to a DECIMAL with a small target scale, the excess variable in the lossy truncation path could exceed the 20-element pow10 table, causing an ArrayIndexOutOfBoundsException. Since Ryu produces at most 17 significand digits, any excess >= olength means the entire significand is truncated away. Handle this case by setting output and olength to zero directly, skipping the table lookup. Add a regression test that triggers the crash with 1e-25 cast to DECIMAL(18,2). Co-Authored-By: Claude Opus 4.6 <[email protected]>

Document the shift-range precondition on mulShift64 ([65, 127]) and the bit-count precondition on multipleOfPowerOf2 ([0, 63]) in RyuDouble.java. Add an assertion in multipleOfPowerOf2 to catch violations during development. Fix incorrect comment in NumbersDoubleToDecimalTest that claimed naturalScale=42 for 1e-25 when the actual value is 25. Reorder the two test methods so testTargetLossyTruncateAllFractionalDigits comes before testTargetLossyTruncateExcessExceedsPow10Table. Add testFormatDoubleSubnormals to NumbersTest covering the Numbers.append formatting path for subnormal doubles: smallest, largest, negative, and mid-range subnormals, all verified via round-trip through Double.parseDouble. Co-Authored-By: Claude Opus 4.6 <[email protected]>

coderabbitai · 2026-02-19T20:27:32Z

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch mt_double-to-decimal

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

Remove unnecessary (double) casts in NumbersDoubleToDecimalTest and NumbersTest where int-to-double and float-to-double widening conversions are implicit. Also inline a redundant local variable in testTargetBulkRoundTrip. Co-Authored-By: Claude Opus 4.6 <[email protected]>

Move the two large power-of-5 lookup table initializations (DOUBLE_POW5_INV_SPLIT and DOUBLE_POW5_SPLIT) out of the static initializer and into dedicated private static methods. This reduces the <clinit> bytecode from 9596 bytes to 30 bytes, keeping each init method well under the 8000-byte linter threshold (4542 and 5034 bytes respectively). Co-Authored-By: Claude Opus 4.6 <[email protected]>

glasstiger · 2026-02-20T13:32:22Z

[PR Coverage check]

😍 pass : 286 / 304 (94.08%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/std/RyuDouble.java	156	168	92.86%
🔵	io/questdb/std/Numbers.java	124	130	95.38%
🔵	io/questdb/griffin/engine/functions/cast/CastDoubleToDecimalFunctionFactory.java	6	6	100.00%

bluestreak01 · 2026-02-20T20:11:33Z

@CodeRabbit pls review this PR critically

coderabbitai · 2026-02-20T20:15:14Z

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR details and file list
gh pr view 6792 --json title,body,additions,deletions,changedFiles,files --repo questdb/questdb

Length of output: 2717

🏁 Script executed:

#!/bin/bash
# Get the diff for the main implementation files
gh pr diff 6792 --repo questdb/questdb -- core/src/main/java/io/questdb/std/RyuDouble.java

Length of output: 36

🏁 Script executed:

#!/bin/bash
# Get the Numbers.java changes
gh pr diff 6792 --repo questdb/questdb -- core/src/main/java/io/questdb/std/Numbers.java

Length of output: 36

🏁 Script executed:

#!/bin/bash
# Get the CastDoubleToDecimalFunctionFactory changes
gh pr diff 6792 --repo questdb/questdb -- core/src/main/java/io/questdb/griffin/engine/functions/cast/CastDoubleToDecimalFunctionFactory.java

Length of output: 36

🏁 Script executed:

#!/bin/bash
# Get the test files to check coverage
gh pr diff 6792 --repo questdb/questdb -- core/src/test/java/io/questdb/test/std/NumbersDoubleToDecimalTest.java

Length of output: 36

🏁 Script executed:

#!/bin/bash
# Check if there are benchmark or performance tests
fd -e java -x grep -l "benchmark\|@Benchmark\|JMH" {} \; | head -20

Length of output: 1379

🏁 Script executed:

#!/bin/bash
# Get the full diff for the PR
gh pr diff 6792 --repo questdb/questdb > /tmp/pr_diff.txt
echo "Diff size: $(wc -l /tmp/pr_diff.txt)"

Length of output: 32

🏁 Script executed:

#!/bin/bash
# Extract RyuDouble.java from the diff
cat /tmp/pr_diff.txt | sed -n '/diff --git a\/core\/src\/main\/java\/io\/questdb\/std\/RyuDouble.java/,/^diff --git/p' | head -700

Length of output: 43751

🏁 Script executed:

#!/bin/bash
# Extract Numbers.java changes
cat /tmp/pr_diff.txt | sed -n '/diff --git a\/core\/src\/main\/java\/io\/questdb\/std\/Numbers.java/,/^diff --git/p' | head -800

Length of output: 31793

🏁 Script executed:

#!/bin/bash
# Check the actual files to understand the implementation
cat core/src/main/java/io/questdb/std/RyuDouble.java | head -300

Length of output: 27009

🏁 Script executed:

#!/bin/bash
# Check Numbers.java for the new doubleToDecimal method
cat core/src/main/java/io/questdb/std/Numbers.java | grep -A 50 "doubleToDecimal"

Length of output: 3586

🏁 Script executed:

#!/bin/bash
# Check the benchmark that was added
cat benchmarks/src/main/java/org/questdb/DoubleToDecimalBenchmark.java

Length of output: 5330

🏁 Script executed:

#!/bin/bash
# Check test coverage for edge cases
cat core/src/test/java/io/questdb/test/std/NumbersDoubleToDecimalTest.java | head -400

Length of output: 14664

🏁 Script executed:

#!/bin/bash
# Get more detailed view of the RyuDouble implementation focusing on critical sections
cat core/src/main/java/io/questdb/std/RyuDouble.java | grep -n -A 20 "mulShift64\|multipleOfPowerOf2\|multipleOfPowerOf5"

Length of output: 3880

🏁 Script executed:

#!/bin/bash
# Check the fast path for integer values that was mentioned in comments
cat core/src/main/java/io/questdb/std/RyuDouble.java | grep -n -B 5 -A 15 "Fast path"

Length of output: 885

🏁 Script executed:

#!/bin/bash
# Examine the CastDoubleToDecimalFunctionFactory changes
cat core/src/main/java/io/questdb/griffin/engine/functions/cast/CastDoubleToDecimalFunctionFactory.java

Length of output: 6335

🏁 Script executed:

#!/bin/bash
# Check which lines are not covered in the new code
# Look for complex branches, error paths, or edge cases
rg -n "assert |throw |if.*Infinity|if.*NaN" core/src/main/java/io/questdb/std/RyuDouble.java core/src/main/java/io/questdb/std/Numbers.java | head -50

Length of output: 6391

🏁 Script executed:

#!/bin/bash
# Check if there are any TODOs, FIXMEs, or XXX comments
rg -i "TODO|FIXME|XXX|HACK" core/src/main/java/io/questdb/std/RyuDouble.java core/src/main/java/io/questdb/std/Numbers.java

Length of output: 3353

🏁 Script executed:

#!/bin/bash
# Look at the test for the specific edge cases around subnormals
cat core/src/test/java/io/questdb/test/std/NumbersTest.java | grep -n -A 30 "testFormatDoubleSubnormals"

Length of output: 1383

🏁 Script executed:

#!/bin/bash
# Check if there are performance benchmarks comparing old vs new
fd -e java benchmark | xargs grep -l "Dragon4\|Ryu\|DoubleToDecimal"

Length of output: 66

@bluestreak01, I'll provide a critical review of this PR. Several concerns need to be addressed before merging.

Critical Issues

1. Unsafe assertion in production code (RyuDouble.java:391)

private static boolean multipleOfPowerOf2(long value, int p) {
    assert p >= 0 && p < 64;
    return (value & ((1L << p) - 1)) == 0;
}

Problem: This assertion can be disabled with -da in production. If p >= 64, the shift 1L << p masks to 6 bits and produces undefined behavior (e.g., 1L << 64 becomes 1L << 0).

Question: Why not use a runtime check or document the precondition more explicitly? The comment on line 388 says "Callers guarantee p < 63" but this is fragile - prove it or validate it.

2. ThreadLocal safety inconsistency (Numbers.java:73, CastDoubleToDecimalFunctionFactory.java:94,133)

private static final ThreadLocal<int[]> tlE10 = new ThreadLocal<>(() -> new int[1]);

And in the cast functions:

`@Override`
public boolean isThreadSafe() {
    return false;  // Line 94, 133
}

Problem: If doubleToDecimal() uses ThreadLocal, it should be thread-safe. But the functions that call it report isThreadSafe() = false. This is inconsistent.

Questions:

Can multiple threads call Numbers.doubleToDecimal() concurrently? (Answer: Yes, it's a static utility)
Then why do the functions report not thread-safe?
Is there a hidden shared mutable state in the Decimal* objects themselves?

3. Lossy truncation semantics are wrong for SQL CAST (Numbers.java:772-783)

// Truncate extra fractional digits if lossy
if (lossy && naturalScale > scale) {
    int excess = naturalScale - scale;
    if (excess >= olength) {
        output = 0;
        olength = 0;
    } else {
        output /= pow10[excess];  // Line 779: TRUNCATION, not rounding
        olength -= excess;
    }

Problem: SQL CAST semantics typically require rounding, not truncation. For example:

1.9999::DECIMAL(2,1) should give 2.0, not 1.9
But this code truncates: 1.9999 → mantissa 19999, excess=3, divide by 1000 → 19 → 1.9

Evidence: Check PostgreSQL, Oracle, SQL Server - they all round on overflow, not truncate.

Required action: Either fix the truncation to use rounding, or document why QuestDB differs from standard SQL semantics.

4. mulShift64 unsigned arithmetic correction is subtle (RyuDouble.java:376,378)

long high1 = Math.multiplyHigh(m, mul1) + ((mul1 >> 63) & m);
long high0 = Math.multiplyHigh(m, mul0) + ((mul0 >> 63) & m);

Concern: This correction for treating signed multiplyHigh as unsigned is correct in theory, but it's one of the most error-prone parts of the Ryu algorithm.

Questions:

Have you validated this against the reference C implementation?
What test specifically exercises the case where mul0 or mul1 have MSB set?
Can you add a targeted test with known values to prove this path is correct?

5. Benchmark doesn't measure what matters (DoubleToDecimalBenchmark.java:46-47)

"SELECT sum(d::DECIMAL(18,2)) FROM bench_double",
"SELECT sum(d::DECIMAL(38,10)) FROM bench_double",

Problem: Using sum() aggregation measures:

Conversion time (what you care about)
Addition time (decimal arithmetic)
Network serialization
Query planning overhead

The actual per-row conversion cost is buried in noise.

Required action: Create a proper JMH microbenchmark that isolates just the doubleToDecimal() call, like:

`@Benchmark`
public void doubleToDecimal64(Blackhole bh) {
    for (double d : testData) {
        Numbers.doubleToDecimal(d, sink64, sink128, sink256);
        bh.consume(sink64);
    }
}

6. Test coverage gaps for error paths

Coverage: 94.08% sounds good, but what are the 12 uncovered lines in RyuDouble.java?

Questions to answer:

Are the uncovered lines in error paths, edge cases, or main paths?
Line 391's assertion - is this tested to ensure p < 64 always holds?
Lines 489-499: The q <= 21 branch and multipleOfPowerOf5 checks - are these fully exercised?
What about the q < 63 check on line 520? What happens at q = 63?

Action required: Run coverage report and explicitly test the uncovered branches, or justify why they're unreachable.

7. Fast path integer detection has off-by-one risk (RyuDouble.java:438-440)

if (e2 >= -52 && e2 <= 0) {
    long mask = (1L << -e2) - 1;
    if ((m2 & mask) == 0) {

Question:

When e2 = 0, mask becomes (1L << 0) - 1 = 0. Is this intentional?
For e2 = -52, mask becomes (1L << 52) - 1. But m2 can have bit 52 set (line 433). Does this work correctly for the boundary case 2^52?

Test: Add explicit test for Math.pow(2, 52), Math.pow(2, 52) + 1, Math.pow(2, 53) to prove this works.

8. Removal of FDBigInteger dependency (pom.xml:0, Numbers.java:73)

Observation: You removed --add-exports java.base/jdk.internal.math which is great for portability.

Question: Did you verify that the new Ryu-based implementation produces identical results for all edge cases compared to the old Dragon4 implementation?

Action: Add a diff test that runs both implementations (if you keep old code temporarily) on millions of random doubles and compares outputs character-by-character.

9. Decimal construction in loop - error recovery? (Numbers.java:715-720)

target.ofZero();
for (int i = 0; i < olength; i++) {
    int d = (int) (output / pow10[olength - 1 - i] % 10);
    if (d != 0) {
        target.addPowerOfTenMultiple(decExp - 1 - i + naturalScale, d);
    }
}

Question: If addPowerOfTenMultiple() throws or fails partway through the loop, is target left in a corrupted state? Should you build in a temporary and then copy atomically?

10. Scale validation order (Numbers.java:789-798)

// Validate scale
if (naturalScale > scale) {
    throw NumericException.instance()
            .put("scale overflow: ").put(naturalScale).put(" > ").put(scale);
}

// Validate precision
int integerDigits = Math.max(0, decExp);
int requiredPrecision = integerDigits + scale;
if (requiredPrecision > precision) {

Minor issue: After lossy truncation (line 779), naturalScale is set to scale, so line 789 check is redundant. Either remove it or add a comment explaining it's defensive.

Required Actions

Fix or justify truncation vs rounding (#3) - blocking
Provide JMH microbenchmark (#5) - blocking for perf claims
Analyze uncovered lines (#6) - blocking for correctness confidence
Add boundary tests for fast path (#7)
Clarify thread-safety model (#2)
Document or validate assertion (#1)

Show me the coverage report and we can drill into the uncovered lines together.

🏁 Script executed:

#!/bin/bash
# Check what the old Dragon4 implementation looked like for comparison
git log --all --full-history --oneline -- "**/Numbers.java" | head -5

Length of output: 235

🏁 Script executed:

#!/bin/bash
# Look for any existing rounding behavior in decimal arithmetic
rg -n "round|Round" core/src/main/java/io/questdb/std/Decimal*.java | head -30

Length of output: 3755

🏁 Script executed:

#!/bin/bash
# Check if there are existing correctness tests comparing double->string->decimal vs direct
rg -n "parseDouble|Double.parse" core/src/test/java/io/questdb/test/std/NumbersDoubleToDecimalTest.java | head -20

Length of output: 403

🏁 Script executed:

#!/bin/bash
# Check what the "Fix pow10 array out-of-bounds in lossy truncation" commit fixed
git show d95abf08 --stat

Length of output: 1028

🏁 Script executed:

#!/bin/bash
# Check if there's any documentation about the lossy parameter and its intended semantics
rg -n -B 3 -A 10 "boolean lossy" core/src/main/java/io/questdb/std/Numbers.java core/src/main/java/io/questdb/griffin/engine/functions/cast/CastDoubleToDecimalFunctionFactory.java

Length of output: 1236

🏁 Script executed:

#!/bin/bash
# Look at how the Decimal64.fromDouble method handles rounding - this is the OLD way
git show master:core/src/main/java/io/questdb/std/Decimal64.java | grep -A 20 "fromDouble"

Length of output: 739

🏁 Script executed:

#!/bin/bash
# Check if there are any SQL standard compliance tests or references
rg -i "sql.*standard|postgres|oracle" core/src/test/java/io/questdb/test/griffin/engine/functions/cast/CastDoubleToDecimalFunctionFactoryTest.java | head -10

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Let's see what the actual behavior was before with the old Dragon4 - find a commit before this branch
git log --all --oneline --grep="Dragon4\|double.*decimal" | head -10

Length of output: 306

🏁 Script executed:

#!/bin/bash
# Check the actual test expectations to see if truncation vs rounding is intentional
cat core/src/test/java/io/questdb/test/griffin/engine/functions/cast/CastDoubleToDecimalFunctionFactoryTest.java | grep -A 5 -B 5 "lossy\|truncat\|round" | head -50

Length of output: 390

🏁 Script executed:

#!/bin/bash
# Get the full testLossy test to understand the intended behavior
cat core/src/test/java/io/questdb/test/griffin/engine/functions/cast/CastDoubleToDecimalFunctionFactoryTest.java | grep -A 30 "testLossy"

Length of output: 1369

🏁 Script executed:

#!/bin/bash
# Check the commit that replaced Dragon4 with Ryu to see the original justification
git show a3b5f052 --stat

Length of output: 1963

🏁 Script executed:

#!/bin/bash
# Look at the PR description or commit message for the Dragon4 replacement
git show a3b5f052 | head -100

Length of output: 5142

🏁 Script executed:

#!/bin/bash
# Check what the casting behavior test expects - does it round or truncate?
rg -n "123.456|123.45" core/src/test/java/io/questdb/test/griffin/engine/functions/cast/CastDoubleToDecimalFunctionFactoryTest.java

Length of output: 1515

🏁 Script executed:

#!/bin/bash
# Look for more context around the 1.9999 case I mentioned
cat core/src/test/java/io/questdb/test/griffin/engine/functions/cast/CastDoubleToDecimalFunctionFactoryTest.java | grep -B 3 -A 3 "1.99\|1.9\|2.0"

Length of output: 451

🏁 Script executed:

#!/bin/bash
# Check if the old BigDecimal-based implementation is still used anywhere for comparison
rg -n "BigDecimal.*setScale.*HALF_UP" core/src/main/java/io/questdb/

Length of output: 415

🏁 Script executed:

#!/bin/bash
# Get more detail on the comment about "lossy truncation" behavior
git show d95abf08

Length of output: 2871

@bluestreak01, I need to revise my earlier concern #3. After examining the code more carefully:

Update on "Lossy Truncation" (`#3` from my earlier review)

I was wrong about this being "wrong" - but it IS a breaking semantic change.

What changed:

Old implementation (master branch):

BigDecimal bd = new BigDecimal(value).setScale(scale, RoundingMode.HALF_UP);

→ 123.456 → DECIMAL(5,2) → 123.46 (rounded)

New implementation (this PR):

output /= pow10[excess];  // Truncation

→ 123.456 → DECIMAL(5,2) → 123.45 (truncated)

The evidence:

From your test (testLossy):

assertSql(
    "cast\n" + "123.45\n",  // Expected: 123.45 (truncated)
    "WITH data AS (SELECT 123.456 AS value) SELECT cast(value as DECIMAL(5,2)) FROM data"
);

This test expects truncation, meaning you've intentionally changed the semantics from rounding to truncation.

My critical question:

WHY make this breaking change?

Is this documented in the PR description? (I don't see it mentioned)
Is there a justification for diverging from:
- Standard SQL behavior (which typically rounds)
- PostgreSQL behavior (which rounds)
- Your own Decimal64.fromDouble() which still uses HALF_UP rounding
Have you audited all existing user queries that might silently produce different results after this change?

What I expect to see:

Either:

Justify: Explain in the PR description why truncation is preferred (performance? simpler? matches some other system?)
Or fix: Change line 779 from output /= pow10[excess] to proper rounding logic

If truncation is intentional, then at minimum:

Document this breaking change prominently in the PR description
Add a migration guide entry
Consider a config flag or deprecation period

Additional findings from scripts:

Good news:

The fromDouble() methods in Decimal64/128/256 still use HALF_UP rounding, so they're unaffected
The bug fix in commit d95abf08 for array bounds is correct

Bad news:

No coverage report provided yet (#6 from earlier review still stands)
Still need JMH microbenchmark (#5)
Still need boundary test validation (#7)

Bottom line: Is truncation vs rounding intentional? If yes, defend it. If no, fix it. Either way, this needs to be explicit in the PR description because it's a user-visible breaking change.

🧠 Learnings used

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

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

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

mtopolnik and others added 11 commits February 18, 2026 16:58

DOUBLE->DECIMAL direct conversion

95e1a42

Cleanup

3b54287

mtopolnik added SQL Issues or changes relating to SQL execution Performance Performance improvements labels Feb 19, 2026

mtopolnik and others added 8 commits February 19, 2026 21:28

Merge branch 'master' into mt_double-to-decimal

5cb9fc9

Update tests with Ryu's string values

876d853

Auto-reorder test code

0deb60e

Undo over-eager cast removal

1b41642

Fix Javadoc

3f8fd91

Auto-indent

2ceb16e

bluestreak01 approved these changes Feb 20, 2026

View reviewed changes

bluestreak01 merged commit 1fc8adc into master Feb 20, 2026
44 checks passed

bluestreak01 deleted the mt_double-to-decimal branch February 20, 2026 20:35

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

perf(sql): improve double-to-decimal cast performance (questdb#6792)

38c1685

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(sql): improve double-to-decimal cast performance#6792

perf(sql): improve double-to-decimal cast performance#6792
bluestreak01 merged 19 commits intomasterfrom
mt_double-to-decimal

mtopolnik commented Feb 19, 2026

Uh oh!

coderabbitai bot commented Feb 19, 2026 •

edited

Loading

Review skipped

Uh oh!

glasstiger commented Feb 20, 2026

Uh oh!

bluestreak01 commented Feb 20, 2026

Uh oh!

coderabbitai bot commented Feb 20, 2026

Critical Issues

1. Unsafe assertion in production code (RyuDouble.java:391)

2. ThreadLocal safety inconsistency (Numbers.java:73, CastDoubleToDecimalFunctionFactory.java:94,133)

3. Lossy truncation semantics are wrong for SQL CAST (Numbers.java:772-783)

4. mulShift64 unsigned arithmetic correction is subtle (RyuDouble.java:376,378)

5. Benchmark doesn't measure what matters (DoubleToDecimalBenchmark.java:46-47)

6. Test coverage gaps for error paths

7. Fast path integer detection has off-by-one risk (RyuDouble.java:438-440)

8. Removal of FDBigInteger dependency (pom.xml:0, Numbers.java:73)

9. Decimal construction in loop - error recovery? (Numbers.java:715-720)

10. Scale validation order (Numbers.java:789-798)

Required Actions

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mtopolnik commented Feb 19, 2026

Summary

Test plan

Uh oh!

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

Uh oh!

Review skipped

Uh oh!

glasstiger commented Feb 20, 2026

[PR Coverage check]

file detail

Uh oh!

bluestreak01 commented Feb 20, 2026

Uh oh!

coderabbitai bot commented Feb 20, 2026

Critical Issues

1. Unsafe assertion in production code (RyuDouble.java:391)

2. ThreadLocal safety inconsistency (Numbers.java:73, CastDoubleToDecimalFunctionFactory.java:94,133)

3. Lossy truncation semantics are wrong for SQL CAST (Numbers.java:772-783)

4. mulShift64 unsigned arithmetic correction is subtle (RyuDouble.java:376,378)

5. Benchmark doesn't measure what matters (DoubleToDecimalBenchmark.java:46-47)

6. Test coverage gaps for error paths

7. Fast path integer detection has off-by-one risk (RyuDouble.java:438-440)

8. Removal of FDBigInteger dependency (pom.xml:0, Numbers.java:73)

9. Decimal construction in loop - error recovery? (Numbers.java:715-720)

10. Scale validation order (Numbers.java:789-798)

Required Actions

Update on "Lossy Truncation" (#3 from my earlier review)

What changed:

The evidence:

My critical question:

What I expect to see:

Additional findings from scripts:

Good news:

Bad news:

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Feb 19, 2026 •

edited

Loading

Update on "Lossy Truncation" (`#3` from my earlier review)