[systemds] branch main updated: [SYSTEMDS-3463] Fix compilation of unique() unary aggregate operations

[mboehm7]

This is an automated email from the ASF dual-hosted git repository.

mboehm7 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/systemds.git


The following commit(s) were added to refs/heads/main by this push:
     new b43396e754 [SYSTEMDS-3463] Fix compilation of unique() unary aggregate 
operations
b43396e754 is described below

commit b43396e754957b675a2c30c51970a8573ef21535
Author: Matthias Boehm <mboe...@gmail.com>
AuthorDate: Mon Feb 6 19:21:46 2023 +0100

    [SYSTEMDS-3463] Fix compilation of unique() unary aggregate operations
    
    So far unique() in default hybrid mode, failed with compilation issues
    due to it's unknown size but missing spark instructions. We now
    properly force this operation into single node (while otherwise
    allowing hybrid and spark operations), and explicitly introduce a DAG
    cut after such operations, in order to give dynamic recompilation a
    chance to obtain the actual size and compile following operations
    accordingly.
---
 src/main/java/org/apache/sysds/hops/AggUnaryOp.java              | 9 ++++++---
 .../hops/rewrite/RewriteSplitDagDataDependentOperators.java      | 4 +++-
 .../sysds/test/functions/builtin/part1/BuiltinAucTest.java       | 6 ++++--
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/src/main/java/org/apache/sysds/hops/AggUnaryOp.java 
b/src/main/java/org/apache/sysds/hops/AggUnaryOp.java
index 23439b182e..468caa707e 100644
--- a/src/main/java/org/apache/sysds/hops/AggUnaryOp.java
+++ b/src/main/java/org/apache/sysds/hops/AggUnaryOp.java
@@ -385,9 +385,12 @@ public class AggUnaryOp extends MultiThreadedHop
                        _etype = ExecType.SPARK;
                }
 
-               //mark for recompile (forever)
-               setRequiresRecompileIfNecessary();
-               
+               //ensure cp exec type for single-node operations
+               if( _op == AggOp.UNIQUE ) {
+                       _etype = ExecType.CP;
+               } else {
+                       setRequiresRecompileIfNecessary();
+               }
                return _etype;
        }
 
diff --git 
a/src/main/java/org/apache/sysds/hops/rewrite/RewriteSplitDagDataDependentOperators.java
 
b/src/main/java/org/apache/sysds/hops/rewrite/RewriteSplitDagDataDependentOperators.java
index 239ca2372e..0c2801da97 100644
--- 
a/src/main/java/org/apache/sysds/hops/rewrite/RewriteSplitDagDataDependentOperators.java
+++ 
b/src/main/java/org/apache/sysds/hops/rewrite/RewriteSplitDagDataDependentOperators.java
@@ -26,6 +26,7 @@ import java.util.HashSet;
 import java.util.List;
 
 import org.apache.sysds.api.DMLScript;
+import org.apache.sysds.common.Types.AggOp;
 import org.apache.sysds.common.Types.ExecMode;
 import org.apache.sysds.common.Types.OpOp1;
 import org.apache.sysds.common.Types.OpOp3;
@@ -321,7 +322,8 @@ public class RewriteSplitDagDataDependentOperators extends 
StatementBlockRewrite
                        || (HopRewriteUtils.isParameterBuiltinOp(hop, 
ParamBuiltinOp.GROUPEDAGG) 
                                && 
!((ParameterizedBuiltinOp)hop).isKnownNGroups() && !noSplitRequired)
                        || ((HopRewriteUtils.isUnary(hop, OpOp1.COMPRESS) || 
hop.requiresCompression()) &&
-                               (!HopRewriteUtils.hasOnlyWriteParents(hop, 
true, true)));
+                               (!HopRewriteUtils.hasOnlyWriteParents(hop, 
true, true)))
+                       || (HopRewriteUtils.isAggUnaryOp(hop, AggOp.UNIQUE) & 
!noSplitRequired);
                //note: for compression we probe for write parents (part of 
noSplitRequired) directly
                // because we want to split even if the dimensions are known 
        }
diff --git 
a/src/test/java/org/apache/sysds/test/functions/builtin/part1/BuiltinAucTest.java
 
b/src/test/java/org/apache/sysds/test/functions/builtin/part1/BuiltinAucTest.java
index 49502e8371..fec9e94bdf 100644
--- 
a/src/test/java/org/apache/sysds/test/functions/builtin/part1/BuiltinAucTest.java
+++ 
b/src/test/java/org/apache/sysds/test/functions/builtin/part1/BuiltinAucTest.java
@@ -26,6 +26,7 @@ import org.apache.sysds.common.Types.ExecMode;
 import org.apache.sysds.runtime.matrix.data.MatrixValue.CellIndex;
 import org.apache.sysds.test.AutomatedTestBase;
 import org.apache.sysds.test.TestConfiguration;
+import org.apache.sysds.utils.Statistics;
 
 public class BuiltinAucTest extends AutomatedTestBase
 {
@@ -106,7 +107,7 @@ public class BuiltinAucTest extends AutomatedTestBase
        
        private void runAucTest(double auc, double[] Y, double[] P)
        {
-               ExecMode platformOld = setExecMode(ExecMode.SINGLE_NODE);
+               ExecMode platformOld = setExecMode(ExecMode.HYBRID);
 
                try
                {
@@ -122,9 +123,10 @@ public class BuiltinAucTest extends AutomatedTestBase
                        //execute test
                        runTest(true, false, null, -1);
 
-                       //compare matrices 
+                       //compare auc and proper plans w/o spark instructions
                        double val = readDMLMatrixFromOutputDir("C").get(new 
CellIndex(1,1));
                        Assert.assertEquals("Incorrect values: ", auc, val, 
eps);
+                       Assert.assertEquals(0, 
Statistics.getNoOfExecutedSPInst());
                }
                finally {
                        rtplatform = platformOld;