Copilot commented on code in PR #18927:
URL: https://github.com/apache/pinot/pull/18927#discussion_r3532914380
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -881,23 +888,16 @@ public Object convert(String value) {
throw new IllegalStateException();
}
} catch (Exception e) {
- throw new IllegalArgumentException(
- "Cannot convert value: '" + value + "' to type: " + this + " (" +
e.getMessage() + ")", e);
+ throw new IllegalArgumentException("Cannot convert value: '" + value +
"' to type: " + this);
}
}
public boolean equals(Object value1, Object value2) {
- if (this == UUID) {
- return UuidUtils.equals(toBytesValue(value1), toBytesValue(value2));
- }
- return this == BYTES ? Arrays.equals(toBytesValue(value1),
toBytesValue(value2)) : value1.equals(value2);
+ return this == BYTES || this == UUID ? Arrays.equals((byte[]) value1,
(byte[]) value2) : value1.equals(value2);
}
public int hashCode(Object value) {
- if (this == UUID) {
- return UuidUtils.hashCode(toBytesValue(value));
- }
- return this == BYTES ? Arrays.hashCode(toBytesValue(value)) :
value.hashCode();
+ return this == BYTES || this == UUID ? Arrays.hashCode((byte[]) value) :
value.hashCode();
}
Review Comment:
DataType.equals/hashCode now unconditionally cast BYTES/UUID values to
byte[]. This breaks existing callers that pass ByteArray (including values
produced by DataType.convertInternal for BYTES/UUID, and existing tests that
call UUID.toString/equals/compare with ByteArray). It will throw
ClassCastException at runtime for those code paths. Consider supporting both
byte[] and ByteArray here (and similarly in compare/toString below) to preserve
the documented internal-storage form.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -881,23 +888,16 @@ public Object convert(String value) {
throw new IllegalStateException();
}
} catch (Exception e) {
- throw new IllegalArgumentException(
- "Cannot convert value: '" + value + "' to type: " + this + " (" +
e.getMessage() + ")", e);
+ throw new IllegalArgumentException("Cannot convert value: '" + value +
"' to type: " + this);
}
Review Comment:
DataType.convert now catches all exceptions and throws a new
IllegalArgumentException without the original cause/message. This loses
important context (e.g., UUID parsing errors like "Invalid UUID value"), and
breaks existing tests that assert the UUID conversion error mentions "Invalid
UUID". Consider restoring the cause and including the underlying message in the
error text.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -910,26 +910,23 @@ public int hashCode(Object value) {
public int compare(Object value1, Object value2) {
switch (this) {
case INT:
+ case BOOLEAN:
return Integer.compare((int) value1, (int) value2);
case LONG:
+ case TIMESTAMP:
return Long.compare((long) value1, (long) value2);
case FLOAT:
return Float.compare((float) value1, (float) value2);
case DOUBLE:
return Double.compare((double) value1, (double) value2);
case BIG_DECIMAL:
return ((BigDecimal) value1).compareTo((BigDecimal) value2);
- case BOOLEAN:
- return Boolean.compare((boolean) value1, (boolean) value2);
- case TIMESTAMP:
- return Long.compare((long) value1, (long) value2);
case STRING:
case JSON:
return ((String) value1).compareTo((String) value2);
case BYTES:
- return ByteArray.compare(toBytesValue(value1), toBytesValue(value2));
case UUID:
- return UuidUtils.compare(toBytesValue(value1), toBytesValue(value2));
+ return ByteArray.compare((byte[]) value1, (byte[]) value2);
case MAP:
Review Comment:
DataType.compare for BYTES/UUID now casts the compared values to byte[].
Several call sites use DataType.convertInternal(...) (which returns ByteArray
for BYTES/UUID) and then compare values via DataType.compare, which will now
ClassCastException. Update compare to handle ByteArray inputs for these types.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -946,14 +943,11 @@ public String toString(Object value) {
if (this == BIG_DECIMAL) {
return ((BigDecimal) value).toPlainString();
}
- if (this == UUID) {
- if (value instanceof UUID) {
- return UuidUtils.toString((UUID) value);
- }
- return UuidUtils.toString(toBytesValue(value));
- }
if (this == BYTES) {
- return BytesUtils.toHexString(toBytesValue(value));
+ return BytesUtils.toHexString((byte[]) value);
+ }
+ if (this == UUID) {
+ return UuidUtils.toString((byte[]) value);
}
Review Comment:
DataType.toString for BYTES/UUID now assumes the input is byte[]. However
DataType.convertInternal returns ByteArray for these types, and existing
callers/tests stringify internal values (e.g., UUID.toString(ByteArray)). This
will now ClassCastException. Consider accepting ByteArray here as well.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -1000,25 +994,8 @@ public Comparable convertInternal(String value) {
throw new IllegalStateException();
}
} catch (Exception e) {
- throw new IllegalArgumentException(
- "Cannot convert value: '" + value + "' to type: " + this + " (" +
e.getMessage() + ")", e);
- }
- }
-
- private byte[] toBytesValue(Object value) {
- if (value instanceof byte[]) {
- return (byte[]) value;
- }
- if (value instanceof ByteArray) {
- return ((ByteArray) value).getBytes();
- }
- if (value instanceof UUID) {
- return UuidUtils.toBytes((UUID) value);
- }
- if (value instanceof String && this == UUID) {
- return UuidUtils.toBytes((String) value);
+ throw new IllegalArgumentException("Cannot convert value: '" + value +
"' to type: " + this);
}
Review Comment:
DataType.convertInternal similarly drops the original exception
cause/message when conversion fails. Including the cause improves debuggability
and keeps error messages consistent with DataType.convert.
--
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]
