Repository: prestodb/presto
Issue: Compilation for try_cast is broken

The exception handling is completely broken because exceptions clear the stack. It needs to be in a separate method. This query reproduces it:

```
select concat('foo', try_cast(x as varchar)) from (values (1)) a (x);
```

Exception:

```
java.lang.VerifyError: Instruction type does not match stack map
```


Changed files:
presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java
presto-main/src/main/java/com/facebook/presto/sql/gen/ByteCodeUtils.java
presto-main/src/main/java/com/facebook/presto/sql/gen/CompilerOperations.java
presto-main/src/test/java/com/facebook/presto/sql/gen/TestExpressionCompiler.java
presto-main/src/main/java/com/facebook/presto/sql/gen/TryCastCodeGenerator.java

diff --git a/presto-main/src/main/java/com/facebook/presto/sql/gen/ByteCodeUtils.java b/presto-main/src/main/java/com/facebook/presto/sql/gen/ByteCodeUtils.java
index 468704ffe1..2cb18b820d 100644
--- a/presto-main/src/main/java/com/facebook/presto/sql/gen/ByteCodeUtils.java
+++ b/presto-main/src/main/java/com/facebook/presto/sql/gen/ByteCodeUtils.java
@@ -152,7 +152,8 @@ public final class ByteCodeUtils
         MethodType methodType = binding.getType();
 
         Signature signature = function.getSignature();
-        Class<?> unboxedReturnType = Primitives.unwrap(methodType.returnType());
+        Class<?> returnType = methodType.returnType();
+        Class<?> unboxedReturnType = Primitives.unwrap(returnType);
 
         LabelNode end = new LabelNode("end");
         Block block = new Block(context)
@@ -181,23 +182,35 @@ public final class ByteCodeUtils
         block.append(invoke(context, binding, function.getSignature()));
 
         if (function.isNullable()) {
-            if (unboxedReturnType.isPrimitive() && unboxedReturnType != void.class) {
-                LabelNode notNull = new LabelNode("notNull");
-                block.dup(methodType.returnType())
-                        .ifNotNullGoto(notNull)
-                        .putVariable("wasNull", true)
-                        .comment("swap boxed null with unboxed default")
-                        .pop(methodType.returnType())
-                        .pushJavaDefault(unboxedReturnType)
-                        .gotoLabel(end)
-                        .visitLabel(notNull)
-                        .append(unboxPrimitive(context, unboxedReturnType));
-            }
-            else {
-                block.dup(methodType.returnType())
-                        .ifNotNullGoto(end)
-                        .putVariable("wasNull", true);
-            }
+            block.append(unboxPrimitiveIfNecessary(context, returnType));
+        }
+        block.visitLabel(end);
+
+        return block;
+    }
+
+    public static Block unboxPrimitiveIfNecessary(CompilerContext context, Class<?> boxedType)
+    {
+        Block block = new Block(context);
+        LabelNode end = new LabelNode("end");
+        Class<?> unboxedType = Primitives.unwrap(boxedType);
+
+        if (unboxedType.isPrimitive() && unboxedType != void.class) {
+            LabelNode notNull = new LabelNode("notNull");
+            block.dup(boxedType)
+                    .ifNotNullGoto(notNull)
+                    .putVariable("wasNull", true)
+                    .comment("swap boxed null with unboxed default")
+                    .pop(boxedType)
+                    .pushJavaDefault(unboxedType)
+                    .gotoLabel(end)
+                    .visitLabel(notNull)
+                    .append(unboxPrimitive(context, unboxedType));
+        }
+        else {
+            block.dup(boxedType)
+                    .ifNotNullGoto(end)
+                    .putVariable("wasNull", true);
         }
         block.visitLabel(end);
 
diff --git a/presto-main/src/main/java/com/facebook/presto/sql/gen/CompilerOperations.java b/presto-main/src/main/java/com/facebook/presto/sql/gen/CompilerOperations.java
index c0d534724b..0f91ebca5f 100644
--- a/presto-main/src/main/java/com/facebook/presto/sql/gen/CompilerOperations.java
+++ b/presto-main/src/main/java/com/facebook/presto/sql/gen/CompilerOperations.java
@@ -59,14 +59,6 @@ public final class CompilerOperations
         return left > right;
     }
 
