Draft
Conversation
jalphonso
force-pushed
the
accumulo-access
branch
8 times, most recently
from
e86e7ba to
0e0b4d2
Compare
ddanielr
reviewed
Mar 26, 2026
jalphonso
force-pushed
the
accumulo-access
branch
3 times, most recently
from
07765cd to
65db1ad
Compare
jalphonso
force-pushed
the
accumulo-access
branch
5 times, most recently
from
8bc250c to
98f8567
Compare
jalphonso
force-pushed
the
accumulo-access
branch
from
98f8567 to
b07ead2
Compare
jsduncan2
reviewed
Apr 7, 2026
Comment on lines
+40
to
+43
| if (expr.length == 0) { | ||
| return EMPTY_EXPRESSION; | ||
| } | ||
| return ACCESS.newExpression(new String(expr, UTF_8)); |
Collaborator
There was a problem hiding this comment.
Could we delegate to toAccessExpression(byte[] visibilityBytes)?
Suggested change
| if (expr.length == 0) { | |
| return EMPTY_EXPRESSION; | |
| } | |
| return ACCESS.newExpression(new String(expr, UTF_8)); | |
| return toAccessExpression(expr); |
jalphonso
force-pushed
the
accumulo-access
branch
2 times, most recently
from
cceab28 to
e72fd54
Compare
jsduncan2
reviewed
Apr 8, 2026
Comment on lines
+158
to
+161
| @SuppressWarnings({"unchecked", "rawtypes"}) | ||
| public static Markings<?> combine(MarkingFunctions<?> markingFunctions, Collection<? extends Markings<?>> markings) throws MarkingFunctions.Exception { | ||
| return ((MarkingFunctions) markingFunctions).combine(markings); | ||
| } |
Collaborator
There was a problem hiding this comment.
I think we can be a little more specific with generics here:
Suggested change
| @SuppressWarnings({"unchecked", "rawtypes"}) | |
| public static Markings<?> combine(MarkingFunctions<?> markingFunctions, Collection<? extends Markings<?>> markings) throws MarkingFunctions.Exception { | |
| return ((MarkingFunctions) markingFunctions).combine(markings); | |
| } | |
| public static <T extends Markings<?>, U extends MarkingFunctions<T>> T combine(U markingFunctions, Collection<T> markings) throws MarkingFunctions.Exception { | |
| return markingFunctions.combine(markings); | |
| } |
jsduncan2
reviewed
Apr 8, 2026
Comment on lines
+178
to
+181
| if (markings1 instanceof AccessExpressionMarkings && markings2 instanceof AccessExpressionMarkings) { | ||
| AccessExpressionMarkings aem1 = (AccessExpressionMarkings) markings1; | ||
| AccessExpressionMarkings aem2 = (AccessExpressionMarkings) markings2; | ||
| return combine(markingFunctions, List.of(aem1, aem2)); |
Collaborator
There was a problem hiding this comment.
If the above generic suggestion works, i think you'll need to modify this slightly:
Suggested change
| if (markings1 instanceof AccessExpressionMarkings && markings2 instanceof AccessExpressionMarkings) { | |
| AccessExpressionMarkings aem1 = (AccessExpressionMarkings) markings1; | |
| AccessExpressionMarkings aem2 = (AccessExpressionMarkings) markings2; | |
| return combine(markingFunctions, List.of(aem1, aem2)); | |
| if (markingFunctions instanceof MarkingFunctions.Default && markings1 instanceof AccessExpressionMarkings && markings2 instanceof AccessExpressionMarkings) { | |
| MarkingFunctions<AccessExpressionMarkings> defaultMarkingFunctions = (MarkingFunctions.Default) markingFunctions; | |
| AccessExpressionMarkings aem1 = (AccessExpressionMarkings) markings1; | |
| AccessExpressionMarkings aem2 = (AccessExpressionMarkings) markings2; | |
| return combine(defaultMarkingFunctions, List.of(aem1, aem2)); |
jsduncan2
reviewed
Apr 8, 2026
| * Many modules still require {@link ColumnVisibility} for the native Accumulo Key/Mutation API, while the marking functions layer has migrated to | ||
| * {@link AccessExpression}. This utility provides zero-copy-where-possible conversions between the two. | ||
| */ | ||
| public final class AccessExpressionUtil { |
Collaborator
There was a problem hiding this comment.
This class could use some unit tests. Let me know if you'd like some help making them.
jsduncan2
reviewed
Apr 8, 2026
| import com.fasterxml.jackson.annotation.JsonTypeInfo; | ||
|
|
||
| @JsonTypeInfo(use = JsonTypeInfo.Id.CLASS) | ||
| public interface Markings<T> { |
Collaborator
There was a problem hiding this comment.
Can we restrict T here at all?
jsduncan2
reviewed
Apr 9, 2026
Comment on lines
5
to
10
| public interface HasMarkings { | ||
|
|
||
| void setMarkings(Map<String,String> markings); | ||
| void setMarkings(Markings<?> markings); | ||
|
|
||
| Map<String,String> getMarkings(); | ||
| Markings<?> getMarkings(); | ||
| } |
Collaborator
There was a problem hiding this comment.
I'm guessing there's some issue you'll eventually run into, but here's a naive suggestion (which will require a lot of associated changes, some of which may be impossible):
public interface HasMarkings <T> {
void setMarkings(Markings<T> markings);
Markings<T> getMarkings();
}
jschmidt10
reviewed
Apr 9, 2026
| return new ColumnVisibility(new ColumnVisibility(expressions.stream().map(ColumnVisibility::flatten).filter(b -> b.length > 0) | ||
| .map(b -> "(" + new String(b, UTF_8) + ")").collect(Collectors.joining("&")).getBytes(UTF_8)).flatten()); | ||
| @Override | ||
| public ColumnVisibility combineVisibilities(Collection<ColumnVisibility> visibilities) { |
Collaborator
There was a problem hiding this comment.
Just curious, why maintain this method vs deleting?
Collaborator
Author
There was a problem hiding this comment.
There is extensive usage in the HS code
jalphonso
force-pushed
the
accumulo-access
branch
from
db7ddfe to
7e909ed
Compare
jalphonso
force-pushed
the
accumulo-access
branch
3 times, most recently
from
b95c774 to
5e3c97e
Compare
added 23 commits
April 21, 2026 12:40
Conflicts:
microservices/services/config/pom.xml
warehouse/query-core/src/main/java/datawave/query/tables/shard/CountAggregatingIterator.java
jalphonso
force-pushed
the
accumulo-access
branch
from
6033cf1 to
3211c05
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.