BACtaki commented on code in PR #1677:
URL: https://github.com/apache/systemds/pull/1677#discussion_r1002844611
##########
src/main/java/org/apache/sysds/runtime/instructions/InstructionUtils.java:
##########
@@ -420,7 +421,11 @@ else if ( opcode.equalsIgnoreCase("uacmin") ) {
AggregateOperator agg = new
AggregateOperator(Double.POSITIVE_INFINITY, Builtin.getBuiltinFnObject("min"));
aggun = new AggregateUnaryOperator(agg,
ReduceRow.getReduceRowFnObject(), numThreads);
}
-
+ else if ( opcode.equalsIgnoreCase("uacd") ||
opcode.equalsIgnoreCase("uacdr")
+ || opcode.equalsIgnoreCase("uacdc") ) {
Review Comment:
The way it is written currently, `countDistinct()` and
`countDistinctApprox()` are constructed directly in
`AggregateUnaryCPInstruction` directly, i.e. outside
`InstructionUtils.parseBasicAggregateUnaryOperator()`:
```
public static AggregateUnaryCPInstruction parseInstruction(String str) {
[..]
if(opcode.equalsIgnoreCase("nrow") ||
opcode.equalsIgnoreCase("ncol")
|| opcode.equalsIgnoreCase("length") ||
opcode.equalsIgnoreCase("exists")
|| opcode.equalsIgnoreCase("lineage")){
[..]
}
else if(opcode.equalsIgnoreCase("uacd")){
CountDistinctOperator op = new
CountDistinctOperator(AUType.COUNT_DISTINCT)
.setDirection(Types.Direction.RowCol)
.setIndexFunction(ReduceAll.getReduceAllFnObject());
return new AggregateUnaryCPInstruction(op, in1, out,
AUType.COUNT_DISTINCT, opcode, str);
}
else if(opcode.equalsIgnoreCase("uacdr")){
[..]
}
else if(opcode.equalsIgnoreCase("uacdc")){
[..]
}
else if(opcode.equalsIgnoreCase("uacdap")){
[..]
}
else if(opcode.equalsIgnoreCase("uacdapr")){
[..]
}
else if(opcode.equalsIgnoreCase("uacdapc")){
[..]
}
else if(opcode.equalsIgnoreCase("uarimax") ||
opcode.equalsIgnoreCase("uarimin")){
[..]
}
else { //DEFAULT BEHAVIOR
AggregateUnaryOperator aggun = InstructionUtils
.parseBasicAggregateUnaryOperator(opcode,
Integer.parseInt(parts[3]));
return new AggregateUnaryCPInstruction(aggun, in1, out,
AUType.DEFAULT, opcode, str);
}
}
```
This is a result of `CountDistinctOperator` not inheriting from
`AggregateUnaryOperator`. I have made this change in the next iteration. The
switch logic has therefore been moved to
`InstructionUtils.parseBasicAggregateUnaryOperator()`.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
