dlmarion commented on PR #4206: URL: https://github.com/apache/accumulo/pull/4206#issuecomment-1919564002
> * Should we deprecate ColumnVisibility.quote()? There are two instances of that method (one for string and one for byte[]). This branch deprecates one of those. When I merged I removed the single deprecation and threw in a TODO. If we want to deprecate, thinking we should do both or neither. I made a comment in apache/accumulo#3746 about this > * This branch had ColumnVis wrap and AccessExpression. In [updates column visibility to use accumulo access library #3746](https://github.com/apache/accumulo/pull/3746) ColVis was still wrapping a byte array instead of an AccessExpress. I kept the byte[] to avoid going from byte[]->String->byte[] in some cases. Also avoid an extra object allocation in some cases. However I am not sure about this. Wrapping the AccessExpression seems cleaner, but may be inefficient in some circumstances w/ more copies and object allocations. I liked the simplicity of ColVis wrapping AccessExpression. The VisibilityEvaluator is changing to use AccessEvaluator, so the byte[] in ColVis *must* be a valid AccessExpression. In ColVis(byte[]) constructor, you are calling `AccessExpression.validate`, so I'm not sure that you are saving much w/r/t object creation. I wonder if the answer is for ColVis to have `AccessExpression` and `byte[]` members that are set in the constructor to avoid the runtime cost of calling `AccessExpression.getExpression`. > * The existing changes in [updates column visibility to use accumulo access library #3746](https://github.com/apache/accumulo/pull/3746) did not need to add a new List<byte[]> constructor to AccessEvaluator, so I kept those. So if we want to keep those, then may not need to add that method in accumulo-access. Fine by me, whatever works. -- 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]
