[SYSTEMML-2440] Fix robustness value type casts to double/long This patch fixes special cases of value type casting to double and long. Under special circumstances, scalars after list indexing can be of type string but still hold the correct values. We now use a more robust casting mechanism that works for all combinations of value types, including strings and safe casts for double to long (according to machine precision).
Furthermore, this also includes a fix of the parser to correctly propagate unknown value types after list indexing to prevent rewrite anomalies that incorrectly removed casts to double which again created incorrect results for string inputs. Project: http://git-wip-us.apache.org/repos/asf/systemml/repo Commit: http://git-wip-us.apache.org/repos/asf/systemml/commit/9593b7fb Tree: http://git-wip-us.apache.org/repos/asf/systemml/tree/9593b7fb Diff: http://git-wip-us.apache.org/repos/asf/systemml/diff/9593b7fb Branch: refs/heads/master Commit: 9593b7fbef093c5976b21dec5c2dd4c33acebe00 Parents: 78c5c22 Author: Matthias Boehm <[email protected]> Authored: Wed Jul 18 21:52:03 2018 -0700 Committer: Matthias Boehm <[email protected]> Committed: Wed Jul 18 22:07:15 2018 -0700 ---------------------------------------------------------------------- .../hops/recompile/LiteralReplacement.java | 24 ++++-------------- .../sysml/parser/BuiltinFunctionExpression.java | 4 +-- .../instructions/cp/ScalarObjectFactory.java | 26 ++++++++++++++++++-- .../instructions/cp/VariableCPInstruction.java | 10 ++++---- 4 files changed, 36 insertions(+), 28 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/systemml/blob/9593b7fb/src/main/java/org/apache/sysml/hops/recompile/LiteralReplacement.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/hops/recompile/LiteralReplacement.java b/src/main/java/org/apache/sysml/hops/recompile/LiteralReplacement.java index d6bac40..b2344c5 100644 --- a/src/main/java/org/apache/sysml/hops/recompile/LiteralReplacement.java +++ b/src/main/java/org/apache/sysml/hops/recompile/LiteralReplacement.java @@ -131,7 +131,7 @@ public class LiteralReplacement return ret; } - private static LiteralOp replaceLiteralValueTypeCastScalarRead( Hop c, LocalVariableMap vars ) + private static LiteralOp replaceLiteralValueTypeCastScalarRead(Hop c, LocalVariableMap vars) { LiteralOp ret = null; @@ -141,30 +141,16 @@ public class LiteralReplacement && c.getInput().get(0) instanceof DataOp && c.getDataType()==DataType.SCALAR ) { Data dat = vars.get(c.getInput().get(0).getName()); - if( dat != null ) //required for selective constant propagation - { + if( dat != null ) { //required for selective constant propagation ScalarObject sdat = (ScalarObject)dat; - UnaryOp cast = (UnaryOp) c; - switch( cast.getOp() ) { - case CAST_AS_INT: - ret = new LiteralOp(sdat.getLongValue()); - break; - case CAST_AS_DOUBLE: - ret = new LiteralOp(sdat.getDoubleValue()); - break; - case CAST_AS_BOOLEAN: - ret = new LiteralOp(sdat.getBooleanValue()); - break; - default: - //otherwise: do nothing - } - } + ret = ScalarObjectFactory.createLiteralOp(sdat, (UnaryOp) c); + } } return ret; } - private static LiteralOp replaceLiteralValueTypeCastLiteral( Hop c, LocalVariableMap vars ) + private static LiteralOp replaceLiteralValueTypeCastLiteral(Hop c, LocalVariableMap vars) { LiteralOp ret = null; http://git-wip-us.apache.org/repos/asf/systemml/blob/9593b7fb/src/main/java/org/apache/sysml/parser/BuiltinFunctionExpression.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/parser/BuiltinFunctionExpression.java b/src/main/java/org/apache/sysml/parser/BuiltinFunctionExpression.java index 519bd7d..c2c48cb 100644 --- a/src/main/java/org/apache/sysml/parser/BuiltinFunctionExpression.java +++ b/src/main/java/org/apache/sysml/parser/BuiltinFunctionExpression.java @@ -651,8 +651,8 @@ public class BuiltinFunctionExpression extends DataIdentifier output.setDataType(DataType.SCALAR); output.setDimensions(0, 0); output.setBlockDimensions (0, 0); - output.setValueType((id.getValueType()!=ValueType.UNKNOWN) ? - id.getValueType() : ValueType.DOUBLE); + output.setValueType((id.getValueType()!=ValueType.UNKNOWN + || id.getDataType()==DataType.LIST) ? id.getValueType() : ValueType.DOUBLE); break; case CAST_AS_MATRIX: checkNumParameters(1); http://git-wip-us.apache.org/repos/asf/systemml/blob/9593b7fb/src/main/java/org/apache/sysml/runtime/instructions/cp/ScalarObjectFactory.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/runtime/instructions/cp/ScalarObjectFactory.java b/src/main/java/org/apache/sysml/runtime/instructions/cp/ScalarObjectFactory.java index 7f5342a..b3d5d4a 100644 --- a/src/main/java/org/apache/sysml/runtime/instructions/cp/ScalarObjectFactory.java +++ b/src/main/java/org/apache/sysml/runtime/instructions/cp/ScalarObjectFactory.java @@ -21,6 +21,7 @@ package org.apache.sysml.runtime.instructions.cp; import org.apache.sysml.hops.HopsException; import org.apache.sysml.hops.LiteralOp; +import org.apache.sysml.hops.UnaryOp; import org.apache.sysml.parser.Expression.ValueType; import org.apache.sysml.runtime.util.UtilFunctions; @@ -58,8 +59,8 @@ public abstract class ScalarObjectFactory public static ScalarObject createScalarObject(ValueType vt, ScalarObject so) { switch( vt ) { - case DOUBLE: return new DoubleObject(so.getDoubleValue()); - case INT: return new IntObject(so.getLongValue()); + case DOUBLE: return castToDouble(so); + case INT: return castToLong(so); case BOOLEAN: return new BooleanObject(so.getBooleanValue()); case STRING: return new StringObject(so.getStringValue()); default: throw new RuntimeException("Unsupported scalar value type: "+vt.name()); @@ -90,4 +91,25 @@ public abstract class ScalarObjectFactory throw new HopsException("Unsupported literal value type: "+so.getValueType()); } } + + public static LiteralOp createLiteralOp(ScalarObject so, UnaryOp cast) { + switch( cast.getOp() ) { + case CAST_AS_DOUBLE: return new LiteralOp(castToDouble(so).getDoubleValue()); + case CAST_AS_INT: return new LiteralOp(castToLong(so).getLongValue()); + case CAST_AS_BOOLEAN: return new LiteralOp(so.getBooleanValue()); + default: return null; //otherwise: do nothing + } + } + + public static IntObject castToLong(ScalarObject so) { + //note: cast with robustness for various combinations of value types + return new IntObject(!(so instanceof StringObject) ? + so.getLongValue() : UtilFunctions.toLong(Double.parseDouble(so.getStringValue()))); + } + + public static DoubleObject castToDouble(ScalarObject so) { + //note: cast with robustness for various combinations of value types + return new DoubleObject(!(so instanceof StringObject) ? + so.getDoubleValue() : Double.parseDouble(so.getStringValue())); + } } http://git-wip-us.apache.org/repos/asf/systemml/blob/9593b7fb/src/main/java/org/apache/sysml/runtime/instructions/cp/VariableCPInstruction.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/runtime/instructions/cp/VariableCPInstruction.java b/src/main/java/org/apache/sysml/runtime/instructions/cp/VariableCPInstruction.java index a1c89a3..4da1a17 100644 --- a/src/main/java/org/apache/sysml/runtime/instructions/cp/VariableCPInstruction.java +++ b/src/main/java/org/apache/sysml/runtime/instructions/cp/VariableCPInstruction.java @@ -643,13 +643,13 @@ public class VariableCPInstruction extends CPInstruction { break; } case CastAsDoubleVariable:{ - ScalarObject scalarInput = ec.getScalarInput(getInput1()); - ec.setScalarOutput(output.getName(), new DoubleObject(scalarInput.getDoubleValue())); + ScalarObject in = ec.getScalarInput(getInput1()); + ec.setScalarOutput(output.getName(), ScalarObjectFactory.castToDouble(in)); break; } - case CastAsIntegerVariable:{ - ScalarObject scalarInput = ec.getScalarInput(getInput1()); - ec.setScalarOutput(output.getName(), new IntObject(scalarInput.getLongValue())); + case CastAsIntegerVariable:{ + ScalarObject in = ec.getScalarInput(getInput1()); + ec.setScalarOutput(output.getName(), ScalarObjectFactory.castToLong(in)); break; } case CastAsBooleanVariable:{
