DonnyZone commented on a change in pull request #1347: [CALCITE-3224] New
RexNode-to-Expression CodeGen Implementation
URL: https://github.com/apache/calcite/pull/1347#discussion_r368909203
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##########
@@ -2544,35 +2556,117 @@ private void
checkNullableTimestamp(CalciteAssert.Config config) {
+ "from\n"
+ "\"hr\".\"emps\"")
.planContains(
- "final String inp2_ = current.name;")
- .planContains(
- "final int inp1_ = current.deptno;")
- .planContains(
- "static final int $L4J$C$5_2 = 5 - 2;")
- .planContains(
- "static final Integer $L4J$C$Integer_valueOf_5_2_ =
Integer.valueOf($L4J$C$5_2);")
- .planContains(
- "static final int $L4J$C$Integer_valueOf_5_2_intValue_ =
$L4J$C$Integer_valueOf_5_2_.intValue();")
- .planContains("static final boolean "
- + "$L4J$C$org_apache_calcite_runtime_SqlFunctions_eq_ = "
- + "org.apache.calcite.runtime.SqlFunctions.eq(\"\", \"\");")
- .planContains("static final boolean "
- + "$L4J$C$_org_apache_calcite_runtime_SqlFunctions_eq_ = "
- + "!$L4J$C$org_apache_calcite_runtime_SqlFunctions_eq_;")
- .planContains("return inp2_ == null "
- + "|| $L4J$C$_org_apache_calcite_runtime_SqlFunctions_eq_ "
- + "|| current.empid <= inp1_ && inp1_ * 8 <= 8 "
- + "? (String) null "
- + ": org.apache.calcite.runtime.SqlFunctions.substring("
- + "org.apache.calcite.runtime.SqlFunctions.trim(true, true, \" \",
"
- + "org.apache.calcite.runtime.SqlFunctions.substring(inp2_, "
- + "Integer.valueOf(inp1_ * 0 + 1).intValue()), true),
$L4J$C$Integer_valueOf_5_2_intValue_);")
+ " final org.apache.calcite.test.JdbcTest.Employee
current = (org.apache.calcite.test.JdbcTest.Employee)
inputEnumerator.current();\n"
+ + " final String input_value = current.name;\n"
+ + " final int input_value0 = current.deptno;\n"
+ + " Integer case_when_value;\n"
+ + " if
($L4J$C$org_apache_calcite_runtime_SqlFunctions_eq_) {\n"
+ + " case_when_value = $L4J$C$Integer_valueOf_1_;\n"
+ + " } else {\n"
+ + " case_when_value = (Integer) null;\n"
+ + " }\n"
+ + " final Integer binary_call_value1 =
case_when_value == null ? (Integer) null :
Integer.valueOf(Integer.valueOf(input_value0 * 0) + case_when_value);\n"
+ + " final String method_call_value = input_value ==
null || binary_call_value1 == null ? (String) null :
org.apache.calcite.runtime.SqlFunctions.substring(input_value,
binary_call_value1.intValue());\n"
+ + " final String trim_value = method_call_value ==
null ? (String) null : org.apache.calcite.runtime.SqlFunctions.trim(true, true,
\" \", method_call_value, true);\n"
+ + " Integer case_when_value0;\n"
+ + " if (current.empid > input_value0) {\n"
+ + " case_when_value0 = $L4J$C$Integer_valueOf_5_;\n"
+ + " } else {\n"
+ + " Integer case_when_value1;\n"
+ + " if (current.deptno * 8 > 8) {\n"
+ + " case_when_value1 =
$L4J$C$Integer_valueOf_5_;\n"
+ + " } else {\n"
+ + " case_when_value1 = (Integer) null;\n"
+ + " }\n"
+ + " case_when_value0 = case_when_value1;\n"
+ + " }\n"
+ + " final Integer binary_call_value3 =
case_when_value0 == null ? (Integer) null : Integer.valueOf(case_when_value0 -
$L4J$C$Integer_valueOf_2_);\n"
+ + " return trim_value == null || binary_call_value3
== null ? (String) null :
org.apache.calcite.runtime.SqlFunctions.substring(trim_value,
binary_call_value3.intValue());"
+ )
.returns("T=ll\n"
+ "T=ic\n"
+ "T=bastian\n"
+ "T=eodore\n");
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-3142";>[CALCITE-3142]
+ * An NPE when rounding a nullable numeric</a>. */
+ @Test public void testRoundingNPE() {
+ CalciteAssert.that()
+ .query("SELECT ROUND(CAST((X/Y) AS NUMERIC), 2) "
+ + "FROM (VALUES (1, 2), (NULLIF(5, 5), NULLIF(5, 5))) A(X, Y)")
+ .planContains(" final Object[] current = (Object[])
inputEnumerator.current();\n"
Review comment:
Essential assertions in `RexCall`'s implementors are preserved. We just
remove some `Assert`/`AssertionError`/`AlwaysNull` in the framework, because
they are introduced by the current algorithm which makes `RexNode`'s nullable
state immutable.
Technically speaking, the nullable state of `RexNode` can be immutable
during codegen. In the new one, each `RexNode` is visited only once. From its
type, we can know whether it is nullable directly, without maintaining any
states. Therefore, many checks (e.g., `isNullable(...)`) are not necessary
again.
To conclude, the following things are extra and make the progress
complicated. We removed them in the new one.
1. `private final Map<? extends RexNode, Boolean> exprNullableMap` in
`RexToLixTranslator`. Each `RexNode` is visited only once, we do not need it
again.
2. `private final RexToLixTranslator parent` in `RexToLixTranslator`. We do
not need to recursively get a `RexNode`'s nullable state again.
3. `AlwaysNull` exception. `AlwaysNull` in current codegen is the root cause
of some underyling problems. It buries some real problems and brings
'magic/undeterministic' result. From my personal point of view, I'm not in
favour of `AlwaysNull`. According to document, it is proposed for 'unusual'
situation. Since a `RexNode` is legal, the code generation progress should not
step into such situation. What's worse is, the exception is catched and returns
`null` aggressively in some places.
> Thrown in the unusual (but not erroneous) situation where the expression
> we are translating is the null literal but we have already checked that
> it is not null.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
