You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "meatballspaghetti (via GitHub)" <gi...@apache.org> on 2024/08/08 13:49:44 UTC
[PR] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
meatballspaghetti opened a new pull request, #4791:
URL: https://github.com/apache/accumulo/pull/4791
This PR attempts to address all String-to-bytes and bytes-to-String conversions so they all consistently use UTF-8 (see more commentary in #3815). Being a follow-on to #3815, this PR mostly makes changes to test code that had not previously been covered.
The following fixes have been made:
- `getBytes()` method: `String.getBytes()` -> `String.getBytes(UTF_8)`
- String construction from byte array: `new String(byte[])` -> `new String(byte[], UTF_8)`
Fixes #4765 "Fix String-to-bytes and bytes-toString for UTF-8 for all remaining code"
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #4791:
URL: https://github.com/apache/accumulo/pull/4791#issuecomment-2294204377
> > Looks good to me, but since this is for 2.1, I'll wait to merge it until I can be sure I'm prepared to merge it into the main branch and elasticity and work through any merge conflicts that might arise.
>
> Sorry I did not see this before merging into 2.1. Just saw the approvals and went ahead and merged it. I will attempt to merge it up into main and elasticity. Were there any specific issues or difficulties you foresaw with the merge or did you just mean you didn't have the time to do it when the comment was made so you were planning on doing it later?
I just didn't have time to do it when I reviewed it, and was planning on doing it later, but if it's already done, then that's not a problem at all!
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "meatballspaghetti (via GitHub)" <gi...@apache.org>.
meatballspaghetti commented on code in PR #4791:
URL: https://github.com/apache/accumulo/pull/4791#discussion_r1714365194
##########
core/src/test/java/org/apache/accumulo/core/data/KeyTest.java:
##########
@@ -39,10 +40,14 @@ public class KeyTest {
@Test
public void testDeletedCompare() {
- Key k1 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
- Key k2 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
- Key k3 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, true);
- Key k4 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, true);
+ Key k1 = new Key("r1".getBytes(UTF_8), "cf".getBytes(UTF_8), "cq".getBytes(UTF_8), new byte[0],
Review Comment:
There are assertEquals statements a couple lines below to confirm that each pair of identical Key objects are indeed identical (they are not used outside of this particular unit test). If I reduce these four variables to two, those assertEquals statements would no longer apply. Should I proceed with that and delete the assertions, or leave all as is?
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "meatballspaghetti (via GitHub)" <gi...@apache.org>.
meatballspaghetti commented on code in PR #4791:
URL: https://github.com/apache/accumulo/pull/4791#discussion_r1715925939
##########
core/src/test/java/org/apache/accumulo/core/data/KeyTest.java:
##########
@@ -39,10 +40,14 @@ public class KeyTest {
@Test
public void testDeletedCompare() {
- Key k1 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
- Key k2 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
- Key k3 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, true);
- Key k4 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, true);
+ Key k1 = new Key("r1".getBytes(UTF_8), "cf".getBytes(UTF_8), "cq".getBytes(UTF_8), new byte[0],
Review Comment:
I see what you're saying, I agree with it. I can implement/push the change once I can connect to an instance again.
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #4791:
URL: https://github.com/apache/accumulo/pull/4791#discussion_r1711947691
##########
core/src/test/java/org/apache/accumulo/core/data/KeyTest.java:
##########
@@ -39,10 +40,14 @@ public class KeyTest {
@Test
public void testDeletedCompare() {
- Key k1 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
- Key k2 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
- Key k3 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, true);
- Key k4 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, true);
+ Key k1 = new Key("r1".getBytes(UTF_8), "cf".getBytes(UTF_8), "cq".getBytes(UTF_8), new byte[0],
Review Comment:
Even though its not exactly in the scope of this PR, it could be nice to refactor these all to use a new shared variable since they are all the same.
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "meatballspaghetti (via GitHub)" <gi...@apache.org>.
meatballspaghetti commented on PR #4791:
URL: https://github.com/apache/accumulo/pull/4791#issuecomment-2291997745
> Changes look good and consistent with #3815.
>
> One thing that may have been missed is changing instances of the method ref equivalents: `String::new` and `String::getBytes` use UTF_8. I found instances of `String::new` that could be changed, but no instances of `String::getBytes`.
Maybe I'm misunderstanding, but `String::new` just creates an empty String with no encoding involved and no conversion from bytes. Are you suggesting that I convert each instance of `String::new` to `s -> s.getBytes(UTF_8)`? Since they're just empty Strings to being with, I wasn't sure if that would be applicable here.
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "meatballspaghetti (via GitHub)" <gi...@apache.org>.
meatballspaghetti commented on PR #4791:
URL: https://github.com/apache/accumulo/pull/4791#issuecomment-2292128755
@kevinrr888 Makes sense, thanks. Updated.
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo merged PR #4791:
URL: https://github.com/apache/accumulo/pull/4791
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #4791:
URL: https://github.com/apache/accumulo/pull/4791#discussion_r1715414105
##########
core/src/test/java/org/apache/accumulo/core/data/KeyTest.java:
##########
@@ -39,10 +40,14 @@ public class KeyTest {
@Test
public void testDeletedCompare() {
- Key k1 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
- Key k2 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
- Key k3 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, true);
- Key k4 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, true);
+ Key k1 = new Key("r1".getBytes(UTF_8), "cf".getBytes(UTF_8), "cq".getBytes(UTF_8), new byte[0],
Review Comment:
Sorry I was pretty vague in my comment. I meant making new variables for the row, cf and cq that are used to construct the four `Key`s. For example:
```java
final byte[] row = "r1".getBytes(UTF_8);
final byte[] cf = "cf".getBytes(UTF_8);
final byte[] cq = "cq".getBytes(UTF_8);
Key k2 = new Key(row, cf, cq, new byte[0], 0, false);
```
Either way I think the code is fine as is and no changes need to made unless you want to.
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "meatballspaghetti (via GitHub)" <gi...@apache.org>.
meatballspaghetti commented on code in PR #4791:
URL: https://github.com/apache/accumulo/pull/4791#discussion_r1718863905
##########
core/src/test/java/org/apache/accumulo/core/data/KeyTest.java:
##########
@@ -39,10 +40,14 @@ public class KeyTest {
@Test
public void testDeletedCompare() {
- Key k1 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
- Key k2 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, false);
- Key k3 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, true);
- Key k4 = new Key("r1".getBytes(), "cf".getBytes(), "cq".getBytes(), new byte[0], 0, true);
+ Key k1 = new Key("r1".getBytes(UTF_8), "cf".getBytes(UTF_8), "cq".getBytes(UTF_8), new byte[0],
Review Comment:
Done, updated.
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on PR #4791:
URL: https://github.com/apache/accumulo/pull/4791#issuecomment-2292067193
@meatballspaghetti - Here is an example of what I'm talking about from `TokenFileIT`:
```
int i = scanner.stream().map(Map.Entry::getValue).map(Value::get).map(String::new)
.map(Integer::parseInt).collect(onlyElement());
```
Here, `String::new` is used as a method reference to the `String` constructor that takes `byte[]` as its arg since `Value::get` returns `byte[]`. I'm suggesting the above (and elsewhere this applies) be changed to
```
int i = scanner.stream().map(Map.Entry::getValue).map(Value::get).map(e -> new String(e, UTF_8))
.map(Integer::parseInt).collect(onlyElement());
```
--
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] Fix String-to-bytes and bytes-to-String for UTF-8 Pt. 2 [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #4791:
URL: https://github.com/apache/accumulo/pull/4791#issuecomment-2293783052
> Looks good to me, but since this is for 2.1, I'll wait to merge it until I can be sure I'm prepared to merge it into the main branch and elasticity and work through any merge conflicts that might arise.
Sorry I did not see this before merging into 2.1. Just saw the approvals and went ahead and merged it. I will attempt to merge it up into main and elasticity. Were there any specific issues or difficulties you foresaw with the merge or did you just mean you didn't have the time to do it when the comment was made so you were planning on doing it later?
--
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