lidavidm commented on code in PR #44056:
URL: https://github.com/apache/arrow/pull/44056#discussion_r1759631841
##########
java/gandiva/src/main/java/org/apache/arrow/gandiva/evaluator/FunctionSignature.java:
##########
@@ -73,7 +75,8 @@ public boolean equals(Object signature) {
@Override
public int hashCode() {
- return Objects.hashCode(this.name.toLowerCase(), this.returnType,
this.paramTypes);
+ return Objects.hashCode(
+ this.name.toLowerCase(Locale.getDefault()), this.returnType,
this.paramTypes);
Review Comment:
Locale.ROOT
##########
java/vector/src/main/java/org/apache/arrow/vector/VectorSchemaRoot.java:
##########
@@ -372,6 +376,16 @@ public boolean equals(VectorSchemaRoot other) {
return true;
}
+ @Override
+ public int hashCode() {
Review Comment:
This is a mutable object, I'm not sure we want it to have a hashCode...
##########
java/flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/accessor/impl/numeric/ArrowFlightJdbcDecimalVectorAccessorTest.java:
##########
@@ -176,6 +176,7 @@ public void
testShouldGetStringMethodFromDecimalVector(Supplier<ValueVector> vec
@ParameterizedTest
@MethodSource("data")
+ @SuppressWarnings("BigDecimalEquals")
Review Comment:
use compareTo
##########
java/gandiva/src/main/java/org/apache/arrow/gandiva/exceptions/GandivaException.java:
##########
@@ -29,6 +29,11 @@ public GandivaException(String msg, Exception cause) {
@Override
public String toString() {
- return getMessage();
+ return this.getMessage();
+ }
+
+ @Override
+ public String getMessage() {
Review Comment:
Why is the trivial override needed?
##########
java/gandiva/src/test/java/org/apache/arrow/gandiva/expression/TreeBuilderTest.java:
##########
@@ -39,30 +40,31 @@ public void testMakeLiteral() throws GandivaException {
assertEquals(true, node.getBooleanNode().getValue());
- n = TreeBuilder.makeLiteral(new Integer(10));
+ n = TreeBuilder.makeLiteral(10);
node = n.toProtobuf();
assertEquals(10, node.getIntNode().getValue());
- n = TreeBuilder.makeLiteral(new Long(50));
+ n = TreeBuilder.makeLiteral(Long.valueOf(50));
node = n.toProtobuf();
assertEquals(50, node.getLongNode().getValue());
- Float f = new Float(2.5);
+ Float f = (float) 2.5;
Review Comment:
2.5f
##########
java/gandiva/src/test/java/org/apache/arrow/gandiva/expression/TreeBuilderTest.java:
##########
@@ -39,30 +40,31 @@ public void testMakeLiteral() throws GandivaException {
assertEquals(true, node.getBooleanNode().getValue());
- n = TreeBuilder.makeLiteral(new Integer(10));
+ n = TreeBuilder.makeLiteral(10);
node = n.toProtobuf();
assertEquals(10, node.getIntNode().getValue());
- n = TreeBuilder.makeLiteral(new Long(50));
+ n = TreeBuilder.makeLiteral(Long.valueOf(50));
node = n.toProtobuf();
assertEquals(50, node.getLongNode().getValue());
- Float f = new Float(2.5);
+ Float f = (float) 2.5;
Review Comment:
And does it need to be boxed?
##########
java/vector/src/main/java/org/apache/arrow/vector/compare/TypeEqualsVisitor.java:
##########
@@ -62,10 +62,16 @@ public TypeEqualsVisitor(ValueVector right, boolean
checkName, boolean checkMeta
}
/** Check type equals without passing IN param in VectorVisitor. */
+ @SuppressWarnings("NonOverridingEquals")
public boolean equals(ValueVector left) {
Review Comment:
...this shoulda been named something different but it's way too late now
##########
java/gandiva/src/test/java/org/apache/arrow/gandiva/expression/TreeBuilderTest.java:
##########
@@ -39,30 +40,31 @@ public void testMakeLiteral() throws GandivaException {
assertEquals(true, node.getBooleanNode().getValue());
- n = TreeBuilder.makeLiteral(new Integer(10));
+ n = TreeBuilder.makeLiteral(10);
node = n.toProtobuf();
assertEquals(10, node.getIntNode().getValue());
- n = TreeBuilder.makeLiteral(new Long(50));
+ n = TreeBuilder.makeLiteral(Long.valueOf(50));
Review Comment:
50L doesn't work?
##########
java/vector/src/main/java/org/apache/arrow/vector/BitVectorHelper.java:
##########
@@ -385,7 +385,7 @@ public static void concatBits(
}
// copy the first bit set
- if (input1 != output) {
+ if (!input1.equals(output)) {
Review Comment:
Hmm, I wonder if this was intentional or not.
##########
java/vector/src/main/java/org/apache/arrow/vector/compare/TypeEqualsVisitor.java:
##########
@@ -138,12 +144,12 @@ public Boolean visit(LargeListViewVector left, Void
value) {
private boolean compareField(Field leftField, Field rightField) {
- if (leftField == rightField) {
+ if (leftField.equals(rightField)) {
Review Comment:
I _think_ this was intentional: it's a cheap "is this actually the same
object" check, before doing the rest of the checks below
--
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]
