lidavidm commented on code in PR #44056:
URL: https://github.com/apache/arrow/pull/44056#discussion_r1758438504
##########
java/flight/flight-core/src/test/java/org/apache/arrow/flight/TestFlightService.java:
##########
@@ -150,7 +150,9 @@ public FlightInfo getFlightInfo(CallContext context,
FlightDescriptor descriptor
Exception e =
assertThrows(
FlightRuntimeException.class, () ->
client.getSchema(FlightDescriptor.path("test")));
- assertEquals("No schema is present in FlightInfo", e.getMessage());
+ assertEquals(
Review Comment:
Can we do some sort of `assertTrue(e.getMessage().contains(...))`?
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/ServerSessionMiddleware.java:
##########
@@ -16,8 +16,10 @@
*/
package org.apache.arrow.flight;
+import com.google.common.base.Splitter;
Review Comment:
Ideally we wouldn't use Guava? I suppose as long as this interface seems
stable...
Is there an apache-commons equivalent we can use?
##########
java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/ArrowFlightStatementExecuteUpdateTest.java:
##########
@@ -220,7 +220,10 @@ public void
testShouldFailToPrepareStatementForBadStatement() {
*/
assertThat(
e.getMessage(),
- is(format("Error while executing SQL \"%s\": Query not found",
badQuery)));
+ is(
Review Comment:
Would prefer contains
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/ArrowFlightJdbcDriver.java:
##########
@@ -277,7 +278,8 @@ Optional<Map<Object, Object>> getUrlsArgs(String url)
throws SQLException {
static Properties lowerCasePropertyKeys(final Properties properties) {
final Properties resultProperty = new Properties();
- properties.forEach((k, v) ->
resultProperty.put(k.toString().toLowerCase(), v));
+ properties.forEach(
+ (k, v) ->
resultProperty.put(k.toString().toLowerCase(Locale.getDefault()), v));
Review Comment:
let's use Locale.ROOT rather than the system locale
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/SetSessionOptionsResult.java:
##########
@@ -45,12 +45,14 @@ static ErrorValue
fromProtocol(Flight.SetSessionOptionsResult.ErrorValue s) {
return values()[s.getNumber()];
}
+ @SuppressWarnings("EnumOrdinal")
Review Comment:
IMO, the right way to do this would be to add a constructor and store the
Protobuf enum value in the enum instance
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/grpc/AddWritableBuffer.java:
##########
@@ -70,8 +70,8 @@ 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:
I think this intentionally did not throw, as evidenced by the "falling back"
message
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/grpc/GetReadableBuffer.java:
##########
@@ -47,8 +47,8 @@ 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:
Same here
##########
java/flight/flight-core/src/main/java/org/apache/arrow/flight/SetSessionOptionsResult.java:
##########
@@ -45,12 +45,14 @@ static ErrorValue
fromProtocol(Flight.SetSessionOptionsResult.ErrorValue s) {
return values()[s.getNumber()];
}
+ @SuppressWarnings("EnumOrdinal")
Flight.SetSessionOptionsResult.ErrorValue toProtocol() {
return Flight.SetSessionOptionsResult.ErrorValue.values()[ordinal()];
}
}
/** Per-option extensible error response container. */
+ @SuppressWarnings("JavaLangClash")
Review Comment:
What was the problem here?
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcDecimalVectorAccessor.java:
##########
@@ -71,6 +71,7 @@ public String getString() {
}
@Override
+ @SuppressWarnings("BigDecimalEquals")
Review Comment:
Use compareTo instead
https://stackoverflow.com/questions/10950914/how-to-check-if-bigdecimal-variable-0-in-java
##########
java/flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/utils/ArrowFlightConnectionConfigImpl.java:
##########
@@ -241,7 +243,7 @@ public Object get(final Properties properties) {
Preconditions.checkNotNull(properties, "Properties cannot be null.");
Object value = properties.get(camelName);
if (value == null) {
- value = properties.get(camelName.toLowerCase());
+ value = properties.get(camelName.toLowerCase(Locale.getDefault()));
Review Comment:
Locale.ROOT
##########
java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java:
##########
@@ -1240,7 +1240,7 @@ public static class PreparedStatement implements
AutoCloseable {
* used in the {@code PreparedStatement} setters.
*/
public void setParameters(final VectorSchemaRoot parameterBindingRoot) {
- if (parameterBindingRoot == this.parameterBindingRoot) {
+ if (parameterBindingRoot.equals(this.parameterBindingRoot)) {
Review Comment:
I think this was intentionally an identity check
--
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]
