lidavidm commented on code in PR #39529:
URL: https://github.com/apache/arrow/pull/39529#discussion_r1455672602
##########
java/algorithm/src/test/java/org/apache/arrow/algorithm/dictionary/TestHashTableDictionaryEncoder.java:
##########
@@ -17,12 +17,12 @@
package org.apache.arrow.algorithm.dictionary;
+import static java.nio.charset.StandardCharsets.UTF_8;
Review Comment:
Let's avoid static imports, or at least, pick one way of doing it and stick
to it.
##########
java/algorithm/src/test/java/org/apache/arrow/algorithm/dictionary/TestLinearDictionaryEncoder.java:
##########
@@ -17,12 +17,12 @@
package org.apache.arrow.algorithm.dictionary;
+import static java.nio.charset.StandardCharsets.UTF_8;
Review Comment:
Ditto.
##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestServerMiddleware.java:
##########
@@ -186,7 +184,7 @@ public void doGetUncaught() {
Assertions.assertEquals(FlightStatusCode.OK, status.code());
Assertions.assertNull(status.cause());
Assertions.assertNotNull(err);
- Assertions.assertEquals(EXPECTED_EXCEPTION.getMessage(),
err.getMessage());
+ Assertions.assertEquals(new RuntimeException("test").getMessage(),
err.getMessage());
Review Comment:
Probably just hardcode "test" here instead of bouncing through the exception
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImpl.java:
##########
@@ -33,6 +33,8 @@
import org.apache.calcite.avatica.ConnectionConfigImpl;
import org.apache.calcite.avatica.ConnectionProperty;
+import com.google.common.base.Ascii;
Review Comment:
Ditto.
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorSchemaRoot.java:
##########
@@ -339,6 +339,7 @@ public VectorSchemaRoot slice(int index, int length) {
/**
* Determine if two VectorSchemaRoots are exactly equal.
*/
+
Review Comment:
Did you mean to do this?
##########
java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java:
##########
@@ -571,18 +571,18 @@ public void testMapWithListValue() throws Exception {
assertEquals(1L, getResultKey(resultStruct));
ArrayList<Long> list = (ArrayList<Long>) getResultValue(resultStruct);
assertEquals(3, list.size()); // value is a list with 3 elements
- assertEquals(new Long(50), list.get(0));
- assertEquals(new Long(100), list.get(1));
- assertEquals(new Long(200), list.get(2));
+ assertEquals(Long.valueOf(50), list.get(0));
Review Comment:
ditto (I'm not going to point out all of these)
##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestDictionaryUtils.java:
##########
@@ -78,7 +79,7 @@ public void testCreateSchema() {
Schema newSchema = DictionaryUtils.generateSchema(schema, dictProvider,
dictionaryUsed);
// assert that a new schema is created.
- assertTrue(schema != newSchema);
+ assertNotEquals(schema, newSchema);
Review Comment:
Shouldn't this be assertNotSame?
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/grpc/GetReadableBuffer.java:
##########
@@ -51,7 +51,7 @@ public class GetReadableBuffer {
tmpField = f;
tmpClazz = clazz;
} catch (Exception e) {
- new RuntimeException("Failed to initialize GetReadableBuffer, falling
back to slow path", e).printStackTrace();
+ throw new RuntimeException("Failed to initialize GetReadableBuffer,
falling back to slow path", e);
Review Comment:
This is intentionally NOT throwing.
##########
java/flight/flight-core/pom.xml:
##########
@@ -86,6 +86,10 @@
<groupId>com.google.protobuf</groupId>
<artifactId>protobuf-java</artifactId>
</dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java-util</artifactId>
Review Comment:
no version?
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/grpc/AddWritableBuffer.java:
##########
@@ -72,7 +72,7 @@ public class AddWritableBuffer {
tmpBufChainOut = tmpBufChainOut2;
} catch (Exception ex) {
- new RuntimeException("Failed to initialize AddWritableBuffer, falling
back to slow path", ex).printStackTrace();
+ throw new RuntimeException("Failed to initialize AddWritableBuffer,
falling back to slow path", ex);
Review Comment:
This is very much intentionally NOT throwing.
##########
java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ArrowFlightStatementExecuteUpdateTest.java:
##########
@@ -207,9 +207,12 @@ public void
testShouldFailToPrepareStatementForBadStatement() {
* we simply throw an `IllegalArgumentException` for queries not
registered
* in our `MockFlightSqlProducer`.
*/
+ String expectedMessage = format("Error while executing SQL \"%s\": " +
+ "org.apache.arrow.flight.FlightRuntimeException: INVALID_ARGUMENT:
Query not found",
Review Comment:
Yeah, something is wrong with the exception class. INVALID_ARGUMENT seems
like a fine addition but the exception name shouldn't be there.
##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestFlightService.java:
##########
@@ -149,12 +150,14 @@ public FlightInfo getFlightInfo(CallContext context,
FlightInfo flightInfo = client.getInfo(FlightDescriptor.path("test"));
Assertions.assertEquals(Optional.empty(),
flightInfo.getSchemaOptional());
Assertions.assertEquals(new Schema(Collections.emptyList()),
flightInfo.getSchema());
- Assertions.assertArrayEquals(flightInfo.getAppMetadata(),
"foo".getBytes());
+ Assertions.assertArrayEquals(flightInfo.getAppMetadata(),
"foo".getBytes(StandardCharsets.UTF_8));
Exception e = Assertions.assertThrows(
FlightRuntimeException.class,
() -> client.getSchema(FlightDescriptor.path("test")));
- Assertions.assertEquals("No schema is present in FlightInfo",
e.getMessage());
+ String expectedMessage =
"org.apache.arrow.flight.FlightRuntimeException: INVALID_ARGUMENT:" +
Review Comment:
We don't want the exception name in getMessage, I think - that would be
provided by toString already. Can you confirm what an exception looks like when
printed?
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/UrlParser.java:
##########
@@ -22,6 +22,8 @@
import java.util.HashMap;
import java.util.Map;
+import com.google.common.base.Splitter;
Review Comment:
Ditto.
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcDriver.java:
##########
@@ -43,6 +44,9 @@
import org.apache.calcite.avatica.Meta;
import org.apache.calcite.avatica.UnregisteredDriver;
+import com.google.common.base.Ascii;
+import com.google.common.base.Splitter;
Review Comment:
Let's not take on more usage of Guava than we absolutely have to. Can we
suppress those and undo this?
##########
java/flight/flight-sql/src/test/java/org/apache/arrow/flight/sql/example/FlightSqlExample.java:
##########
@@ -140,6 +141,7 @@
import org.apache.commons.pool2.impl.GenericObjectPool;
import org.slf4j.Logger;
+import com.google.common.base.Splitter;
Review Comment:
Ditto.
##########
java/vector/src/test/java/org/apache/arrow/vector/TestListVector.java:
##########
@@ -107,9 +107,9 @@ public void testCopyFrom() throws Exception {
Object result = outVector.getObject(0);
ArrayList<Long> resultSet = (ArrayList<Long>) result;
assertEquals(3, resultSet.size());
- assertEquals(new Long(1), (Long) resultSet.get(0));
- assertEquals(new Long(2), (Long) resultSet.get(1));
- assertEquals(new Long(3), (Long) resultSet.get(2));
+ assertEquals(Long.valueOf(1), resultSet.get(0));
Review Comment:
same here - 1L?
##########
java/vector/src/main/java/org/apache/arrow/vector/util/Text.java:
##########
@@ -202,15 +202,17 @@ public int find(String what, int start) {
public void set(String string) {
try {
ByteBuffer bb = encode(string, true);
- bytes = bb.array();
+ bytes = new byte[bb.remaining()];
+ bb.get(bytes);
+ bb.position(bb.position() - bytes.length); // move it back so we can
decode it
Review Comment:
This is an extra allocation, what's the reason for this?
##########
java/algorithm/src/test/java/org/apache/arrow/algorithm/sort/TestCompositeVectorComparator.java:
##########
@@ -17,6 +17,8 @@
package org.apache.arrow.algorithm.sort;
+import static java.nio.charset.StandardCharsets.UTF_8;
Review Comment:
Ditto (I'm not going to point out all the rest)
##########
java/vector/src/main/java/org/apache/arrow/vector/complex/impl/PromotableWriter.java:
##########
@@ -301,13 +302,15 @@ public boolean isEmptyStruct() {
return writer.isEmptyStruct();
}
+ @Override
protected FieldWriter getWriter() {
return writer;
}
private FieldWriter promoteToUnion() {
String name = vector.getField().getName();
- TransferPair tp =
vector.getTransferPair(vector.getMinorType().name().toLowerCase(),
vector.getAllocator());
+ TransferPair tp =
vector.getTransferPair(vector.getMinorType().name().toLowerCase(Locale.getDefault()),
+ vector.getAllocator());
Review Comment:
Hmm, setting aside the question of why we lowercase in the first place, we
should use Locale.ROOT to be consistent?
##########
java/vector/src/test/java/org/apache/arrow/vector/TestLargeListVector.java:
##########
@@ -723,23 +723,23 @@ public void testGetBufferAddress() throws Exception {
Object result = listVector.getObject(0);
ArrayList<Long> resultSet = (ArrayList<Long>) result;
assertEquals(3, resultSet.size());
- assertEquals(new Long(50), resultSet.get(0));
- assertEquals(new Long(100), resultSet.get(1));
- assertEquals(new Long(200), resultSet.get(2));
+ assertEquals(Long.valueOf(50), resultSet.get(0));
+ assertEquals(Long.valueOf(100), resultSet.get(1));
+ assertEquals(Long.valueOf(200), resultSet.get(2));
Review Comment:
weird; wouldn't 200L be ok 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
