Repository: systemml Updated Branches: refs/heads/master 1049f5e56 -> 9593b7fbe
[SYSTEMML-2455] Fix list indexing size propagation on parser-validate So far only the compiler from hops downward correctly propagated sizes for list indexing. However, this led to validation issues on parsing for operations like as.scalar which fails with incorrect input sizes. This patch does a best effort size propagation for list and otherwise indicates unknowns which is correct in all cases. Furthermore, this patch also includes a refactoring of indexed identifier validation to avoid code duplication. Project: http://git-wip-us.apache.org/repos/asf/systemml/repo Commit: http://git-wip-us.apache.org/repos/asf/systemml/commit/78c5c228 Tree: http://git-wip-us.apache.org/repos/asf/systemml/tree/78c5c228 Diff: http://git-wip-us.apache.org/repos/asf/systemml/diff/78c5c228 Branch: refs/heads/master Commit: 78c5c22884c77808913963a5eaf9d6236365d31d Parents: 1049f5e Author: Matthias Boehm <[email protected]> Authored: Wed Jul 18 21:00:40 2018 -0700 Committer: Matthias Boehm <[email protected]> Committed: Wed Jul 18 22:07:09 2018 -0700 ---------------------------------------------------------------------- .../org/apache/sysml/parser/Identifier.java | 73 +++++++------------- .../functions/misc/ListAndStructTest.java | 12 ++++ .../scripts/functions/misc/ListIxAndCasts.R | 35 ++++++++++ .../scripts/functions/misc/ListIxAndCasts.dml | 30 ++++++++ 4 files changed, 101 insertions(+), 49 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/systemml/blob/78c5c228/src/main/java/org/apache/sysml/parser/Identifier.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/sysml/parser/Identifier.java b/src/main/java/org/apache/sysml/parser/Identifier.java index bf36a2f..0a5f79d 100644 --- a/src/main/java/org/apache/sysml/parser/Identifier.java +++ b/src/main/java/org/apache/sysml/parser/Identifier.java @@ -141,59 +141,40 @@ public abstract class Identifier extends Expression @Override public void validateExpression(HashMap<String,DataIdentifier> ids, HashMap<String,ConstIdentifier> constVars, boolean conditional) { - if( getOutput() instanceof DataIdentifier ) { - // set properties for Data identifier - String name = ((DataIdentifier)this.getOutput()).getName(); + String name = ((DataIdentifier)getOutput()).getName(); Identifier id = ids.get(name); if ( id == null ){ //undefined variables are always treated unconditionally as error in order to prevent common script-level bugs raiseValidateError("Undefined Variable (" + name + ") used in statement", false, LanguageErrorCodes.INVALID_PARAMETERS); } - this.getOutput().setProperties(id); + getOutput().setProperties(id); // validate IndexedIdentifier -- which is substype of DataIdentifer with index - if (this.getOutput() instanceof IndexedIdentifier){ - + if( getOutput() instanceof IndexedIdentifier ){ // validate the row / col index bounds (if defined) - IndexedIdentifier indexedIdentiferOut = (IndexedIdentifier)this.getOutput(); - - if (indexedIdentiferOut.getRowLowerBound() != null) { - indexedIdentiferOut.getRowLowerBound().validateExpression(ids, constVars, conditional); - Expression tempExpr = indexedIdentiferOut.getRowLowerBound(); - if (tempExpr.getOutput().getDataType() == Expression.DataType.MATRIX){ - raiseValidateError("Matrix values for row lower index bound are not supported, which includes indexed identifiers.", conditional); + IndexedIdentifier ixId = (IndexedIdentifier)getOutput(); + Expression[] exp = new Expression[]{ixId.getRowLowerBound(), + ixId.getRowUpperBound(), ixId.getColLowerBound(), ixId.getColUpperBound()}; + String[] msg = new String[]{"row lower", "row upper", "column lower", "column upper"}; + for( int i=0; i<4; i++ ) { + if( exp[i] != null ) { + exp[i].validateExpression(ids, constVars, conditional); + if (exp[i].getOutput().getDataType() == Expression.DataType.MATRIX){ + raiseValidateError("Matrix values for "+msg[i]+" index bound are " + + "not supported, which includes indexed identifiers.", conditional); + } } } - if (indexedIdentiferOut.getRowUpperBound() != null) { - indexedIdentiferOut.getRowUpperBound().validateExpression(ids, constVars, conditional); - Expression tempExpr = indexedIdentiferOut.getRowUpperBound(); - if (tempExpr.getOutput().getDataType() == Expression.DataType.MATRIX){ - raiseValidateError("Matrix values for row upper index bound are not supported, which includes indexed identifiers.", conditional); - } - } - - if (indexedIdentiferOut.getColLowerBound() != null) { - indexedIdentiferOut.getColLowerBound().validateExpression(ids,constVars, conditional); - Expression tempExpr = indexedIdentiferOut.getColLowerBound(); - if (tempExpr.getOutput().getDataType() == Expression.DataType.MATRIX){ - raiseValidateError("Matrix values for column lower index bound are not supported, which includes indexed identifiers.", conditional); - } - } - - if (indexedIdentiferOut.getColUpperBound() != null) { - indexedIdentiferOut.getColUpperBound().validateExpression(ids, constVars, conditional); - Expression tempExpr = indexedIdentiferOut.getColUpperBound(); - if (tempExpr.getOutput().getDataType() == Expression.DataType.MATRIX){ - raiseValidateError("Matrix values for column upper index bound are not supported, which includes indexed identifiers.", conditional); - } - } - - if( this.getOutput().getDataType() != DataType.LIST ) { - IndexPair updatedIndices = ((IndexedIdentifier)this.getOutput()).calculateIndexedDimensions(ids, constVars, conditional); - ((IndexedIdentifier)this.getOutput()).setDimensions(updatedIndices._row, updatedIndices._col); + if( getOutput().getDataType() == DataType.LIST ) { + int dim1 = (((IndexedIdentifier)getOutput()).getRowUpperBound() == null) ? 1 : - 1; + ((IndexedIdentifier)getOutput()).setDimensions(dim1, 1); + } + else { //default + IndexPair updatedIndices = ((IndexedIdentifier)getOutput()).calculateIndexedDimensions(ids, constVars, conditional); + ((IndexedIdentifier)getOutput()).setDimensions(updatedIndices._row, updatedIndices._col); } } } @@ -203,15 +184,9 @@ public abstract class Identifier extends Expression } public void computeDataType() { - - if ((_dim1 == 0) && (_dim2 == 0)) { - _dataType = DataType.SCALAR; - } else if ((_dim1 >= 1) || (_dim2 >= 1)){ - // Vector also set as matrix - // Data type is set as matrix, if either of dimensions is -1 - _dataType = DataType.MATRIX; - } else _dataType = DataType.UNKNOWN; - + _dataType = ((_dim1 == 0) && (_dim2 == 0)) ? + DataType.SCALAR : ((_dim1 >= 1) || (_dim2 >= 1)) ? + DataType.MATRIX : DataType.UNKNOWN; } public void setBooleanProperties(){ http://git-wip-us.apache.org/repos/asf/systemml/blob/78c5c228/src/test/java/org/apache/sysml/test/integration/functions/misc/ListAndStructTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/sysml/test/integration/functions/misc/ListAndStructTest.java b/src/test/java/org/apache/sysml/test/integration/functions/misc/ListAndStructTest.java index e8e3d1a..45eeca6 100644 --- a/src/test/java/org/apache/sysml/test/integration/functions/misc/ListAndStructTest.java +++ b/src/test/java/org/apache/sysml/test/integration/functions/misc/ListAndStructTest.java @@ -42,6 +42,7 @@ public class ListAndStructTest extends AutomatedTestBase private static final String TEST_NAME7 = "ListAsMatrix"; private static final String TEST_NAME8 = "ListUnnamedRix"; private static final String TEST_NAME9 = "ListNamedRix"; + private static final String TEST_NAME10 = "ListIxAndCasts"; private static final String TEST_DIR = "functions/misc/"; private static final String TEST_CLASS_DIR = TEST_DIR + ListAndStructTest.class.getSimpleName() + "/"; @@ -58,6 +59,7 @@ public class ListAndStructTest extends AutomatedTestBase addTestConfiguration( TEST_NAME7, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME7, new String[] { "R" }) ); addTestConfiguration( TEST_NAME8, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME8, new String[] { "R" }) ); addTestConfiguration( TEST_NAME9, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME9, new String[] { "R" }) ); + addTestConfiguration( TEST_NAME10, new TestConfiguration(TEST_CLASS_DIR, TEST_NAME10, new String[] { "R" }) ); } @Test @@ -150,6 +152,16 @@ public class ListAndStructTest extends AutomatedTestBase runListStructTest(TEST_NAME9, true); } + @Test + public void testListIndexingAndCasts() { + runListStructTest(TEST_NAME10, false); + } + + @Test + public void testListIndexingAndCastsRewrites() { + runListStructTest(TEST_NAME10, true); + } + private void runListStructTest(String testname, boolean rewrites) { boolean oldFlag = OptimizerUtils.ALLOW_ALGEBRAIC_SIMPLIFICATION; http://git-wip-us.apache.org/repos/asf/systemml/blob/78c5c228/src/test/scripts/functions/misc/ListIxAndCasts.R ---------------------------------------------------------------------- diff --git a/src/test/scripts/functions/misc/ListIxAndCasts.R b/src/test/scripts/functions/misc/ListIxAndCasts.R new file mode 100644 index 0000000..35f92a5 --- /dev/null +++ b/src/test/scripts/functions/misc/ListIxAndCasts.R @@ -0,0 +1,35 @@ +#------------------------------------------------------------- +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +#------------------------------------------------------------- + + +args <- commandArgs(TRUE) +options(digits=22) +library("Matrix") + +X = list(1.1, 2.2, 3.3, 4.4); +Y = list(a=1.1, b=2.2, c=3.3, d=4.4); +while(FALSE){} + +R = as.double(unlist(X[1])) + as.integer(unlist(X[3])); +R = R + as.double(unlist(Y["a"])) + as.integer(unlist(Y["c"])); +R = as.matrix(R); + +writeMM(as(R, "CsparseMatrix"), paste(args[1], "R", sep="")); http://git-wip-us.apache.org/repos/asf/systemml/blob/78c5c228/src/test/scripts/functions/misc/ListIxAndCasts.dml ---------------------------------------------------------------------- diff --git a/src/test/scripts/functions/misc/ListIxAndCasts.dml b/src/test/scripts/functions/misc/ListIxAndCasts.dml new file mode 100644 index 0000000..b61d866 --- /dev/null +++ b/src/test/scripts/functions/misc/ListIxAndCasts.dml @@ -0,0 +1,30 @@ +#------------------------------------------------------------- +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +#------------------------------------------------------------- + +X = list(1.1, 2.2, 3.3, 4.4); +Y = list(a=1.1, b=2.2, c=3.3, d=4.4); +while(FALSE){} + +R = as.double(as.scalar(X[1])) + as.integer(as.scalar(X[3])); +R = R + as.double(as.scalar(Y["a"])) + as.integer(as.scalar(Y["c"])); +R = as.matrix(R); + +write(R, $1);
