From 534c71100e2ff9a36c988f70748cdfd143f06c64 Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Fri, 19 Dec 2025 20:56:03 +0000 Subject: [PATCH 1/5] Added AccessEvaluator.canAccess(ParsedAccessExpression) In the MultiAccessEvaluatorImpl case it was calling AccessEvaluator.canAccess(byte[]) for each AccessEvaluator. canAccess(byte[]) would re-parse the AccessExpression each time. This change enables the MultiAccessEvaluatorImpl to get the ParsedAccessExpression and call the new method on each AccessEvaluator. The new method traverses the parse tree for evalation so that parsing is not performed multiple times. The downside is that there are two different methods in use, but the tests were already checking this and still pass. Closes #78 --- .../accumulo/access/AccessEvaluator.java | 8 +++ .../access/impl/AccessEvaluatorImpl.java | 59 +++++++++++++++++++ .../access/impl/MultiAccessEvaluatorImpl.java | 22 ++++--- 3 files changed, 80 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/access/AccessEvaluator.java b/core/src/main/java/org/apache/accumulo/access/AccessEvaluator.java index ea0fb2c..39c7b63 100644 --- a/core/src/main/java/org/apache/accumulo/access/AccessEvaluator.java +++ b/core/src/main/java/org/apache/accumulo/access/AccessEvaluator.java @@ -80,6 +80,14 @@ public sealed interface AccessEvaluator permits AccessEvaluatorImpl, MultiAccess */ boolean canAccess(AccessExpression accessExpression); + /** + * @param parsedAccessExpression object resulting from call to AccessExpression.parse. This method + * would be useful if passing a single AccessExpression to multiple AccessEvaluators. + * @return true if the expression is visible using the authorizations supplied at creation, false + * otherwise + */ + boolean canAccess(ParsedAccessExpression parsedAccessExpression); + /** * Creates an AccessEvaluator from an Authorizations object * diff --git a/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java b/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java index d5189c2..6b07a4c 100644 --- a/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java +++ b/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java @@ -19,12 +19,14 @@ package org.apache.accumulo.access.impl; import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.Objects.requireNonNull; import static org.apache.accumulo.access.impl.ByteUtils.BACKSLASH; import static org.apache.accumulo.access.impl.ByteUtils.QUOTE; import static org.apache.accumulo.access.impl.ByteUtils.isQuoteOrSlash; import static org.apache.accumulo.access.impl.ByteUtils.isQuoteSymbol; import java.util.HashSet; +import java.util.List; import java.util.Set; import java.util.function.Predicate; @@ -32,6 +34,7 @@ import org.apache.accumulo.access.AccessExpression; import org.apache.accumulo.access.Authorizations; import org.apache.accumulo.access.InvalidAccessExpressionException; +import org.apache.accumulo.access.ParsedAccessExpression; public final class AccessEvaluatorImpl implements AccessEvaluator { @@ -159,4 +162,60 @@ boolean evaluate(byte[] accessExpression) throws InvalidAccessExpressionExceptio }; return ParserEvaluator.parseAccessExpression(accessExpression, atp, authToken -> true); } + + @Override + public 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 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 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); + if (authToken[0] == '"' && authToken[authToken.length - 1] == '"') { + bytesWrapper.set(authToken, 1, authToken.length - 2); + } else { + bytesWrapper.set(authToken, 0, authToken.length); + } + return authorizedPredicate.test(bytesWrapper); + } + } diff --git a/core/src/main/java/org/apache/accumulo/access/impl/MultiAccessEvaluatorImpl.java b/core/src/main/java/org/apache/accumulo/access/impl/MultiAccessEvaluatorImpl.java index 8c268f8..05d7701 100644 --- a/core/src/main/java/org/apache/accumulo/access/impl/MultiAccessEvaluatorImpl.java +++ b/core/src/main/java/org/apache/accumulo/access/impl/MultiAccessEvaluatorImpl.java @@ -18,8 +18,6 @@ */ package org.apache.accumulo.access.impl; -import static java.nio.charset.StandardCharsets.UTF_8; - import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -28,6 +26,7 @@ import org.apache.accumulo.access.AccessExpression; import org.apache.accumulo.access.Authorizations; import org.apache.accumulo.access.InvalidAccessExpressionException; +import org.apache.accumulo.access.ParsedAccessExpression; public final class MultiAccessEvaluatorImpl implements AccessEvaluator { @@ -46,21 +45,26 @@ private MultiAccessEvaluatorImpl(Collection authorizationSets) { @Override public boolean canAccess(String accessExpression) throws InvalidAccessExpressionException { - return canAccess(accessExpression.getBytes(UTF_8)); + return canAccess(AccessExpression.of(accessExpression)); } @Override public boolean canAccess(byte[] accessExpression) throws InvalidAccessExpressionException { + return canAccess(AccessExpression.of(accessExpression)); + } + + @Override + public boolean canAccess(AccessExpression accessExpression) { + return canAccess(accessExpression.parse()); + } + + @Override + public boolean canAccess(ParsedAccessExpression parsedAccessExpression) { for (AccessEvaluator evaluator : evaluators) { - if (!evaluator.canAccess(accessExpression)) { + if (!evaluator.canAccess(parsedAccessExpression)) { return false; } } return true; } - - @Override - public boolean canAccess(AccessExpression accessExpression) { - return canAccess(accessExpression.getExpression()); - } } From e3d24d3f96a8a9fc7c49b8153fcd10111d2c1777 Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Tue, 23 Dec 2025 16:25:27 +0000 Subject: [PATCH 2/5] Implement PR suggestion --- .../accumulo/access/AccessEvaluator.java | 8 -------- .../accumulo/access/AccessExpression.java | 5 +++++ .../access/impl/AccessEvaluatorImpl.java | 9 ++++++--- .../access/impl/AccessExpressionImpl.java | 4 ++++ .../access/impl/MultiAccessEvaluatorImpl.java | 19 ++++++++++--------- .../impl/ParsedAccessExpressionImpl.java | 5 +++++ 6 files changed, 30 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/access/AccessEvaluator.java b/core/src/main/java/org/apache/accumulo/access/AccessEvaluator.java index 39c7b63..ea0fb2c 100644 --- a/core/src/main/java/org/apache/accumulo/access/AccessEvaluator.java +++ b/core/src/main/java/org/apache/accumulo/access/AccessEvaluator.java @@ -80,14 +80,6 @@ public sealed interface AccessEvaluator permits AccessEvaluatorImpl, MultiAccess */ boolean canAccess(AccessExpression accessExpression); - /** - * @param parsedAccessExpression object resulting from call to AccessExpression.parse. This method - * would be useful if passing a single AccessExpression to multiple AccessEvaluators. - * @return true if the expression is visible using the authorizations supplied at creation, false - * otherwise - */ - boolean canAccess(ParsedAccessExpression parsedAccessExpression); - /** * Creates an AccessEvaluator from an Authorizations object * diff --git a/core/src/main/java/org/apache/accumulo/access/AccessExpression.java b/core/src/main/java/org/apache/accumulo/access/AccessExpression.java index cf32670..e120934 100644 --- a/core/src/main/java/org/apache/accumulo/access/AccessExpression.java +++ b/core/src/main/java/org/apache/accumulo/access/AccessExpression.java @@ -182,6 +182,11 @@ public static ParsedAccessExpression parse(byte[] expression) return ParsedAccessExpressionImpl.parseExpression(Arrays.copyOf(expression, expression.length)); } + /** + * @return true if {@link AccessExpression#parse} has been called on this object false if not. + */ + public abstract boolean isParsed(); + /** * Validates an access expression and returns an immutable object with a parse tree. Creating the * parse tree is expensive relative to calling {@link #of(String)} or {@link #validate(String)}, diff --git a/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java b/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java index 6b07a4c..4b8e3e6 100644 --- a/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java +++ b/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java @@ -141,7 +141,11 @@ public static byte[] escape(byte[] auth, boolean shouldQuote) { @Override public boolean canAccess(AccessExpression expression) { - return canAccess(expression.getExpression()); + if (expression.isParsed()) { + return canAccess(expression.parse()); + } else { + return canAccess(expression.getExpression()); + } } @Override @@ -163,8 +167,7 @@ boolean evaluate(byte[] accessExpression) throws InvalidAccessExpressionExceptio return ParserEvaluator.parseAccessExpression(accessExpression, atp, authToken -> true); } - @Override - public boolean canAccess(ParsedAccessExpression pae) { + private boolean canAccess(ParsedAccessExpression pae) { switch (pae.getType()) { case AND: return canAccessParsedAnd(pae.getChildren()); diff --git a/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java b/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java index 9f60e33..fe3adac 100644 --- a/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java +++ b/core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java @@ -48,6 +48,10 @@ public String getExpression() { return expression; } + public boolean isParsed() { + return parseTreeRef.get() != null; + } + @Override public ParsedAccessExpression parse() { ParsedAccessExpression parseTree = parseTreeRef.get(); diff --git a/core/src/main/java/org/apache/accumulo/access/impl/MultiAccessEvaluatorImpl.java b/core/src/main/java/org/apache/accumulo/access/impl/MultiAccessEvaluatorImpl.java index 05d7701..6faab46 100644 --- a/core/src/main/java/org/apache/accumulo/access/impl/MultiAccessEvaluatorImpl.java +++ b/core/src/main/java/org/apache/accumulo/access/impl/MultiAccessEvaluatorImpl.java @@ -18,6 +18,8 @@ */ package org.apache.accumulo.access.impl; +import static java.nio.charset.StandardCharsets.UTF_8; + import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -26,7 +28,6 @@ import org.apache.accumulo.access.AccessExpression; import org.apache.accumulo.access.Authorizations; import org.apache.accumulo.access.InvalidAccessExpressionException; -import org.apache.accumulo.access.ParsedAccessExpression; public final class MultiAccessEvaluatorImpl implements AccessEvaluator { @@ -45,23 +46,23 @@ private MultiAccessEvaluatorImpl(Collection authorizationSets) { @Override public boolean canAccess(String accessExpression) throws InvalidAccessExpressionException { - return canAccess(AccessExpression.of(accessExpression)); + return canAccess(accessExpression.getBytes(UTF_8)); } @Override public boolean canAccess(byte[] accessExpression) throws InvalidAccessExpressionException { - return canAccess(AccessExpression.of(accessExpression)); + for (AccessEvaluator evaluator : evaluators) { + if (!evaluator.canAccess(accessExpression)) { + return false; + } + } + return true; } @Override public boolean canAccess(AccessExpression accessExpression) { - return canAccess(accessExpression.parse()); - } - - @Override - public boolean canAccess(ParsedAccessExpression parsedAccessExpression) { for (AccessEvaluator evaluator : evaluators) { - if (!evaluator.canAccess(parsedAccessExpression)) { + if (!evaluator.canAccess(accessExpression)) { return false; } } diff --git a/core/src/main/java/org/apache/accumulo/access/impl/ParsedAccessExpressionImpl.java b/core/src/main/java/org/apache/accumulo/access/impl/ParsedAccessExpressionImpl.java index 99f6464..833b444 100644 --- a/core/src/main/java/org/apache/accumulo/access/impl/ParsedAccessExpressionImpl.java +++ b/core/src/main/java/org/apache/accumulo/access/impl/ParsedAccessExpressionImpl.java @@ -174,4 +174,9 @@ private static ParsedAccessExpressionImpl parseExpression(Tokenizer tokenizer, return new ParsedAccessExpressionImpl(auth.data, auth.start, auth.len); } } + + @Override + public boolean isParsed() { + return true; + } } From 965a42cfa8f7eba69ed97c86a509e30b49810baa Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Tue, 23 Dec 2025 17:45:41 +0000 Subject: [PATCH 3/5] Added new methods to benchmark --- .../tests/AccessExpressionBenchmark.java | 67 +++++++++++++++++-- 1 file changed, 60 insertions(+), 7 deletions(-) diff --git a/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java b/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java index 64de076..c7e09d0 100644 --- a/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java +++ b/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java @@ -31,6 +31,7 @@ import org.apache.accumulo.access.AccessEvaluator; import org.apache.accumulo.access.AccessExpression; import org.apache.accumulo.access.Authorizations; +import org.apache.accumulo.access.ParsedAccessExpression; import org.apache.accumulo.core.security.ColumnVisibility; import org.apache.accumulo.core.security.VisibilityEvaluator; import org.apache.accumulo.core.security.VisibilityParseException; @@ -73,7 +74,13 @@ public static class VisibilityEvaluatorTests { public static class EvaluatorTests { AccessEvaluator evaluator; - List expressions; + List byteExpressions; + + List strExpressions; + + List accessExpressions; + + List parsedExpressions; } @State(Scope.Benchmark) @@ -116,7 +123,10 @@ public void loadData() throws IOException { // Create new EvaluatorTests et = new EvaluatorTests(); - et.expressions = new ArrayList<>(); + et.byteExpressions = new ArrayList<>(); + et.strExpressions = new ArrayList<>(); + et.accessExpressions = new ArrayList<>(); + et.parsedExpressions = new ArrayList<>(); if (testDataSet.auths.length == 1) { et.evaluator = AccessEvaluator.of(Authorizations.of(Set.of(testDataSet.auths[0]))); @@ -132,7 +142,11 @@ public void loadData() throws IOException { allTestExpressionsStr.add(exp); byte[] byteExp = exp.getBytes(UTF_8); allTestExpressions.add(byteExp); - et.expressions.add(byteExp); + et.byteExpressions.add(byteExp); + et.strExpressions.add(exp); + AccessExpression ae = AccessExpression.of(byteExp); + et.accessExpressions.add(ae); + et.parsedExpressions.add(ae.parse()); vet.expressions.add(byteExp); vet.columnVisibilities.add(new ColumnVisibility(byteExp)); } @@ -183,13 +197,52 @@ public void measureStringValidation(BenchmarkState state, Blackhole blackhole) { } /** - * Measures the time it takes to parse and evaluate an expression. This has to create the parse - * tree an operate on it. + * Measures the time it takes to parse and evaluate an expression byte[]. This has to create the + * parse tree an operate on it. */ @Benchmark - public void measureParseAndEvaluation(BenchmarkState state, Blackhole blackhole) { + public void measureBytesParseAndEvaluation(BenchmarkState state, Blackhole blackhole) { for (EvaluatorTests evaluatorTests : state.getEvaluatorTests()) { - for (byte[] expression : evaluatorTests.expressions) { + for (byte[] expression : evaluatorTests.byteExpressions) { + blackhole.consume(evaluatorTests.evaluator.canAccess(expression)); + } + } + } + + /** + * Measures the time it takes to parse and evaluate an expression String. This has to create the + * parse tree an operate on it. + */ + @Benchmark + public void measureStringParseAndEvaluation(BenchmarkState state, Blackhole blackhole) { + for (EvaluatorTests evaluatorTests : state.getEvaluatorTests()) { + for (String expression : evaluatorTests.strExpressions) { + blackhole.consume(evaluatorTests.evaluator.canAccess(expression)); + } + } + } + + /** + * Measures the time it takes to parse and evaluate an AccessExpression. This has to create the + * parse tree an operate on it. + */ + @Benchmark + public void measureExpressionEvaluation(BenchmarkState state, Blackhole blackhole) { + for (EvaluatorTests evaluatorTests : state.getEvaluatorTests()) { + for (ParsedAccessExpression expression : evaluatorTests.parsedExpressions) { + blackhole.consume(evaluatorTests.evaluator.canAccess(expression)); + } + } + } + + /** + * Measures the time it takes to evaluate a ParsedAccessExpression. This evaluates the existing + * parse tree. + */ + @Benchmark + public void measureParsedEvaluation(BenchmarkState state, Blackhole blackhole) { + for (EvaluatorTests evaluatorTests : state.getEvaluatorTests()) { + for (ParsedAccessExpression expression : evaluatorTests.parsedExpressions) { blackhole.consume(evaluatorTests.evaluator.canAccess(expression)); } } From 78d44f037657e579db76e0e49f449c4d58236102 Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Tue, 23 Dec 2025 18:14:52 +0000 Subject: [PATCH 4/5] Fix bug in new benchmark method --- .../apache/accumulo/access/tests/AccessExpressionBenchmark.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java b/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java index c7e09d0..ff4381a 100644 --- a/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java +++ b/core/src/test/java/org/apache/accumulo/access/tests/AccessExpressionBenchmark.java @@ -229,7 +229,7 @@ public void measureStringParseAndEvaluation(BenchmarkState state, Blackhole blac @Benchmark public void measureExpressionEvaluation(BenchmarkState state, Blackhole blackhole) { for (EvaluatorTests evaluatorTests : state.getEvaluatorTests()) { - for (ParsedAccessExpression expression : evaluatorTests.parsedExpressions) { + for (AccessExpression expression : evaluatorTests.accessExpressions) { blackhole.consume(evaluatorTests.evaluator.canAccess(expression)); } } From ee500965a754dcd0ce4ef3cf1dcda3c4e8274b36 Mon Sep 17 00:00:00 2001 From: Dave Marion Date: Wed, 7 Jan 2026 21:21:34 +0000 Subject: [PATCH 5/5] Implement short-circuit suggestion --- .../apache/accumulo/access/impl/AccessEvaluatorImpl.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java b/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java index 4b8e3e6..a711d55 100644 --- a/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java +++ b/core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java @@ -203,11 +203,12 @@ private boolean canAccessParsedOr(List children) { if (children.isEmpty()) { throw new IllegalArgumentException("Malformed OR expression, no children"); } - boolean result = false; for (ParsedAccessExpression child : children) { - result |= canAccess(child); + if (canAccess(child)) { + return true; + } } - return result; + return false; } private boolean canAccessParsedAuthorization(String token) {