[GitHub] [systemml] j143 commented on a change in pull request #893: Federated Frames

[GitBox <gi...@apache.org>]

j143 commented on a change in pull request #893:
URL: https://github.com/apache/systemml/pull/893#discussion_r415398205



##########
File path: src/main/java/org/apache/sysds/runtime/util/UtilFunctions.java
##########
@@ -41,6 +36,18 @@
 import org.apache.sysds.runtime.matrix.data.Pair;
 import org.apache.sysds.runtime.meta.TensorCharacteristics;
 
+import java.util.ArrayList;

Review comment:
       Changes such as 
   1. Formatting (whitespace addition/removal)
   2. Rearragement of imports
   
   These kind of changes, we cannot be decisive. Now, the reviewing person needs to be able give OK for the functionality and formatting changes.
   **TL;DR**: If there is a need for change of formatting opening a separate PR would be more helpful.
   

##########
File path: src/test/java/org/apache/sysds/test/functions/federated/FederatedConstructionTest.java
##########
@@ -62,28 +70,59 @@ public void setUp() {
 	}
 
 	@Test
-	public void federatedConstructionCP() {
-		federatedConstruction(Types.ExecMode.SINGLE_NODE);
+	public void federatedMatrixConstructionCP() {
+		federatedMatrixConstruction(Types.ExecMode.SINGLE_NODE);
+	}
+
+	@Test
+	public void federatedMatrixConstructionSP() {
+		federatedMatrixConstruction(Types.ExecMode.SPARK);
 	}
 
+	public void federatedMatrixConstruction(Types.ExecMode execMode) {
+		getAndLoadTestConfiguration(TEST_NAME);
+		// write input matrix
+		double[][] A = getRandomMatrix(rows, cols, -1, 1, 1, 1234);
+		writeInputMatrixWithMTD("A", A, false, new MatrixCharacteristics(rows, cols, blocksize, rows * cols));
+		federatedConstruction(execMode, MATRIX_TEST_FILE_NAME, "A", null);
+	}
+	
 	@Test
-	public void federatedConstructionSP() {
-		federatedConstruction(Types.ExecMode.SPARK);
+	public void federatedFrameConstructionCP() throws IOException {
+		federatedFrameConstruction(Types.ExecMode.SINGLE_NODE);
+	}
+	
+	/* like other federated functionality, SPARK execution mode is not yet working (waiting for better integration

Review comment:
       Should we mark this a TODO.

##########
File path: src/test/java/org/apache/sysds/test/TestUtils.java
##########
@@ -268,78 +268,171 @@ else if (Integer.parseInt(expRcn[2]) != Integer.parseInt(rcn[2])) {
 			fail("unable to read file: " + e.getMessage());
 		}
 	}
-
+	
 	/**
 	 * <p>
-	 * Compares the expected values calculated in Java by testcase and which are
-	 * in the normal filesystem, with those calculated by SystemDS located in
-	 * HDFS
+	 * Read the cell values of the expected file and actual files. Schema is used for correct parsing if the file is a
+	 * frame and if it is null FP64 will be used for all values (useful for Matrices).
 	 * </p>
-	 * 
-	 * @param expectedFile
-	 *            file with expected values, which is located in OS filesystem
-	 * @param actualDir
-	 *            file with actual values, which is located in HDFS
-	 * @param epsilon
-	 *            tolerance for value comparison
+	 *
+	 * @param schema         the schema of the frame, can be null (for FP64)
+	 * @param expectedFile   the file with expected values
+	 * @param actualDir      the directory where the actual values were written
+	 * @param expectedValues the HashMap where the expected values will be written to
+	 * @param actualValues   the HashMap where the actual values will be written to
 	 */
-	@SuppressWarnings("resource")
-	public static void compareDMLMatrixWithJavaMatrix(String expectedFile, String actualDir, double epsilon) {
+	private static void readActualAndExpectedFile(ValueType[] schema, String expectedFile, String actualDir,
+		HashMap<CellIndex, Object> expectedValues, HashMap<CellIndex, Object> actualValues) {
 		try {
 			Path outDirectory = new Path(actualDir);
 			Path compareFile = new Path(expectedFile);
 			FileSystem fs = IOUtilFunctions.getFileSystem(outDirectory, conf);
 			FSDataInputStream fsin = fs.open(compareFile);
-			
-			HashMap<CellIndex, Double> expectedValues = new HashMap<>();
-			
-			try( BufferedReader compareIn = new BufferedReader(new InputStreamReader(fsin)) ) {
+
+			try(BufferedReader compareIn = new BufferedReader(new InputStreamReader(fsin))) {

Review comment:
       The test configuration has been modified, would you mind rebasing the latest.

##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/caching/FrameObject.java
##########
@@ -155,15 +182,46 @@ public long getNumColumns() {
 		return dc.getCols();
 	}
 	
+	private FrameBlock readFederated() {
+		FrameBlock result = new FrameBlock(_schema);
+		// provide long support?

Review comment:
       `int` may not be sufficient. Either this be marked a TODO or rationally assume to that cases would fall within range.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org