[GitHub] [systemds] Baunsgaard commented on a change in pull request #1051: [SYSTEMDS-2605] Fine-Grained Privacy Constraints 2 Rebased

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

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



##########
File path: src/main/java/org/apache/sysds/runtime/instructions/Instruction.java
##########
@@ -38,7 +41,21 @@
 		FEDERATED
 	}
 	
-	private static final Log LOG = LogFactory.getLog(Instruction.class.getName());
+	protected static final Log LOG = LogFactory.getLog(Instruction.class.getName());

Review comment:
       I made this private to ensure that the LOG is located in the same file it is called from.
   If you want to make use of it somewhere else make a new Log object in that file.

##########
File path: src/main/java/org/apache/sysds/runtime/instructions/spark/SPInstruction.java
##########
@@ -41,16 +41,15 @@
 	}
 
 	protected final SPType _sptype;
-	protected final Operator _optr;
 	protected final boolean _requiresLabelUpdate;
 
 	protected SPInstruction(SPType type, String opcode, String istr) {
 		this(type, null, opcode, istr);
 	}
 
 	protected SPInstruction(SPType type, Operator op, String opcode, String istr) {
+		super(op);

Review comment:
       can you explain to me this super class call?

##########
File path: src/test/java/org/apache/sysds/test/functions/privacy/GLMTest.java
##########
@@ -154,7 +155,10 @@ public GLMTest (int numRecords_, int numFeatures_, int distFamilyType_, double d
 			{  100,  10,  2, -1.0,  4,  0.0,  0.01, 3.0,  -2.0,  1.0,  1.0, GLMType.Bernoullicloglog1 }, // Bernoulli {-1, 1}.cloglog
 			{  200,  10,  2, -1.0,  5,  0.0,  0.01, 3.0,   0.0,  2.0,  1.0, GLMType.Bernoullicauchit },  // Bernoulli {-1, 1}.cauchit
 		};
-		return Arrays.asList(data);
+		if ( runAll )
+			return Arrays.asList(data);
+		else 
+			return Arrays.asList( new Object[][]{data[0]} );

Review comment:
       again the filter?

##########
File path: src/main/java/org/apache/sysds/runtime/instructions/Instruction.java
##########
@@ -38,7 +41,21 @@
 		FEDERATED
 	}
 	
