add DeepCopy to ColumnVisibility #5217
Conversation
|
I think this looks ok, but have added @keith-turner as a reviewer. He has recently modified newer versions of this code. The Node and parse tree are deprecated in 3.1 and will be removed in 4.0 and has been replaced by the AccessExpression in github.com/apache/accumulo-access. I also reviewed the commit history to see if there was a clone or deepCopy method that used to exist, but was removed for some reason, and did not see anything. |
|
Looked at 3.1 and 4.0 version of Accumulo to see how this change would merge forward. While doing that I realized the deprecated code could be removed in 4.0 in ColumnVisibility. Removing that would influence how this would merge forward which would help thinking through these changes. I will try to remove the deprecated code soon to see how that may influence these changes. |
|
Opened #5232 to remove the deprecated column visibility code in 4.0 |
jalphonso
force-pushed
the
task/deepcopy-columnvis
branch
from
d73a47d to
93942d0
Compare
nice |
Thanks Dave |
Thanks Keith. I refactored the code to use the copy constructor instead of deepCopy to keep in line with the API. I do agree it looks cleaner this way and keeps the guarantee that the expression has been previously validated. |
| @@ -505,6 +516,12 @@ public ColumnVisibility(byte[] expression) { | |||
| validate(expression); | |||
| } | |||
|
|
|||
| public ColumnVisibility(ColumnVisibility visibility) { | |||
There was a problem hiding this comment.
This needs javadoc with a @since 2.1.4 tag. Would also be nice to mention in the javadoc that deep copy is done.
| @@ -125,6 +125,17 @@ public Node(int start, int end) { | |||
| this.end = end; | |||
| } | |||
|
|
|||
| public Node(Node node) { | |||
There was a problem hiding this comment.
This also needs the javadoc since tag and could document that a deep copy is done.
jalphonso
force-pushed
the
task/deepcopy-columnvis
branch
from
d6765f1 to
a53da7d
Compare
| List<Node> childrenNew = new ArrayList<>(node.children.size()); | ||
| for (Node child : node.children) { | ||
| childrenNew.add(new Node(child)); | ||
| } |
There was a problem hiding this comment.
Looked over the existing code and noticed it had some handling to avoid allocating empty list in the nodes. Can do that here also.
| List<Node> childrenNew = new ArrayList<>(node.children.size()); | |
| for (Node child : node.children) { | |
| childrenNew.add(new Node(child)); | |
| } | |
| List<Node> childrenNew = node.children.isEmpty() ? EMPTY : new ArrayList<>(node.children.size()); | |
| for (Node child : node.children) { | |
| childrenNew.add(new Node(child)); | |
| } |
Added in DeepCopy methods for ColumnVisibility and the related Node class to save processing time in datawave.