IMPALA-10173: (Addendum) Fix substitution for unsafe expressions, column-level compatibility check

Expression substitution recreates cast expressions without considering
the compatibility level introduced by IMPALA-10173. In unsafe mode, the
recreation causes IllegalStateException. This change fixes this
behavior by storing the compatibility level in each CastExpr, and
reusing it when the expression substitution recreates the cast
expression.

For example: 'select "1", "1" union select 1, "1"'

Also, Set operation's common type calculations did not distinguish
compatibility levels for each column slot, if one column slot's common
type was considered unsafe, every other slot was treated as unsafe.
This change fixes this behavior by reinitializing the compatibility
level for every column slot, enabling cases where one column slot
contains unsafely casted constant values and another contains
non-constant expressions with regular casts.
These queries failed before this change with 'Unsafe implicit cast is
prohibited for non-const expression' error.

For example: 'select "1", 1 union select 1, int_col from unsafe_insert'

Tests:
 - test cases added to insert-unsafe.test

Change-Id: I39d13f177482f74ec39570118adab609444c6929
Reviewed-on: http://gerrit.cloudera.org:8080/20184
Reviewed-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Tested-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>

fe/src/main/java/org/apache/impala/analysis/Analyzer.java

@@ -3387,12 +3387,12 @@ public class Analyzer {
 
     TypeCompatibility regularCompatibility = this.getRegularCompatibilityLevel();
     TypeCompatibility permissiveCompatibility = this.getPermissiveCompatibilityLevel();
-    TypeCompatibility compatibilityLevel = regularCompatibility;
 
     // Determine compatible types for exprs, position by position.
     List<Expr> firstList = exprLists.get(0);
     List<Expr> widestExprs = new ArrayList<>(firstList.size());
     for (int i = 0; i < firstList.size(); ++i) {
+      TypeCompatibility compatibilityLevel = regularCompatibility;
       // Type compatible with the i-th exprs of all expr lists.
       // Initialize with type of i-th expr in first list.
       Type compatibleType = firstList.get(i).getType();

fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java

@@ -18,6 +18,7 @@
 package org.apache.impala.analysis;
 
 import org.apache.impala.catalog.Type;
+import org.apache.impala.catalog.TypeCompatibility;
 import org.apache.impala.common.AnalysisException;
 import org.apache.impala.thrift.TBoolLiteral;
 import org.apache.impala.thrift.TExprNode;
@@ -89,11 +90,12 @@ public class BoolLiteral extends LiteralExpr {
   }
 
   @Override
-  protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
+  protected Expr uncheckedCastTo(Type targetType, TypeCompatibility compatibility)
+      throws AnalysisException {
     if (targetType.equals(this.type_)) {
       return this;
     } else {
-      return new CastExpr(targetType, this);
+      return new CastExpr(targetType, this, compatibility);
     }
   }
 

fe/src/main/java/org/apache/impala/analysis/CastExpr.java

@@ -25,6 +25,7 @@ import org.apache.impala.catalog.PrimitiveType;
 import org.apache.impala.catalog.ScalarFunction;
 import org.apache.impala.catalog.ScalarType;
 import org.apache.impala.catalog.Type;
+import org.apache.impala.catalog.TypeCompatibility;
 import org.apache.impala.common.AnalysisException;
 import org.apache.impala.thrift.TCastExpr;
 import org.apache.impala.thrift.TExpr;
@@ -51,10 +52,14 @@ public class CastExpr extends Expr {
   // Stores the value of the FORMAT clause.
   private final String castFormat_;
 
+  // Stores the compatibility level with which the cast was defined.
+  private final TypeCompatibility compatibility_;
+
   /**
    * C'tor for "pre-analyzed" implicit casts.
    */
-  public CastExpr(Type targetType, Expr e, String format) {
+  public CastExpr(
+      Type targetType, Expr e, String format, TypeCompatibility compatibility) {
     super();
     Preconditions.checkState(targetType.isValid());
     Preconditions.checkNotNull(e);
@@ -62,6 +67,7 @@ public class CastExpr extends Expr {
     targetTypeDef_ = null;
     isImplicit_ = true;
     castFormat_ = format;
+    compatibility_ = compatibility;
     // replace existing implicit casts
     if (e instanceof CastExpr) {
       CastExpr castExpr = (CastExpr) e;
@@ -84,7 +90,11 @@ public class CastExpr extends Expr {
   }
 
   public CastExpr(Type targetType, Expr e) {
-    this(targetType, e, null);
+    this(targetType, e, null, TypeCompatibility.DEFAULT);
+  }
+
+  public CastExpr(Type targetType, Expr e, TypeCompatibility compatibility) {
+    this(targetType, e, null, compatibility);
   }
 
   /**
@@ -101,6 +111,7 @@ public class CastExpr extends Expr {
     targetTypeDef_ = targetTypeDef;
     children_.add(e);
     castFormat_ = format;
+    compatibility_ = TypeCompatibility.DEFAULT;
   }
 
   /**
@@ -112,6 +123,7 @@ public class CastExpr extends Expr {
     isImplicit_ = other.isImplicit_;
     noOp_ = other.noOp_;
     castFormat_ = other.castFormat_;
+    compatibility_ = TypeCompatibility.DEFAULT;
   }
 
   private static String getFnName(Type targetType) {
@@ -272,6 +284,8 @@ public class CastExpr extends Expr {
 
   public boolean isImplicit() { return isImplicit_; }
 
+  public TypeCompatibility getCompatibility() { return compatibility_; }
+
   @Override
   protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
     Preconditions.checkState(!isImplicit_);
@@ -317,7 +331,8 @@ public class CastExpr extends Expr {
       // for every type since it is redundant with STRING. Casts to go through 2 casts:
       // (1) cast to string, to stringify the value
       // (2) cast to CHAR, to truncate or pad with spaces
-      CastExpr tostring = new CastExpr(ScalarType.STRING, children_.get(0), castFormat_);
+      CastExpr tostring =
+          new CastExpr(ScalarType.STRING, children_.get(0), castFormat_, compatibility_);
       tostring.analyze();
       children_.set(0, tostring);
     }

fe/src/main/java/org/apache/impala/analysis/DateLiteral.java

@@ -17,13 +17,8 @@
 
 package org.apache.impala.analysis;
 
-import java.util.Arrays;
-import java.time.format.DateTimeFormatter;
-import java.time.format.DateTimeParseException;
-import java.time.LocalDate;
-
-import com.google.common.base.Preconditions;
 import org.apache.impala.catalog.Type;
+import org.apache.impala.catalog.TypeCompatibility;
 import org.apache.impala.common.AnalysisException;
 import org.apache.impala.common.InternalException;
 import org.apache.impala.service.FeSupport;
@@ -111,11 +106,12 @@ public class DateLiteral extends LiteralExpr {
   }
 
   @Override
-  protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
+  protected Expr uncheckedCastTo(Type targetType, TypeCompatibility compatibility)
+      throws AnalysisException {
     if (targetType.equals(type_)) {
       return this;
     } else {
-      return new CastExpr(targetType, this);
+      return new CastExpr(targetType, this, compatibility);
     }
   }
 

fe/src/main/java/org/apache/impala/analysis/Expr.java

@@ -1110,7 +1110,14 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
     if (smap == null) return result;
     result = result.substituteImpl(smap, analyzer);
     result.analyze(analyzer);
-    if (preserveRootType && !type_.equals(result.getType())) result = result.castTo(type_);
+    if (preserveRootType && !type_.equals(result.getType())) {
+      if (this instanceof CastExpr) {
+        CastExpr thisCastExpr = (CastExpr) this;
+        TypeCompatibility compatibility = thisCastExpr.getCompatibility();
+        return result.castTo(type_, compatibility);
+      }
+      return result.castTo(type_);
+    }
     return result;
   }
 
@@ -1570,7 +1577,7 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
           "targetType=" + targetType + " type=" + type);
       }
     }
-    return uncheckedCastTo(targetType);
+    return uncheckedCastTo(targetType, compatibility);
   }
 
   public final Expr castTo(Type targetType) throws AnalysisException {
@@ -1578,20 +1585,23 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl
   }
 
   /**
-   * Create an expression equivalent to 'this' but returning targetType;
-   * possibly by inserting an implicit cast,
-   * or by returning an altogether new expression
-   * or by returning 'this' with a modified return type'.
-   * @param targetType
-   *          type to be cast to
-   * @return cast expression, or converted literal,
-   *         should never return null
-   * @throws AnalysisException
-   *           when an invalid cast is asked for, for example,
-   *           failure to convert a string literal to a date literal
+   * Create an expression equivalent to 'this' but returning targetType; possibly by
+   * inserting an implicit cast, or by returning an altogether new expression or by
+   * returning 'this' with a modified return type'.
+   *
+   * @param targetType    type to be cast to
+   * @param compatibility compatibility level used to calculate the cast
+   * @return cast expression, or converted literal, should never return null
+   * @throws AnalysisException when an invalid cast is asked for, for example, failure to
+   *                           convert a string literal to a date literal
    */
+  protected Expr uncheckedCastTo(Type targetType, TypeCompatibility compatibility)
+      throws AnalysisException {
+    return new CastExpr(targetType, this, compatibility);
+  }
+
   protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
-    return new CastExpr(targetType, this);
+    return uncheckedCastTo(targetType, TypeCompatibility.DEFAULT);
   }
 
   /**

fe/src/main/java/org/apache/impala/analysis/NullLiteral.java

@@ -18,6 +18,7 @@
 package org.apache.impala.analysis;
 
 import org.apache.impala.catalog.Type;
+import org.apache.impala.catalog.TypeCompatibility;
 import org.apache.impala.thrift.TExprNode;
 import org.apache.impala.thrift.TExprNodeType;
 
@@ -44,7 +45,7 @@ public class NullLiteral extends LiteralExpr {
   public static NullLiteral create(Type type) {
     NullLiteral l = new NullLiteral();
     l.analyzeNoThrow(null);
-    l.uncheckedCastTo(type);
+    l.uncheckedCastTo(type, TypeCompatibility.DEFAULT);
     return l;
   }
 
@@ -65,7 +66,7 @@ public class NullLiteral extends LiteralExpr {
   public String getStringValue() { return "NULL"; }
 
   @Override
-  protected Expr uncheckedCastTo(Type targetType) {
+  protected Expr uncheckedCastTo(Type targetType, TypeCompatibility compatibility) {
     Preconditions.checkState(targetType.isValid());
     type_ = targetType;
     return this;

fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java

@@ -23,6 +23,7 @@ import java.math.RoundingMode;
 
 import org.apache.impala.catalog.ScalarType;
 import org.apache.impala.catalog.Type;
+import org.apache.impala.catalog.TypeCompatibility;
 import org.apache.impala.common.AnalysisException;
 import org.apache.impala.common.SqlCastException;
 import org.apache.impala.thrift.TDecimalLiteral;
@@ -462,10 +463,11 @@ public class NumericLiteral extends LiteralExpr {
    * @throws SqlCastException
    */
   @Override
-  protected Expr uncheckedCastTo(Type targetType) throws SqlCastException {
+  protected Expr uncheckedCastTo(Type targetType, TypeCompatibility compatibility)
+      throws SqlCastException {
     Preconditions.checkState(targetType.isNumericType() || targetType.isStringType());
     if (targetType.isStringType()) {
-      return new CastExpr(targetType, this);
+      return new CastExpr(targetType, this, compatibility);
     }
     if (type_ == targetType) return this;
     try {
@@ -479,7 +481,7 @@ public class NumericLiteral extends LiteralExpr {
         return new NumericLiteral(converted, targetType);
       }
     } catch (SqlCastException e) {
-      return new CastExpr(targetType, this);
+      return new CastExpr(targetType, this, compatibility);
     }
   }
 

fe/src/main/java/org/apache/impala/analysis/SlotRef.java

@@ -28,6 +28,7 @@ import org.apache.impala.catalog.HdfsFileFormat;
 import org.apache.impala.catalog.StructType;
 import org.apache.impala.catalog.TableLoadingException;
 import org.apache.impala.catalog.Type;
+import org.apache.impala.catalog.TypeCompatibility;
 import org.apache.impala.common.AnalysisException;
 import org.apache.impala.common.UnsupportedFeatureException;
 import org.apache.impala.thrift.TExprNode;
@@ -460,12 +461,13 @@ public class SlotRef extends Expr {
   }
 
   @Override
-  protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
+  protected Expr uncheckedCastTo(Type targetType, TypeCompatibility compatibility)
+      throws AnalysisException {
     if (type_.isNull()) {
       // Hack to prevent null SlotRefs in the BE
       return NullLiteral.create(targetType);
     } else {
-      return super.uncheckedCastTo(targetType);
+      return super.uncheckedCastTo(targetType, compatibility);
     }
   }
 

fe/src/main/java/org/apache/impala/analysis/StringLiteral.java

@@ -23,6 +23,7 @@ import java.math.BigDecimal;
 
 import org.apache.impala.catalog.ScalarType;
 import org.apache.impala.catalog.Type;
+import org.apache.impala.catalog.TypeCompatibility;
 import org.apache.impala.common.AnalysisException;
 import org.apache.impala.compat.MetastoreShim;
 import org.apache.impala.thrift.TExprNode;
@@ -148,7 +149,8 @@ public class StringLiteral extends LiteralExpr {
   }
 
   @Override
-  protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
+  protected Expr uncheckedCastTo(Type targetType, TypeCompatibility compatibility)
+      throws AnalysisException {
     Preconditions.checkState(targetType.isNumericType() || targetType.isDateOrTimeType()
         || targetType.equals(this.type_) || targetType.isStringType());
     if (targetType.equals(this.type_)) {
@@ -161,7 +163,7 @@ public class StringLiteral extends LiteralExpr {
       // Let the BE do the cast
       // - it is in Boost format in case target type is TIMESTAMP
       // - CCTZ is used for conversion in case target type is DATE.
-      return new CastExpr(targetType, this);
+      return new CastExpr(targetType, this, compatibility);
     }
     return this;
   }

fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java

@@ -21,6 +21,7 @@ import java.util.Arrays;
 
 import com.google.common.base.Preconditions;
 import org.apache.impala.catalog.Type;
+import org.apache.impala.catalog.TypeCompatibility;
 import org.apache.impala.common.AnalysisException;
 import org.apache.impala.thrift.TExprNode;
 import org.apache.impala.thrift.TExprNodeType;
@@ -84,11 +85,12 @@ public class TimestampLiteral extends LiteralExpr {
   }
 
   @Override
-  protected Expr uncheckedCastTo(Type targetType) throws AnalysisException {
+  protected Expr uncheckedCastTo(Type targetType, TypeCompatibility compatibility)
+      throws AnalysisException {
     if (targetType.equals(type_)) {
       return this;
     } else {
-      return new CastExpr(targetType, this);
+      return new CastExpr(targetType, this, compatibility);
     }
   }
 

fe/src/test/java/org/apache/impala/analysis/NumericLiteralTest.java

@@ -23,6 +23,7 @@ import java.math.BigDecimal;
 
 import org.apache.impala.catalog.ScalarType;
 import org.apache.impala.catalog.Type;
+import org.apache.impala.catalog.TypeCompatibility;
 import org.apache.impala.common.AnalysisException;
 import org.apache.impala.common.InvalidValueException;
 import org.apache.impala.common.SqlCastException;
@@ -435,23 +436,24 @@ public class NumericLiteralTest {
     {
       // Integral types
       NumericLiteral n = new NumericLiteral(BigDecimal.ZERO);
-      Expr result = n.uncheckedCastTo(Type.BIGINT);
+      Expr result = n.uncheckedCastTo(Type.BIGINT, TypeCompatibility.DEFAULT);
       assertSame(n, result);
       assertEquals(Type.BIGINT, n.getType());
-      result = n.uncheckedCastTo(Type.TINYINT);
+      result = n.uncheckedCastTo(Type.TINYINT, TypeCompatibility.DEFAULT);
       assertSame(n, result);
       assertEquals(Type.TINYINT, n.getType());
-      result = n.uncheckedCastTo(ScalarType.createDecimalType(5, 0));
+      result = n.uncheckedCastTo(
+          ScalarType.createDecimalType(5, 0), TypeCompatibility.DEFAULT);
       assertSame(n, result);
       assertEquals(ScalarType.createDecimalType(5, 0), n.getType());
     }
     {
       // Integral types, with overflow
       NumericLiteral n = new NumericLiteral(ABOVE_SMALLINT);
-      Expr result = n.uncheckedCastTo(Type.BIGINT);
+      Expr result = n.uncheckedCastTo(Type.BIGINT, TypeCompatibility.DEFAULT);
       assertSame(n, result);
       assertEquals(Type.BIGINT, n.getType());
-      Expr result2 = n.uncheckedCastTo(Type.SMALLINT);
+      Expr result2 = n.uncheckedCastTo(Type.SMALLINT, TypeCompatibility.DEFAULT);
       assertTrue(result2 instanceof CastExpr);
       assertEquals(Type.SMALLINT, result2.getType());
     }
@@ -459,10 +461,11 @@ public class NumericLiteralTest {
       // Decimal types, with overflow
       // Note: not safe to reuse above value after exception
       NumericLiteral n = new NumericLiteral(ABOVE_SMALLINT);
-      Expr result = n.uncheckedCastTo(Type.BIGINT);
+      Expr result = n.uncheckedCastTo(Type.BIGINT, TypeCompatibility.DEFAULT);
       assertSame(n, result);
       assertEquals(Type.BIGINT, n.getType());
-      Expr result2 = n.uncheckedCastTo(ScalarType.createDecimalType(2, 0));
+      Expr result2 = n.uncheckedCastTo(
+          ScalarType.createDecimalType(2, 0), TypeCompatibility.DEFAULT);
       assertTrue(result2 instanceof CastExpr);
       assertEquals(ScalarType.createDecimalType(2, 0), result2.getType());
     }
@@ -470,13 +473,16 @@ public class NumericLiteralTest {
       // Decimal types
       NumericLiteral n = new NumericLiteral(new BigDecimal("123.45"));
       assertEquals(ScalarType.createDecimalType(5, 2), n.getType());
-      Expr result = n.uncheckedCastTo(ScalarType.createDecimalType(6, 3));
+      Expr result = n.uncheckedCastTo(
+          ScalarType.createDecimalType(6, 3), TypeCompatibility.DEFAULT);
       assertSame(n, result);
       assertEquals(ScalarType.createDecimalType(6, 3), n.getType());
-      result = n.uncheckedCastTo(ScalarType.createDecimalType(5, 2));
+      result = n.uncheckedCastTo(
+          ScalarType.createDecimalType(5, 2), TypeCompatibility.DEFAULT);
       assertSame(n, result);
       assertEquals(ScalarType.createDecimalType(5, 2), n.getType());
-      result = n.uncheckedCastTo(ScalarType.createDecimalType(4, 1));
+      result = n.uncheckedCastTo(
+          ScalarType.createDecimalType(4, 1), TypeCompatibility.DEFAULT);
       assertNotSame(n, result);
       assertEquals(ScalarType.createDecimalType(4, 1), result.getType());
       assertEquals("123.5", ((NumericLiteral) result).toSql());
@@ -602,29 +608,30 @@ public class NumericLiteralTest {
   public void testCast() throws SqlCastException {
     NumericLiteral n = NumericLiteral.create(1000);
     assertEquals(Type.SMALLINT, n.getType());
-    Expr result = n.uncheckedCastTo(Type.TINYINT);
+    Expr result = n.uncheckedCastTo(Type.TINYINT, TypeCompatibility.DEFAULT);
     assertTrue(result instanceof CastExpr);
     assertEquals(Type.TINYINT, result.getType());
 
-    result = n.uncheckedCastTo(Type.INT);
+    result = n.uncheckedCastTo(Type.INT, TypeCompatibility.DEFAULT);
     assertSame(n, result);
     assertEquals(Type.INT, n.getType());
 
     n = new NumericLiteral(new BigDecimal("123.45"));
     assertEquals(ScalarType.createDecimalType(5, 2), n.getType());
-    assertSame(n, n.uncheckedCastTo(ScalarType.createDecimalType(6, 3)));
+    assertSame(n,
+        n.uncheckedCastTo(ScalarType.createDecimalType(6, 3), TypeCompatibility.DEFAULT));
     assertEquals(ScalarType.createDecimalType(6, 3), n.getType());
 
     n = new NumericLiteral(new BigDecimal("123.45"));
     Type newType = ScalarType.createDecimalType(4, 1);
-    result = n.uncheckedCastTo(newType);
+    result = n.uncheckedCastTo(newType, TypeCompatibility.DEFAULT);
     assertNotSame(result, n);
     assertTrue(result instanceof NumericLiteral);
     assertEquals(newType, result.getType());
     NumericLiteral n2 = (NumericLiteral) result;
     assertEquals("123.5", n2.getValue().toString());
 
-    Expr result2 = n2.uncheckedCastTo(Type.SMALLINT);
+    Expr result2 = n2.uncheckedCastTo(Type.SMALLINT, TypeCompatibility.DEFAULT);
     assertNotSame(result2, result);
     assertEquals(Type.SMALLINT, result2.getType());
     assertEquals("124", ((NumericLiteral)result2).getValue().toString());

testdata/workloads/functional-query/queries/QueryTest/insert-unsafe.test

@@ -213,4 +213,13 @@ AnalysisException: Unsafe implicit cast is prohibited for non-const expression:
 INSERT INTO unsafe_insert(double_col) select string_col from unsafe_insert;
 ---- CATCH
 AnalysisException: Unsafe implicit cast is prohibited for non-const expression: string_col
-
+====
+---- QUERY
+# Mixing unsafe and regular compatibility on column level, unsafe union between 1 and "1"
+# regular union between column 'string_col' and "100".
+INSERT INTO unsafe_insert(int_col, string_col) select 1, string_col from unsafe_insert union select "1", "100";
+====
+---- QUERY
+# Regression test for expression substitution on unsafe casts.
+INSERT INTO unsafe_insert(int_col, string_col) select "1", "1" from unsafe_insert union select 1, "1";
+====