rubenada commented on PR #5223: URL: https://github.com/apache/calcite/pull/5223#issuecomment-5451381187
Honestly, I don't know if we can realistically fall into a risky scenario for this class (looks like a "patter-matching" fix). But the check won't hurt. I have created a specific ticket for this: "[CALCITE-7750](https://issues.apache.org/jira/browse/CALCITE-7750) Bound plain-notation expansion of DECIMAL literals in Primitive.checkOverflow" Please use it on the commit message. Also, please add some unit tests for this in PrimitiveTest.java, e.g.: ``` @Test void testCharToDecimalCastWithinBounds() { assertThat(Primitive.charToDecimalCast("1.5", 5, 2), is(new BigDecimal("1.50"))); assertThat(Primitive.charToDecimalCast("0", 38, 0), is(new BigDecimal("0"))); // scale < 0 branch, well below the bound. assertThat(Primitive.charToDecimalCast("1000", 4, -3), is(new BigDecimal("1000"))); } /** Test case for * <a href="https://issues.apache.org/jira/browse/CALCITE-7750";>[CALCITE-7750] * Bound plain-notation expansion of DECIMAL literals in Primitive.checkOverflow</a>. */ @Test void testCharToDecimalCastRejectsPathologicalScale() { IllegalArgumentException e = assertThrows(IllegalArgumentException.class, () -> Primitive.charToDecimalCast("1E10000", 1, -10_000)); assertThat(e.getMessage(), containsString("plain-notation bound")); } ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
