From 56a417687506284cf2221dad51dc6c6b60d3a695 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Fri, 20 Dec 2019 00:19:22 +0200 Subject: [PATCH] SQL: Fix issue with CAST and NULL checking. (#50371) Previously, during expression optimisation, CAST would be considered nullable if the casted expression resulted to a NULL literal, and would be always non-nullable otherwise. As a result if CASE was wrapped by a null check function like IS NULL or IS NOT NULL it was simplified to TRUE/FALSE, eliminating the actual casting operation. So in case of an expression with an erroneous casting like CAST('foo' AS DATETIME) IS NULL it would be simplified to FALSE instead of throwing an Exception signifying the attempt to cast 'foo' to a DATETIME type. CAST now always returns Nullability.UKNOWN except from the case that its result evaluated to a constant NULL, where it returns Nullability.TRUE. This way the IS NULL/IS NOT NULL don't get simplified to FALSE/TRUE and the CAST actually gets evaluated resulting to a thrown Exception. Fixes: #50191 (cherry picked from commit 671e07a931cd828661e226cba22a5d38804a17a5) --- .../sql/expression/function/scalar/Cast.java | 2 +- .../nulls/CheckNullProcessorTests.java | 6 --- .../xpack/sql/optimizer/OptimizerTests.java | 49 +++++++++++++++++++ 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java index c3c0da0570903..35bc87eabd7b2 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/Cast.java @@ -66,7 +66,7 @@ public Nullability nullable() { if (from().isNull()) { return Nullability.TRUE; } - return field().nullable(); + return Nullability.UNKNOWN; } @Override diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/CheckNullProcessorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/CheckNullProcessorTests.java index 01b8ba80de260..4ed35e2f9caa7 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/CheckNullProcessorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/CheckNullProcessorTests.java @@ -9,15 +9,9 @@ import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.test.AbstractWireSerializingTestCase; import org.elasticsearch.xpack.sql.expression.function.scalar.Processors; -import org.elasticsearch.xpack.sql.expression.gen.processor.ConstantProcessor; -import org.elasticsearch.xpack.sql.expression.gen.processor.Processor; public class CheckNullProcessorTests extends AbstractWireSerializingTestCase { - private static final Processor FALSE = new ConstantProcessor(false); - private static final Processor TRUE = new ConstantProcessor(true); - private static final Processor NULL = new ConstantProcessor((Object) null); - public static CheckNullProcessor randomProcessor() { return new CheckNullProcessor(randomFrom(CheckNullProcessor.CheckNullOperation.values())); } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java index 8efb687428945..1731f39b711fd 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.optimizer; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.sql.SqlIllegalArgumentException; import org.elasticsearch.xpack.sql.analysis.analyzer.Analyzer.PruneSubqueryAliases; import org.elasticsearch.xpack.sql.analysis.index.EsIndex; import org.elasticsearch.xpack.sql.expression.Alias; @@ -447,10 +448,46 @@ public void testNullFoldingIsNull() { assertEquals(false, foldNull.rule(new IsNull(EMPTY, TRUE)).fold()); } + public void testNullFoldingIsNullWithCast() { + FoldNull foldNull = new FoldNull(); + + Cast cast = new Cast(EMPTY, L("foo"), DataType.DATE); + IsNull isNull = new IsNull(EMPTY, cast); + final IsNull isNullOpt = (IsNull) foldNull.rule(isNull); + assertEquals(isNull, isNullOpt); + + SqlIllegalArgumentException sqlIAE = + expectThrows(SqlIllegalArgumentException.class, () -> isNullOpt.asPipe().asProcessor().process(null)); + assertEquals("cannot cast [foo] to [date]: Text 'foo' could not be parsed at index 0", sqlIAE.getMessage()); + + isNull = new IsNull(EMPTY, new Cast(EMPTY, NULL, randomFrom(DataType.values()))); + assertTrue((Boolean) ((IsNull) foldNull.rule(isNull)).asPipe().asProcessor().process(null)); + } + public void testNullFoldingIsNotNull() { FoldNull foldNull = new FoldNull(); assertEquals(true, foldNull.rule(new IsNotNull(EMPTY, TRUE)).fold()); assertEquals(false, foldNull.rule(new IsNotNull(EMPTY, NULL)).fold()); + + Cast cast = new Cast(EMPTY, L("foo"), DataType.DATE); + IsNotNull isNotNull = new IsNotNull(EMPTY, cast); + assertEquals(isNotNull, foldNull.rule(isNotNull)); + } + + public void testNullFoldingIsNotNullWithCast() { + FoldNull foldNull = new FoldNull(); + + Cast cast = new Cast(EMPTY, L("foo"), DataType.DATE); + IsNotNull isNotNull = new IsNotNull(EMPTY, cast); + final IsNotNull isNotNullOpt = (IsNotNull) foldNull.rule(isNotNull); + assertEquals(isNotNull, isNotNullOpt); + + SqlIllegalArgumentException sqlIAE = + expectThrows(SqlIllegalArgumentException.class, () -> isNotNullOpt.asPipe().asProcessor().process(null)); + assertEquals("cannot cast [foo] to [date]: Text 'foo' could not be parsed at index 0", sqlIAE.getMessage()); + + isNotNull = new IsNotNull(EMPTY, new Cast(EMPTY, NULL, randomFrom(DataType.values()))); + assertFalse((Boolean) ((IsNotNull) foldNull.rule(isNotNull)).asPipe().asProcessor().process(null)); } public void testGenericNullableExpression() { @@ -470,6 +507,18 @@ public void testGenericNullableExpression() { assertNullLiteral(rule.rule(new RLike(EMPTY, NULL, "123"))); } + public void testNullFoldingOnCast() { + FoldNull foldNull = new FoldNull(); + + Cast cast = new Cast(EMPTY, NULL, randomFrom(DataType.values())); + assertEquals(Nullability.TRUE, cast.nullable()); + assertNull(foldNull.rule(cast).fold()); + + cast = new Cast(EMPTY, L("foo"), DataType.DATE); + assertEquals(Nullability.UNKNOWN, cast.nullable()); + assertEquals(cast, foldNull.rule(cast)); + } + public void testNullFoldingDoesNotApplyOnLogicalExpressions() { FoldNull rule = new FoldNull();