[GitHub] [systemds] Baunsgaard commented on a change in pull request #1062: [SYSTEMDS-2546,2547] Fix federated rbind/cbind with support for local data

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

Baunsgaard commented on a change in pull request #1062:
URL: https://github.com/apache/systemds/pull/1062#discussion_r492843396



##########
File path: src/main/java/org/apache/sysds/runtime/instructions/InstructionUtils.java
##########
@@ -140,6 +141,19 @@ public static int checkNumFields( String[] parts, int expected1, int expected2 )
 		return numFields; 
 	}
 
+	public static int checkNumFields( String[] parts, int... expected ) {
+		int numParts = parts.length;
+		int numFields = numParts - 1; //account for opcode
+		
+		if (Arrays.stream(expected).noneMatch((i) -> numFields == i))
+			throw new DMLRuntimeException(
+				"checkNumFields() -- expected number ("
+					+ Arrays.stream(expected).mapToObj(Integer::toString).reduce((a, b) -> a + ", " + b).orElse("")
+					+ ") != is not equal to actual number (" + numFields + ").");

Review comment:
       this looks overkill, maybe use a string builder

##########
File path: src/main/java/org/apache/sysds/runtime/instructions/fed/AppendFEDInstruction.java
##########
@@ -33,71 +32,74 @@
 import org.apache.sysds.runtime.meta.DataCharacteristics;
 
 public class AppendFEDInstruction extends BinaryFEDInstruction {
-	protected boolean _cbind; //otherwise rbind
-	
-	protected AppendFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand out,
-		boolean cbind, String opcode, String istr) {
+	protected boolean _cbind; // otherwise rbind
+
+	protected AppendFEDInstruction(Operator op, CPOperand in1, CPOperand in2, CPOperand out, boolean cbind,
+		String opcode, String istr) {
 		super(FEDType.Append, op, in1, in2, out, opcode, istr);
 		_cbind = cbind;
 	}
-	
+
 	public static AppendFEDInstruction parseInstruction(String str) {
 		String[] parts = InstructionUtils.getInstructionPartsWithValueType(str);
-		InstructionUtils.checkNumFields(parts, 5, 4);
-		
+		InstructionUtils.checkNumFields(parts, 6, 5, 4);
+
 		String opcode = parts[0];
 		CPOperand in1 = new CPOperand(parts[1]);
 		CPOperand in2 = new CPOperand(parts[2]);
 		CPOperand out = new CPOperand(parts[parts.length - 2]);
 		boolean cbind = Boolean.parseBoolean(parts[parts.length - 1]);
-		
+
 		Operator op = new ReorgOperator(OffsetColumnIndex.getOffsetColumnIndexFnObject(-1));
 		return new AppendFEDInstruction(op, in1, in2, out, cbind, opcode, str);
 	}
-	
+
 	@Override
 	public void processInstruction(ExecutionContext ec) {
-		//get inputs
+		// get inputs
 		MatrixObject mo1 = ec.getMatrixObject(input1.getName());
 		MatrixObject mo2 = ec.getMatrixObject(input2.getName());
 		DataCharacteristics dc1 = mo1.getDataCharacteristics();
 		DataCharacteristics dc2 = mo1.getDataCharacteristics();
-		
-		//check input dimensions
-		if (_cbind && mo1.getNumRows() != mo2.getNumRows()) {
-			throw new DMLRuntimeException(
-				"Append-cbind is not possible for federated input matrices " + input1.getName() + " and "
-				+ input2.getName() + " with different number of rows: " + mo1.getNumRows() + " vs "
-				+ mo2.getNumRows());
-		}
-		else if (!_cbind && mo1.getNumColumns() != mo2.getNumColumns()) {
-			throw new DMLRuntimeException(
-				"Append-rbind is not possible for federated input matrices " + input1.getName() + " and "
-				+ input2.getName() + " with different number of columns: " + mo1.getNumColumns()
-				+ " vs " + mo2.getNumColumns());
+
+		// check input dimensions
+		if(_cbind && mo1.getNumRows() != mo2.getNumRows()) {
+			throw new DMLRuntimeException("Append-cbind is not possible for federated input matrices "
+				+ input1.getName() + " and " + input2.getName() + " with different number of rows: " + mo1.getNumRows()
+				+ " vs " + mo2.getNumRows());

Review comment:
       again, maybe string builder.




----------------------------------------------------------------
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