Jackie-Jiang commented on code in PR #18927:
URL: https://github.com/apache/pinot/pull/18927#discussion_r3533008172
##########
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:
These are external-form methods: for `BYTES`/`UUID` the value handed to
`equals`/`hashCode` is always `byte[]` (default null values and post-transform
record values). The internal `ByteArray` form produced by `convertInternal` is
only ever ordered/compared via its own `Comparable.compareTo` — in `Range`,
`ValueBasedSegmentPruner`, `SelectionQuerySegmentPruner`, and
`MergeRangeFilterOptimizer` — never routed back through
`DataType.equals`/`hashCode`. The enum-level Javadoc documents this `byte[]`
contract, so accepting `ByteArray` here would be dead defensive code. The one
test that mixed the two forms has been updated.
##########
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:
Same as the `equals`/`hashCode` thread: `DataType.compare` is an
external-form (`byte[]`) method. The `ByteArray` returned by `convertInternal`
is ordered via its own `Comparable.compareTo` at the range/pruner call sites
(`Range`, `ValueBasedSegmentPruner`, `SelectionQuerySegmentPruner`,
`MergeRangeFilterOptimizer`), not through `DataType.compare`, so there is no
`ClassCastException` path in production.
##########
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:
Same as the `equals`/`compare` threads — `toString` takes the external
`byte[]` form (e.g. via `getDefaultNullValueString`, whose backing value is
always `byte[]`). `convertInternal`'s `ByteArray` output is never stringified
through `DataType.toString` in production; the test that did so has been
updated.
##########
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:
This is intentional. The thrown message `Cannot convert value: '<value>' to
type: <type>` already identifies both the offending value and the target type,
which is enough to diagnose why a string can't be converted to a given data
type. The affected test has been updated to assert this message.
##########
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:
Same as the `convert` thread — the `Cannot convert value: '<value>' to type:
<type>` message identifies the value and target type, which is sufficient to
diagnose the failure, so the underlying cause isn't propagated.
--
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]