-	private static final Log LOG = LogFactory.getLog(Instruction.class.getName());
+	protected static final Log LOG = LogFactory.getLog(Instruction.class.getName());
+	protected final Operator _optr;
+
+	protected Instruction(Operator _optr){
+		this._optr = _optr;
+	}
+
+	// local flag for debug output
+	private static final boolean LTRACE = false;
+	static {
+		// for internal debugging only
+		if( LTRACE ) {
+			Logger.getLogger("org.apache.sysds.runtime.instructions.Instruction").setLevel(Level.TRACE);

Review comment:
       we removed all the instances of this overwriting of logging.
   Please change to use the file in:
   /src/test/resources/log4j.properties
   
   if you want to debug something specific.
   

##########
File path: src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedWorkerHandler.java
##########
@@ -280,8 +281,9 @@ private FederatedResponse execInstruction(FederatedRequest request) {
 		ExecutionContext ec = _ecm.get(request.getTID());
 		BasicProgramBlock pb = new BasicProgramBlock(null);
 		pb.getInstructions().clear();
-		pb.getInstructions().add(InstructionParser
-			.parseSingleInstruction((String)request.getParam(0)));
+		Instruction receivedInstruction = InstructionParser
+				.parseSingleInstruction((String)request.getParam(0));
+		pb.getInstructions().add(receivedInstruction);

Review comment:
       I don't see what this change does.

##########
File path: src/main/java/org/apache/sysds/runtime/privacy/FineGrained/FineGrainedPrivacyList.java
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.
+ */
+
+package org.apache.sysds.runtime.privacy.FineGrained;
+
+import java.io.Serializable;
+import java.util.AbstractMap;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.sysds.runtime.privacy.PrivacyConstraint.PrivacyLevel;
+
+/**
+ * Simple implementation of retrieving fine-grained privacy constraints
+ * based on pairs in an ArrayList.
+ */
+public class FineGrainedPrivacyList implements FineGrainedPrivacy {
+
+	private ArrayList<Map.Entry<DataRange, PrivacyLevel>> constraintCollection = new ArrayList<>();
+
+	@Override
+	public void put(DataRange dataRange, PrivacyLevel privacyLevel) {
+		constraintCollection.add(new AbstractMap.SimpleEntry<DataRange, PrivacyLevel>(dataRange, privacyLevel));
+	}
+
+	@Override
+	public Map<DataRange,PrivacyLevel> getPrivacyLevel(DataRange searchRange) {
+		Map<DataRange, PrivacyLevel> matches = new LinkedHashMap<>();

Review comment:
       if possible can it not be a linkedHashMap, since the performance of that is ... slow, in many cases.

##########
File path: src/main/java/org/apache/sysds/parser/DataExpression.java
##########
@@ -2239,9 +2251,44 @@ public boolean isRead()
 	 * Sets privacy of identifier if privacy variable parameter is set.  
 	 */
 	private void setPrivacy(){
-		Expression eprivacy = getVarParam("privacy");
-		if ( eprivacy != null ){
-			getOutput().setPrivacy(PrivacyLevel.valueOf(eprivacy.toString()));
+		Expression eprivacy = getVarParam(PRIVACY);

Review comment:
       can this method be split up into multiple, or add some comments?

##########
File path: src/main/java/org/apache/sysds/runtime/privacy/PrivacyPropagator.java
##########
@@ -414,4 +591,25 @@ else if ( inst instanceof SqlCPInstruction )
 			instructionOutputNames = new String[]{((SqlCPInstruction) inst).getOutputVariableName()};
 		return instructionOutputNames;
 	}
+
+	private static ArrayList<CPOperand> getOutputOperands(Instruction inst){
+		// The order of the following statements is important
+		if ( inst instanceof MultiReturnParameterizedBuiltinCPInstruction )
+			return ((MultiReturnParameterizedBuiltinCPInstruction) inst).getOutputs();
+		else if ( inst instanceof MultiReturnBuiltinCPInstruction )
+			return ((MultiReturnBuiltinCPInstruction) inst).getOutputs();
+		else if ( inst instanceof ComputationCPInstruction )
+			return getSingletonList(((ComputationCPInstruction) inst).getOutput());
+		else if ( inst instanceof VariableCPInstruction )
+			return getSingletonList(((VariableCPInstruction) inst).getOutput());
+		else if ( inst instanceof SqlCPInstruction )
+			return getSingletonList(((SqlCPInstruction) inst).getOutput());
+		return new ArrayList<CPOperand>();

Review comment:
       not 100% sure, but the if else is instance of is probably slow, and there should be some alternative. 

##########
File path: src/main/java/org/apache/sysds/runtime/privacy/FineGrained/FineGrainedPrivacyList.java
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.
+ */
+
+package org.apache.sysds.runtime.privacy.FineGrained;
+
+import java.io.Serializable;
+import java.util.AbstractMap;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.sysds.runtime.privacy.PrivacyConstraint.PrivacyLevel;
+
+/**
+ * Simple implementation of retrieving fine-grained privacy constraints
+ * based on pairs in an ArrayList.
+ */
+public class FineGrainedPrivacyList implements FineGrainedPrivacy {
+
+	private ArrayList<Map.Entry<DataRange, PrivacyLevel>> constraintCollection = new ArrayList<>();
+
+	@Override
+	public void put(DataRange dataRange, PrivacyLevel privacyLevel) {
+		constraintCollection.add(new AbstractMap.SimpleEntry<DataRange, PrivacyLevel>(dataRange, privacyLevel));
+	}
+
+	@Override
+	public Map<DataRange,PrivacyLevel> getPrivacyLevel(DataRange searchRange) {
+		Map<DataRange, PrivacyLevel> matches = new LinkedHashMap<>();
+		for ( Map.Entry<DataRange, PrivacyLevel> constraint : constraintCollection ){
+			if ( constraint.getKey().overlaps(searchRange) ) 
+				matches.put(constraint.getKey(), constraint.getValue());
+		}
+		return matches;
+	}
+
+	@Override
+	public Map<DataRange,PrivacyLevel> getPrivacyLevelOfElement(long[] searchIndex) {
+		Map<DataRange, PrivacyLevel> matches = new LinkedHashMap<>();
+		constraintCollection.forEach( constraint -> { 
+			if (constraint.getKey().contains(searchIndex)) 
+				matches.put(constraint.getKey(), constraint.getValue()); 
+		} );
+		return matches;
+	}
+
+	@Override
+	public DataRange[] getDataRangesOfPrivacyLevel(PrivacyLevel privacyLevel) {
+		ArrayList<DataRange> matches = new ArrayList<>();
+		constraintCollection.forEach(constraint -> { if (constraint.getValue() == privacyLevel) matches.add(constraint.getKey()); } );
+		return matches.toArray(new DataRange[0]);
+	}
+
+	@Override
+	public void removeAllConstraints() {
+		constraintCollection.clear();
+	}
+
+	@Override
+	public boolean hasConstraints() {
+		return !constraintCollection.isEmpty();
+	}
+
+	@Override
+	public Map<String, long[][][]> getAllConstraints() {

Review comment:
       also this array of array of array, seems extravagant. Preferably it would be nice in some other format.

##########
File path: src/main/java/org/apache/sysds/runtime/privacy/PrivacyPropagator.java
##########
@@ -19,15 +19,19 @@
 
 package org.apache.sysds.runtime.privacy;
 
+import java.util.*;

Review comment:
       no wildcard imports.

##########
File path: src/test/java/org/apache/sysds/test/functions/privacy/BuiltinGLMTest.java
##########
@@ -228,6 +229,9 @@ public void runtestGLM(PrivacyConstraint privacyConstraint, Class<?> expectedExc
 				{  100,   10,  2,  1.0,  2,  0.0,  3.0,   0.0,  2.0,  2.5 },   // Binomial two-column.logit
 				{  200,   10,  2,  1.0,  3,  0.0,  3.0,   0.0,  2.0,  2.5 },   // Binomial two-column.probit
 		};
-		return Arrays.asList(data);
+		if ( runAll )
+			return Arrays.asList(data);
+		else
+			return Arrays.asList(new Object[][]{data[0]});

Review comment:
       is it to slow to run all or?

##########
File path: src/main/java/org/apache/sysds/runtime/instructions/cp/MultiReturnBuiltinCPInstruction.java
##########
@@ -47,6 +47,10 @@ public CPOperand getOutput(int i) {
 		return _outputs.get(i);
 	}
 
+	public ArrayList<CPOperand> getOutputs(){

Review comment:
       Maybe change these to List type return?




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