Jackie-Jiang commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3618271204
##########
pinot-plugins/pinot-input-format/pinot-avro-base/src/main/java/org/apache/pinot/plugin/inputformat/avro/AvroSchemaUtil.java:
##########
@@ -60,6 +67,28 @@ public static DataType valueOf(Schema.Type avroType) {
}
}
+ /**
Review Comment:
(minor) Markdown style
##########
pinot-plugins/pinot-input-format/pinot-avro-base/src/main/java/org/apache/pinot/plugin/inputformat/avro/AvroSchemaUtil.java:
##########
@@ -20,14 +20,21 @@
import com.fasterxml.jackson.databind.node.ArrayNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
+import org.apache.avro.LogicalType;
+import org.apache.avro.LogicalTypes;
import org.apache.avro.Schema;
import org.apache.pinot.spi.data.FieldSpec;
import org.apache.pinot.spi.data.FieldSpec.DataType;
import org.apache.pinot.spi.utils.JsonUtils;
+import org.apache.pinot.spi.utils.UuidUtils;
/// Stateless helpers for mapping between Avro schema shapes and Pinot's
[DataType] / Avro JSON schema representations.
public class AvroSchemaUtil {
+ // Avro logical-type name for UUID (see org.apache.avro.LogicalTypes).
Value-level logical-type conversion lives
+ // in AvroRecordExtractor; this class only deals with schema-shape mapping.
+ private static final String UUID = "uuid";
+
Review Comment:
(nit) Put this below the private util class constructor
##########
pinot-plugins/pinot-input-format/pinot-avro-base/src/main/java/org/apache/pinot/plugin/inputformat/avro/AvroSchemaUtil.java:
##########
@@ -60,6 +67,28 @@ public static DataType valueOf(Schema.Type avroType) {
}
}
+ /**
Review Comment:
Should we deprecate the other `valueOf()` method since it doesn't honor
logical type?
##########
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:
I understand it is not introduced in this PR, but the contract should be to
switch on actual data type instead of stored type. For BOOLEAN data type, it
should be converted to a BOOLEAN avro type as well, instead of INT.
Also, BIG_DECIMAL is missing
--
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]
