xiangfu0 commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3609468216
##########
pinot-core/src/main/java/org/apache/pinot/core/util/SegmentProcessorAvroUtils.java:
##########
@@ -24,20 +24,24 @@
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
+import org.apache.avro.Conversion;
+import org.apache.avro.LogicalType;
+import org.apache.avro.LogicalTypes;
import org.apache.avro.Schema;
import org.apache.avro.SchemaBuilder;
import org.apache.avro.generic.GenericData;
-import
org.apache.pinot.core.segment.processing.framework.SegmentProcessorFramework;
import org.apache.pinot.spi.data.FieldSpec;
import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.data.readers.GenericRow;
import org.apache.pinot.spi.ingestion.segment.writer.SegmentWriter;
+import org.apache.pinot.spi.utils.UuidUtils;
/**
- * Helper methods for avro related conversions needed, when using AVRO as
intermediate format in segment processing.
- * AVRO is used as intermediate processing format in {@link
SegmentProcessorFramework} and file-based impl of
- * {@link SegmentWriter}
+ * Helper methods for Avro conversions used when serializing Pinot {@link
GenericRow}s to Avro — by the file-based
Review Comment:
Done — converted the class and the `convert*`/`Converts a Pinot schema`
javadocs in this file to `///` markdown (the rest of the file was already
markdown).
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-core/src/main/java/org/apache/pinot/core/util/SegmentProcessorAvroUtils.java:
##########
@@ -57,20 +61,85 @@ 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[]) {
+ // Array elements are written as-is. For MV UUID
(array<string{logicalType:uuid}>) the elements are the raw
+ // 16-byte values; the uuid Conversion registered on the writer's data
model (getAvroDataModel) renders each
+ // element to its canonical string at write time.
reusableRecord.put(field, Arrays.asList((Object[]) value));
- } else {
- if (value instanceof byte[]) {
- value = ByteBuffer.wrap((byte[]) value);
+ } else if (value instanceof byte[]) {
+ // A byte[] bound for a plain BYTES field must be wrapped as
ByteBuffer (GenericDatumWriter requires it for the
+ // bytes type). A byte[] bound for a UUID field
(string{logicalType:uuid}) is left raw so the uuid Conversion
+ // registered on the writer's data model (getAvroDataModel) renders it
to a canonical string at write time.
+ Schema.Field avroField = avroSchema.getField(field);
+ if (avroField != null && avroField.schema().getType() ==
Schema.Type.BYTES) {
Review Comment:
A `byte[]` value only ever originates from a BYTES or UUID column (both
stored as 16-byte BYTES). The check keys off the Avro field type: BYTES fields
need `ByteBuffer.wrap` (GenericDatumWriter requires it), while UUID fields
(`string{logicalType:uuid}`) must stay raw `byte[]` so the registered
`UuidConversion` renders them. No other type yields a `byte[]`, so the `else`
branch is only ever UUID — it won't affect other types.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-plugins/pinot-input-format/pinot-avro-base/src/main/java/org/apache/pinot/plugin/inputformat/avro/AvroUtils.java:
##########
@@ -160,6 +161,16 @@ public static org.apache.avro.Schema
getAvroSchemaFromPinotSchema(Schema pinotSc
SchemaBuilder.FieldAssembler<org.apache.avro.Schema> fieldAssembler =
SchemaBuilder.record("record").fields();
for (FieldSpec fieldSpec : pinotSchema.getAllFieldSpecs()) {
+ if (fieldSpec.getDataType() == DataType.UUID) {
Review Comment:
Not a bug. This path (`getAvroSchemaFromPinotSchema`) intentionally switches
on the *stored* type because it serializes Pinot's physically-stored values
(BOOLEAN→int, TIMESTAMP→long, etc.). UUID is special-cased so it keeps its
logical Avro form (`string{logicalType:uuid}`) for round-trip fidelity instead
of collapsing to bytes; switching the whole method to the original type would
change BOOLEAN/TIMESTAMP output too, which is out of scope here.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-plugins/pinot-input-format/pinot-avro-base/src/main/java/org/apache/pinot/plugin/inputformat/avro/AvroUtils.java:
##########
@@ -323,16 +335,17 @@ private static void
extractSchemaWithComplexTypeHandling(org.apache.avro.Schema
extractSchemaWithComplexTypeHandling(elementType, fieldsToUnnest,
delimiter, path, pinotSchema, fieldTypeMap,
timeUnit, collectionNotUnnestedToJson);
} else if (collectionNotUnnestedToJson ==
ComplexTypeConfig.CollectionNotUnnestedToJson.NON_PRIMITIVE
- && AvroSchemaUtil.isPrimitiveType(elementType.getType())) {
- addFieldToPinotSchema(pinotSchema,
AvroSchemaUtil.valueOf(elementType.getType()), path, false, fieldTypeMap,
- timeUnit);
+ && (AvroSchemaUtil.isPrimitiveType(elementType.getType())
Review Comment:
`isPrimitiveType` returns true for
INT/LONG/FLOAT/DOUBLE/BOOLEAN/STRING/ENUM, so STRING-backed UUID is already
covered by it. The extra `|| valueOf(elementType) == UUID` clause only matters
for FIXED(16)-backed UUID (raw Avro type FIXED, which is not primitive): it
lets such array elements unnest as a UUID column instead of being dropped.
_🤖 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]
