Fix flaky client-v2 testDynamicWithPrimitives (deterministic Decimal samples) - #2965
Open
polyglotAI-bot wants to merge 1 commit into
Open
Fix flaky client-v2 testDynamicWithPrimitives (deterministic Decimal samples)#2965polyglotAI-bot wants to merge 1 commit into
polyglotAI-bot wants to merge 1 commit into
Conversation
…samples DataTypesTestingPOJO seeded its four Decimal fields with BigDecimal.valueOf(random.nextDouble()). When a small-magnitude random value's scale exceeded the scale inferred for a Dynamic Decimal column (the width is chosen from precision, but the scale is forced to that width's maximum), the value was rounded on write; the read-back value no longer equaled the original and the round-trip check divide(value, RoundingMode.UNNECESSARY) threw "Rounding necessary" (~1% of runs, seen on the ClickHouse 25.8 CI matrix). Use fixed Decimal samples whose scale fits each inferred width, so the round-trip is exact and the test is deterministic. The samples also now land in distinct Decimal32/64/128/256 widths (the random doubles almost always landed in Decimal64). Fixes: #2964
|
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2964.
client-v2DataTypeTests.testDynamicWithPrimitivesis flaky — it intermittently fails withjava.lang.ArithmeticException: Rounding necessary(~1% of runs, observed on the ClickHouse25.8CI matrix). The shared fixtureDataTypesTestingPOJOassigned its fourDecimalfields from an unseededRandomviaBigDecimal.valueOf(random.nextDouble()). When aBigDecimalis inserted into aDynamiccolumn,SerializerUtils.valueToColumnForDynamicTypechooses the decimal width from the value'sprecision()but forces the scale to that width's maximum (Decimal32→9, Decimal64→18, Decimal128→38, Decimal256→76). A small-magnitude random double can have ascale()greater than that fixed scale (e.g.0.0123456789: precision 9 →Decimal32(9), but scale 10 > 9), so the value is rounded on write, the read-back no longer equals the original, and the test'stmpDec.divide(value, RoundingMode.UNNECESSARY)throwsRounding necessary. Because the input is random, the failure is non-deterministic.This changes only the test fixture: the four
Decimalsamples become fixed values whose scale fits each inferred width, so every round-trip is exact and the test is deterministic. The values were also chosen to land in distinctDecimal32/64/128/256widths — the random doubles almost always landed inDecimal64, so this additionally broadens what the test exercises.Changes
client-v2/.../datatypes/DataTypesTestingPOJO.java: replace the fourBigDecimal.valueOf(random.nextDouble())decimal assignments with fixed samples:decimal32 = 0.123456789(precision 9 →Decimal32(9))decimal64 = 0.123456789012345678(precision 18 →Decimal64(18))decimal128 = 0.12345678901234567890123456789012345678(precision 38 →Decimal128(38))decimal256 = 0.12345678901234567890123456789012345678901234567890123456789(precision 59 →Decimal256(76))scale()≤ the inferred width's fixed scale (so the round-trip is lossless), and none end in a trailing zero (the test doesstripTrailingZeros()before comparing, so a trailing zero would break the string compare).Test
No new test — the fix makes the existing
testDynamicWithPrimitivesdeterministic; that test is the regression guard. Verified in a devbox against a live ClickHouse server (client-v2 integration path,client.insert/client.queryAll):decimal32 = 0.0123456789(scale 10 > Decimal32's fixed scale 9) reproduces the exact failure deterministically —ArithmeticException: Rounding necessaryattestDynamicWithPrimitives.The assertion itself is unchanged — its intent (exact Decimal round-trip through a
Dynamiccolumn) is preserved; only the non-deterministic input is removed.Notes
CHANGELOG.md/docs/features.mdentry.valueToColumnForDynamicTypekeying the decimal width offprecision()alone and forcing the band's max scale, which can silently truncate a high-scale/low-precisiondecimal on aDynamicinsert — is a separate concern and is not addressed here.Pre-PR validation gate
Rounding necessary)DataTypesTestingPOJO's decimal getters are used only bytestDynamicWithPrimitives; the other user (testVariantWithSimpleDataTypes) skips allDecimalcases