You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "Amemeda (via GitHub)" <gi...@apache.org> on 2026/07/07 19:07:14 UTC
[PR] Adding new Mutation constructor with ByteSequence parameter to remove 1 TODO [accumulo]
Amemeda opened a new pull request, #6463:
URL: https://github.com/apache/accumulo/pull/6463
Removing '# TODO: these test resources should be moved into the correct package for the module' from ConditionalMutation.java
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding new Mutation constructor with ByteSequence parameter to remove 1 TODO [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6463:
URL: https://github.com/apache/accumulo/pull/6463#discussion_r3552611463
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -230,6 +248,26 @@ public Mutation(CharSequence row, int initialBufferSize) {
*/
public Mutation() {}
+ /**
+ * Creates a mutation with the specified row, This constructor creates a copy of the fields.
+ */
+ public Mutation(ByteSequence row) {
+ byte[] rowBytes;
+ int rowOffset;
+ int rowLen;
+
+ if (row.isBackedByArray()) {
+ rowBytes = row.getBackingArray();
+ rowOffset = row.offset();
+ } else {
+ rowBytes = row.toArray();
+ rowOffset = 0;
+ }
+ rowLen = row.length();
+
+ init(rowBytes, rowOffset, rowLen, true);
Review Comment:
I think there is a bug here. I think init() and copyIfNeeded() will only copy the row bytes leaving the other fields not set (`buffer` and `estRowAndLargeValSize`). I think we can just use the existing byte-array constructor which sets things up properly:
```suggestion
this(row.isBackedByArray() ? row.getBackingArray() : row.toArray(),
row.isBackedByArray() ? row.offset() : 0, row.length());
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding new Mutation constructor with ByteSequence parameter to remove 1 TODO [accumulo]
Posted by "Amemeda (via GitHub)" <gi...@apache.org>.
Amemeda commented on PR #6463:
URL: https://github.com/apache/accumulo/pull/6463#issuecomment-4907684472
Removes 1 TODO from #2699
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding new Mutation constructor with ByteSequence parameter to remove 1 TODO [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6463:
URL: https://github.com/apache/accumulo/pull/6463#discussion_r3552611463
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -230,6 +248,26 @@ public Mutation(CharSequence row, int initialBufferSize) {
*/
public Mutation() {}
+ /**
+ * Creates a mutation with the specified row, This constructor creates a copy of the fields.
+ */
+ public Mutation(ByteSequence row) {
+ byte[] rowBytes;
+ int rowOffset;
+ int rowLen;
+
+ if (row.isBackedByArray()) {
+ rowBytes = row.getBackingArray();
+ rowOffset = row.offset();
+ } else {
+ rowBytes = row.toArray();
+ rowOffset = 0;
+ }
+ rowLen = row.length();
+
+ init(rowBytes, rowOffset, rowLen, true);
Review Comment:
I think there is a bug here. I think init() and copyIfNeeded() will only copy the row bytes leaving the other fields not set (`buffer` and `estRowAndLargeValSize`). I think we can just use the existing byte-array constructor which sets things up properly:
edit: code suggestion in another comment
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove 1 TODO from ConditionalMutation.java [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #6463:
URL: https://github.com/apache/accumulo/pull/6463#issuecomment-4928435403
This probably doesn't need to be done and the TODO can just be removed. Updating the public API creates churn that can be disruptive and more to maintain. I can see that it might have been worth doing this if somehow Mutation was altered to keep a reference to the ByteSequence and used that when it was serialized, instead of merely moving the toArray into a constructor, in order to possibly avoid an array copy, but it's not really necessary, and easier to just remove the TODO.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding new Mutation constructor with ByteSequence parameter to remove 1 TODO [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6463:
URL: https://github.com/apache/accumulo/pull/6463#discussion_r3551491221
##########
core/src/main/java/org/apache/accumulo/core/data/ConditionalMutation.java:
##########
@@ -60,7 +60,6 @@ public ConditionalMutation(CharSequence row, Condition... conditions) {
}
public ConditionalMutation(ByteSequence row, Condition... conditions) {
- // TODO add ByteSequence methods to mutations
super(row.toArray());
Review Comment:
I'm assuming the new constructor would be called here too. I'm curious how many places in the code would take advantage of the new constructor for the amount of code that it adds to Mutation. If there are not many places, then I think it might be better to just remove the TODO.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding new Mutation constructor with ByteSequence parameter to remove 1 TODO [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6463:
URL: https://github.com/apache/accumulo/pull/6463#discussion_r3552767791
##########
core/src/main/java/org/apache/accumulo/core/data/Mutation.java:
##########
@@ -230,6 +248,26 @@ public Mutation(CharSequence row, int initialBufferSize) {
*/
public Mutation() {}
+ /**
+ * Creates a mutation with the specified row, This constructor creates a copy of the fields.
+ */
+ public Mutation(ByteSequence row) {
+ byte[] rowBytes;
+ int rowOffset;
+ int rowLen;
+
+ if (row.isBackedByArray()) {
+ rowBytes = row.getBackingArray();
+ rowOffset = row.offset();
+ } else {
+ rowBytes = row.toArray();
+ rowOffset = 0;
+ }
+ rowLen = row.length();
+
+ init(rowBytes, rowOffset, rowLen, true);
Review Comment:
```suggestion
this(row.isBackedByArray() ? row.getBackingArray() : row.toArray(),
row.isBackedByArray() ? row.offset() : 0, row.length());
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove 1 TODO from ConditionalMutation.java [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #6463:
URL: https://github.com/apache/accumulo/pull/6463
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org