xiangfu0 commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3532470340
##########
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:
Addressed — `normalizePrimaryKeyValue` now guards with
`Preconditions.checkArgument(value != null, "Got null value for UUID primary
key column: %s", column)` before calling `UuidUtils.toBytes`, so a null UUID
primary key fails with a clear message naming the column instead of an opaque
NPE.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Addressed — the Javadoc no longer implies a defensive copy. It now reads:
"The `byte[]` fast path just skips the type dispatch of
`UuidUtils.toBytes(Object)`; neither path copies the buffer
(`UuidUtils.toBytes(byte[])` validates the width and returns it as-is)."
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
