You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@calcite.apache.org by "Danny Chen (Jira)" <ji...@apache.org> on 2019/09/29 06:15:00 UTC
[jira] [Comment Edited] (CALCITE-3368) Some problems about
expression is null simplification
[ https://issues.apache.org/jira/browse/CALCITE-3368?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16940255#comment-16940255 ]
Danny Chen edited comment on CALCITE-3368 at 9/29/19 6:14 AM:
--------------------------------------------------------------
I tried this test case in SqlToRelConverterTest:
{code:java}
@Test public void testExpressionIsNullSimplify() {
final String sql = "SELECT (empno + sal) is null\n"
+ "FROM emp";
sql(sql).ok();
}
{code}
Now Calcite returns a the plan after sql-to-rel conversion:
{code:sql}
LogicalProject(EXPR$0=[false])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
{code}
I dug a little and found that the SafeRexVisitor thinks that PLUS is a safe expression, so "(a + b) is null" can be simplified to "false" if its return type is not nullable[1].
[~kgyrtkirk], why do you think that "a + b" is a safe expression ? If "a + b" overflows, then most of the engines would throws exception or just returns null. But if we simplify "(a + b) is null" to false, we would never have such behaviors again(Because this simplify happens in sql-to-rel conversion, we even have no way to configure it).
[1] [https://github.com/apache/calcite/blob/2dc97e6723e1b5bf762540f87ffffb5cd1a848a1/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L792]
was (Author: danny0405):
I tried this test case in SqlToRelConverterTest:
{code:java}
@Test public void testExpressionIsNullSimplify() {
final String sql = "SELECT (empno + sal) is null\n"
+ "FROM emp";
sql(sql).ok();
}
{code}
Now Calcite returns a the plan after sql-to-rel conversion:
{code:sql}
LogicalProject(EXPR$0=[false])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
{code}
I digged a little and found that the SafeRexVisitor thinks that PLUS is a safe expression, so "(a + b) is null" can be simplified to "false" if its return type is not nullable[1].
[~kgyrtkirk], why do you think that "a + b" is a safe expression ? If "a + b" overflows, then most of the engines would throws exception or just returns null. But if we simplify "(a + b) is null" to false, we would never have such behaviors again(Because this simplify happens in sql-to-rel conversion, we even have no way to configure it).
[1] [https://github.com/apache/calcite/blob/2dc97e6723e1b5bf762540f87ffffb5cd1a848a1/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L792]
> Some problems about expression is null simplification
> -----------------------------------------------------
>
> Key: CALCITE-3368
> URL: https://issues.apache.org/jira/browse/CALCITE-3368
> Project: Calcite
> Issue Type: Bug
> Components: core
> Affects Versions: 1.21.0
> Reporter: Leonard Xu
> Assignee: Danny Chen
> Priority: Major
>
> 'is null' expression in SQL may be optimized incorrectly in the underlying implementation.
>
> When I write a Fink SQL to test overflow just like
> {code:java}
> select
> case when (f0 + f1) is null then 'null' else 'not null' end
> from testTable
> {code}
> , I found expression '(f0 + f1) is null ' has been optimized by Calcite, and the optimization may be incorrect.
>
> The underlying implementation is that Calcite's simplification logic of isNull expression in SQL will convert from
> *"f(operand0, operand1) IS NULL"* to
> *"operand0 IS NULL OR operand1 IS NULL"* if the Policy of RexNode‘s SqlKind is ANY。
> This simplification leads to the expression will not calculate the real value of *f(operand0, operand1)* (eg.. '(f0 + f1)' in my case ),but '(f0 + f1)' maybe overflows after operation.
> {code:java}
> //org.apache.calcite.rex.RexSimplify.java
> private RexNode simplifyIsNull(RexNode a) {
> // Simplify the argument first,
> // call ourselves recursively to see whether we can make more progress.
> // For example, given
> // "(CASE WHEN FALSE THEN 1 ELSE 2) IS NULL" we first simplify the
> // argument to "2", and only then we can simplify "2 IS NULL" to "FALSE".
> a = simplify(a, UNKNOWN);
> if (!a.getType().isNullable() && isSafeExpression(a)) {
> return rexBuilder.makeLiteral(false);
> }
> if (RexUtil.isNull(a)) {
> return rexBuilder.makeLiteral(true);
> }
> if (a.getKind() == SqlKind.CAST) {
> return null;
> }
> switch (Strong.policy(a.getKind())) {
> case NOT_NULL:
> return rexBuilder.makeLiteral(false);
> case ANY:
> // "f" is a strong operator, so "f(operand0, operand1) IS NULL" simplifies
> // to "operand0 IS NULL OR operand1 IS NULL"
> final List<RexNode> operands = new ArrayList<>();
> for (RexNode operand : ((RexCall) a).getOperands()) {
> final RexNode simplified = simplifyIsNull(operand);
> if (simplified == null) {
> operands.add(
> rexBuilder.makeCall(SqlStdOperatorTable.IS_NULL, operand));
> } else {
> operands.add(simplified);
> }
> }
> return RexUtil.composeDisjunction(rexBuilder, operands, false);
> case AS_IS:
> default:
> return null;
> }
> }{code}
> And most of calculating SqlKinds are assigned *Policy.ANY* at present.
> {code:java}
> //org.apache.calcite.plan.Strong.java
> public static Policy policy(SqlKind kind) {
> return MAP.getOrDefault(kind, Policy.AS_IS);
> }
> ....
> map.put(SqlKind.PLUS, Policy.ANY);
> map.put(SqlKind.PLUS_PREFIX, Policy.ANY);
> map.put(SqlKind.MINUS, Policy.ANY);
> map.put(SqlKind.MINUS_PREFIX, Policy.ANY);
> map.put(SqlKind.TIMES, Policy.ANY);
> map.put(SqlKind.DIVIDE, Policy.ANY);
> * that operator evaluates to null. */
> public enum Policy {
> /** This kind of expression is never null. No need to look at its arguments,
> * if it has any. */
> NOT_NULL,
> /** This kind of expression has its own particular rules about whether it
> * is null. */
> CUSTOM,
> /** This kind of expression is null if and only if at least one of its
> * arguments is null. */
> ANY,
> /** This kind of expression may be null. There is no way to rewrite. */
> AS_IS,
> }{code}
>
> It may be an obvious nonequivalent simplification in SQL. And this issue come from Flink (FLINK-14030).
> [~danny0405], Could you have a look at this?
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)