[CALCITE-2684] AssertionError on RexBuilder when creating BigDecimal RexLiteral (Ruben Quesada Lopez) #928
Conversation
| new SqlTypeFactoryImpl(RelDataTypeSystem.DEFAULT); | ||
| final RexBuilder builder = new RexBuilder(typeFactory); | ||
| final RexNode literal1 = builder.makeExactLiteral(new BigDecimal("25")); | ||
| assertThat(((RexLiteral) literal1).getValueAs(BigDecimal.class) |
There was a problem hiding this comment.
Would you please factor the common logic to assertBigDecimalLiteral(String inputText, String expectedOutputText)?
That would make test much more readable (it won't include repeated builder.makeExactLiteral...), and the failure message would be much easier to understand.
Thanks.
|
|
||
| private void assertBigDecimalLiteral(RexBuilder builder, String val) { | ||
| final RexNode literal = builder.makeExactLiteral(new BigDecimal(val)); | ||
| assertThat(((RexLiteral) literal).getValueAs(BigDecimal.class).toString(), is(val)); |
There was a problem hiding this comment.
Please add message parameter to assertThat so it is clear WHAT is test testing
There was a problem hiding this comment.
Message parameter included.
|
|
||
| private void assertBigDecimalLiteral(RexBuilder builder, String val) { | ||
| final RexNode literal = builder.makeExactLiteral(new BigDecimal(val)); | ||
| assertThat("Generated wrong BigDecimal RexLiteral", |
There was a problem hiding this comment.
"Generated wrong BigDecimal RexLiteral" does not clarify much. testBigDecimalLiteral clarifies much neither.
Exception message should provide clear (to a developer who never looked into the test method source) indication on why the test failed.
For instance:
assertThat("builder.makeExactLiteral(new BigDecimal(" + val + ")).toString()", ..., is(val))
There was a problem hiding this comment.
Thanks for the suggestion, I agree.
Code modified.
|
|
||
| private void assertBigDecimalLiteral(RexBuilder builder, String val) { | ||
| final RexNode literal = builder.makeExactLiteral(new BigDecimal(val)); | ||
| assertThat("builder.makeExactLiteral(new BigDecimal(" + val + ")).toString()", |
There was a problem hiding this comment.
Ah, there's .getValueAs(BigDecimal.class) as well.
Would you please add it to the message?
|
LGTM |
|
changes look good, but can you squash the commits into 1 ? |
rubenada
force-pushed
the
CALCITE-2684
branch
from
85ae4bd
to
a6ebfb1
Compare
|
Commits squashed into a single one |
| @@ -926,12 +926,11 @@ public RexLiteral makeLiteral(boolean b) { | |||
| public RexLiteral makeExactLiteral(BigDecimal bd) { | |||
| RelDataType relType; | |||
| int scale = bd.scale(); | |||
| long l = bd.unscaledValue().longValue(); | |||
| assert scale >= 0; | |||
…literal larger than 2^63 (Ruben Quesada Lopez) Close apache#928 For [CALCITE-2701]: Close apache#935
…literal larger than 2^63 (Ruben Quesada Lopez) Close apache#928 For [CALCITE-2701]: Close apache#935
…literal larger than 2^63 (Ruben Quesada Lopez) Close apache#928 For [CALCITE-2701]: Close apache#935
The method RexBuilder#makeExactLiteral(java.math.BigDecimal) throws an AssertionError if the BigDecimal parameter has an unscaled value that overflows long.
Moreover, with the current implementation, it can be possible to have this AssertionError, even in the cases when the variable l would not be used at all (decimal number).
Provided unit tests (the last one will fail with the current implementation).