You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@calcite.apache.org by "Maryann Xue (JIRA)" <ji...@apache.org> on 2018/01/12 04:12:00 UTC
[jira] [Commented] (CALCITE-1054) NPE caused by wrong code
generation for Timestamp fields
[ https://issues.apache.org/jira/browse/CALCITE-1054?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16323517#comment-16323517 ]
Maryann Xue commented on CALCITE-1054:
--------------------------------------
Could you please review this PR https://github.com/apache/calcite/pull/604, [~julianhyde]? After all this time, the generated code (before my fix) looks different and somewhat better now, as shown below, but the same problem persists.
{code}
/* 14 */ public boolean moveNext() {
/* 15 */ while (inputEnumerator.moveNext()) {
/* 16 */ final java.sql.Timestamp inp18_ = ((org.apache.calcite.test.ReflectiveSchemaTest.EveryType) inputEnumerator.current()).sqlTimestamp;
/* 17 */ final long v0 = org.apache.calcite.runtime.SqlFunctions.toLongOptional(inp18_).longValue();
/* 18 */ if (inp18_ != null && v0 >= -31536000000L && (inp18_ != null && v0 < 883612800000L)) {
/* 19 */ return true;
/* 20 */ }
/* 21 */ }
/* 22 */ return false;
/* 23 */ }
{code}
The code generated after my proposed fix:
{code}
/* 14 */ public boolean moveNext() {
/* 15 */ while (inputEnumerator.moveNext()) {
/* 16 */ final java.sql.Timestamp inp18_ = ((org.apache.calcite.test.ReflectiveSchemaTest.EveryType) inputEnumerator.current()).sqlTimestamp;
/* 17 */ final Long v = org.apache.calcite.runtime.SqlFunctions.toLongOptional(inp18_);
/* 18 */ if (inp18_ != null && v.longValue() >= -31536000000L && (inp18_ != null && v.longValue() < 883612800000L)) {
/* 19 */ return true;
/* 20 */ }
/* 21 */ }
/* 22 */ return false;
/* 23 */ }
{code}
I noticed that a piece of code had been put there in {{RexToLixTranslator.translate0()}} dedicated to handling nullable fields like in this case:
{code}
// If nullHandled is different, then it might be unsafe to compute
// early (i.e. unbox of null value should not happen _before_ ternary).
// Thus we wrap it into brand-new ParameterExpression,
// and we are guaranteed that ParameterExpression will not be shared
String unboxVarName = "v_unboxed";
if (input instanceof ParameterExpression) {
unboxVarName = ((ParameterExpression) input).name + "_unboxed";
}
ParameterExpression unboxed = Expressions.parameter(nullHandled.getType(),
list.newName(unboxVarName));
list.add(Expressions.declare(Modifier.FINAL, unboxed, nullHandled));
{code}
This will create a new Parameter unique to the unboxed expression, so that later on the {{BlockBuilder}} runs {{optimize}}, it will inline this variable and thus it will not appear in a declaration statement (as in line 17 of the wrong code block shown above). Date/Time/Timestamp types, because they have different internal representations, go through a different code path and thus did not have this special handling of unboxed values. My proposed change involves 2 things:
1. When doing type cast and deducing the target type, instead of looking at both sourceType nullability and the null map of the translator (which by then has already set it NOT NULL), it now looks at the sourceType nullability only.
2. Ensure the right unboxed value handling at the end of {{implementCall()}}.
> NPE caused by wrong code generation for Timestamp fields
> --------------------------------------------------------
>
> Key: CALCITE-1054
> URL: https://issues.apache.org/jira/browse/CALCITE-1054
> Project: Calcite
> Issue Type: Bug
> Components: core
> Affects Versions: 1.6.0, 1.5.0, 1.4.0-incubating
> Reporter: Frankie Bollaert
> Assignee: Julian Hyde
> Priority: Minor
>
> Problem occurs when:
> - Execute a query containing 2 checks on a Timestamp field
> - Table contains records which have NULL values for this field
> Example query:
> {code}
> select * from aTable where aTimestamp > timestamp '2015-1-1 00:00:00' and aTimestamp < timestamp '2015-2-1 00:00:00';
> {code}
> {code}
> /* 48 */ public boolean moveNext() {
> /* 49 */ while (inputEnumerator.moveNext()) {
> /* 50 */ final java.sql.Timestamp inp23_ = (java.sql.Timestamp) ((Object[]) inputEnumerator.current())[23];
> /* 51 */ final long v = org.apache.calcite.runtime.SqlFunctions.toLong(inp23_);
> /* 52 */ if (inp23_ != null && v > 1420070400000L && (inp23_ != null && v < 1422748800000L)) {
> /* 53 */ return true;
> /* 54 */ }
> /* 55 */ }
> /* 56 */ return false;
> /* 57 */ }
> {code}
> Stack trace snippet
> {code}
> Caused by: java.lang.NullPointerException
> at org.apache.calcite.runtime.SqlFunctions.toLong(SqlFunctions.java:1094)
> at org.apache.calcite.runtime.SqlFunctions.toLong(SqlFunctions.java:1089)
> at Baz$1$1.moveNext(ANONYMOUS.java:51)
> at org.apache.calcite.linq4j.Linq4j$EnumeratorIterator.<init>(Linq4j.java:677)
> at org.apache.calcite.linq4j.Linq4j.enumeratorIterator(Linq4j.java:103)
> {code}
> The generated code also looks wrong for date fields.
> {code}
> /* 15 */ public boolean moveNext() {
> /* 16 */ while (inputEnumerator.moveNext()) {
> /* 17 */ final java.sql.Date current = (java.sql.Date) inputEnumerator.current();
> /* 18 */ final int v = org.apache.calcite.runtime.SqlFunctions.toInt(current);
> /* 19 */ if (current != null && v > 2780 && (current != null && v < 5290)) {
> /* 20 */ return true;
> /* 21 */ }
> /* 22 */ }
> /* 23 */ return false;
> /* 24 */ }
> {code}
> \\
> Other types of fields do not have this problem. Below is what the generated code looks like in the case of a String field. *On line 20 there is a null check.* This is the type of check that needs to be generated for Timestamp fields as well.
> {code}
> select empno from sales.emps where gender > 'A' and gender < 'Z';
> {code}
> {code}
> /* 17 */ public boolean moveNext() {
> /* 18 */ while (inputEnumerator.moveNext()) {
> /* 19 */ final Object[] current = (Object[]) inputEnumerator.current();
> /* 20 */ final String inp3_ = current[3] == null ? (String) null : current[3].toString();
> /* 21 */ if (inp3_ != null && org.apache.calcite.runtime.SqlFunctions.gt(inp3_, $L4J$C$org_apache_calcite_runtime_SqlFunctions_rtrim_A_) && (inp3_ != null && org.apache.calcite.runtime.SqlFunctions.lt(inp3_, $L4J$C$org_apache_calcite_runtime_SqlFunctions_rtrim_Z_))) {
> /* 22 */ return true;
> /* 23 */ }
> /* 24 */ }
> /* 25 */ return false;
> /* 26 */ }
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)