keith-turner commented on code in PR #93:
URL: https://github.com/apache/accumulo-access/pull/93#discussion_r2669662849
##########
core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java:
##########
@@ -159,4 +166,59 @@ boolean evaluate(byte[] accessExpression) throws
InvalidAccessExpressionExceptio
};
return ParserEvaluator.parseAccessExpression(accessExpression, atp,
authToken -> true);
}
+
+ private boolean canAccess(ParsedAccessExpression pae) {
+ switch (pae.getType()) {
+ case AND:
+ return canAccessParsedAnd(pae.getChildren());
+ case OR:
+ return canAccessParsedOr(pae.getChildren());
+ case AUTHORIZATION:
+ return canAccessParsedAuthorization(pae.getExpression());
+ case EMPTY:
+ return true;
+ default:
+ throw new IllegalArgumentException("Unhandled type: " + pae.getType());
+ }
+ }
+
+ private boolean canAccessParsedAnd(List<ParsedAccessExpression> children) {
+ requireNonNull(children, "null children list passed to method");
+ if (children.isEmpty() || children.size() < 2) {
+ throw new IllegalArgumentException(
+ "Malformed AND expression, children: " + children.size() + " -> " +
children);
+ }
+ boolean result = true;
+ for (ParsedAccessExpression child : children) {
+ result &= canAccess(child);
+ if (result == false) {
+ return result;
+ }
+ }
+ return result;
+ }
+
+ private boolean canAccessParsedOr(List<ParsedAccessExpression> children) {
+ requireNonNull(children, "null children list passed to method");
+ if (children.isEmpty()) {
+ throw new IllegalArgumentException("Malformed OR expression, no
children");
+ }
+ boolean result = false;
+ for (ParsedAccessExpression child : children) {
+ result |= canAccess(child);
Review Comment:
This can short circuit. Tried that locally and is sped things up w/ the new
benchmakrs.
```
AccessExpressionBenchmark.measureLegacyEvaluationOnly thrpt 12 22.171 ±
0.136 ops/us
AccessExpressionBenchmark.measureParsedEvaluation thrpt 12 17.707 ±
0.154 ops/us
```
I made the following change for the above results.
```java
for (ParsedAccessExpression child : children) {
if (canAccess(child)) {
return true;
}
}
return false;
```
##########
core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java:
##########
@@ -159,4 +166,59 @@ boolean evaluate(byte[] accessExpression) throws
InvalidAccessExpressionExceptio
};
return ParserEvaluator.parseAccessExpression(accessExpression, atp,
authToken -> true);
}
+
+ private boolean canAccess(ParsedAccessExpression pae) {
+ switch (pae.getType()) {
+ case AND:
+ return canAccessParsedAnd(pae.getChildren());
+ case OR:
+ return canAccessParsedOr(pae.getChildren());
+ case AUTHORIZATION:
+ return canAccessParsedAuthorization(pae.getExpression());
+ case EMPTY:
+ return true;
+ default:
+ throw new IllegalArgumentException("Unhandled type: " + pae.getType());
+ }
+ }
+
+ private boolean canAccessParsedAnd(List<ParsedAccessExpression> children) {
+ requireNonNull(children, "null children list passed to method");
+ if (children.isEmpty() || children.size() < 2) {
+ throw new IllegalArgumentException(
+ "Malformed AND expression, children: " + children.size() + " -> " +
children);
+ }
+ boolean result = true;
+ for (ParsedAccessExpression child : children) {
+ result &= canAccess(child);
+ if (result == false) {
+ return result;
+ }
+ }
+ return result;
+ }
+
+ private boolean canAccessParsedOr(List<ParsedAccessExpression> children) {
+ requireNonNull(children, "null children list passed to method");
+ if (children.isEmpty()) {
+ throw new IllegalArgumentException("Malformed OR expression, no
children");
+ }
+ boolean result = false;
+ for (ParsedAccessExpression child : children) {
+ result |= canAccess(child);
+ }
+ return result;
+ }
+
+ private boolean canAccessParsedAuthorization(String token) {
+ var bytesWrapper = ParserEvaluator.lookupWrappers.get();
+ var authToken = token.getBytes(UTF_8);
Review Comment:
This is allocating objects and copying data, could be the cause of being
slower than the accumulo code. Not really sure how to avoid this, and w/ the
changes in #96 it may not be worth trying to avoid at this point.
--
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]
