Copilot commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3525490467
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -780,16 +774,37 @@ private DedupRecordInfo getDedupRecordInfo(GenericRow
row) {
}
private RecordInfo getRecordInfo(GenericRow row, int docId) {
- PrimaryKey primaryKey = row.getPrimaryKey(_schema.getPrimaryKeyColumns());
+ PrimaryKey primaryKey = getPrimaryKey(row);
Comparable comparisonValue = getComparisonValue(row);
boolean deleteRecord = _deleteRecordColumn != null &&
BooleanUtils.toBoolean(row.getValue(_deleteRecordColumn));
return new RecordInfo(primaryKey, docId, comparisonValue, deleteRecord);
}
+ private PrimaryKey getPrimaryKey(GenericRow row) {
+ List<String> primaryKeyColumns = _schema.getPrimaryKeyColumns();
+ int numPrimaryKeyColumns = primaryKeyColumns.size();
+ Object[] values = new Object[numPrimaryKeyColumns];
+ for (int i = 0; i < numPrimaryKeyColumns; i++) {
+ String primaryKeyColumn = primaryKeyColumns.get(i);
+ Object value = row.getValue(primaryKeyColumn);
+ DataType dataType =
_schema.getFieldSpecFor(primaryKeyColumn).getDataType();
+ values[i] = normalizePrimaryKeyValue(value, dataType);
+ }
Review Comment:
`getPrimaryKey()` can throw a NullPointerException when a UUID primary-key
value is null because `normalizePrimaryKeyValue()` calls
`UuidUtils.toBytes(value)` (which dereferences `value`). Even if null PKs are
invalid, this should fail with a clear, consistent exception message instead of
an NPE that obscures the root cause.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/BloomFilterCreator.java:
##########
@@ -44,12 +47,25 @@ default void add(Object[] values, @Nullable int[] dictIds) {
for (Object value : values) {
add(BytesUtils.toHexString((byte[]) value));
}
+ } else if (getDataType() == FieldSpec.DataType.UUID) {
+ for (Object value : values) {
+ add(uuidToCanonicalString(value));
+ }
} else {
for (Object value : values) {
add(value.toString());
}
}
}
+
+ /// Renders a UUID value (typically a 16-byte big-endian {@code byte[]} from
segment ingest) as its canonical
+ /// lowercase RFC 4122 string. Avoids the defensive byte[] copy that {@code
UuidUtils.toBytes(Object)} would
+ /// perform on the {@code byte[]} branch.
Review Comment:
The Javadoc for `uuidToCanonicalString` says `UuidUtils.toBytes(Object)`
would do a defensive `byte[]` copy on the `byte[]` branch, but
`UuidUtils.toBytes(byte[])` explicitly returns the caller-owned buffer as-is
(after validating width). This comment is misleading about the actual
allocation behavior.
--
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]
