[jira] [Commented] (CALCITE-2684) AssertionError on RexBuilder when creating BigDecimal RexLiteral
[
https://issues.apache.org/jira/browse/CALCITE-2684?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16708433#comment-16708433
]
Ruben Quesada Lopez commented on CALCITE-2684:
--
You're welcome, [~julianhyde]
> AssertionError on RexBuilder when creating BigDecimal RexLiteral
>
>
> Key: CALCITE-2684
> URL: https://issues.apache.org/jira/browse/CALCITE-2684
> Project: Calcite
> Issue Type: Bug
>Affects Versions: 1.17.0
>Reporter: Ruben Quesada Lopez
>Assignee: Julian Hyde
>Priority: Minor
> Fix For: 1.18.0
>
>
> The method {{RexBuilder#makeExactLiteral(java.math.BigDecimal)}} throws an
> AssertionError if the BigDecimal parameter has an unscaled value that
> overflows long:
> {code:java}
> public RexLiteral makeExactLiteral(BigDecimal bd) {
> ...
> long l = bd.unscaledValue().longValue(); // narrowing conversion
> BigInteter to long
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert fails if l
> overflew long
> ...
> {code}
> 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)
> {code:java}
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert can fail, even if
> scale == 0 (l would not be needed)
> if (scale == 0) {
> if ((l >= Integer.MIN_VALUE) && (l <= Integer.MAX_VALUE))
> relType = typeFactory.createSqlType(SqlTypeName.INTEGER);
> else
> relType = typeFactory.createSqlType(SqlTypeName.BIGINT);
> } else {
> int precision = bd.unscaledValue().abs().toString().length();
> if (precision > scale)
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, precision,
> scale);
> else
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, scale + 1,
> scale);
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2684) AssertionError on RexBuilder when creating BigDecimal RexLiteral
[
https://issues.apache.org/jira/browse/CALCITE-2684?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16692884#comment-16692884
]
Ruben Quesada Lopez commented on CALCITE-2684:
--
PR modified
> AssertionError on RexBuilder when creating BigDecimal RexLiteral
>
>
> Key: CALCITE-2684
> URL: https://issues.apache.org/jira/browse/CALCITE-2684
> Project: Calcite
> Issue Type: Bug
>Affects Versions: 1.17.0
>Reporter: Ruben Quesada Lopez
>Assignee: Julian Hyde
>Priority: Minor
> Fix For: 1.18.0
>
>
> The method {{RexBuilder#makeExactLiteral(java.math.BigDecimal)}} throws an
> AssertionError if the BigDecimal parameter has an unscaled value that
> overflows long:
> {code:java}
> public RexLiteral makeExactLiteral(BigDecimal bd) {
> ...
> long l = bd.unscaledValue().longValue(); // narrowing conversion
> BigInteter to long
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert fails if l
> overflew long
> ...
> {code}
> 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)
> {code:java}
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert can fail, even if
> scale == 0 (l would not be needed)
> if (scale == 0) {
> if ((l >= Integer.MIN_VALUE) && (l <= Integer.MAX_VALUE))
> relType = typeFactory.createSqlType(SqlTypeName.INTEGER);
> else
> relType = typeFactory.createSqlType(SqlTypeName.BIGINT);
> } else {
> int precision = bd.unscaledValue().abs().toString().length();
> if (precision > scale)
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, precision,
> scale);
> else
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, scale + 1,
> scale);
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2684) AssertionError on RexBuilder when creating BigDecimal RexLiteral
[
https://issues.apache.org/jira/browse/CALCITE-2684?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16692869#comment-16692869
]
Ruben Quesada Lopez commented on CALCITE-2684:
--
I agree, that's a clearer solution, I'll change it.
> AssertionError on RexBuilder when creating BigDecimal RexLiteral
>
>
> Key: CALCITE-2684
> URL: https://issues.apache.org/jira/browse/CALCITE-2684
> Project: Calcite
> Issue Type: Bug
>Affects Versions: 1.17.0
>Reporter: Ruben Quesada Lopez
>Assignee: Julian Hyde
>Priority: Minor
> Fix For: 1.18.0
>
>
> The method {{RexBuilder#makeExactLiteral(java.math.BigDecimal)}} throws an
> AssertionError if the BigDecimal parameter has an unscaled value that
> overflows long:
> {code:java}
> public RexLiteral makeExactLiteral(BigDecimal bd) {
> ...
> long l = bd.unscaledValue().longValue(); // narrowing conversion
> BigInteter to long
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert fails if l
> overflew long
> ...
> {code}
> 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)
> {code:java}
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert can fail, even if
> scale == 0 (l would not be needed)
> if (scale == 0) {
> if ((l >= Integer.MIN_VALUE) && (l <= Integer.MAX_VALUE))
> relType = typeFactory.createSqlType(SqlTypeName.INTEGER);
> else
> relType = typeFactory.createSqlType(SqlTypeName.BIGINT);
> } else {
> int precision = bd.unscaledValue().abs().toString().length();
> if (precision > scale)
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, precision,
> scale);
> else
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, scale + 1,
> scale);
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2684) AssertionError on RexBuilder when creating BigDecimal RexLiteral
[
https://issues.apache.org/jira/browse/CALCITE-2684?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16692867#comment-16692867
]
Julian Hyde commented on CALCITE-2684:
--
I guess you're right. It's fairly subtle, though.
I would have checked whether {{bd}} is between
{{BigDecimal.valueOf(Integer.MIN_VALUE)}} and
{{BigDecimal.valueOf(Integer.MAX_VALUE)}}. Since all arithmetic is in
{{BigDecimal}} there is no chance of overflow.
> AssertionError on RexBuilder when creating BigDecimal RexLiteral
>
>
> Key: CALCITE-2684
> URL: https://issues.apache.org/jira/browse/CALCITE-2684
> Project: Calcite
> Issue Type: Bug
>Affects Versions: 1.17.0
>Reporter: Ruben Quesada Lopez
>Assignee: Julian Hyde
>Priority: Minor
> Fix For: 1.18.0
>
>
> The method {{RexBuilder#makeExactLiteral(java.math.BigDecimal)}} throws an
> AssertionError if the BigDecimal parameter has an unscaled value that
> overflows long:
> {code:java}
> public RexLiteral makeExactLiteral(BigDecimal bd) {
> ...
> long l = bd.unscaledValue().longValue(); // narrowing conversion
> BigInteter to long
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert fails if l
> overflew long
> ...
> {code}
> 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)
> {code:java}
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert can fail, even if
> scale == 0 (l would not be needed)
> if (scale == 0) {
> if ((l >= Integer.MIN_VALUE) && (l <= Integer.MAX_VALUE))
> relType = typeFactory.createSqlType(SqlTypeName.INTEGER);
> else
> relType = typeFactory.createSqlType(SqlTypeName.BIGINT);
> } else {
> int precision = bd.unscaledValue().abs().toString().length();
> if (precision > scale)
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, precision,
> scale);
> else
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, scale + 1,
> scale);
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2684) AssertionError on RexBuilder when creating BigDecimal RexLiteral
[
https://issues.apache.org/jira/browse/CALCITE-2684?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16692845#comment-16692845
]
Ruben Quesada Lopez commented on CALCITE-2684:
--
Thanks for your feedback, [~julianhyde].
In the example that you describe, a BIGINT will be generated. The reason for
that is that the {{bd.unscaledValue().longValue()}} will overflow the long
range, therefore the first condition of the "if" will be false
{{(BigDecimal.valueOf(l, scale).equals(bd))}}.
{code:java}
...
long l = bd.unscaledValue().longValue();
if ((BigDecimal.valueOf(l, scale).equals(bd))
&& (l >= Integer.MIN_VALUE) && (l <= Integer.MAX_VALUE)) {
relType = typeFactory.createSqlType(SqlTypeName.INTEGER);
} else {
relType = typeFactory.createSqlType(SqlTypeName.BIGINT);
}
{code}
The logic that I'm trying to apply is that: check if {{l}} is in the integer
range (to generate an INTEGER) only if {{l}} is the "right long value", i.e. it
did not overflow, and in order to check that we verify if we can rebuild the
original BigDecimal parameters using {{l}} and {{scale}}. Otherwise, generate a
BIGINT.
> AssertionError on RexBuilder when creating BigDecimal RexLiteral
>
>
> Key: CALCITE-2684
> URL: https://issues.apache.org/jira/browse/CALCITE-2684
> Project: Calcite
> Issue Type: Bug
>Affects Versions: 1.17.0
>Reporter: Ruben Quesada Lopez
>Assignee: Julian Hyde
>Priority: Minor
> Fix For: 1.18.0
>
>
> The method {{RexBuilder#makeExactLiteral(java.math.BigDecimal)}} throws an
> AssertionError if the BigDecimal parameter has an unscaled value that
> overflows long:
> {code:java}
> public RexLiteral makeExactLiteral(BigDecimal bd) {
> ...
> long l = bd.unscaledValue().longValue(); // narrowing conversion
> BigInteter to long
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert fails if l
> overflew long
> ...
> {code}
> 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)
> {code:java}
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert can fail, even if
> scale == 0 (l would not be needed)
> if (scale == 0) {
> if ((l >= Integer.MIN_VALUE) && (l <= Integer.MAX_VALUE))
> relType = typeFactory.createSqlType(SqlTypeName.INTEGER);
> else
> relType = typeFactory.createSqlType(SqlTypeName.BIGINT);
> } else {
> int precision = bd.unscaledValue().abs().toString().length();
> if (precision > scale)
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, precision,
> scale);
> else
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, scale + 1,
> scale);
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (CALCITE-2684) AssertionError on RexBuilder when creating BigDecimal RexLiteral
[
https://issues.apache.org/jira/browse/CALCITE-2684?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16692120#comment-16692120
]
Julian Hyde commented on CALCITE-2684:
--
I agree this is an issue. I don't agree with your solution. What if the literal
is 2 ^ 66 (i.e. (Long.MAX_VALUE + 1) * 8)? I expect that BigDecimal.longValue()
will be 0 (because the high-order bits are removed). Therefore we would
incorrectly use an INTEGER for this.
Can you write a test for this, and report what happens?
> AssertionError on RexBuilder when creating BigDecimal RexLiteral
>
>
> Key: CALCITE-2684
> URL: https://issues.apache.org/jira/browse/CALCITE-2684
> Project: Calcite
> Issue Type: Bug
>Affects Versions: 1.17.0
>Reporter: Ruben Quesada Lopez
>Assignee: Julian Hyde
>Priority: Minor
> Fix For: 1.18.0
>
>
> The method {{RexBuilder#makeExactLiteral(java.math.BigDecimal)}} throws an
> AssertionError if the BigDecimal parameter has an unscaled value that
> overflows long:
> {code:java}
> public RexLiteral makeExactLiteral(BigDecimal bd) {
> ...
> long l = bd.unscaledValue().longValue(); // narrowing conversion
> BigInteter to long
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert fails if l
> overflew long
> ...
> {code}
> 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)
> {code:java}
> ...
> assert BigDecimal.valueOf(l, scale).equals(bd); // assert can fail, even if
> scale == 0 (l would not be needed)
> if (scale == 0) {
> if ((l >= Integer.MIN_VALUE) && (l <= Integer.MAX_VALUE))
> relType = typeFactory.createSqlType(SqlTypeName.INTEGER);
> else
> relType = typeFactory.createSqlType(SqlTypeName.BIGINT);
> } else {
> int precision = bd.unscaledValue().abs().toString().length();
> if (precision > scale)
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, precision,
> scale);
> else
> relType = typeFactory.createSqlType(SqlTypeName.DECIMAL, scale + 1,
> scale);
> }
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
