gortiz commented on code in PR #11735:
URL: https://github.com/apache/pinot/pull/11735#discussion_r1345306409
##########
pinot-plugins/pinot-input-format/pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufRecordExtractor.java:
##########
@@ -55,14 +55,12 @@ public void init(@Nullable Set<String> fields,
RecordExtractorConfig recordExtra
/**
* For fields that are not set, we want to populate a null, instead of proto
default.
- * @param fieldDescriptor
- * @param message
*/
private Object getFieldValue(Descriptors.FieldDescriptor fieldDescriptor,
Message message) {
- // Note w.r.t proto3 - If a field is not declared with optional keyword,
there's no way to distinguish
- // if its explicitly set to a proto default or not been set at all i.e
hasField() returns false
- // and we would use null.
- if (fieldDescriptor.isRepeated() || !fieldDescriptor.hasPresence() ||
message.hasField(fieldDescriptor)) {
+ // In order to support null, the field needs to support _field presence_
+ // See
https://github.com/protocolbuffers/protobuf/blob/main/docs/field_presence.md
+ // or FieldDescriptor#hasPresence()
+ if (!fieldDescriptor.hasPresence() || message.hasField(fieldDescriptor)) {
Review Comment:
Removing fieldDescriptor.isRepeated() because fieldDescriptor.hasPresence()
already returns null when the field is repeated. It is also explicitly
indicated in its javadoc.
--
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]
