xiangfu0 commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3618336878
##########
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:
Done — moved the `UUID` constant below the private constructor.
_🤖 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/AvroSchemaUtil.java:
##########
@@ -60,6 +67,28 @@ public static DataType valueOf(Schema.Type avroType) {
}
}
+ /**
Review Comment:
Done — converted this method's javadoc to `///` markdown.
_🤖 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/AvroSchemaUtil.java:
##########
@@ -60,6 +67,28 @@ public static DataType valueOf(Schema.Type avroType) {
}
}
+ /**
Review Comment:
`valueOf(Schema.Type)` takes a bare Avro type with no schema, so it
structurally can't honor logical types — it's the low-level primitive
`valueOf(Schema)` delegates to, and it's also used by `SegmentTestUtils`.
Rather than `@Deprecated` (which would flag its own internal fallback and those
callers), I clarified in its javadoc that it doesn't honor logical types and
that `valueOf(Schema)` should be preferred when a full `Schema` is available.
Happy to hard-deprecate as a follow-up if you'd rather.
_🤖 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:
Agreed the contract reads cleaner on the actual type. The catch:
`getAvroSchemaFromPinotSchema` builds the schema for GenericRows whose values
are already in *stored* form (BOOLEAN→Integer, TIMESTAMP→Long), so the Avro
field types have to match those values — switching BOOLEAN→`boolean` (and
adding BIG_DECIMAL) needs a coordinated value conversion, like the UUID
Conversion this PR adds. Since that's a pre-existing, broader change with
round-trip/compat implications beyond UUID, I'd prefer to leave this thread
open and do it as a dedicated follow-up (switch on actual type +
BOOLEAN/BIG_DECIMAL/TIMESTAMP logical types + value conversions) rather than
fold it in here.
_🤖 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]
