xiangfu0 commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3555954756
##########
pinot-core/src/main/java/org/apache/pinot/core/util/SegmentProcessorAvroUtils.java:
##########
@@ -57,20 +61,50 @@ public static GenericData.Record
convertGenericRowToAvroRecord(GenericRow generi
*/
public static GenericData.Record convertGenericRowToAvroRecord(GenericRow
genericRow,
GenericData.Record reusableRecord, Set<String> fields) {
+ Schema avroSchema = reusableRecord.getSchema();
for (String field : fields) {
Object value = genericRow.getValue(field);
if (value instanceof Object[]) {
- reusableRecord.put(field, Arrays.asList((Object[]) value));
+ Schema.Field avroField = avroSchema.getField(field);
+ if (avroField != null && isUuidArrayLogicalType(avroField.schema())) {
+ // MV UUID columns are emitted with an Avro
array<string{logicalType:uuid}> schema; convert each
+ // 16-byte element to its canonical UUID string so
GenericDatumWriter accepts the record.
Review Comment:
Good suggestion — registering an Avro `Conversion` on the writer's
`GenericData` would be cleaner than the manual `byte[]` → canonical-string
rendering. Tracking this as a follow-up rather than expanding this PR's scope.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-core/src/main/java/org/apache/pinot/core/segment/processing/genericrow/GenericRowDeserializer.java:
##########
@@ -164,6 +164,16 @@ public void deserialize(long offset, GenericRow buffer) {
multiValue[j] = new String(stringBytes, UTF_8);
}
break;
+ case BYTES:
Review Comment:
MV `BYTES` (de)serialization is required by MV `UUID` here, since UUID's
stored type is `BYTES` — so that part stays in this PR. The MV `BIG_DECIMAL`
gap is genuinely separable and will be handled in a follow-up bugfix.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -753,16 +747,39 @@ 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, primaryKeyColumn);
+ }
+ return new PrimaryKey(values);
+ }
+
+ private Object normalizePrimaryKeyValue(@Nullable Object value, DataType
dataType, String column) {
Review Comment:
You're right — it is redundant. After #18927 the record transformer already
yields `byte[]` for UUID columns, and `GenericRow.getPrimaryKey` wraps `byte[]`
into `ByteArray`, so the schema-aware normalization adds nothing. Tracking
removal (reverting to `row.getPrimaryKey(_schema.getPrimaryKeyColumns())`) as a
follow-up.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/BloomFilterCreator.java:
##########
@@ -33,6 +34,8 @@ public interface BloomFilterCreator extends IndexCreator {
default void add(Object value, int dictId) {
if (getDataType() == FieldSpec.DataType.BYTES) {
add(BytesUtils.toHexString((byte[]) value));
+ } else if (getDataType() == FieldSpec.DataType.UUID) {
Review Comment:
Yes — the value reaching `add()` is already the canonical 16-byte form.
Storing the hex (storage) format would work, but it has to change together with
the query side: `ValueBasedSegmentPruner` currently probes the bloom filter
with the same canonical-string rendering, so switching only the creator would
make every UUID probe miss. Keeping both on the canonical string for now;
tracking the hex alignment as a follow-up.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/columnminmaxvalue/ColumnMinMaxValueGenerator.java:
##########
@@ -360,26 +360,28 @@ private void
addColumnMinMaxValueWithoutDictionary(ColumnMetadata columnMetadata
break;
}
case BYTES: {
+ // ByteArray.compare is unsigned byte-wise lexicographic; for
canonical 16-byte big-endian UUIDs this is
+ // identical to UuidUtils.compare's unsigned 64-bit-word ordering,
so a single comparator is sufficient.
byte[] min = null;
byte[] max = null;
if (isSingleValue) {
for (int docId = 0; docId < numDocs; docId++) {
byte[] value = rawIndexReader.getBytes(docId, readerContext);
- if (min == null || ByteArray.compare(value, min) > 0) {
+ if (min == null || ByteArray.compare(value, min) < 0) {
Review Comment:
Agreed — the raw no-dictionary BYTES min/max comparisons were inverted (min
stored as max and vice versa). It's a real bug but orthogonal to UUID support,
so it will be extracted into its own PR.
_🤖 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]
