[jira] [Commented] (CALCITE-3142) An NPE when rounding a nullable numeric

["Feng Zhu (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-3142?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16882691#comment-16882691 ] 

Feng Zhu commented on CALCITE-3142:
-----------------------------------

Hi [~vladimirsitnikov] [~zabetak] [~julianhyde], I opened an initial PR to demonstrate the approach for this kind of issues.
I need your help to see whether it is in a right direction.

 

*Root Cause:*
It is not safe to implement a RexCall in current translator's BlockBuilder, even all its operands have be translated with IS_NULL. 
Current implementation relies on BlockBuilder's optimization phase to inline these unsafe operations, bringing potential risks like NPE. 

*Approach:*
We first use another translator (called "notNullTranslator") with a new constructed BlockBuilder to keep this piece of code.
After that, we wrap this code with conditions and add it back. 

We use "*_if_*" expression for conditional clause. For example, the new generated code for this jira is:
{code:java}
public Object current() {
  final Object[] current = (Object[]) inputEnumerator.current();
  final Integer inp0_ = (Integer) current[0];
  final Integer inp1_ = (Integer) current[1];
  java.math.BigDecimal _call_result = (java.math.BigDecimal) null;
  if (!(inp0_ == null || inp1_ == null)) {
    final java.math.BigDecimal v1 = new java.math.BigDecimal(inp0_.intValue() / inp1_.intValue());
    _call_result = org.apache.calcite.runtime.SqlFunctions.sround(v1, 2);
  }
  final java.math.BigDecimal _call_result0 = _call_result;
  return _call_result0;
}
{code}
The whole codegen process is shown in the figure below.

!newcodegen.png!

*TODO:*
This approach ensures the code correctness but introduces many patterns that need to be optimized in BlockBuilder.
If you agree, I will summarize thses patterns and open separated Jira tasks.

> An NPE when rounding a nullable numeric
> ---------------------------------------
>
>                 Key: CALCITE-3142
>                 URL: https://issues.apache.org/jira/browse/CALCITE-3142
>             Project: Calcite
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 1.20.0
>            Reporter: Muhammad Gelbana
>            Assignee: Feng Zhu
>            Priority: Major
>              Labels: pull-request-available
>         Attachments: newcodegen.png
>
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> The following query throws a NPE in the generated code because it assumes the divided value to be an initialized Java object (Not null), which is fine for the first row, but not for the second.
> {code:sql}
> SELECT ROUND(CAST((X/Y) AS NUMERIC), 2) FROM (VALUES (1, 2), (NULLIF(5, 5), NULLIF(5, 5))) A(X, Y){code}
> If I modify the query a little bit, it runs ok:
>  – No casting
> {code:sql}
> SELECT ROUND((X/Y), 2) FROM (VALUES (1, 2), (NULLIF(5, 5), NULLIF(5, 5))) A(X, Y){code}
> – No rounding
> {code:sql}
> SELECT (X/Y)::NUMERIC FROM (VALUES (1, 2), (NULLIF(5, 5), NULLIF(5, 5))) A(X, Y){code}
> +This is the optimized generated code+
> {code:java}
> final Object[] current = (Object[]) inputEnumerator.current();
> final Integer inp0_ = (Integer) current[0];
> final Integer inp1_ = (Integer) current[1];
> final java.math.BigDecimal v1 = new java.math.BigDecimal(
>   inp0_.intValue() / inp1_.intValue()); // <<< NPE
> return inp0_ == null || inp1_ == null ? (java.math.BigDecimal) null : org.apache.calcite.runtime.SqlFunctions.sround(v1, 2);{code}
> +This is the non-optimized one+
> {code:java}
> final Object[] current = (Object[]) inputEnumerator.current();
> final Integer inp0_ = (Integer) current[0];
> final boolean inp0__unboxed = inp0_ == null;
> final Integer inp1_ = (Integer) current[1];
> final boolean inp1__unboxed = inp1_ == null;
> final boolean v = inp0__unboxed || inp1__unboxed;
> final int inp0__unboxed0 = inp0_.intValue(); // <<< NPE
> final int inp1__unboxed0 = inp1_.intValue(); // <<< NPE
> final int v0 = inp0__unboxed0 / inp1__unboxed0;
> final java.math.BigDecimal v1 = new java.math.BigDecimal(
>   v0);
> final java.math.BigDecimal v2 = v ? (java.math.BigDecimal) null : org.apache.calcite.runtime.SqlFunctions.sround(v1, 2);
> return v2;{code}



--
This message was sent by Atlassian JIRA
(v7.6.14#76016)