-    public static void propagateInterruptedException(Throwable throwable)
-    {
-        if (throwable instanceof InterruptedException) {
-            Thread.currentThread().interrupt();
-            throw new RuntimeException(throwable);
-        }
-    }
-
     public static boolean in(Object value, Set<?> set)
     {
         return set.contains(value);
diff --git a/presto-main/src/main/java/com/facebook/presto/sql/gen/TryCastCodeGenerator.java b/presto-main/src/main/java/com/facebook/presto/sql/gen/TryCastCodeGenerator.java
index 8c74c604c5..b09c3a3f8d 100644
--- a/presto-main/src/main/java/com/facebook/presto/sql/gen/TryCastCodeGenerator.java
+++ b/presto-main/src/main/java/com/facebook/presto/sql/gen/TryCastCodeGenerator.java
@@ -16,14 +16,23 @@ package com.facebook.presto.sql.gen;
 import com.facebook.presto.byteCode.Block;
 import com.facebook.presto.byteCode.ByteCodeNode;
 import com.facebook.presto.byteCode.CompilerContext;
-import com.facebook.presto.byteCode.control.TryCatch;
+import com.facebook.presto.byteCode.instruction.LabelNode;
 import com.facebook.presto.metadata.Signature;
 import com.facebook.presto.spi.type.Type;
 import com.facebook.presto.sql.relational.RowExpression;
+import com.google.common.primitives.Primitives;
 
+import java.lang.invoke.MethodHandle;
 import java.util.List;
 
-import static com.facebook.presto.byteCode.ParameterizedType.type;
+import static com.facebook.presto.sql.gen.ByteCodeUtils.ifWasNullPopAndGoto;
+import static com.facebook.presto.sql.gen.ByteCodeUtils.invoke;
+import static com.facebook.presto.sql.gen.ByteCodeUtils.unboxPrimitiveIfNecessary;
+import static com.facebook.presto.type.UnknownType.UNKNOWN;
+import static java.lang.invoke.MethodHandles.catchException;
+import static java.lang.invoke.MethodHandles.constant;
+import static java.lang.invoke.MethodHandles.dropArguments;
+import static java.lang.invoke.MethodType.methodType;
 
 public class TryCastCodeGenerator
         implements ByteCodeGenerator
@@ -31,19 +40,43 @@ public class TryCastCodeGenerator
     @Override
     public ByteCodeNode generateExpression(Signature signature, ByteCodeGeneratorContext generatorContext, Type returnType, List<RowExpression> arguments)
     {
-        ByteCodeNode castByteCode = new CastCodeGenerator().generateExpression(signature, generatorContext, returnType, arguments);
-
         CompilerContext context = generatorContext.getContext();
-        Block catchBlock = new Block(context)
-                .comment("propagate InterruptedException")
-                .invokeStatic(CompilerOperations.class, "propagateInterruptedException", void.class, Throwable.class)
-                .comment("wasNull = true;")
-                .putVariable("wasNull", true)
-                .comment("restore stack after exception")
-                .getVariable("output") // TODO: this is quite a strong assumption to make. This code should not be sensitive to whether "output" was on the stack before the call
-                .comment("return dummy value for null")
-                .pushJavaDefault(returnType.getJavaType());
-
-        return new TryCatch(context, "TRY_CAST", castByteCode, catchBlock, type(Exception.class));
+        RowExpression argument = arguments.get(0);
+
+        Class<?> argumentType = argument.getType().getJavaType();
+        Class<?> unboxedReturnType = returnType.getJavaType();
+        Class<?> boxedReturnType = Primitives.wrap(unboxedReturnType);
+
+        if (argument.getType().equals(UNKNOWN)) {
+            return new Block(context)
+                    .putVariable("wasNull", true)
+                    .pushJavaDefault(unboxedReturnType);
+        }
+
+        MethodHandle function = generatorContext
+                .getRegistry()
+                .getCoercion(argument.getType(), returnType)
+                .getMethodHandle()
+                .asType(methodType(boxedReturnType, argumentType));
+
+        MethodHandle tryCast = exceptionToNull(function, boxedReturnType, RuntimeException.class);
+
+        Binding tryCastBinding = generatorContext.getCallSiteBinder().bind(tryCast);
+
+        LabelNode end = new LabelNode("end");
+
+        return new Block(context)
+                .comment("call tryCast method")
+                .append(generatorContext.generate(argument))
+                .append(ifWasNullPopAndGoto(context, end, unboxedReturnType, argumentType))
+                .append(invoke(generatorContext.getContext(), tryCastBinding, "tryCast"))
+                .append(unboxPrimitiveIfNecessary(context, boxedReturnType))
+                .visitLabel(end);
+    }
+
+    private static MethodHandle exceptionToNull(MethodHandle target, Class<?> type, Class<? extends Throwable> throwable)
+    {
+        MethodHandle toNull = dropArguments(constant(type, null), 0, throwable);
+        return catchException(target, throwable, toNull);
     }
 }
