Jackie-Jiang commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3609451395
##########
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:
What is the definition of primitive type? Seems STRING is also identified as
primitive type
##########
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:
(nit) Switch to markdown
##########
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:
Should we specialize BYTES or specialize UUID? Will this break other tyeps?
##########
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:
Is this a bug in the original code? Should it switch on actual type instead
of stored type?
--
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]
