[
https://issues.apache.org/jira/browse/CALCITE-3173?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17040645#comment-17040645
]
Danny Chen commented on CALCITE-3173:
-------------------------------------
Seems an umbrella issue and not possible to be resolved in release 1.22. So i
remove the resolve version tag.
> RexNode Code Generation Problem
> -------------------------------
>
> Key: CALCITE-3173
> URL: https://issues.apache.org/jira/browse/CALCITE-3173
> Project: Calcite
> Issue Type: Improvement
> Components: core
> Affects Versions: 1.20.0
> Reporter: Feng Zhu
> Assignee: Feng Zhu
> Priority: Critical
> Labels: pull-request-available
> Attachments: code.png, codegen.png
>
>
> *Abstract:* Both RexImpTable and BlockBuilder have codegen issues, but it is
> interesting that they can work together well for most cases.
> We can illustrate the problem with a simple test case in JdbcTest, in
> which the "commission" column is nullable.
> {code:java}
> @Test public void testNullSafeCheck() {
> CalciteAssert.hr()
> .query("select \"commission\" + 10 as s from \"hr\".\"emps\"")
> .returns("S=1010\n"
> + "S=510\n"
> + "S=null\n"
> + "S=260\n");
> }
> {code}
> This test case can pass as the BlockBuilder is in default optimization
> mode. However, when we set it into un-optimization mode in _EnumerableCalc_,
> this test will fail due to NPE. The following picture demonstrates their
> differences.
> !code.png!
> *1.RexImpTable generates unsafe code*
> Before translating the RexCall (_*Add*_), RexImpTable firstly translate
> its operands with (nullAs=*IS_NULL*) [1] to detect whether it is null (i.e.,
> {color:#ff0000}_inp4_unboxed_{color}). Then RexImpTable sets this operand's
> nullable in RexToLixTranslator as {color:#FF0000}false{color} [2]. After
> that, the operand will be translated again with *NOT_POSSIBLE* [3] to get the
> value (i.e., {color:#ff0000}_inp4_0_unboxed_{color}). In the end, the RexCall
> is implemented by NotNullImplementor.However, it is not safe to conduct
> operations like unboxing in the second translation phase.
> *2.BlockBuiler optimization's semantic issue buries NPE*
> BlockBuilder.optimize() changes the code semantic in this case. For
> conditional-like clauses (if...else, ?:, etc), InlineVariableVisitor will
> wrongly make variables inlined.
> In general, they can work together for most cases. However, when some
> special branch is triggered by query, the problem will be exposed. For
> example, the NewExpression (_new java.math.BigDecimal_) in CALCITE-3143
> breaks the inline optimization phase.
>
> *How to fix?*
> I have digged into this problem a couple of days and tried many
> approaches to fix it. But in this progress, I found the limitation in current
> implementation. The whole recursive framework essentially conducts a
> sequential codegen beheavior, and may visit a RexNode again and again with
> different NullAs settings.
> Due to the limitation, it is difficult to implement null-safe codegen
> semantics with branching logic. We can also find that there are many branches
> for special cases in current implementation. Even we can handle potential
> issues every time, the logic will becomes more and more complex and
> unfriendly for maintenance.
>
> Therefore, I propose to re-consider this part, including several initial
> points.
> (1) {color:#ff0000}_Visitor Pattern_{color} (RexVisitor<Result>).
> Theoretically, RexNode can be translated into Expression by visiting the node
> only once. We can implement RexVisitor rather than current recursive
> translation.
> (2)The {color:#ff0000}Result{color} consists of three items (code:
> BuilderStatement, isNull: ParameterExpression, value: Expression).So it is
> easy to decide how to implement a RexNode according to its children.
>
> Please correct me if I make something wrong. Look forward to suggestions!
>
> [1][https://github.com/apache/calcite/blob/1748f0503e7b626a8d0165f1698adb8b61bbc31e/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java#L1062]
> [2][https://github.com/apache/calcite/blob/1748f0503e7b626a8d0165f1698adb8b61bbc31e/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java#L1064]
> [3][https://github.com/apache/calcite/blob/1748f0503e7b626a8d0165f1698adb8b61bbc31e/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java#L1113]
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