diff --git a/presto-main/src/test/java/com/facebook/presto/sql/gen/TestExpressionCompiler.java b/presto-main/src/test/java/com/facebook/presto/sql/gen/TestExpressionCompiler.java
index 094181a730..3aed863516 100644
--- a/presto-main/src/test/java/com/facebook/presto/sql/gen/TestExpressionCompiler.java
+++ b/presto-main/src/test/java/com/facebook/presto/sql/gen/TestExpressionCompiler.java
@@ -543,8 +543,13 @@ public class TestExpressionCompiler
         assertExecute("try_cast('foo' as varchar)", "foo");
         assertExecute("try_cast('foo' as bigint)", null);
         assertExecute("try_cast(bound_string as bigint)", null);
+        assertExecute("try_cast(cast(null as varchar) as bigint)", null);
+        assertExecute("try_cast(bound_long / 13  as bigint)", 94);
         assertExecute("coalesce(try_cast('123' as bigint), 456)", 123L);
         assertExecute("coalesce(try_cast('foo' as bigint), 456)", 456L);
+        assertExecute("concat('foo', cast('bar' as varchar))", "foobar");
+        assertExecute("try_cast(try_cast(123 as varchar) as bigint)", 123L);
+        assertExecute("try_cast('foo' as varchar) || try_cast('bar' as varchar)", "foobar");
 
         Futures.allAsList(futures).get();
     }
diff --git a/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java b/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java
index a65a5e4b5c..015fedab8b 100644
--- a/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java
+++ b/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java
@@ -2248,6 +2248,9 @@ public abstract class AbstractTestQueries
 
         assertQuery("SELECT try_cast('foo' AS BIGINT) FROM orders", "SELECT CAST(null AS BIGINT) FROM orders");
         assertQuery("SELECT try_cast(clerk AS BIGINT) FROM orders", "SELECT CAST(null AS BIGINT) FROM orders");
+        assertQuery("SELECT try_cast(orderkey * orderkey AS VARCHAR) FROM orders", "SELECT CAST(orderkey * orderkey AS VARCHAR) FROM orders");
+        assertQuery("SELECT try_cast(try_cast(orderkey AS VARCHAR) AS BIGINT) FROM orders", "SELECT orderkey FROM orders");
+        assertQuery("SELECT try_cast(clerk AS VARCHAR) || try_cast(clerk AS VARCHAR) FROM orders", "SELECT clerk || clerk FROM orders");
 
         assertQuery("SELECT coalesce(try_cast('foo' AS BIGINT), 456) FROM orders", "SELECT 456 FROM orders");
         assertQuery("SELECT coalesce(try_cast(clerk AS BIGINT), 456) FROM orders", "SELECT 456 FROM orders");