srowen commented on a change in pull request #33594:
URL: https://github.com/apache/spark/pull/33594#discussion_r680109344
##########
File path:
common/kvstore/src/main/java/org/apache/spark/util/kvstore/KVTypeInfo.java
##########
@@ -130,7 +130,7 @@ String getParentIndexName(String indexName) {
Class<?> getType();
}
- private class FieldAccessor implements Accessor {
+ private static class FieldAccessor implements Accessor {
Review comment:
static is better for inner classes that need no reference to the outer
class. While I don't think it comes up -- those hidden references can cause GC
or serialization issues in theory. Just cleaner.
##########
File path:
common/network-common/src/test/java/org/apache/spark/network/protocol/MessageWithHeaderSuite.java
##########
@@ -150,7 +150,7 @@ private ByteBuf doWrite(MessageWithHeader msg, int
minExpectedWrites) throws Exc
@Override
public long count() {
- return 8 * writeCount;
+ return 8L * writeCount;
Review comment:
The idea here is that 8 * writeCount is int * int, which could overflow,
even though the result is meant to be long. Cast before the multiply. Probably
not actually a bug in practice but who knows.
##########
File path:
streaming/src/test/java/test/org/apache/spark/streaming/Java8APISuite.java
##########
@@ -547,7 +547,7 @@ public void testPairMap2() { // Maps pair -> single
JavaPairDStream<String, Integer> pairStream =
JavaPairDStream.fromJavaDStream(stream);
JavaDStream<Integer> reversed = pairStream.map(Tuple2::_2);
JavaTestUtils.attachTestOutputStream(reversed);
- List<List<Tuple2<Integer, String>>> result = JavaTestUtils.runStreams(ssc,
2, 2);
+ List<List<Integer>> result = JavaTestUtils.runStreams(ssc, 2, 2);
Review comment:
The generic types were just wrong. It happens to work because of erasure.
##########
File path:
sql/core/src/test/java/test/org/apache/spark/sql/JavaDatasetSuite.java
##########
@@ -1548,7 +1548,7 @@ public void test() {
A("www.elgoog.com"),
B("www.google.com");
- private String url;
+ private final String url;
Review comment:
Enums shouldn't be mutable!
##########
File path:
core/src/test/java/org/apache/spark/unsafe/map/AbstractBytesToBytesMapSuite.java
##########
@@ -217,19 +217,6 @@ public void setAndRetrieveAKey() {
Assert.assertArrayEquals(valueData,
getByteArray(loc.getValueBase(), loc.getValueOffset(),
recordLengthBytes));
- try {
Review comment:
This test has the same problem as the above, but, is actually further
just 'wrong' since a long time ago, when putNewValue was replaced by append. It
is no longer expected to assert, or that's what I work out from the commit log.
##########
File path:
sql/catalyst/src/test/java/org/apache/spark/sql/catalyst/expressions/RowBasedKeyValueBatchSuite.java
##########
@@ -127,30 +127,43 @@ public void emptyBatch() throws Exception {
try (RowBasedKeyValueBatch batch =
RowBasedKeyValueBatch.allocate(keySchema,
valueSchema, taskMemoryManager, DEFAULT_CAPACITY)) {
Assert.assertEquals(0, batch.numRows());
+
+ boolean asserted = false;
try {
batch.getKeyRow(-1);
- Assert.fail("Should not be able to get row -1");
Review comment:
Same problem -- if it 'failed' it would still pass as the fail() is
caught by the catch block
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
