You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/04/22 16:02:47 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request, #2646: Replace iterator usage
DomGarguilo opened a new pull request, #2646:
URL: https://github.com/apache/accumulo/pull/2646
I was looking through places where the use of `Scanner.Iterator` might be improved by using the new `Scanner.stream` method and found a few patterns to replace that seemed worth-while.
* In a lot of places, we create an interator, assert that the iterator `hasNext`, get the next entry, compare that entry to what we expect it to be, then assert that `hasNext` is false. All this to make sure there is just one entry. To simplify I changed these places to `.stream().collect(onlyElement())` and then perform the `assertEquals` on the single returned value.
* Convert `Iterators.size(s.iterator()) == 0` -> `s.stream().findAny().isEmpty()`
* Convert `Iterators.size(s.iterator()) > 0` -> `s.stream().findAny().isPresent()`
I also wanted to see if anyone had an opinion on which of the following equivalent options is better:
1. `scanner.stream().map(Entry::getValue).map(Value::get).map(String::new).map(Integer::parseInt).collect(onlyElement());`
2. `scanner.stream().map(entry -> Integer.parseInt(new String(entry.getValue().get()))).collect(onlyElement());`
I have both in these changes. To me, one does not seem significantly, different/more readable than the other so I thought I would ask.
--
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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2646: Replace iterator usage
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2646:
URL: https://github.com/apache/accumulo/pull/2646#discussion_r857905995
##########
test/src/main/java/org/apache/accumulo/test/mapreduce/AccumuloOutputFormatIT.java:
##########
@@ -164,11 +162,9 @@ public void testMR() throws Exception {
assertNull(e1);
try (Scanner scanner = c.createScanner(table2, new Authorizations())) {
- Iterator<Entry<Key,Value>> iter = scanner.iterator();
- assertTrue(iter.hasNext());
- Entry<Key,Value> entry = iter.next();
- assertEquals(Integer.parseInt(new String(entry.getValue().get())), 100);
- assertFalse(iter.hasNext());
+ int actual = scanner.stream().map(Entry::getValue).map(Value::get).map(String::new)
+ .map(Integer::parseInt).collect(onlyElement());
Review Comment:
I've used both. I prefer option 1, if all things are equal, but use option 2 on a case-by-case basis if it's shorter, more clear, or have reason to think it might perform better.
--
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
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2646: Replace iterator usage
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2646:
URL: https://github.com/apache/accumulo/pull/2646#discussion_r857265707
##########
test/src/main/java/org/apache/accumulo/test/functional/RowDeleteIT.java:
##########
@@ -105,8 +106,7 @@ public void run() throws Exception {
}
try (Scanner scanner = c.createScanner(tableName, Authorizations.EMPTY)) {
- count = Iterators.size(scanner.iterator());
- assertEquals(0, count, "count == " + count);
+ assertTrue(scanner.stream().findAny().isEmpty());
bw.close();
Review Comment:
Your change looks fine, but it looks like there's something weird with the existing `bw.close()`. It won't be closed if the assertion fails. The BatchWriter should be in a try-with-resources block of its own, wrapping this scanner block and other stuff above here. That way, it's always closed.
##########
test/src/main/java/org/apache/accumulo/test/mapreduce/AccumuloOutputFormatIT.java:
##########
@@ -164,11 +162,9 @@ public void testMR() throws Exception {
assertNull(e1);
try (Scanner scanner = c.createScanner(table2, new Authorizations())) {
- Iterator<Entry<Key,Value>> iter = scanner.iterator();
- assertTrue(iter.hasNext());
- Entry<Key,Value> entry = iter.next();
- assertEquals(Integer.parseInt(new String(entry.getValue().get())), 100);
- assertFalse(iter.hasNext());
+ int actual = scanner.stream().map(Entry::getValue).map(Value::get).map(String::new)
+ .map(Integer::parseInt).collect(onlyElement());
Review Comment:
It's slightly shorter if you just call parseInt on the only element (some suggestion applies elsewhere in this PR):
```suggestion
int actual = Integer.parseInt(scanner.stream().map(Entry::getValue).map(Value::get).map(String::new).collect(onlyElement()));
```
##########
test/src/main/java/org/apache/accumulo/test/mapred/AccumuloOutputFormatIT.java:
##########
@@ -233,11 +232,9 @@ public void testMR() throws Exception {
assertNull(e1);
try (Scanner scanner = c.createScanner(table2, new Authorizations())) {
- Iterator<Entry<Key,Value>> iter = scanner.iterator();
- assertTrue(iter.hasNext());
- Entry<Key,Value> entry = iter.next();
- assertEquals(Integer.parseInt(new String(entry.getValue().get())), 100);
- assertFalse(iter.hasNext());
+ int actual = scanner.stream().map(Entry::getValue).map(Value::get).map(String::new)
+ .map(Integer::parseInt).collect(onlyElement());
+ assertEquals(actual, 100);
Review Comment:
Expected should come first for the failure messages to be correct (saw this in several places throughout this PR).
```suggestion
assertEquals(100, actual);
```
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2646: Replace iterator usage
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2646:
URL: https://github.com/apache/accumulo/pull/2646#discussion_r857735260
##########
test/src/main/java/org/apache/accumulo/test/mapreduce/AccumuloOutputFormatIT.java:
##########
@@ -164,11 +162,9 @@ public void testMR() throws Exception {
assertNull(e1);
try (Scanner scanner = c.createScanner(table2, new Authorizations())) {
- Iterator<Entry<Key,Value>> iter = scanner.iterator();
- assertTrue(iter.hasNext());
- Entry<Key,Value> entry = iter.next();
- assertEquals(Integer.parseInt(new String(entry.getValue().get())), 100);
- assertFalse(iter.hasNext());
+ int actual = scanner.stream().map(Entry::getValue).map(Value::get).map(String::new)
+ .map(Integer::parseInt).collect(onlyElement());
Review Comment:
Personally, it seems to be a bit more intuitive or readable, when using multiple `.map()` operations, to just keep with that pattern and do the `parseInt` within a `.map()` as well, even if it is slightly longer.
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2646: Replace iterator usage
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2646:
URL: https://github.com/apache/accumulo/pull/2646#discussion_r857771631
##########
test/src/main/java/org/apache/accumulo/test/mapred/AccumuloOutputFormatIT.java:
##########
@@ -233,11 +232,9 @@ public void testMR() throws Exception {
assertNull(e1);
try (Scanner scanner = c.createScanner(table2, new Authorizations())) {
- Iterator<Entry<Key,Value>> iter = scanner.iterator();
- assertTrue(iter.hasNext());
- Entry<Key,Value> entry = iter.next();
- assertEquals(Integer.parseInt(new String(entry.getValue().get())), 100);
- assertFalse(iter.hasNext());
+ int actual = scanner.stream().map(Entry::getValue).map(Value::get).map(String::new)
+ .map(Integer::parseInt).collect(onlyElement());
+ assertEquals(actual, 100);
Review Comment:
Good catch, all should be addressed in cf5fb07
--
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
[GitHub] [accumulo] DomGarguilo merged pull request #2646: Replace iterator usage
Posted by GitBox <gi...@apache.org>.
DomGarguilo merged PR #2646:
URL: https://github.com/apache/accumulo/pull/2646
--
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
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2646: Replace iterator usage
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2646:
URL: https://github.com/apache/accumulo/pull/2646#discussion_r857739464
##########
test/src/main/java/org/apache/accumulo/test/mapreduce/AccumuloOutputFormatIT.java:
##########
@@ -164,11 +162,9 @@ public void testMR() throws Exception {
assertNull(e1);
try (Scanner scanner = c.createScanner(table2, new Authorizations())) {
- Iterator<Entry<Key,Value>> iter = scanner.iterator();
- assertTrue(iter.hasNext());
- Entry<Key,Value> entry = iter.next();
- assertEquals(Integer.parseInt(new String(entry.getValue().get())), 100);
- assertFalse(iter.hasNext());
+ int actual = scanner.stream().map(Entry::getValue).map(Value::get).map(String::new)
+ .map(Integer::parseInt).collect(onlyElement());
Review Comment:
This reminds me of my comment in the description of this PR:
> I also wanted to see if anyone had an opinion on which of the following equivalent options is better:
>
> 1. `scanner.stream().map(Entry::getValue).map(Value::get).map(String::new).map(Integer::parseInt).collect(onlyElement());`
>
> 2. `scanner.stream().map(entry -> Integer.parseInt(new String(entry.getValue().get()))).collect(onlyElement());`
I'm starting to think I prefer option 1 over option 2. It seems easier to read and keep track of what is happening especially with all the nested parentheses in option 2.
--
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